Warn on all out-of-range literals in pats/exprs
ClosedPublic

Authored by harpocrates on Sep 25 2018, 4:35 PM.

Details

Summary

These changes were motivated by Trac #13256. While poking around, I
realized we weren't very consistent in our "-Woverflowed-literals"
warnings. This patch fixes that by:

  • warning earlier on in the pipeline (ie. before we've desugared 'Int' patterns into 'I# Int#')
  • handling 'HsLit' as well as 'HsOverLit' (this covers unboxed literals)
  • covering more pattern / expression forms

4/6 of the warnings in the 'Overflow' test are due to this patch. The
other two are mostly for completeness.

Also fixed a missing empty-enumeration warning for 'Natural'.

This warnings were tripped up by the 'Bounded Word' instance (see Trac #9505),
but the fix was obvious and simple: use unboxed word literals.

Test Plan

make TEST=Overflow && make TEST=T10930

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
harpocrates created this revision.Sep 25 2018, 4:35 PM

Apologies for some extraneous comment-only changes. I rely on the generated Haddock documentation for navigating GHC's source and the lack of doc comments in this area of the codebase was a somewhat frustrating experience.

I can revert the comment-noise if someone feels strongly about it.

  • Fix empty enumeration of natural
harpocrates edited the summary of this revision. (Show Details)Sep 25 2018, 11:26 PM
harpocrates edited the test plan for this revision. (Show Details)
harpocrates updated the Trac tickets for this revision.

I can't claim to understand the code here in great detail, but this looks generally right to me. (@hvr, do you mind giving this a look over?)

It appears that this patch introduces some new warnings. Can you characterize exactly which warnings are new with this patch, and record this in the 8.8.1 release notes? (Also the 8.8 Migration Guide, once this patch lands.)

compiler/deSugar/Match.hs
477

It would be worth adding a brief comment here stating why we don't warn if the code is generated. (I'm not sure of the reason for this myself after reading the patch—was there some test case that motivated this special case?)

Also, you linked Trac #13256 as a related ticket, but didn't add a corresponding test case. Does this patch actually fix Trac #13256? If so, it would be good to test this.

  • Address nits
  • Add a test case for Trac #15460, which is also now solved
  • Update users guide

Also, you linked Trac #13256 as a related ticket, but didn't add a corresponding test case. Does this patch actually fix Trac #13256? If so, it would be good to test this.

This does fix Trac #13256. The Overflow test covers what is essentially the same situation, I've added an extra test case anyways.


Side note: I just found D4475 and D5006 which add support for Int8#/Word8# and Int16#/Word16#. Those should have their own cases in warnAboutOverflowedLiterals.

compiler/deSugar/Match.hs
477

I added some more comments on the eqn_orig :: Origin field I added to EquationInfo (from which o here originates). Does that help at all?

Side note: I just found D4475 and D5006 which add support for Int8#/Word8# and Int16#/Word16#. Those should have their own cases in warnAboutOverflowedLiterals.

Indeed they should. I suppose the most prudent thing to do would be to open new tickets about these after they've landed. Or, if this Diff lands before the other ones, you could beg them to rebase their changes and make this change to warnAboutOverflowedLiterals themselves ;)

compiler/deSugar/Match.hs
477

That's much clearer to me now, thank you!

D4475 just landed, so you could rebase this Diff on top of master and add support for Int8# and Word8#, if you wish.

D4475 just landed

...and it was just reverted. Never mind :)

D4475 just landed, so you could rebase this Diff on top of master and add support for Int8# and Word8#, if you wish.

I don't think there is a rush to do that. Adding cases for Int8#, Word8#, Int16#, etc. is not going to change any warnings since we still won't be able to write literals of these types. Adding cases for these is more a matter of future-proofing (in case we do someday add concrete syntax for these literals). I'll act once both D4475 and D5006 have fully landed.

harpocrates marked 3 inline comments as done.Nov 1 2018, 4:52 PM

Friendly ping!

RyanGlScott requested changes to this revision.Nov 2 2018, 9:58 AM

Alright, I'll try to give an actually in-depth review this time (I'm a bit out of my element here, so forgive me if I ask questions that don't make sense).

This warnings were tripped up by the 'Bounded Word' instance (see Trac Trac #9505),
but the fix was obvious and simple: use unboxed word literals.

I'm a bit confused by this sentence. Does this ticket fix Trac #9505? If so, is "the fix" that you refer to here ("use unboxed word literals") applied somewhere?

compiler/deSugar/MatchLit.hs
110–111

Passing around dflags explicitly feels silly here, since we're already in a DsM context. Can't we just retrieve it in the body of the function with getDynFlags?

163

Similarly, why pass DynFlags explicitly here?

164

Having an Either here like this feels somewhat clunky, since it forces all call sites to wrap their argument in a mysterious Left or Right. A suggested refactoring would be:

  • Have the type of warnAboutOverflowedLiterals be DynFlags -> Maybe (Integer, Name) -> DsM (), and make it an internal function.
  • Write two functions on top of warnAboutOverflowedLiterals that accept HsOverLit GhcTc and HsLit GhcTc, and call those functions in the places where warnAboutOverflowedLiterals is currently invoked.
282

Hm, this is tricky. Is there a test case that checks for an empty enumeration involving a negative Natural literal? If not, it would be good to add one. (And then refer to that test case from here as a reference.)

This revision now requires changes to proceed.Nov 2 2018, 9:58 AM
harpocrates planned changes to this revision.Nov 2 2018, 10:25 AM

Thanks for the review - will update later today or tomorrow.

compiler/deSugar/MatchLit.hs
110–111

You can, but it makes the otherwise pure logic messier (note that the old dsOverLit' also passed in DynFlags). I'll see what I can do...

163

Same reason as before. Note that the old warnAboutOverflowedLiterals also passes in its DynFlags.

164

Seems reasonable, will do!

282

I'll add one.

libraries/base/GHC/Enum.hs
637

Replying to https://phabricator.haskell.org/D5181#145921: this is the change that I was referring to. It doesn't "fix" the ticket.

bgamari requested changes to this revision.Nov 2 2018, 12:51 PM

Thanks for the ping!

This looks reasonable. There is only one documentation issue inline that we should address before merging.

However, there is a suspicious bit in the existing code that we should probably fix. Perhaps you could pick this up in another patch, @harpocrates?

compiler/deSugar/MatchLit.hs
183

I know you didn't write this but this case (and the intPrim case below) looks very suspicious. In particular, isn't this using the word size of the build machine to bounds check a value for the target machine?

To put it another way: if I were cross-compiling for a 64-bit machine on a 32-bit machine I would get an incorrect warning if I write the literal 0xffffffffffffffff.

269

This looks similarly suspicious.

305

What is a "simple" literal?

harpocrates updated this revision to Diff 18597.Nov 5 2018, 3:30 PM
harpocrates marked 10 inline comments as done.
  • Fix empty enumeration of natural
  • Address nits
  • Add a test case for Trac #15460, which is also now solved
  • Update users guide
  • Address review feedback

I'll deal with the case where host and target have differently-sized Int/Word in another patch. That is definitely the sort of thing for which a warning would be super useful!
I see how what we are currently doing is not going to cut it...

Other than that, I think I've covered all other feedback. I appreciate the careful reviews!

RyanGlScott accepted this revision.Nov 5 2018, 5:44 PM

Is there anything blocking this from being merged?

My apologies for forgetting about this patch! We should indeed land this soon.

...alas, it seems like this patch has a merge conflict with the latest master. Do you mind rebasing this once more, @harpocrates?

harpocrates updated this revision to Diff 18973.Sun, Dec 2, 2:46 PM
  • Forgot to commit some changes...
This revision was not accepted when it landed; it landed in state Needs Review.Mon, Dec 3, 6:48 AM
This revision was automatically updated to reflect the committed changes.