Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Splice Plugin: expands TH splices and QuasiQuotes #759

Merged
merged 64 commits into from
Jan 5, 2021
Merged

Splice Plugin: expands TH splices and QuasiQuotes #759

merged 64 commits into from
Jan 5, 2021

Conversation

konn
Copy link
Collaborator

@konn konn commented Dec 30, 2020

This plugin provides an ability to expand splices in the source code.
This is particularly useful when:

  1. TH macro produces invalid (e.g. type-error) code, but the output AST is relatively complex and you want to investigate the generated code in detail.
    • Of course, passing -ddump-splices just helps in simple case; but it omits some parts when the output size is relatively large.
  2. You want to get rid of TH code for some reasons (e.g. Perhaps for cross-compilation?)
    • This use-case is, however, primarily out of the scope of this PR at this stage, as in several cases just dumping generated code doesn't give us the working code:
      1. Hygiene problem: unique flavour of Name sometimes get in the way so that x and x_awa2221 won't much as a plain source code
      2. TH macro often includes non-exported symbols (it is a customary pattern to hide an implementation detail to the users and let macro take care of them), hence generated code doesn't have access to them.

By the way, I really need the feature for the reason 1, and I'm pretty sure that it must help us a lot when debugging complex macros.

Demo (added afterwards)

2020-12-31.4.47.39.mov

Some notes

Ide.TreeTransform

I really loved the functionality provided in Ide.TreeTransform, which was provided in Tactic Plugin.
I wanted to use it also in this plugin, so I extract the module into the new package.
If there is any problem, please tell me @isovector @WorldSEnder

Non-invasive expansion

The current implementation just replaces the splice with the generated code.
However, it might be sometimes the case that one wants to retain the original splice and just append generated code as the comment right after the splice.
I once tried to use ghc-exactprint, but couldn't work out the way to insert new comments with it.

Declaration Splice

It seems HieAST doesn't contain Declaration Splices at all, and contrary to the rest of ASTs, they are one-to-many correspondence with the original splice.
Hence, I just skip the implementation of the expansion of declarations. Perhaps worth another PR?

TODOs:

  • Implement golden tests
  • Support declaration splices
  • Support commented-out style of expansion; i.e. not replacing the old splice, but expands them surrounded by the comment braces, right after the splice.

Credits

This pull request also incorporating splice hover support, which was at first implemented in mpickering/ghcide/top-level-splices-hover.
Thank you @mpickering to allow me to include it and thank you @wz1000 for suggesting that!

@wz1000
Copy link
Collaborator

wz1000 commented Dec 30, 2020

You can use runMetaHook to directly capture the result of a splice when ghc is running it: https://hackage.haskell.org/package/ghc-8.10.1/docs/Hooks.html#v:runMetaHook

@konn
Copy link
Collaborator Author

konn commented Dec 30, 2020

You can use runMetaHook to directly capture the result of a splice when ghc is running it: https://hackage.haskell.org/package/ghc-8.10.1/docs/Hooks.html#v:runMetaHook

Thanks for pointing it out. I wasn't aware of it. I'll give it a try!

@wz1000
Copy link
Collaborator

wz1000 commented Dec 30, 2020

See this for an example of how to set and use a MetaHook: mpickering/ghcide@d9e59ab

In this code, the input to the splice is being captured - you would like to capture the output instead.

@konn
Copy link
Collaborator Author

konn commented Dec 31, 2020

Hmm, as far as I tried, it seems that MetaHook combined with hscTypecheckRename works only when there is no type-errors:

captureSplices ::
    HscEnvEq ->
    ParsedModule ->
    (HscEnv -> ExpandedSplices -> MaybeT IO a) ->
    MaybeT IO a
captureSplices hscEnvEq pm k = do
    var <- liftIO $ newIORef (mempty :: ExpandedSplices)
    let hsEnv0 = hscEnv hscEnvEq
        dflags0 = hsc_dflags hsEnv0
        hooks0 = hooks dflags0
        dflags = dflags0 {hooks = hooks0 {runMetaHook = Just $ mkSpliceHook var}}
        hsEnv = hsEnv0 {hsc_dflags = dflags}
    void $
        liftIO $
            hscTypecheckRename
                hsEnv
                (pm_mod_summary pm)
                HsParsedModule {hpm_module = parsedSource pm, hpm_src_files = pm_extra_src_files pm, hpm_annotations = pm_annotations pm}
    k hsEnv =<< liftIO (readIORef var)

