Holes in GHC

[Thijs Alkemade]

Hi Simon,

On Tue, Aug 21, 2012 at 7:14 PM, Simon Peyton-Jones
<simonpj at microsoft.com> wrote:
> Can you give me read/write access to your github repo?  I'm simonpj on github.
> That way I can add comments/questions in code, and generally clean up.
> It would make things easier if you could merge with HEAD so that I don't have to mess around moving libraries back in time.

Done. I've merged with HEAD as of right now. The only subrepository
I've forked is the testsuite repository, which you can find on
https://github.com/xnyhps/testsuite.

I've tried to get validate to pass all tests, I had to fix this one:
https://github.com/xnyhps/testsuite/commit/5016899250ace0d2db227569073ad83573909728,
and I've added this
https://github.com/xnyhps/testsuite/commit/47f1bb7f7c8aced6e59f8d3e2413f6c6c5b6016e
as a test case for holes. It's not a very complicated example of
holes, but I'm a bit unsure how to properly write a test case for it.
If you have some ideas to do this better, I'd be interested.

Some other tests were reporting to be too (in)efficient, but compared
to running validate on HEAD without my patch it seemed the same thing
happened there, so I haven't look further into those.

>
> ------------------------------
> You've put "LANGUAGE Holes" in TcErrors which means I can't bootstrap.
>

Sorry, that was sloppy, I've fixed it.

> ------------------------------
> You have this in your patch file, which can't be right
> +  | CHoleCan {
> +      cc_ev       :: CtEvidence,
> +      cc_hole_ty  :: TcTauType, -- Not a Xi! See same not as above
> +      cc_depth    :: SubGoalDepth        -- See Note [WorkList]
> +    }
> +
>  \end{code}
>
>  \begin{code}
> @@ -933,6 +940,9 @@ ctPred (CTyEqCan { cc_tyvar = tv, cc_rhs = xi })
>  ctPred (CFunEqCan { cc_fun = fn, cc_tyargs = xis1, cc_rhs = xi2 })
>    = mkTcEqPred (mkTyConApp fn xis1) xi2
>  ctPred (CIrredEvCan { cc_ty = xi }) = xi
> +ctPred (CHoleCan { cc_flavor = fl, cc_hole_ty = xi })
> +  = xi
> +
>
> since c_flavor isn't a field of CHoleCan.
>

This code is gone after merging with HEAD.

> -----------------------
>
> | The 3 currently remaining issues:
> |
> | - Free type variables are not tidied consistently. For every one of
> | these hole warnings, the same TidyEnv is reused, without taking the
> | updates from the other holes into account. I'm pretty sure I know where
> | this happens and how I could fix it.
>
> This should be easy.
> * TcErrors.reportUnsolved uses tyVarsOfWC to find the free type variables
>   of unsolved constraints
> * It then uses tidyFreeTyVars to assign them names
> * And that gives an env used in tidying.
>
> So it should just work.  I hope you are letting the various 'tidy' calls in TcErrors do the work.
>

I traced the problem back to not zonking the hole constraint properly,
reporting them now works as it should (I've removed the hack I was
using, and cleaned up some code that didn't need to be monadic
anymore).

(The reason they weren't zonked properly is that I've given zonkCt a
special case for CHoleCans
(https://github.com/xnyhps/ghc/blob/eee4242df72cf0a7910e896c3bd6d0fb20b97e37/compiler/typecheck/TcMType.lhs#L619),
as holes themselves do not carry enough information for me to turn
them from CNonCanonicals back into CHoleCans (they become
CIrredEvCans). I'm still not sure this is the right thing to do here.)

> | - What I thought would be the local environment doesn't actually seem
> | to be it. The holes store in their origin the result of `getLclTypeEnv'
> | at their location, but as the Note [Bindings with closed types] says,
> | the TopLevelFlag of these don't actually differentiate the top level
> | from the non-top level bindings. I think it would be more helpful to
> | only show the non-top level bindings at the hole's location, any hints
> | about how to obtain just these would be appreciated.
>
> In this context "local" means "this module" rather than "not top level".  Use isExternalName to distinguish top-level things from nested things.
>

This works now, thanks!


> | - The holes do not have very accurate source location information, like
> | some other errors have. The hole has its origin, ("test2.hs:3:16"), but
> | somehow not something like: "In the expression: folder _ x _, In an
> | equation for `test': test x = foldr _ x _". Help with how that is
> | supposed to work would also be appreciated.
>
> That's odd.  Every Wanted constraint has a WantedLoc (TcRnTypes), which includes a [ErrCtxt], which is that stack of "In ..; In... " stuff you see.
>
> Code looks plausible.

I've fixed the way I was constructing the CtLoc, this was wrong and
was always using [] as the [ErrCtxt].


Regards,
Thijs Alkemade