Add support for ImplicitParams and RecursiveDo in TH
ClosedPublic

Authored by mgsloan on Mar 7 2016, 7:36 PM.

Details

Summary

This adds TH support for the ImplicitParams and RecursiveDo extensions.

I'm submitting this as one review because I cannot cleanly make the two commits independent.

Initially, my goal was just to add ImplicitParams support, and I found that reasonably straightforward, so figured I might as well use my newfound knowledge to address some other TH omissions.

Test Plan

Validate

Diff Detail

There are a very large number of changes, so older changes are hidden. Show Older Changes
goldfire added inline comments.Mar 7 2016, 9:15 PM
compiler/deSugar/DsMeta.hs
1484

Are you sure the two are the same? I'm not terribly familiar with -XRecursiveDo, but I seem to recall that GHC does some analysis to figure out which are the recursive binders. Have you tested a generate example, that uses rec but isn't actually recursive?

Or perhaps this analysis has already been done, and the RecStmt is already the minimal mutually recursive group.

1505

Why sort_by_loc?

compiler/hsSyn/Convert.hs
375

And why not? Is this still work-in-progress? But even if this doesn't work, it should never panic.

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

I'm not convinced this newtype is paying its weight. But if you feel strongly, I don't.

1723

Just to keep parallel with the rest of this declaration, would you mind switching to use -- ^ instead of -- |?

testsuite/tests/th/TH_recursiveDo.stdout
2

This output is going to be fragile. Look around for other TH tests to see how to do this. I think it involves -dsuppress-uniques, but I'm not sure off the top of my head.

This revision now requires changes to proceed.Mar 7 2016, 9:15 PM
mgsloan updated this revision to Diff 6870.Mar 10 2016, 1:45 AM
mgsloan edited edge metadata.

Addresses goldfire's feedback

  • Improve error handling for TH ImplicitParams
  • Make TH RecursiveDo test less fragile
  • Use a type syn for IPName instead of newtype
  • Add a test for invalid IP names in TH
  • Fix a couple warnings in hsSyn/Convert.hs
  • Fix syntax of a haddock comment
  • Add TH support for ImplicitParams constraints
mgsloan updated this revision to Diff 6871.Mar 10 2016, 2:05 AM
mgsloan marked 2 inline comments as done.
mgsloan edited edge metadata.
  • Improve error handling for TH ImplicitParams
  • Make TH RecursiveDo test less fragile
  • Use a type syn for IPName instead of newtype
  • Add a test for invalid IP names in TH
  • Fix a couple warnings in hsSyn/Convert.hs
  • Fix syntax of a haddock comment
  • Add TH support for ImplicitParams constraints
  • Clarify message when IP binds used in wrong place
mgsloan updated this revision to Diff 6872.Mar 10 2016, 2:07 AM
mgsloan edited edge metadata.
  • Improve error handling for TH ImplicitParams
  • Make TH RecursiveDo test less fragile
  • Use a type syn for IPName instead of newtype
  • Add a test for invalid IP names in TH
  • Fix a couple warnings in hsSyn/Convert.hs
  • Fix syntax of a haddock comment
  • Add TH support for ImplicitParams constraints
  • Clarify message when IP binds used in wrong place
mgsloan updated this revision to Diff 6873.Mar 10 2016, 2:25 AM
mgsloan marked an inline comment as done.
mgsloan edited edge metadata.
  • Improve error handling for TH ImplicitParams
  • Make TH RecursiveDo test less fragile
  • Use a type syn for IPName instead of newtype
  • Add a test for invalid IP names in TH
  • Fix a couple warnings in hsSyn/Convert.hs
  • Fix syntax of a haddock comment
  • Add TH support for ImplicitParams constraints
  • Clarify message when IP binds used in wrong place
  • Update haddock style on IPBindD constructor
mgsloan marked 2 inline comments as done.Mar 10 2016, 2:33 AM

Thanks for the quick and insightful feedback, @goldfire ! Not sure how I omitted the IParamT stuff, good catch.

I find DsMeta's approach somewhat hilarious. Clever and interesting, certainly, but there's gotta be a better way.

compiler/deSugar/DsMeta.hs
1484

The analysis is actually irrelevant to the scope rules of recursive do. The analysis splits up recursive binding groups. The "worst case of this is when a statement at the beginning refers to a variable bound right before the end. In that case, the whole thing is one big recursive binding group.

1505

I honestly don't know. I was emulating the code below. I think it's reasonable to assume that if val bindings needs sort_by_loc, then IP bindings also need sort_by_loc

compiler/hsSyn/Convert.hs
375

