Typed holes support in Template Haskell
AbandonedPublic

Authored by jstolarek on Apr 10 2015, 3:45 AM.

Details

Reviewers
simonpj
austin
goldfire
bgamari
Trac Issues
#10267
Summary

Add full support for typed holes in Template Haskell. This means both coverting typed holes inside splices into TH AST as well as generating typed holes from TH AST.

Test Plan
{-# LANGUAGE TemplateHaskell #-}

module T10267 where

import Language.Haskell.TH

[d| i :: forall a. a -> a
    i x = _ |]

$(return [
   SigD (mkName "j")
        (ForallT [PlainTV (mkName "a")]
                 []
                 (AppT (AppT ArrowT (VarT (mkName "a"))) (VarT (mkName "a"))))
 , FunD (mkName "j")
        [Clause [VarP (mkName "x")] (NormalB (UnboundVarE (mkName "_"))) []]
 ]

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Branch
master
Lint
Lint ErrorsExcuse: #10979
SeverityLocationCodeMessage
Errordocs/users_guide/glasgow_exts.rst:10629MERGECONFLICT1Unresolved merge conflict
Unit
No Unit Test Coverage
Build Status
Buildable 6054
Build 6785: GHC Patch Validation (amd64/Linux)
Build 6784: arc lint + arc unit
jstolarek retitled this revision from to Typed holes support in Template Haskell.Apr 10 2015, 3:45 AM
jstolarek updated this object.
jstolarek edited the test plan for this revision. (Show Details)
jstolarek added reviewers: goldfire, simonpj.
jstolarek updated the Trac tickets for this revision.

This patch adds preliminary support for typed holes in Template Haskell but it does not work as intended yet. There are problems with handling names. If I say

[d| i :: forall a. a -> a
    i x = _ |]

I get

Illegal variable name: ‘_’
When splicing a TH declaration: i_0 x_1 = __1627391561

This is because _ is a reserved identifier (per Lexeme.reservedIds) and variable names can't be reserved identifiers - see inline comment for details.

So suppose I say this instead:

[d| i :: a -> a
    i x = _x |]

This is also a hole because the name starts with an underscore. Now I get this error:

The exact Name ‘_x’ is not in scope
  Probable cause: you used a unique Template Haskell name (NameU),
  perhaps via newName, but did not bind it
  If that's it, then -ddump-splices might be useful

I believe this error originates from TcSplice. It is obviously correct - _x is not in scope as it is not bound anywhere! I don't have any idea how to handle this.

I'm not sure if the approach of changing HsExpr = ... | HsUnboundVar RdrName to HsExpr = ... | HsUnboundVar id is a good one. I imagine that with enough mangling I could handle RdrName in DsMeta. But I don't think this would solve the above problems.

Thoughts?

compiler/deSugar/DsMeta.hs
1171

This exact line was the motivation to change HsUnboundVar constructor to be parametrized by id rather than always store RdrName - DsMeta works on Names not RdrNames.

compiler/hsSyn/Convert.hs
718

If I say

[d| i :: a -> a
    i x = _ |]

this is where the _ variable name gets rejected as invalid. vName calls cvtName, which calls okOcc to check whether the variable name is valid. okOcc calls Lexeme.okVarOcc, which calls Lexeme.okVarIdOcc and that returns False because _ is member of Lexeme.reserved.Ids.

compiler/hsSyn/HsExpr.hs
505

This is an important change in data representation.

compiler/rename/RnExpr.hs
102–105

Instead of re-using RdrName I generate a Name with new unique.

381–382

Another place that creates a Name instead of RdrName. Is it correct to assume that name for a hole is an internal name?

austin requested changes to this revision.May 10 2015, 6:18 PM

(Requesting changes to move out of need-review queue.)

This revision now requires changes to proceed.May 10 2015, 6:18 PM
goldfire requested changes to this revision.Jul 13 2015, 12:43 PM

See comments inline.

compiler/deSugar/DsMeta.hs
1171

This code is working too hard! By the fact that you have an HsUnboundVar, you know that looking up the variable isn't going to get you useful information. Just build a TH.Name using the NameS (rather uninformative) name flavour.

Should be able to work just fine with RdrName.

2239

NB: Since you started this patch, all of the TH names have moved to a new THNames module, in the prelude folder.

compiler/hsSyn/Convert.hs
718

How is this treated in the parser? As noted in Trac #10583, don't trust anything Lexeme says. If the parser creates a HsVar with a _-valued RdrName in it, so can you. If not, do whatever the parser does.

compiler/hsSyn/HsExpr.hs
505

I don't think this change is required.

compiler/rename/RnExpr.hs
102–105

Remove this code, as you don't have to change HsUnboundVar.

305

Can undo this change.

381–382

You can undo this change.

388

And undo this, too.

libraries/template-haskell/Language/Haskell/TH/Syntax.hs
1421

It's not immediately obvious to me that we need a new UnboundVarE. You could just use VarE and ConE. The idea is that DsMeta would know the difference between an unbound variable and a bound one. If we have UnboundVarE, then DsMeta could pass on this tidbit of knowledge into the TH AST.
(I don't see any advantage to an UnboundConE separate from UnboundVarE. For that matter, I'm not sure if the distinction between ConE and VarE pays its weight, but that's a worry for another day.)

But, in Convert, VarE and UnboundVarE should be treated identically, because the whole lot is going to go through the renamer again, anyway.

I guess I've convinced myself that UnboundVarE is fine, but it really should be the same in Convert.

jstolarek updated this revision to Diff 4126.Sep 8 2015, 12:40 PM

Patch rewrite against HEAD.

I improved the implementation in DsMeta - hopefully it does the right thing now. Sadly, I still don't know how to fix the problem in Convert module. When the parser sees a variable name starting with an underscore it just creates an HsVar. Then I think the crucial part is in RnExpr, where we have:

rnExpr (HsVar v)
  = do { mb_name <- lookupOccRn_maybe v
       ; case mb_name of {
           Nothing -> rnUnboundVar v ;

rnUnboundVar :: RdrName -> RnM (HsExpr Name, FreeVars)

So basically when the lookup of variable name fails it is converted to an unbound variable. Not sure how to proceed here. One idea I had (and implemenbted in this patch) was changing the definition of Convert.okOcc to accept variable names starting with an underscore. This works when I say:

[d| i :: a -> a
    i = _ |]

but if I instead say:

[d| i :: a -> a
    i = _foo |]

then _foo is reported as being out of scope in a TH quotation. I have not yet debugged this - it was a quick hack that I came up with when writing this summary of changes. If this is the right way to proceed then I will try to debug this. Guidance appreciated.

Hopefully my inline comments are helpful.

compiler/deSugar/DsMeta.hs
1170

Better not to do MkC matching like this -- it defeats some of the very moderate type safety afforded by the whole Core construction. Instead, write more helper functions with type signatures.

compiler/hsSyn/Convert.hs
1235

Eek. This means that _blah will be accepted as a constructor! No. Instead, I would modify okVarOcc to accept leading underscores, including the string "_". Due to the chaos that is Lexeme.hs, you should probably look around for other uses of okVarOcc to make sure you're not breaking anything. And comment why _ is a valid identifier name.

But I agree with this broad approach. Keep plowing forward. I imagine the code that attempts to convert [| foo |] into [| $(lift foo) |] (when foo is local) is to blame for the problem with _foo in a quote.

libraries/template-haskell/Language/Haskell/TH/Syntax.hs
1421

Since we're keeping UnboundVarE, it needs a little documentation. Basically: quotes may be converted to TH with UnboundVarE, as a help to any processing thereafter to know that the variable is unbound. But VarE and UnboundVarE are treated equally in a splice. So VarE (mkName "_hole") is just fine.

(This is somewhat the converse of UInfixE and InfixE, where the former happens only in input [splices] but never output [quotes].)

austin requested changes to this revision.Sep 21 2015, 5:32 PM

(Moving back into 'Request Changes' pending @goldfire's comments.)

This revision now requires changes to proceed.Sep 21 2015, 5:32 PM
jstolarek updated this revision to Diff 4343.Sep 28 2015, 8:38 AM
  • WIP on adding typed holes to TH
  • Hack okOcc to accept names starting with _
  • Permit underscore as TH identifier
  • Refactor DsMeta per Richard's suggestions
jstolarek marked 2 inline comments as done.Sep 28 2015, 9:06 AM
jstolarek added inline comments.
compiler/basicTypes/Lexeme.hs
143–146

Here we admit _ as an identifier. This shouldn't affect anything but Template Haskell.

compiler/deSugar/DsMeta.hs
1170

Refactored. Is this better?

compiler/hsSyn/Convert.hs
1235

I modified Lexeme.hs per your suggestion.

I imagine the code that attempts to convert [| foo |] into [| $(lift foo) |] (when foo is local) is to blame for the problem with _foo in a quote.

I looked around that piece of code and it does not look like it is responsible. Output of -ddump-rn-trace is unfortunately quite sparse but it leads me to believe that the sequence of calls leading to that error goes like this: rnBracket -> rn_bracket (DecBrL case) -> rnSrcDecls. rnSrcDecls is quite large and I have not yet pin-pointed the place where this error would happen. It is also not clear to me why this error gets raised when we rename a bracket but when rnSrcDecls is called on a top-level declaration everything is fine.

I'm pasting output of -ddump-rn-trace below:

[1 of 1] Compiling T10267           ( T10267.hs, T10267.o )
rn1 []
rn1: checking family instance consistency
rn1a
rn12
getLocalNonValBinders 1 []
extendGlobalRdrEnvRn 2 GlobalRdrEnv (locals only) {
                         }
getLocalNonValBinders 2 []
extendGlobalRdrEnvRn 2 GlobalRdrEnv (locals only) {
                         }
extendGlobalRdrEnvRn 2 GlobalRdrEnv (locals only) {
                         }
rnSrcDecls []
extendGlobalRdrEnvRn 2 GlobalRdrEnv (locals only) {
                         }
Start rnTyClDecls
rnTycl []
       []
Start rnmono
finish rnmono
finish rnSrc
finish Dus [(Just [], []), (Nothing, []), (Nothing, [])]
rn13
extendGlobalRdrEnvRn 2 GlobalRdrEnv (locals only) {
                         }
addUsedRdrName 2 []
Renaming untyped TH bracket
getLocalNonValBinders 1 []
extendGlobalRdrEnvRn 2 GlobalRdrEnv (locals only) {
                         }
getLocalNonValBinders 2 []
extendGlobalRdrEnvRn 2 GlobalRdrEnv (locals only) {
                         }
extendGlobalRdrEnvRn 2 GlobalRdrEnv (locals only) {
                         }
rnSrcDecls [i_aqA]
extendGlobalRdrEnvRn 2 GlobalRdrEnv (locals only) {
                         i (unique iIb): i_aqA defined at T10267.hs:18:5 }
Start rnTyClDecls
rnTycl []
       []
Start rnmono
bhtv [a]
     []
     []
     []
     []
     []
     LocalRdrEnv {
       env = []
       in_scope = {} }
     LocalRdrEnv {
       env = [vIB :-> (vIB) a_aqB]
       in_scope = {a_aqB} }
finish rnmono i_aqA :: a_aqB -> a_aqB
              i_aqA = _foo_02A
finish rnSrc i_aqA :: a_aqB -> a_aqB
             i_aqA = _foo_02A
finish Dus [(Just [], []), (Just [aqA :-> i_aqA], []),
            (Nothing, []), (Nothing, [])]
rn_bracket dec [(Just [], []), (Just [aqA :-> i_aqA], []),
                (Nothing, []), (Nothing, [])]
               []

T10267.hs:18:9: error:
    Not in scope: ‘_foo’
    In the Template Haskell quotation
      [d| i :: a -> a
          i = _foo |]

Take a look at RnExpr, line 87. I think that may be the problem.

Take a look at RnExpr, line 87. I think that may be the problem.

Spot-on :-) Untyped splices and brackets now work correctly. Typed Template Haskell support is blocked by Trac #10945 and Trac #10946. I tried debugging these two but failed - see my comments on these tickets. it would be great to have them fixed with this patch here but I need guidance.

Those two tickets suggest that typed Template Haskell is behind the curve, anyway. I would say to post a new ticket saying that typed holes are panicky in typed Template Haskell and then forge onward with this patch. If we just got them working in normal TH, that would still be a nice thing to have.

I would say to post a new ticket saying that typed holes are panicky in typed Template Haskell

That's essentially what Trac #10946 says (I think). I'll try to upload the final patch within a week.

Indeed it does. Thanks.

jstolarek updated this revision to Diff 4457.Oct 9 2015, 7:11 AM

Rebase against master

jstolarek marked 11 inline comments as done.Oct 9 2015, 9:51 AM

I rebased my patch on top of master branch. Please review.

One thing came as a surprise. During validation I got one unexpected failure from T1476b test. Allow me to quote the source of the test:

baz = [| \ $( return $ VarP $ mkName "x" ) -> x |]

-- If this test starts passing, nested pattern splices scope correctly.
-- Good for you! Now, update the TH manual accordingly

Richard, this test was added by you last year (c5cf81fcce05ec160edc5be907297ff05c33). Is this a knock-on effect of changes in RnExpr.rnUnboundVar? That's the only explanation that comes to my mind. Are we happy with that?

compiler/typecheck/TcRnTypes.hs
741–744

I'm actually thinking about turning this Bool into an isomorphic data type with more informative constructor names. Thoughts?

goldfire requested changes to this revision.Oct 12 2015, 9:42 AM

I also want to see a test where a plain unbound variable is quoted, but not then immediately spliced. I would want such a thing to produce no warning. (I think.)

Pending these minor changes, I accept. Thanks, Janek!

compiler/typecheck/TcRnTypes.hs
741–744

That is almost always a helpful refactoring.

docs/users_guide/7.12.1-notes.rst
131

Expand this a bit. Perhaps mention UnboundVarE. But definitely mention that unbound variables are now quotable. This is a big deal! I've seen singletons code that defines a whole lot of undefined things at the term level just so that later promoteOnly (or singletonsOnly) code would work. With the change you're implementing, those useless term-level definitions won't need to be there.

This revision now requires changes to proceed.Oct 12 2015, 9:42 AM

By "quoted but not immediately spliced" do you mean something like this:

$(foo [e| _x |] )

?

What about the behaviour of T1476b? Is that correct?

I've seen `singletons` code that defines a whole lot of `undefined` things at the term level just so that later `promoteOnly` (or `singletonsOnly`) code would work.

You mean client code or the code in the library? Can you point me to it?

jstolarek updated this revision to Diff 4515.Oct 16 2015, 10:40 AM
  • Refactor RnSplice
  • Refactor Splice constructor in ThStage
  • Update User's Guide
  • Update Testsuite
  • Fix warnings
  • Fix User's Guide typos

I believe I addressed all the issues raised in the recent review:

  • I updated the User's Guide to reflect the change in behaviour introduced by this patch. Long story short: we can now have nested pattern splices because TH only checks whether a variable is bound when a quotation is spliced.
  • I updated the Release Notes to mention the above and quoting of unbound variables.
  • I refactored Splice constructor of ThStage data type to use a specialized dat atype rather than a Bool
  • I created a test that quotes an unbound variable in one module and then splices that quote in another module.
  • Some other refactoring (ptext . sLit ==> text)

Validation passes cleanly on my machine. Please review. I hope this gets merged soon :-)

Note to self: it looks like Note [rnSplicePat] in RnSplice also needs updating but I need to speak to Richard first. I don't fully understand why nested pattern splices work. Let's say I have:

baz = [| \ $foo -> x |]

My understanding is that the renamer renames the quote and turns x into an HsUNboundVar. But then $foo might bring x into scope in which case x in the RHS really is in scope and thus should be a HsVar. I tested this and the behaviour is as desired: if $foo does not bring x into scope an error is reported, and if it does bring x into scope then everything works. Still, I don't understand how that happens.

goldfire accepted this revision.Oct 16 2015, 1:02 PM

Hooray! Thanks!

Yes, I'm happy with T1476b.

And the singletons comment was about client code. But it affects the library, too. Note that the updated Data.Singletons.Prelude.List module has to import Data.Maybe just to get listToMaybe to pass the renamer. After this patch, that import wouldn't be necessary.

Nested pattern splices: An HsUnboundVar becomes a TH.UnboundVarE with a mkName ... name. When that mkName ... name is spliced back in, renaming happens, because TH syntax gets renamed during splicing. At that point (essentially, the code's second trip through the renamer), everything is fine. The renamer resolves mkName ... names using lexical scoping and all is good.

jstolarek updated this revision to Diff 4520.Oct 16 2015, 2:20 PM

Final patch merged with HEAD

jstolarek abandoned this revision.Oct 16 2015, 2:21 PM

This is now merged into master. Abandoning revision.

jstolarek reclaimed this revision.Oct 20 2015, 1:33 PM

Reclaiming revision just to close it propery (becuase Phab seems to allow that now).

jstolarek abandoned this revision.Oct 20 2015, 1:34 PM

Nope, it doesn't... sorry for unnecessary noise.

If you leave the line Differential Revision: https://phabricator.haskell.org/Dxxxx in the commit message, Phabricator will close the Diff by itself. You don't have to use arc land or whatever at all.