Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lint select from Unit, or Int that incurs widening #10372

Merged
merged 3 commits into from Apr 12, 2023

Conversation

som-snytt
Copy link
Contributor

@scala-jenkins scala-jenkins added this to the 2.13.12 milestone Apr 10, 2023
@som-snytt
Copy link
Contributor Author

Some current inconsistency.

scala> 42.ceil
val res0: Float = 42.0

scala> def i = 42
def i: Int

scala> i.ceil
       ^
       warning: Widening conversion from Int to Float is deprecated because it loses precision. Write `.toFloat` instead.
val res1: Float = 42.0

scala> def c = 'c'
def c: Char

scala> c.ceil
val res2: Float = 99.0

where c.ceil is pronounced "Cécile" and is even less likely to be intended.

With the new warning, the current warning is less correct.

@som-snytt
Copy link
Contributor Author

It warns in two spots that specialize Array[Unit].

The other thought is that (as with the current warning), any promotion is dubious, it doesn't matter which member is selected.

Then just need definitions.RichFloatClass.

@som-snytt som-snytt force-pushed the issue/12728-dubious-selections branch 4 times, most recently from efd7fd9 to b84df96 Compare April 10, 2023 20:34
@som-snytt
Copy link
Contributor Author

The check is only at typer, which gives a pass to

case Literal(Constant(())) =>

Not sure why it was deemed bad to allow case _: () => as the unit value ought to be a singleton. (It is not Unit with Singleton.) I guess it doesn't matter because the test is always boxed unit equality. There's no advantage to case _: Unit.

Something became ().asInstanceOf[Unit] in fields, a private lazy val after init. (That was the test execution will be non-parallel in PartestDefaults.)

@som-snytt som-snytt marked this pull request as ready for review April 10, 2023 22:01
@som-snytt som-snytt requested a review from lrytz April 10, 2023 22:02
Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and very useful.


val sym = tree.symbol orElse member(qual.tpe, name) orElse inCompanionForJavaStatic(qual.tpe.typeSymbol, name)
if ((sym eq NoSymbol) && name != nme.CONSTRUCTOR && mode.inAny(EXPRmode | PATTERNmode)) {
// symbol not found? --> try to convert implicitly to a type that does have the required
// member. Added `| PATTERNmode` to allow enrichment in patterns (so we can add e.g., an
// xml member to StringContext, which in turn has an unapply[Seq] method)

def checkDubiousAdaptation(sel: Tree): Unit = if (!isPastTyper && settings.lintNumericMethods) {
val dubious = ScalaIntegralValueClasses(qualTp.typeSymbol) && (
sel.symbol.owner.eq(BoxedFloatClass) || sel.symbol.owner.eq(RichFloatClass))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

long.isNaN goes through float2Float(long.toFloat), not double2Double, so this seems to cover it all.

@som-snytt som-snytt force-pushed the issue/12728-dubious-selections branch from b84df96 to 905c92f Compare April 11, 2023 18:36
@som-snytt
Copy link
Contributor Author

Rebased, and enhanced partest --realeasy to skip tests that run on today's JDK.

-- 1 - run/blank.scala                           [skipped on Java 19, compiling against JDK8 but must run on 11+]

It would be ideal if --realeasy did not skip in case of explicit scalac: --release:8 but that was too subtle for me.

The option also adds run/t6240-universe-code-gen.scala which is annoying to forget to check. (I had actually fixed it in the wrong commit.)

Selected 1810 tests drawn from 1 named test categories, 2 tests modified on this branch, and other tests you might have forgotten

In case of selection failure,

sbt:root> partest junk
Discarding 1 invalid test paths

Selected 0 tests drawn from the well of despair. I see you're not in a testing mood.
[info] Passed: Total 0, Failed 0, Errors 0, Passed 0

Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@lrytz lrytz merged commit d5247e9 into scala:2.13.x Apr 12, 2023
4 checks passed
@SethTisue SethTisue modified the milestones: 2.13.12, 2.13.11 Apr 12, 2023
@som-snytt som-snytt deleted the issue/12728-dubious-selections branch April 12, 2023 16:01
@SethTisue SethTisue added the release-notes worth highlighting in next release notes label May 3, 2023
@SethTisue SethTisue changed the title Lint select from Unit, or Int that incurs widening Lint select from Unit, or Int that incurs widening Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes worth highlighting in next release notes
Projects
None yet
4 participants