Fix #15845 by defining etaExpandFamInstLHS and using it

Authored by RyanGlScott on Nov 2 2018, 9:40 AM.



Both Trac #9692 and Trac #14179 were caused by GHC being careless
about using eta-reduced data family instance axioms. Each of those
tickets were fixed by manually whipping up some code to eta-expand
the axioms. The same sort of issue has now caused Trac #15845, so I
figured it was high time to factor out the code that each of these
fixes have in common.

This patch introduces the etaExpandFamInstLHS function, which takes
a family instance's type variables, LHS types, and RHS type, and
returns type variables and LHS types that have been eta-expanded if
necessary, in the case of a data family instance. (If it's a type
family instance, etaExpandFamInstLHS just returns the supplied type
variables and LHS types unchanged).

Along the way, I noticed that many references to
Note [Eta reduction for data families] (in FamInstEnv) had
slightly bitrotted (they either referred to a somewhat different
name, or claimed that the Note lived in a different module), so
I took the liberty of cleaning those up.

Test Plan

make test TEST="T9692 T15845"

RyanGlScott created this revision.Nov 2 2018, 9:40 AM
goldfire added inline comments.Nov 2 2018, 11:22 AM

Add (e.g., pretty-printing), just to give an idea of the flavor of these "certain situations".



I find it utterly bizarre that this doesn't also return the RHS. Looking at call sites, I get how this is possible -- namely, that the RHS is always mkTyConApp tycon (mkTyVarTys tc_tvs) (according to the variable names in this function). But it's still somewhat unsettling.

Might be at least worth a comment stating why this isn't necessary.

RyanGlScott marked 3 inline comments as done.

Good point. I've added some commentary to this effect.

I have a hunch that this might also fix Trac #15852.

I have a hunch that this might also fix Trac #15852.

This patch doesn't fix Trac #15852 as-is, although I could certainly believe that using etaExpandFamInstLHS in the right place might very well fix it. Do you have an idea of where that right place might be in the codebase?

I've commented on the ticket, as that seems a better place than here.

I've commented on the ticket, as that seems a better place than here.

If my musings at are accurate, then it looks like fixing Trac #15852 won't be as simple as applying etaExpandFamInstLHS in another location, unfortunately.

In light of this, I'm inclined to label this Diff as being ready to merge. Is there anything else that should be done here?

goldfire accepted this revision.Nov 5 2018, 12:31 PM

Not from me. Ship it!

This revision is now accepted and ready to land.Nov 5 2018, 12:31 PM
This revision was automatically updated to reflect the committed changes.
RyanGlScott marked an inline comment as done.