Quantcast

D3316 status

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

D3316 status

Ben Gamari-2
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.

 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.

 3. TcInteract.matchTypeable now handles Constraint explicitly.

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

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 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?

Cheers,

- Ben

_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

signature.asc (497 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: D3316 status

Richard Eisenberg-4

> On Mar 29, 2017, at 2:31 PM, Ben Gamari <[hidden email]> 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

_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Loading...