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

Add a hook for modifying the dynflags from a plugin #1814

Merged
merged 9 commits into from
May 11, 2021

Conversation

isovector
Copy link
Collaborator

This PR changes the hls-plugin-api, adding a new pluginModifyDynflags hook to the PluginDescriptor. This field has type:

data DynFlagsModifications =
  DynFlagsModifications { dynFlagsModifyGlobal :: DynFlags -> DynFlags
                        , dynFlagsModifyParser :: DynFlags -> DynFlags
                        }

which are DynFlags endos for things you'd like to enable globally, and only for the parsing step. This PR uses the modifyGlobal field to pull some Wingman-specific functionality out of ghcide. The upcoming #1776 uses modifyParser to steal some Source Haskell syntax and allow inline tactic metaprogramming.

ghcide/src/Development/IDE/Core/Rules.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Core/Rules.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Core/Service.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Core/Shake.hs Outdated Show resolved Hide resolved
hls-plugin-api/src/Ide/Types.hs Show resolved Hide resolved
@wz1000
Copy link
Collaborator

wz1000 commented May 9, 2021

I have to admit, I'm a bit concerned about how this interacts with recompilation checking.

@pepeiborra
Copy link
Collaborator

I have to admit, I'm a bit concerned about how this interacts with recompilation checking.

Why? As long as the choice of plugins doesn't change, I think that existing interface files should satisfy the recompilation checks.

@wz1000
Copy link
Collaborator

wz1000 commented May 9, 2021

Why? As long as the choice of plugins doesn't change, I think that existing interface files should satisfy the recompilation checks.

The modsummary is changed using dynFlagsModifyParser before parsing, and this is the modsummary that is saved in the ParsedModule. This means it might diverge from the ModSummary returned by GetModSummary.

@pepeiborra
Copy link
Collaborator

pepeiborra commented May 9, 2021

Why? As long as the choice of plugins doesn't change, I think that existing interface files should satisfy the recompilation checks.

The modsummary is changed using dynFlagsModifyParser before parsing, and this is the modsummary that is saved in the ParsedModule. This means it might diverge from the ModSummary returned by GetModSummary.

Ok, so the risk is that the "parsed" DynFlags are used to produce the interface file, whereas the "modSummary" DynFlags are the ones used to check for recompilation, which could lead to false negative checks. Agreed, this would be nasty.

@isovector do we really need two modification functions?

@isovector
Copy link
Collaborator Author

@isovector do we really need two modification functions?

Well, that's up to yall! All I care about is getting a parse with -XQuasiQuotes enabled, but @wz1000 was concerned about that doing all sorts of horrendous things down the line. I'm happy to take directions here, but am far enough out of my expertise that I'm not sure how much I can contribute other than code.

@wz1000
Copy link
Collaborator

wz1000 commented May 9, 2021

Perhaps you can restore the original ModSummary in the ParsedResult?

@isovector
Copy link
Collaborator Author

@wz1000 PTAL at d452740
@pepeiborra likewise, at 1170c2b

ghcide/src/Development/IDE/Main.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Main.hs Outdated Show resolved Hide resolved
isovector and others added 2 commits May 11, 2021 08:58
Co-authored-by: Pepe Iborra <pepeiborra@me.com>
Co-authored-by: Pepe Iborra <pepeiborra@me.com>
@isovector isovector added the merge me Label to trigger pull request merge label May 11, 2021
@mergify mergify bot merged commit bb99905 into haskell:master May 11, 2021
@isovector isovector deleted the plugin-api-changes branch May 21, 2021 16:35
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.

None yet

4 participants