Resolve for explicit-imports #3682

joyfulmantis · 2023-06-30T22:32:33Z

I am pretty much rewriting the interface with hls for this plugin, as it was pretty poorly written (functions inside both an io and maybe monad, that used neither of them). In the process besides adding support for resolve for the code lens and code actions, I am also adding specific code actions for each import (before there was just one code action for explicit imports for everything)

…resolve

michaelpj · 2023-07-02T09:32:53Z

In the process besides adding support for resolve for the code lens and code actions, I am also adding specific code actions for each import

This is nice, I think I even have an issue for this. In particular, it seems easier for people to write client-side stuff to trigger specific code actions, so this will make it easier for people to define a hotkey for "make current import (only) explicit".

michaelpj · 2023-07-02T09:33:27Z

#496

Probably still needs some clean up and general refactoring

joyfulmantis

The plugin "works" now and there are specific tests for it. While I am pretty happy with the general structure, I will want to go over and see if there is anything I want to refactor or clean up still though

michaelpj

Needs a bit more work but generally looking plausible. I like how we get to unify all the logic into the rule and the (sort of) command handler.

It almost feels like there's a pattern here you could use to unify all of the things. Something like:

You have a rule that generates a bunch of possible actions in a file, with ranges.
You have a command handler/resolve function handler that applies the action
You have a code lens handler that generates lenses for some subset of the actions
You have a code lens resolve handler that calls the command handler and uses some logic to set the title appropriately
You have a code action handler that generates code actions for some subset of the actions.
You have a code action resolve handler that calls the command handler/fills in the edit, depending.

I think it probably would only work for a restricted set of things: you really need your "logic handler" to just produce an edit, so you can use that same edit in all scenarios (applyEdit, filling it in on a code action, etc.).

plugins/hls-explicit-imports-plugin/src/Ide/Plugin/ExplicitImports.hs

michaelpj · 2023-07-03T09:12:04Z

plugins/hls-explicit-imports-plugin/src/Ide/Plugin/ExplicitImports.hs

+      pluginHandlers =
+         -- This plugin provides code lenses
+           mkPluginHandler SMethod_TextDocumentCodeLens (lensProvider pred)
+        <> mkPluginHandler SMethod_CodeLensResolve lensResolveProvider


Do we want to design a similar paired handler function for lenses like we have for code actions?

Since the client never advertises a code lens resolve capability, we have to assume that all clients support it, and can't change our behaviour depending on whether they support it.

seems busted, I opened microsoft/language-server-protocol#1761

I think it's very likely that almost no clients support it so this may just break for many clients.

I think that actually not having a capability for it must mean that all clients (need to?) support it. It's also a relatively important resolve type, because you send all your codelens every edit. Vscode works with resolve, and I know the rust server uses it too.

Could still be useful to have the paired function since maybe we can simplify some stuff that's always the same like the data handling.

michaelpj · 2023-07-03T09:15:36Z

plugins/hls-explicit-imports-plugin/src/Ide/Plugin/ExplicitImports.hs

+runImportCommand :: CommandFunction IdeState EIResolveData
+runImportCommand ideState eird = pluginResponse $ do
+  case eird of
+    ResolveOne uri int -> do


Missing case for ResolveAll?

Also, since you've named the fields, this can be a nice place to use NamedFieldPuns, i.e.

ResolveOne{uri, int} -> ...

just makes sure you can't mix up the positional args

The command actually is responding only to the lens, so shouldn't handle the ResolveAll case. In this case I'm actually reusing the data function, but it might be better to have seperate types.

I think it seems reasonable to share the code for running the action if possible? Having one function that handles either case and then we just call it seems good.

Okay good point, there is no reason to use resolve from codeActions then, but no reason to implement resolve for no reason.

plugins/hls-explicit-imports-plugin/src/Ide/Plugin/ExplicitImports.hs

michaelpj · 2023-07-03T09:37:59Z

plugins/hls-explicit-imports-plugin/src/Ide/Plugin/ExplicitImports.hs

