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

Remove exactprint dependencies from ghcide by introducing hls-refactor-plugin. #3091

Merged
merged 4 commits into from
Aug 30, 2022

Conversation

wz1000
Copy link
Collaborator

@wz1000 wz1000 commented Aug 11, 2022

All code actions have been moved to hls-refactor-plugin

Mostly straightforward, only slight complication was that completion auto imports
depends on exactprint, but I didn't want to remove all completion logic from ghcide
just for this.

Instead, I added logic to dynamically lookup the plugin that provides
the extend import command, so that auto imports work as expected when you have
hls-refactor-plugin enabled.

Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

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

Blind approve

@wz1000 wz1000 force-pushed the wip/refact-plugin branch from bd09b59 to 3a0fbf8 Compare August 12, 2022 06:05
@wz1000 wz1000 requested a review from isovector as a code owner August 12, 2022 06:05
@wz1000 wz1000 force-pushed the wip/refact-plugin branch from 3a0fbf8 to 5155661 Compare August 12, 2022 06:08
@wz1000
Copy link
Collaborator Author

wz1000 commented Aug 12, 2022

@pepeiborra you might want to look the changes to showAstData in ghcide/src/Development/IDE/GHC/Dump.hs and see if you are comfortable with those. We could add a flag to use exactprint for that function.

@wz1000 wz1000 force-pushed the wip/refact-plugin branch from 5155661 to bac9fec Compare August 12, 2022 06:11
@wz1000
Copy link
Collaborator Author

wz1000 commented Aug 12, 2022

Also the async tests now use code lenses instead of code actions, I believe this still preserves the expected test behavior, is that correct @pepeiborra?

@wz1000 wz1000 force-pushed the wip/refact-plugin branch 4 times, most recently from a8c1078 to 65d642a Compare August 12, 2022 07:24
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

Awesome, I think this split is great! Will make it much easier to get initial support for new GHC Versions!

I have some minor nitpicks, thank you very much for your work!