As my first motivation is to debug erroneous macros, it is not desirable such behaviour... Is there any working workaround?

@wz1000
Copy link
Collaborator

wz1000 commented Dec 31, 2020

Did you try setting -fdefer-type-errors? Also, please add this hook into the usual ghcide typecheck pipeline, we really don't want to be typechecking modules twice (not only is this wasteful, it is unsafe in the presence of Template Haskell, see https://gitlab.haskell.org/ghc/ghc/-/issues/18812).

The ghcide typechecking logic (in ghcide/src/Development/IDE/Core/Compile.hs) already has -fdefer-type-errors set, so if you just stick this logic in there (possibly adding another field to the result of typechecking to capture the splices), everything should just work.

This is easy now that we have merged repositories.

@konn
Copy link
Collaborator Author

konn commented Dec 31, 2020

Did you try setting -fdefer-type-errors? Also, please add this hook into the usual ghcide typecheck pipeline, we really don't want to be typechecking modules twice (not only is this wasteful, it is unsafe in the presence of Template Haskell, see https://gitlab.haskell.org/ghc/ghc/-/issues/18812).

The ghcide typechecking logic (in ghcide/src/Development/IDE/Core/Compile.hs) already has -fdefer-type-errors set, so if you just stick this logic in there (possibly adding another field to the result of typechecking to capture the splices), everything should just work.

Ah! I forgot about -fdefer-type-errors and didn't know that they are already included in ghcide. I'll try it!
If I have to extend ghcide, then I can just add the field corresponding to ExpandedSplcies in TcModuleResult, right?

@wz1000
Copy link
Collaborator

wz1000 commented Dec 31, 2020

While you are doing this, maybe you can even pick up @mpickering's patch for #733 ?

The branch is linked in the issue and the implementation should be mostly complete. The same hook can be used to capture all the information required.

@wz1000
Copy link
Collaborator

wz1000 commented Dec 31, 2020

If I have to extend ghcide, then I can just add the field corresponding to ExpandedSplcies in TcModuleResult, right?

Yes, please do.

@konn
Copy link
Collaborator Author

konn commented Dec 31, 2020

OK, I confirmed that setting -fdefer-type-errors makes it possible to expand erroneous splices. I'll start to integrate splice capturing into ghcide!

@konn
Copy link
Collaborator Author

konn commented Jan 3, 2021

I realised that the log told that QQ.hs not found. I've just added hie.yaml including QQ.hs, hopefully, it should now compile.

@konn
Copy link
Collaborator Author

konn commented Jan 3, 2021

It seems newly added stack-8.10.3.yaml lacked my plugin-related packages. Now that I added it and GitHub Actions are all green, I think this should fix all CI.

@wz1000 @mpickering Thank you for your help so far! Are there any other concerns or possible fixes on this PR?

@konn
Copy link
Collaborator Author

konn commented Jan 4, 2021

I found that I forgot to modify DynFlags in default strategy. It must now respects LANGUAGE pragmas in module when parsing-unparsing inside ghc-exactprint.

@konn
Copy link
Collaborator Author

konn commented Jan 4, 2021

...And it seems that CI fails unexpectedly due to the current change in the master branch? I will file another issue.

@konn konn requested a review from wz1000 January 5, 2021 04:58
#if __GLASGOW_HASKELL__ == 808
#else
`extQ` \case
(L l@(RealSrcSpan spLoc) pat :: LPat GhcPs)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use (dL -> L l@(RealSrcSpan spLoc) pat :: Located (Pat GhcPs)) for 8.8

https://hackage.haskell.org/package/ghc-8.8.3/docs/GHC.html#v:dL

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I wasn't aware of that! Last time, I tried to extract location info from XPat, constructor, which gave a slightly different SrcLoc than expected. Contrary, at least on my laptop, it seems that the combination with cL and dL gives a correct SrcSpan and Pattern Splice Expansion successes also in GHC 8.8!

Copy link
Collaborator

@wz1000 wz1000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Seems like you put in a lot of work into making sure this is robust in the face of errors!

@konn
Copy link
Collaborator Author

konn commented Jan 5, 2021

@wz1000 Thanks! I owe you and @mpickering for many suggested improvements!

