Fix #15852 by eta expanding data family instance RHSes, too
ClosedPublic

Authored by RyanGlScott on Nov 12 2018, 6:10 AM.

Details

Summary

When I defined etaExpandFamInstLHS, I blatantly forgot
to eta expand the RHSes of data family instances. (Actually, I
claimed that they didn't need to be eta expanded. I'm not sure
what I was thinking.)

This fixes the issue by changing etaExpandFamInstLHS to
etaExpandFamInst and, well, making it actually eta expand the RHS.

Test Plan

make test TEST=T15852

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.
RyanGlScott created this revision.Nov 12 2018, 6:10 AM
RyanGlScott added inline comments.Nov 12 2018, 6:11 AM
testsuite/tests/partial-sigs/should_compile/DataFamilyInstanceLHS.stderr
9

The use of a wildcard on the RHS here is rather weird. I'm not sure how to fix this.

goldfire requested changes to this revision.Nov 12 2018, 8:54 AM
goldfire added inline comments.
compiler/types/Type.hs
3071

Ah. That's better. As you know, I found the previous treatment puzzling.

testsuite/tests/indexed-types/should_compile/T15852.stderr
8

This still doesn't quite seem right. Where is k2? The code in tcDataFamInstDecl uses mkTyConBindersPreferAnon, which should always make Required binders. So the k2 should appear here. I do think this is a separate bug from the original one, but one that falls under Trac #15852 nonetheless.

testsuite/tests/partial-sigs/should_compile/DataFamilyInstanceLHS.stderr
9

Bah. That's rather infelicitous. Should we whip up names? It's conceivable to do so right in the pretty-printer, as each wildcard should have its own unique, no?

This revision now requires changes to proceed.Nov 12 2018, 8:54 AM
RyanGlScott added inline comments.Nov 14 2018, 1:58 AM
testsuite/tests/indexed-types/should_compile/T15852.stderr
8

All of the binders are in fact Anon/Required—it turns out that the real culprit is tidying. If you print this with -dppr-debug enabled, then you'll see that T15852.R:DFProxyProxy k1 k c j a is actually meant to be T15852.R:DFProxyProxy k1 k2 c j a. The only reason it's displayed as it is without -dppr-debug is that pprType (which pretty-prints the RHS type) tidies the type before pretty-printing, and for whatever reason, that turns k2 into k. Ugh.

Is there a way to tell the tidier not to do this?

testsuite/tests/partial-sigs/should_compile/DataFamilyInstanceLHS.stderr
9

Yes, I suppose we could. But I doubt we'd want to always pretty-print the unique, since there are situations in which printing just the wildcard would be nicer (e.g., if it's just data instance Sing _, printing a wildcard looks arguably nicer than printing, say, data instance Sing _123). I'm not sure what the best way to configure this would be, though...

goldfire added inline comments.Nov 14 2018, 10:31 AM
testsuite/tests/indexed-types/should_compile/T15852.stderr
8

It probably does this because the names are system names, not user names. In any case, this is an outright bug here. The TidyEnv needs to be shared with the forall, the LHS, and the RHS. I don't know off-hand how to arrange for this, but that should fix the problem.

testsuite/tests/partial-sigs/should_compile/DataFamilyInstanceLHS.stderr
9

As a strawman that would fix this problem: whip up names for any wildcards found by eta-expansion. Those are surely mentioned on the RHS. More generally, we could look for free variables on the RHS and make sure those are all named.

RyanGlScott marked 8 inline comments as done.
testsuite/tests/indexed-types/should_compile/T15852.stderr
8

Indeed, we were using entirely different pretty-printing functions for the LHS and RHS which did not share a TidyEnv. I've now fixed this.

testsuite/tests/partial-sigs/should_compile/DataFamilyInstanceLHS.stderr
9

I found a relatively nice way to fix this: just always add numeric suffixes to types named "_" during tidying. See Note [Always number wildcard types when tidying] in OccName.

Rebase on top of master

  • Fix warning
This revision is now accepted and ready to land.Nov 18 2018, 11:00 PM
This revision was automatically updated to reflect the committed changes.