WIP on Doing a combined Step 1 and 3 for Trees That Grow
ClosedPublic

Authored by alanz on Nov 1 2017, 10:01 AM.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
alanz created this revision.Nov 1 2017, 10:01 AM

I had a quick look through this just to get a sense for what is involved. I had a few questions along the way.

compiler/hsSyn/HsExpr.hs
803

Will this ultimately vanish?

compiler/hsSyn/HsExtension.hs
62

This should likely be a Note reference.

alanz added inline comments.Nov 2 2017, 1:14 PM
compiler/hsSyn/HsExpr.hs
803

Ultimately it will be used for TTG for HsExpr.

But given the staging of the work, it should probably go from this diff.

compiler/hsSyn/HsExtension.hs
62

Agree. Except that we seem to have settled on using PlaceHolder and the function noExt :: PlaceHolder instead.

Only half way

compiler/deSugar/DsUtils.hs
808–811

VarPat {}, ditto WildPat

compiler/hsSyn/HsBinds.hs
129

Nothing here gives any clue that this is intended for the output of the renamer. And typechecker I think.

Plus I wonder if we'd be better served by

data HsValBindsLR idL idR
 = ValBinds
      [(RecFlag, LHsBindsLR idL idR)]
     [LSig GhcRn]

Then the parser can generate a giant singleton Rec and the renamer can sort it out. Less fuss.

169

Don't understand. Looks as if XValBinds has only one possible equation, and so could be a type synonym. So why have it at all?

compiler/hsSyn/HsDecls.hs
969

UserTyVar {}

compiler/hsSyn/HsExpr.hs
803

I don't believe in these "quantified over c" synonyms. Let's nuke and re-introduce if needed

alanz added inline comments.Nov 3 2017, 11:00 AM
compiler/hsSyn/HsBinds.hs
129

I agree. I have not really tackled binds as a whole yet, just put in the minimum to keep it compiling.

169

My understanding of what we are trying to do is to fairly mechanically add extension points throughout the AST, so that they are available as a facility for users both within and outside GHC.

compiler/hsSyn/HsExpr.hs
803

They are part of the Trees that Grow paper, and with the current state of implementation are needed.

I tried to remove OutputableX, but ended up having to put the individual constraints in, which started getting very messy.

I would like to revisit this once TTG is fully worked in, all the PostRn, PostTc are gone, and the key parts use a GhcPass _ index.

shayan-najd accepted this revision.Nov 4 2017, 11:45 AM

I have taken a look. It all seems as expected and rather mechanical. There are some bits sticking out, those that Simon and Ben commented on, that I believe are some side-effects of splitting the big patch into multiple steps: the treatment of binders sticks out that can be treated in the next step and there are some bits that can be done when we update HsExpr as well.

I believe, considering my limited understanding of how GHC development works, we should push this patch as soon as possible after
(a) the style improvements that Simon suggested,
(b) removing the commented part related to HsExpr, and
(c) possibly renaming PlaceHolder to `NoExt.

compiler/hsSyn/HsExtension.hs
62

We can change`PlaceHolder` to NoExt as well.
I believe in having two separate terminators NoFieldExt and NoConExt, but if we don't need the latter, then NoExt would do.

This revision is now accepted and ready to land.Nov 4 2017, 11:45 AM
alanz marked 4 inline comments as done.Nov 5 2017, 1:58 PM
alanz added inline comments.
compiler/hsSyn/HsBinds.hs
129

I just tried to do this with the current state of TTG and it got very complicated.

I would rather bring in TTG as a unitary change, so that it is simple to verify initially, and then make other changes such as this.

It is already such a big patch, the more mechanical it can be kept, the better.

alanz updated this revision to Diff 14564.Nov 5 2017, 2:20 PM

Updating from comments on phabricator

simonpj accepted this revision.Nov 6 2017, 7:39 AM

Generally looks good.

Could you update https://ghc.haskell.org/trac/ghc/wiki/ImplementingTreesThatGrow to reflect the current plan? It's out of date.

I don't like the "NewT" nomenclature for the "extra" data constructor for data type T. Could it be XT?

compiler/hsSyn/HsBinds.hs
169

But by giving a type instance like type instance F a b = blah you prevent anyone from extending it later. Ever.

Maybe you mean F (GhcPass a) (GhcPass b) = blah.

compiler/hsSyn/HsExtension.hs
105

Why not define these families with HsPat?

208

Why not define these functions with HsType?

simonpj added inline comments.Nov 6 2017, 7:40 AM
compiler/hsSyn/HsBinds.hs
129

OK but let's not lose track of it... even a ticket?

shayan-najd added a comment.EditedNov 6 2017, 11:30 AM

I don't like the "NewT" nomenclature for the "extra" data constructor for data type T. Could it be XT?

Sound idea, but in my implementation, I recall there were some name clashes: there were some constructors and datatypes sharing the same name.
If we want to use this XT naming scheme, then it makes sense to rename the constructors with the same name as an existing datatype. It's a good code quality improvement anyway.

P.S. Doesn't it also clash with the corresponding type family for new constructor extensions as well, e.g. ... | XPat (XPat x) ?

alanz added inline comments.Nov 6 2017, 1:12 PM
compiler/hsSyn/HsBinds.hs
129
169

Yes, this is one of the things I am working through that was not in Shayan's sample implementation.

And I guess I need to do a full audit through the diff to make sure that it has happened.

compiler/hsSyn/HsExpr.hs
803
compiler/hsSyn/HsExtension.hs
105

Because then the ForAllXPat constraint would need to go into hs-boot files.

Perhaps once https://ghc.haskell.org/trac/ghc/ticket/14429 is done they can move back.

208

Could you update https://ghc.haskell.org/trac/ghc/wiki/ImplementingTreesThatGrow to reflect the current plan? It's out of date.

I have updated the page a bit to reflect the current status.

bgamari requested changes to this revision.Nov 6 2017, 2:32 PM

Requesting changes as this isn't yet finished.

This revision now requires changes to proceed.Nov 6 2017, 2:32 PM
This revision was automatically updated to reflect the committed changes.