Fix #14135 by validity checking matches
ClosedPublic

Authored by dfeuer on Sep 16 2017, 6:04 PM.

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.
carlostome created this revision.Sep 16 2017, 6:04 PM

I don't know enough about this end of the code to have an informed opinion, but matching and unification are different algorithms, and generally only one is the Right Thing in a given scenario. Matching considers the variables in its right-hand argument to be completely rigid -- they cannot be bound to anything in order to induce a match with the left-hand argument. On the other hand, unification can bind variables in either argument to the other; unification is fully symmetric. So, at the least, there should be a reason to prefer one of these functions over the other. Perhaps you're right that unification is what's required here, but I would want an explanation of why this is so before accepting this change.

Are there any experts in this area around? Who implemented COMPLETE?

bgamari updated the Trac tickets for this revision.Sep 17 2017, 7:07 AM

@carlostome has written a summary of his findings in the Trac #14135; however, I'm afraid I don't know enough about this case to judge either. I'll admit that the asymmetry of matching feels more correct in this context though.

RyanGlScott added a subscriber: RyanGlScott.

Simon is the one who added the call to tcMatchTy in 6b77914cd37b697354611bcd87897885c1e5b4a6, so I'll add him as a reviewer.

mpickering resigned from this revision.Sep 19 2017, 4:07 AM

I don't know enough about this either.

simonpj requested changes to this revision.Sep 21 2017, 3:42 AM

Let's not do this. See Trac Trac #14135 comment:6.

This revision now requires changes to proceed.Sep 21 2017, 3:42 AM
carlostome updated this revision to Diff 14034.Sep 21 2017, 4:47 PM
  • Revert "Fix Trac #14135 by changing tcMatchTy for tcUnifyTy"
  • Fix Trac #14135 doing a validity check on matches
RyanGlScott requested changes to this revision.Sep 21 2017, 7:39 PM
RyanGlScott added inline comments.
compiler/deSugar/Check.hs
1276

This x is unused.

1287

What is going on here? This deserves a Note.

testsuite/tests/deSugar/should_compile/T14135.hs
2

You should compile this test with -Wincomplete-patterns enabled—we want to make sure that it emits the right warning!

This revision now requires changes to proceed.Sep 21 2017, 7:39 PM
carlostome updated this revision to Diff 14086.Sep 25 2017, 4:08 AM
  • Fixed bug but unleashed panic! in some other tests

    dependent/should_fail/T13780c.run T13780c [stderr mismatch] (normal) pmcheck/should_compile/T11374.run T11374 [exit code non-0] (normal) th/T4188.run T4188 [exit code non-0] (normal) th/T4188.run T4188 [exit code non-0] (ext-interp) typecheck/should_compile/T13822.run T13822 [exit code non-0] (normal)
bgamari retitled this revision from Fix #14135 by changing tcMatchTy for tcUnifyTy to Fix #14135 by validity checking matches.Oct 3 2017, 10:27 AM
bgamari requested changes to this revision.Oct 3 2017, 10:37 AM

Sorry for the late review, @carlostome. This got buried in the review queue.

compiler/deSugar/Check.hs
1287

Indeed, can you reiterate the idea stated in the ticket here?

This revision now requires changes to proceed.Oct 3 2017, 10:37 AM
simonpj requested changes to this revision.Oct 4 2017, 2:56 AM

I am not sure, but it's possible that you are implementing the suggestion described in comment:8 and 9 of Trac #14135.

If so, great! But please please write a careful, independently-comprehensible Note that describes the problem and the solution. I put some effort into comment:6, which may be a useful starting point in your Note.

My approach to implement what is discussed in comments 8 and 9 is to filter out in allCompleteMatches the sets for which tcMatchTy returns nothing. However, I've run into some problems because some other unrelated tests fail with a GHC panic!. I'm a bit puzzled as some of the failing tests do not seem to use allCompleteMatches at all.

To help us figure out what might be going on, can you post the output of one or two of the failing tests?

For example for the test testsuite/tests/typecheck/should_compile/T13822.hs the panic is:

[1 of 1] Compiling T13822 ( testsuite/tests/typecheck/should_compile/T13822.hs, testsuite/tests/typecheck/should_compile/T13822.o )
ghc-stage2: panic! (the 'impossible' happened)

(GHC version 8.3.20170921 for x86_64-unknown-linux):
      ASSERT failed!
Bad coercion hole {a1fm}: (I (x_a1di |> Sym (Ty
                                               (Sym cobox_a1fd))_N) |> D:R:IK[0])
                          (I (x_a1di |> Sym (Ty (Sym cobox_a1fd))_N) |> D:R:IK[0])
                          nominal
                          <(I (x_a1di |> Sym (Ty (Sym cobox_a1fd))_N) |> D:R:IK[0])>_N
                          I (x_a1di |> Sym (Ty (Sym cobox_a1fd))_N)
                          I (x_a1di[ssk:3] |> Sym (Ty (Sym cobox_a1fd))_N)
                          nominal
Call stack:
    CallStack (from HasCallStack):
      callStackDoc, called at compiler/utils/Outputable.hs:1195:22 in ghc:Outputable
      assertPprPanic, called at compiler/typecheck/TcMType.hs:305:105 in ghc:TcMType
Call stack:
    CallStack (from HasCallStack):
      callStackDoc, called at compiler/utils/Outputable.hs:1144:37 in ghc:Outputable
      pprPanic, called at compiler/utils/Outputable.hs:1193:5 in ghc:Outputable
      assertPprPanic, called at compiler/typecheck/TcMType.hs:305:105 in ghc:TcMType

Please report this as a GHC bug: http://www.haskell.org/ghc/reportabug

austin resigned from this revision.Nov 9 2017, 5:39 PM
dfeuer commandeered this revision.Dec 11 2017, 7:27 AM
dfeuer added a reviewer: carlostome.
dfeuer added a subscriber: dfeuer.

Oh, what fools we mortals be! @carlostome, your assertion failures seem only to happen

dfeuer updated this revision to Diff 14909.Dec 11 2017, 7:27 AM
  • Add a couple comments

The slow validation failures @carlostome saw had nothing whatsoever to do with this patch. I
have started working on pinning down where they showed up, but I haven't gotten far yet.

bgamari accepted this revision.Dec 11 2017, 2:37 PM

Alright, then then I suppose we can move ahead with this. Thanks @carlostome!

This revision was automatically updated to reflect the committed changes.