Comment on lines 33 to 35
-- li (pre $ text (exactPrint a0)),
-- li (showAstDataHtml' a0),
-- li (nested "Raw" $ pre $ showAstData NoBlankSrcSpan NoBlankEpAnnotations a0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are these still comments? Should probably be removed, if we are not going to use them anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I want to get @pepeiborra's opinion on this first.

Comment on lines 29 to 31
-- #if MIN_VERSION_ghc(9,2,1)

-- #else
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove?

Comment on lines 309 to 311
-- #if MIN_VERSION_ghc(9,2,0)
-- , exactPrint x
-- #endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove as well?

hls-plugin-api/src/Ide/PluginUtils.hs Outdated Show resolved Hide resolved
Copy link
Collaborator

@isovector isovector left a comment

Choose a reason for hiding this comment

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

Tactics stuff lgtm

@wz1000 wz1000 force-pushed the wip/refact-plugin branch from 65d642a to 76c92a3 Compare August 12, 2022 07:49
@wz1000 wz1000 requested a review from konn as a code owner August 12, 2022 07:49
@wz1000 wz1000 force-pushed the wip/refact-plugin branch 2 times, most recently from 6076ba9 to 929bd97 Compare August 12, 2022 08:31
@wz1000
Copy link
Collaborator Author

wz1000 commented Aug 12, 2022

@pepeiborra I've also replaced code actions in the benchmarks with code lenses. This is unfortunate, but we should fix this in the future by benchmarking the entire IDE with plugins included instead of just ghcide.

@pepeiborra
Copy link
Collaborator

I'll take a proper look over the weekend

@michaelpj
Copy link
Collaborator

I don't love the idea of dynamically looking up another plugin just in order to get access to some helper code. I think that could do with some clear documentation and a big warning saying not to use it unless you really really have to. I'm not sure what the "right" solution is: probably moving the completions logic to a plugin and pulling out a common dependency?

@wz1000 wz1000 force-pushed the wip/refact-plugin branch from 929bd97 to 2a3c304 Compare August 12, 2022 09:15
@wz1000 wz1000 force-pushed the wip/refact-plugin branch 2 times, most recently from 4fcddfc to a2769a9 Compare August 13, 2022 08:09
@wz1000
Copy link
Collaborator Author

wz1000 commented Aug 13, 2022

All the plugins that use the GetAnnotatedParsedSource rule now have to link it in via pluginRules to ensure they have access to it even if hls-refactor-plugin is disabled.

Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

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

look the changes to showAstData in ghcide/src/Development/IDE/GHC/Dump.hs

I would have extracted this function and traceAst too

Also the async tests now use code lenses instead of code actions, I believe this still preserves the expected test behavior

Yes!

I've also replaced code actions in the benchmarks with code lenses. This is unfortunate, but we should fix this in the future by benchmarking the entire IDE with plugins included instead of just ghcide

We should probably benchmark every plugin in isolation.

If we are serious about composing plugins together, this is behaviour we probably want to support, so that plugins can pass off tasks to each other in such cases.

I agree

Also there's still an inverse dependency, in that extendImportCommandId is still defined in ghcide. So this won't work as a way of composing plugins together unless you have a central registry of the possible command ids to look for.

This is only because completions are still inside ghcide. Once they are extracted to their own plugin, the dependency will be hls-completions -> hls-refactor. But this would mean hls-completions cannot be built until hls-refactor has been ported, so we would probably want to put the command id inside a new package hls-refactor-command-ids. Which can be done right now. @michaelpj what do you think?

All the plugins that use the GetAnnotatedParsedSource rule now have to link it in via pluginRules to ensure they have access to it even if hls-refactor-plugin is disabled.

Yeah, this makes sense, just keep in mind that his-graph only deduplicates rules, not rules actions (created with action :: Action a -> Rules a).

ghcide/src/Development/IDE/GHC/Dump.hs Outdated Show resolved Hide resolved
mkAdditionalEditsCommand pId edits =
mkLspCommand pId (CommandId extendImportCommandId) "extend import" (Just [toJSON edits])
mkAdditionalEditsCommand :: Maybe PluginId -> ExtendImport -> Maybe Command
mkAdditionalEditsCommand (Just pId) edits = Just $ mkLspCommand pId (CommandId extendImportCommandId) "extend import" (Just [toJSON edits])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to introduce an abstraction for this dynamic lookup now?
Something like:

genLspCommand :: IdePlugins a -> CommandId -> Maybe (Text -> ... -> Command)
genLspCommand = fmap (mkLspCommand ...) (lookupPluginId ...)

hls-plugin-api/src/Ide/PluginUtils.hs Outdated Show resolved Hide resolved
extendImportHandler ideState edit@ExtendImport {..} = do
res <- liftIO $ runMaybeT $ extendImportHandler' ideState edit
whenJust res $ \(nfp, wedit@WorkspaceEdit {_changes}) -> do
let (_, List (head -> TextEdit {_range})) = fromJust $ _changes >>= listToMaybe . Map.toList
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be good to copy over the hlint config from ghcide

@michaelpj
Copy link
Collaborator

But this would mean hls-completions cannot be built until hls-refactor has been ported, so we would probably want to put the command id inside a new package hls-refactor-command-ids. Which can be done right now. @michaelpj what do you think?

Well, I think we should think about what a world where we do this more often looks like. I think we'd have to have a package where all the command IDs for all plugins are defined. That wouldn't be so crazy: that's the "interface definition" for a plugin in this case, and having a separate interface definition which calling modules depend on isn't that unusual. I think I'd rather have one such package than one per plugin, though...

It's still quite ugly in that it means that we end up with a bit of logical coupling between all our plugins in this location. I don't have any better ideas, though. I would be happiest if we could just avoid doing it 😅 So I would prefer hls-completions -> hls-refactor, or better hls-completions/hls-refactor -> hls-plugin-utils or whatever. But that's significantly more work.

@wz1000 wz1000 force-pushed the wip/refact-plugin branch 2 times, most recently from 4d044c7 to 7ad882a Compare August 16, 2022 08:52
@wz1000
Copy link
Collaborator Author

wz1000 commented Aug 16, 2022

so we would probably want to put the command id inside a new package hls-refactor-command-ids. Which can be done right now. @michaelpj what do you think?

I will do this in another MR. I think this one is ready to merge.

@wz1000 wz1000 force-pushed the wip/refact-plugin branch 4 times, most recently from c2fb1b1 to 4223717 Compare August 18, 2022 08:33
@wz1000 wz1000 force-pushed the wip/refact-plugin branch 4 times, most recently from 431e3e8 to 628a866 Compare August 29, 2022 09:53
…r-plugin.

All code actions have been moved to hls-refactor-plugin

Mostly straightforward, only slight complication was that completion auto imports
depends on exactprint, but I didn't want to remove all completion logic from ghcide
just for this.

Instead, I added logic to dynamically lookup the plugin that provides
the extend import command, so that auto imports work as expected when you have
hls-refactor-plugin enabled.

Move lookupPluginId out of loop

Replace code actions with lenses in benchmarks

Allow plugins to use GetAnnotatedParsedSource by linking it in when depended on

Move lookupPluginId to IdePlugins

Add default extensions

Move traceAst
@wz1000 wz1000 force-pushed the wip/refact-plugin branch 5 times, most recently from 30679f3 to 455766d Compare August 30, 2022 12:12
@wz1000 wz1000 enabled auto-merge (rebase) August 30, 2022 12:12
@wz1000 wz1000 force-pushed the wip/refact-plugin branch from 455766d to 291416b Compare August 30, 2022 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants