On Mar 29, 2017, at 2:31 PM, Ben Gamari wrote:

Hi Richard,

It took me a bit longer than expected to get back to D3316 but I think I am now done. It wasn't quite as trivial as I thought it might be, so I thought I would summarize what was needed,

1. tcSplitTyConApp_maybe and tcRepSplitTyConApp_maybe have been moved from TcType to Type to avoid import loops. I'm not terribly happy with this change, but I spent a fair amount of time looking for alternatives and it seems this is the least evil. Moreover, there is precedent for this in tcRepSplitAppTy_maybe. It would be nice to refactor TcType and friends to avoid this loop.

Why do this functions get involved? Where do they get called? Unify? If so, good catch!

2. TypeMap now respects the Constraint/Type distinction. I believe this is safe for the TypeMap uses in the typechecker itself. The only other use AFAICT is Specialise.CallKeySet, which I believe should also be fine under this change.

Another good catch.

3. TcInteract.matchTypeable now handles Constraint explicitly.

Hooray.

With all of this the T11715 test passes. I've uploaded my changes relative to your patch as D3396 for your review.

Thanks!! Will look tomorrow.

However, I am a bit concerned with (2) as it runs contrary to your original implementation, which changed coreViewOneStarKind to coreView. What was the reasoning behind this?

The reasoning was that we should consider (instance C Constraint) and (instance C Type) to be overlapping, so any kind of lookup should be over coreView types. This was before the T11480b bug was known and before the decision to let these instances not overlap. So I agree with your decision for (2).

The motivation for moving to tcView here is the dictionary cache, which uses a TypeMap. Namely, if we solve for `Typeable Type`, and later look for `Typeable Constraint` in the dictionary cache, we will incorrectly find the `Typeable Type` dictionary.

Thoughts?

That you've hit this one on the head. :) Richard

Cheers,

- Ben