Move eta-reduced coaxiom compatibility handling quirks into FamInstEnv.
ClosedPublic

Authored by mniip on Oct 5 2018, 11:22 AM.

Details

Summary

The quirk caused an issue where GHC concluded that 'D' is possibly
unifiable with 'D a' (the two types could have the same kind if D is a
data family).

Test Plan

Ensure T9371 stays fixed.
Introduce T15704

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.
mniip created this revision.Oct 5 2018, 11:22 AM
mniip updated this revision to Diff 18238.Oct 5 2018, 11:27 AM

Add squiggly line

I like this fix. Minor wibbles suggested below. Thanks!

compiler/types/FamInstEnv.hs
587–589

This idiom makes me shudder for some reason. Maybe give it a name and put it in Util?

compiler/types/Unify.hs
1028

Might be worth writing out an example here, or at least point to the ticket.

mniip added inline comments.Oct 5 2018, 3:38 PM
compiler/types/FamInstEnv.hs
587–589

Would you rather like commonlhs1 = zipWith const lhs1 lhs2; commonlhs2 = zipWith const lhs2 lhs1? I did think about this for a bit and didn't find either particularly idiomatic.

mniip added inline comments.Oct 5 2018, 4:25 PM
compiler/types/FamInstEnv.hs
587–589

I dug around in Util and found takeList :: [b] -> [a] -> [a] equivalent to zipWith (const id). Would you like that to be used here instead? Or do you think we need exactly a [a] -> [b] -> ([a], [b]) util?

goldfire added inline comments.Oct 7 2018, 9:33 PM
compiler/types/FamInstEnv.hs
587–589

I thought of suggesting takeList, but you'd need to takeList twice to get what you want. So it seems more efficient to do unzip . zip than to use existing utilities. Given that the function has a sensible, standalone specification, I think it's OK to put in Util even if the function currently has only one use site.

mniip updated this revision to Diff 18266.Oct 8 2018, 5:19 PM

Add a zipAndUnzip util. Add a reference to T15704

mniip updated this revision to Diff 18267.Oct 8 2018, 5:22 PM

Oops, messed up my working tree.

mniip updated this revision to Diff 18268.Oct 8 2018, 5:24 PM

That didn't work

Seeing as tests failed on both platforms looks like this has some performance implications.

You're worried about the haddock performance test? Don't be. I've seen that test failing for some time.

You're worried about the haddock performance test? Don't be. I've seen that test failing for some time.

Well then, any further comments on the patch?

Sorry about the Haddock perf test failing; that was my bad. That test has been corrected upstream (in 4eeeb51d5f51083d0ae393009a7fd246223e9791), so rebasing this commit on top of master will fix that issue.

goldfire accepted this revision.Oct 15 2018, 8:42 AM
In D5206#143964, @mniip wrote:

Well then, any further comments on the patch?

Nope. Looks good!

This revision is now accepted and ready to land.Oct 15 2018, 8:42 AM
This revision was automatically updated to reflect the committed changes.