-  {getMinimalImportsResult :: [(LImportDecl GhcRn, Maybe T.Text)]}
+data MinimalImportsResult = MinimalImportsResult
+  { forLens        :: [(Range, Int)]
+  , forCodeActions :: RM.RangeMap (Range, Int)


Why do we need the previous field, can't we just enumerate the (Range, Int) pairs from the RangeMap?

... also why does this have a range in the output? is it the same range as in the input?

From what I've researched it's not possible to easily extract the Range key from the RangeMap. Also the range in the output is the range of the textedit, but the range of the input is (in this case) what the client sends over, which is not identical

plugins/hls-explicit-imports-plugin/src/Ide/Plugin/ExplicitImports.hs

plugins/hls-explicit-imports-plugin/hls-explicit-imports-plugin.cabal

michaelpj

Generally looking good!

plugins/hls-explicit-imports-plugin/src/Ide/Plugin/ExplicitImports.hs

michaelpj · 2023-07-05T15:28:21Z

plugins/hls-explicit-imports-plugin/src/Ide/Plugin/ExplicitImports.hs

 --------------------------------------------------------------------------------

+resolveWTextEdit :: IdeState -> EIResolveData -> ExceptT String (LspT Config IO) WorkspaceEdit
+resolveWTextEdit ideState (ResolveOne uri int) = do


Nit: you could deduplicate a bit by doing:

resolveWTextEdit ideState action = do nfp <- .. (MinimalImports ... case action of ResolveOne ...

michaelpj · 2023-07-05T15:28:39Z

plugins/hls-explicit-imports-plugin/src/Ide/Plugin/ExplicitImports.hs

+  let edits = IM.elems forResolve
+  pure $ mkWorkspaceEdit uri edits
+
+mkWorkspaceEdit :: Uri -> [TextEdit] -> WorkspaceEdit


... and if you did the deduplication you could probably inline this

plugins/hls-explicit-imports-plugin/src/Ide/Plugin/ExplicitImports.hs

michaelpj · 2023-07-05T15:33:17Z

plugins/hls-explicit-imports-plugin/test/Main.hs

+codeActionBreakFile :: FilePath -> Int -> Int -> TestTree
+codeActionBreakFile fp l c = goldenWithExplicitImports " code action" fp codeActionNoResolveCaps $ \doc -> do
+  _ <- waitForDiagnostics
+  changeDoc doc [edit]
  actions <- getCodeActions doc (pointRange l c)


This test doesn't look quite right to me: shouldn't we be getting the code actions, picking one, then editing the file, then resolving the action we picked before and seeing it fail?

So this test causes a parsing error in the file and then gets a code action and executes it. Because we are using "useWithStale" we can still operate on a file that has a parse fail.

Ah I see, that's slightly different then. I think it might also be interesting to put an edit in between getting the actions and resolving one. Potentially one that doesn't cause a parse error, so it just mixes up the uniques, which should cause a failure still.

plugins/hls-explicit-imports-plugin/test/Main.hs

michaelpj · 2023-07-05T15:34:56Z

plugins/hls-explicit-imports-plugin/test/Main.hs

@@ -79,18 +123,16 @@ caTitle _                         = Nothing
 -- code lens tests

 codeLensGoldenTest :: FilePath -> Int -> TestTree


maybe these things could also go in hls-test-utils? Seems like you've been repeating something similar a few times. Good to deduplicate the test stuff too!

plugins/hls-explicit-imports-plugin/src/Ide/Plugin/ExplicitImports.hs

joyfulmantis added 7 commits June 27, 2023 17:10

Generic support for resolve in hls packages

355e95c

Add a new code action resolve helper that falls backs to commands

fb49c31

add resolve capability set to hls-test-utils

d1d299b

Add code lens resolve support

735feca

Merge branch 'resolve-support' into explicit-imports-resolve

89b71a2

Merge remote-tracking branch 'upstream/master' into explicit-imports-…

02262bd

…resolve

WIP

251c917

Finish up explicit imports and add tests

ace6dcf

Probably still needs some clean up and general refactoring

joyfulmantis marked this pull request as ready for review July 2, 2023 18:57

joyfulmantis requested a review from pepeiborra as a code owner July 2, 2023 18:57

joyfulmantis commented Jul 2, 2023

View reviewed changes

michaelpj reviewed Jul 3, 2023

View reviewed changes

joyfulmantis added 7 commits July 3, 2023 23:33

michaelpj's suggestions

af4c70f

refine-imports --no-verify commit

9fb0512

Spelling mistakes and useWithStale for rule

20874be

Turn on -Werror and clean up var names

5895174

Fix test warnings

c778379

Move to maybeT and fix 8.10 flags issue

df8fda0

Added stale action test

b86b156

michaelpj reviewed Jul 4, 2023

View reviewed changes

plugins/hls-explicit-imports-plugin/hls-explicit-imports-plugin.cabal Outdated Show resolved Hide resolved

michaelpj approved these changes Jul 5, 2023

View reviewed changes

joyfulmantis added 4 commits July 10, 2023 22:01

Address suggestions

905edd3

Merge branch 'master' into explicit-imports-resolve

57242a9

merge upstream/master

af46494

fix merge mistake

3f7f55c

joyfulmantis merged commit 27f46d7 into haskell:master Jul 12, 2023
44 of 45 checks passed

fendor mentioned this pull request Aug 8, 2023

Prepare release 2.1.0.0 #3748

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve for explicit-imports #3682

Resolve for explicit-imports #3682

joyfulmantis commented Jun 30, 2023

michaelpj commented Jul 2, 2023

michaelpj commented Jul 2, 2023

joyfulmantis left a comment

michaelpj left a comment

michaelpj Jul 3, 2023

joyfulmantis Jul 3, 2023

michaelpj Jul 3, 2023

michaelpj Jul 3, 2023

joyfulmantis Jul 3, 2023 •

edited

Loading

michaelpj Jul 3, 2023

michaelpj Jul 3, 2023

michaelpj Jul 3, 2023

joyfulmantis Jul 3, 2023

michaelpj Jul 3, 2023

joyfulmantis Jul 3, 2023

michaelpj Jul 3, 2023

michaelpj Jul 3, 2023

joyfulmantis Jul 3, 2023

michaelpj left a comment

michaelpj Jul 5, 2023

michaelpj Jul 5, 2023

michaelpj Jul 5, 2023

joyfulmantis Jul 5, 2023

michaelpj Jul 5, 2023

michaelpj Jul 5, 2023

		@@ -79,18 +123,16 @@ caTitle _ = Nothing
		-- code lens tests

		codeLensGoldenTest :: FilePath -> Int -> TestTree

Resolve for explicit-imports #3682

Resolve for explicit-imports #3682

Conversation

joyfulmantis commented Jun 30, 2023

michaelpj commented Jul 2, 2023

michaelpj commented Jul 2, 2023

joyfulmantis left a comment

Choose a reason for hiding this comment

michaelpj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyfulmantis Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelpj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyfulmantis Jul 3, 2023 •

edited

Loading