Good point! For TH ASTs produced by ast quotes, this should never happen. However, the user could totally provide a IPBindD as a top level dec. This now fails with a decent error message (and there's a test for that!)

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

Agreed. I added it because having String directly looked ugly. Going with a type synonym for documentation purposes.

1723

Done! Seems like there's some mixed usage of -- | and -- ^ for the same declaration, where the -- | is used for the more complex description. I'm emulating this for now, but note that the "-- |" isn't included in the haddock output.

mgsloan marked an inline comment as done.EditedApr 27 2016, 6:04 PM

Pinging @goldfire , I realize you're busy. It's fine to get this in post 8.0. How's it look? I believe I've addressed your feedback adequately. Perhaps other than that one spot where I do sort_by_loc, blindly emulating neighboring code that does something very similar.

Pinging @bgamari, in IRC, @austin mentioned that y'all can follow up from there.

If you ever want to ping me, please email to eir@cis.upenn.edu. Just mentioning my name here is not likely to cause me to see your post -- it's just lucky that I did.

Please add tests for reification of IP types. Relatedly, I see no changes to TcSplice, but I'm skeptical of this omission.

Thanks again for taking a pass through this!

compiler/deSugar/DsMeta.hs
1485

Does this actually work? It seems to in the tests. But I'm quite surprised, because the treatment for BindStmt makes a new gensym for the bound variable, seemingly overriding whatever you've come up with here. Have you inspected the core that a recursive quote desugars to? Regardless, I think some commentary of how this works would be helpful.

EDIT: Perhaps it works because only the ss1 (and not the _ss1) are returned and thus passed to wrapGenSyms. Still, it would be helpful to confirm this by looking at the Core output and then document this fact.

1486

To my mild surprise the following compiles for me:

foo = do putStrLn "hi1"
         rec {}
         putStrLn "hi2"

You should thus be able to handle empty recs.

compiler/hsSyn/Convert.hs
375

Clarify that IP binding blocks are handled separately. I naively expected that all binding declarations were routed through cvtDec.

1666

This is a roundabout way of doing the validity check, I think. Just use okVarOcc.

bgamari edited edge metadata.May 11 2016, 5:55 AM

Ahh, looks like I missed your message @mgsloan. Sorry about that. Taking a look now at the patch now.

bgamari requested changes to this revision.May 11 2016, 6:10 AM
bgamari edited edge metadata.

This looks pretty sensible to me. I've requested a few changes inline, most of which just piggy-back on requests made by @goldfire.

Also, could you add an entry to libraries/template-haskell/Changelog.md?

Otherwise looks good to me.

compiler/deSugar/DsMeta.hs
1486

Agreed; we should be able to handle this case.

compiler/hsSyn/Convert.hs
375

Indeed, a comment to this effect would be helpful.

1666

I agree, okVarOcc does seem a bit more straightforward here.

This revision now requires changes to proceed.May 11 2016, 6:10 AM

@mgsloan, do you plan on continuing this? This is fantastic work, and I'd love to see it make its way into GHC.

austin resigned from this revision.Nov 6 2017, 10:24 PM
mgsloan updated this revision to Diff 17121.Jun 28 2018, 10:28 PM
mgsloan marked 8 inline comments as done.
  • Rebased atop ~2 years worth of changes
  • Attempts to address all review feedback (see comments for details)
  • Usage of "ImplicitParam" instead of "IP" in public API
  • Many ID numbers in THNames were incremented by 20 to make room for another Dec constructor
mgsloan added a comment.EditedJun 28 2018, 10:32 PM

Hey guys, it's been a while. Really sorry for abandoning this and not iterating on it until now. I finally made the time and prioritization to get back to this. A big part of it was the appeal of being able to use GHCi for development of GHC - https://phabricator.haskell.org/D4904 - I find it hard to work on big Haskell projects without that.

Anyway, I've rebased it atop master, addressed the feedback, and otherwise polished it!

Please let me know what you think and whether it needs any additional changes.

compiler/deSugar/DsMeta.hs
1485

mkGenSyms appears to just make a GenSymBind in a deterministic way with LocalId NotExported and VanillaId. Not sure if it was that way back when you reviewed this 2 years ago, though.

I've added an MASSERT to be sure, which checks that the GenSymBinds yielded by mkGenSyms on the collected LStmtsBinders matches up with the ones yielded by addBinds.

1486

Good point! I've fixed this and extended the tests for this case.

compiler/hsSyn/Convert.hs
375

Done!

1666

Good point. I've used unless (okVarOcc n) (failWith (badOcc OccName.varName n)) instead.

mgsloan updated this revision to Diff 17122.Jun 28 2018, 10:39 PM

Remove "reifyPred = reifyType" and just use "reifyType" in "reifyCxt"

mgsloan updated this revision to Diff 17129.Jun 29 2018, 5:50 AM

Fix ID overlap issue by adding 100 to some PrelNames

Thanks so much for picking this back up, @mgsloan! This looks really solid, so my suggestions are mostly minor. (Some of them are in the form of comments which don't necessarily require any action from you.)

compiler/deSugar/DsMeta.hs
1531

This isn't your fault, but dang, we really should leverage TTG here to avoid this unfortunate partiality. (Not necessarily in this Diff, though.)

compiler/prelude/PrelNames.hs
2338 ↗(On Diff #17129)

Congratulations on finally being the person to overflow this :)

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

Do you mind adding some documentation to RecS in the style of other constructors in this module? (I realize that Stmt has heretofore been Haddock-less, but we should try to break that tradition.)

testsuite/tests/th/TH_implicitParamsErr3.stderr
7

Do you have any idea why this test's output (among others) is so strangely indented?

mgsloan updated this revision to Diff 17137.Jun 29 2018, 4:46 PM
mgsloan marked 2 inline comments as done.

Haddock updates

@RyanGlScott Thanks for reviewing! I've added haddocks for the Stmt constructors.

compiler/prelude/PrelNames.hs
2338 ↗(On Diff #17129)

Thanks! :)

It could have been avoided, but it seemed easier just to offset everything and give THNames a bit more breathing space.

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

Done! I added docs for the other constructors too. Adjusted the doc for LetE to match LetS as well

testsuite/tests/th/TH_implicitParamsErr3.stderr
7

Looks like wrapMsg in hsSyn/Convert.hs uses text (pprint item). If it was just item then it would probably indent correctly. Based on git blame, it's been that way since 2009. I'm guessing there is a reason for it, perhaps because sometimes TH expressions can be quite large.

It may be worthwhile to attempt to leave the PrelNames ids unchanged. When switching to and from this branch I'm getting errors like

compiler/main/SysTools/BaseDir.hs:21:1: error:                                                                                       
    Bad interface file: compiler/stage2/build/GhcPrelude.hi
        ghc-stage1: panic! (the 'impossible' happened)
  (GHC version 8.7.20180627 for x86_64-unknown-linux):
        getSymtabName:unknown known-key unique

It is resolved by make clean + make, but it isn't very pleasant to do that.

I trust @RyanGlScott 's instincts, and so given his attention here, I have not done a review myself. Please let me know if there's something that warrants further scrutiny.

mgsloan updated this revision to Diff 17391.Jul 19 2018, 4:46 AM

Avoid changing PrelNames ids

I have pushed an update to this which modifies a few less of the THNames, and entirely avoids modifying compiler/prelude/PrelNames.hs. I found that I needed to do a clean make when switching to this branch. It would work, but it's pretty ugly to break incrementality like that. That not working may indicate a deeper issue, but that is out of scope for this diff.

@RyanGlScott @bgamari I think this is ready. Note that the "changes requested" status from @goldfire no longer applies, as I believe I've addressed his old feedback. Please let me know if anything else needs to be done to get this merged.

RyanGlScott accepted this revision.Jul 19 2018, 7:30 AM

This all looks swell to me. Thanks again for picking this back up, @mgsloan.

The only other suggestion I could make is mentioning these changes in the GHC 8.8.1 release notes, since it does increase the expressive capabilities of Template Haskell. Unfortunately, the GHC 8.8.1 release notes don't exist yet, so it's impossible to do this at the moment! (Nor do I recommend creating them in this Diff—that's something probably best left to a separate patch.)

There is a docs/users_guide/8.8.1-notes.rst file in master now, so this could be mentioned there.

mgsloan updated this revision to Diff 17880.Sep 1 2018, 4:41 PM
  • Add release note for TH support for implicit params + mdo

@RyanGlScott Hey! Sorry for the delay. Finally found some cycles to iterate on this. I've just added one line to the release notes, I figure may as well keep it concise, since users can consult the template-haskell changelog / API.

I also rebased atop master. Is this ready to merge now? Thanks for reviewing!

RyanGlScott accepted this revision.Sep 2 2018, 7:19 AM

Indeed, I think this is ready to land. (The Linux Harbormaster build seemed to fail on the TH_recursiveDo test for some reason, but since the OS X build did not fail on this, I'm guessing that this is a spurious failure.)

Many thanks for seeing this through!

Great, thanks for taking a look @RyanGlScott !

@bgamari I believe this is ready for merge. If the failures turn out to be not spurious, then let me know and I'll take a look. Hoping to have more cycles in the future for GHC dev, but this month is already looking packed. With good stuff, though! Maybe I will see some of you at ICFP

This revision was not accepted when it landed; it landed in state Needs Review.Sep 14 2018, 6:30 AM
This revision was automatically updated to reflect the committed changes.