Skip to content

Conversation

lrytz
Copy link
Member

@lrytz lrytz commented Feb 15, 2023

Add a lint warning for integer divisions that are implicitly converted to floats (someInt / 2: Double). Such expressions are probably a mistake, or otherwise very likely to be misunderstood by reviewers / readers.

The same argument could be made for other integer operations as they can overflow. Int.MaxValue * 2: Double is different from Int.MaxValue.toDouble * 2. But division is much more likely to be a bug because the result is different also without an overflow.

The existing -Ywarn-numeric-widen flag (-Wnumeric-widen on 2.13) warns about all widening conversions, which is probably too restrictive for some users.

While working on this, I noticed a couple of extension methods on integrals that are nonsensical: ceil, floor, isNaN, isNegInfinity, isPosInfinity, isInfinite, isInfinity, isFinite. We could add a warning for those in RefChecks, for example. someInt.isNaN compiles to Predef.float2Float(someInt.toFloat).isNaN, the widening conversion toFloat is inserted during implicit search.

@scala-jenkins scala-jenkins added this to the 2.12.18 milestone Feb 15, 2023
@lrytz lrytz force-pushed the div-to-float branch 2 times, most recently from c11c480 to df1b859 Compare February 15, 2023 14:53
@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Feb 15, 2023
Copy link
Member

@SethTisue SethTisue left a comment

Choose a reason for hiding this comment

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

LGTM except for the minor tweaks I've suggested

@lrytz
Copy link
Member Author

lrytz commented Mar 1, 2023

I tested this, it works well and seems very valueable.

@lrytz lrytz requested a review from SethTisue March 1, 2023 10:00
@SethTisue SethTisue self-assigned this Mar 1, 2023
@SethTisue SethTisue merged commit e6a3a68 into scala:2.12.x Mar 8, 2023
@som-snytt
Copy link
Contributor

I'm going to try this out! I hope the change is not too divisive.

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
Development

Successfully merging this pull request may close these issues.

4 participants