@konn
Copy link
Collaborator Author

konn commented Jan 5, 2021

I think the above last commit that enables pattern splice expansion GHC 8.8 is the last change I would add to this PR and it can now be merged when the CI is all green. Thanks!

@jneira jneira added the merge me Label to trigger pull request merge label Jan 5, 2021
@jneira
Copy link
Member

jneira commented Jan 5, 2021

nice, many thanks for this new plugin

@mergify mergify bot merged commit 8d5cbe1 into haskell:master Jan 5, 2021
pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Jan 9, 2021
* Implements splice location detection

* Corrects detection logic

* Changed to use (bogus) message for code action

* Splice location

* Extract `Ide.TreeTransform` as an independent package

* It once worked, but stops...

* Now it works for inplace expansion for expressions

* generalises tree transformation to general AST element

* Done for Types and Patterns!

* Disabled "commented" style of expansion

* kills redundant imports

* Updates cabal.project

* Nix fix

* Nix fix, fix

* Throws away loading hacks entirely

* Type adjusted for inverse dependency

* Resolves merge conflicts

* WIP: Support hover and goto definition for top-level splices

I can't work out how to properly integrate this information into the
.hie file machinery. Perhaps it would be better to upstream this.

* Modifies splice information to store both spliced expression and expanded ones as well

* Avoid name collision

* formatting erros

* Safer error handling

* Rewrote using updated ghcide  `TypeCheck` results

* Use `liftRnf rwhnf` to force spine of lists

* Stop using `defaultRunMeta` directly to avoid override of preexisting hooks

* Error report

* Add splice information into HIE generation.

* Resolves interace conflict

* Add test

* Changes to use ParsedModule to detect Splice CodeLens

* formatted

* Implements golden test

* mzero for HsDecl

* Decl Splice

* Workaround for Decl expansion and support type-errored macro expansion.

* Only setting up dflags correcly would suffice

* Removes lines accidentally added

* Regression tests for Declaration splice and kind-error ones

* Workaround for GHC 8.8

* Revert "Workaround for GHC 8.8"

This reverts commit 056f769.

* Unsupport pattern splices GHC 8.8

* Corrects line position in GoToHover

* Increases wait time

* Includes only related changes only

* Optimises `something'`

* Adds hie.yaml

* circie ci: Modifies stack-8.10.3.yaml

* Forgot to update dflags in auto-expansion with default strategy

* Forgot to add golden file

* A dummy commit to run CI

* Workaround for GHC 8.8 pattern splices

Co-authored-by: Matthew Pickering <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
)
a

graftWithSmallestM ::
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@konn can you walk me through what this function is supposed to do? My understanding based on the name is that it should only run the transformation on the smallest Located ast that contains the span, but I don't see how that could be the case based on the implementation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you walk me through what this function is supposed to do?

Sure!

My understanding based on the name is that it should only run the transformation on the smallest Located ast that contains the span

That's what I originally intended, but not used at last. Here, I used everywhereM', defined Line 266. which is a bottom-up variant of everywhereM from syb.
Since it is bottom-up, I once expected it rewrites the smallest AST containing the given span.
But now, I think this was mis-implemented: it MUST use bottom-up variant of somewhere instead of everywhereM', otherwise it will transform ALL the AST element containing the span.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind taking a look at my changes in #1543?

Copy link
Collaborator Author

@konn konn Mar 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll take a look this evening (in JST)!

SrcSpan ->
(LHsDecl GhcPs -> TransformT m (Maybe [LHsDecl GhcPs])) ->
Graft m a
graftDeclsWithM dst toDecls = Graft $ \dflags a -> do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@konn I also don't understand what this function is doing. Mind walking me through it? Especially what go does.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intended usage of graftDeclsWithM is to apply a one-to-many transformation on declaration lists.
This transformation is needed when I implemented the expansion of declaration splices, as a single declaration splice can be expanded into multiple declarations.

go is just doing something like concatMapM (or fmap getAp . foldMap . fmap Ap), based on span information: it applies transformation when the span matched, replace a single declaration with the generated multiple declarations if the transformation succeeded.
At the time I wrote it, I couldn't find any one-to-many grafting function in ghc-exactprint, so I wrote this by hand.

@jneira
Copy link
Member

jneira commented Oct 4, 2021

This closed #733, thanks @konn!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants