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

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

Details

Reviewers
hvr
bgamari
Trac Issues
#13256
#10930
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

harpocrates created this revision.Tue, Sep 25, 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)Tue, Sep 25, 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.