New lint check and fix
ClosedPublic

Authored by nomeata on Mar 21 2015, 9:03 AM.

Details

Summary

So that Trac #10176 would not happen again. Will push individual commits, so ignore this summary.

Test Plan

Validate on harbormaster

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.
nomeata retitled this revision from to New lint check: exprIsHNF = True and alts = [] is bogus.Mar 21 2015, 9:03 AM
nomeata updated this object.
nomeata edited the test plan for this revision. (Show Details)
nomeata updated the Trac tickets for this revision.
nomeata updated this revision to Diff 2495.Mar 21 2015, 9:09 AM
nomeata updated this revision to Diff 2496.Mar 21 2015, 10:05 AM

Trim Call Arity to typeArity; this seems to fix (or at least mitigate) this
problem.

nomeata updated this revision to Diff 2497.Mar 21 2015, 2:53 PM

Trim Call Arity based on the strictness signature, if its a bottoming one

nomeata updated this revision to Diff 2498.Mar 21 2015, 3:16 PM

Typos in comments.

Do the tests normally get run with Core lint turned on? If not, don't you need to add that as an extra flag to the test case? The code compiles fine before, it just generates a buggy result.

Do the tests normally get run with Core lint turned on? If not, don't you need to add that as an extra flag to the test case? The code compiles fine before, it just generates a buggy result.

They do. At least the test failed for me before I added the fix with the next patch.

But thanks for checking!

-dcore-lint is indeed passed to all tests (see TEST_HC_OPTS in testsuite/mk/test.mk)

But I just copied the T10176 test into my tree (current master, without above patch), and make TEST=T10176 passes just fine. It shouldn't, right?

This is a devel2 build with GhcLibWays +=p, maybe that's what's interfering.

In D747#20694, @thomie wrote:

-dcore-lint is indeed passed to all tests (see TEST_HC_OPTS in testsuite/mk/test.mk)

But I just copied the T10176 test into my tree (current master, without above patch), and make TEST=T10176 passes just fine. It shouldn't, right?

Did you also copy the new lint check? It will only fail due to that.

Ok, nevermind me. It indeed fails.

Macro shipitquick:

simonpj accepted this revision.Mar 21 2015, 4:47 PM
simonpj added a reviewer: simonpj.
simonpj added a subscriber: simonpj.

Great. Thanks for looking into this so fast.

but see my notes

Simon

compiler/coreSyn/CoreLint.hs
640 ↗(On Diff #2498)

Actually I believe that a stronger condition should also hold: if the alternatives are empty then exprIsBottom should hold. If exprIsHNF holds, that's DEFINITELY wrong, but an empty case is only ok if the scrutinee is bottom, and I think it will in practice always be provably (to GHC) bottom

It is just possible that this might lead to false positives, but they would be interesting and I'd like to know about them.

compiler/simplCore/CallArity.hs
368

always (spelling).

This is a good Note. Would you be willing to add a Lint check for these two conditions? After all, some *other* transformation might cause this badness to happen, and if so we'd like to know.

I think we want to check that (idArity id) respects the two constraints.

This revision is now accepted and ready to land.Mar 21 2015, 4:47 PM
hvr requested changes to this revision.Mar 21 2015, 5:31 PM
hvr added a reviewer: hvr.
hvr added a subscriber: hvr.
hvr added inline comments.
compiler/simplCore/CallArity.hs
613

trailing whitespace

This revision now requires changes to proceed.Mar 21 2015, 5:31 PM
austin requested changes to this revision.Mar 21 2015, 6:09 PM

Minor pedantry: Please clean up the summary a bit. I was very confused at how this could 'fix' Trac #10176 when the description reads about adding a new lint check, which I wouldn't expect to 'fix' a bug somehow! It was much clearer once I saw your history had A) the fix, B) a test, and C) a new lint pass.

Since you'll probably just arc land these commits together, I'd suggest something like "compiler: fix Trac #10176, add test, add lint check", with the summary just expanding a little if you think it should be there.

hvr added a comment.EditedMar 22 2015, 2:31 AM

@austin actually it would make totally sense to commit the fix + regression-test into a separate patch(es) from the lint-checker improvement, as one is a fix, and the other is an enhancement (and e.g. you could then only cherry-pick the fix into 7.10 and leave the linter alone)

In D747#20709, @austin wrote:

Minor pedantry: Please clean up the summary a bit. I was very confused at how this could 'fix' Trac #10176 when the description reads about adding a new lint check, which I wouldn't expect to 'fix' a bug somehow! It was much clearer once I saw your history had A) the fix, B) a test, and C) a new lint pass.

Since you'll probably just arc land these commits together, I'd suggest something like "compiler: fix Trac #10176, add test, add lint check", with the summary just expanding a little if you think it should be there.

My plan was to push the feature branch with the three separate commits (lint check, regression test, call arity fix).

compiler/coreSyn/CoreLint.hs
640 ↗(On Diff #2498)

Quite right. I’ll do it separately, to get this fix on the way.

compiler/simplCore/CallArity.hs
368

I think we want to check that (idArity id) respects the two constraints.

Good idea. Again, I’ll do it separately, if that’s ok.

nomeata updated this revision to Diff 2501.Mar 22 2015, 3:17 AM

Typo, whitespace.

nomeata retitled this revision from New lint check: exprIsHNF = True and alts = [] is bogus to New lint check and fix.Mar 22 2015, 3:18 AM
nomeata updated this object.
hvr accepted this revision.Mar 22 2015, 3:19 AM

I'm looking forward to have the fix landed soon so we can get it into 7.10 :-)

This revision was automatically updated to reflect the committed changes.