Provide explicit import in inlay hints #4235

jetjinser · 2024-05-16T17:17:55Z

This PR introduces new feature to provide explicit imports via inlay hints.

textEdits are applied when the editor accepts inlay hints (double click in vscode). The effect of applying textEdits is to complete imports.This behavior is consistent with code lens.

(For example, in vscode) When the cursor is hover on the label of inlay hints, the tooltip will be displayed as a popup. tooltip is always "Make this import explicit".

Special logic, when the lsp client supports inlay hints, explicit import will be provided in the inlay hints, otherwise it will fallback to the code lens.

module ExplicitUsualCase where                                                                                                                                               

import ExplicitA

main :: IO ()
main = putStrLn $ "hello " ++ a1

module ExplicitUsualCase where                                                                                                                                               

import ExplicitA ( a1 ) -- in inlay hints

main :: IO ()
main = putStrLn $ "hello " ++ a1

to writing test before new release of haskell/lsp.

michaelpj

Okay, so the things we definitely need before we can land this:

Clicking the hints needs to do something. I think that means either including the textEdits field or working out how to connect things to a command.
We need to be able to configure these so you can have one or the other. A simple thing to do would be to add some generic config for "inlay hints on or off", like we have for code lenses etc. Then people at least have some way of configuring it.

michaelpj · 2024-05-17T11:52:09Z

hls-plugin-api/src/Ide/Types.hs

@@ -803,6 +809,12 @@ instance PluginRequestMethod Method_TextDocumentSemanticTokensFull where
 instance PluginRequestMethod Method_TextDocumentSemanticTokensFullDelta where
  combineResponses _ _ _ _ (x :| _) = x

+instance PluginRequestMethod Method_TextDocumentInlayHint where
+  combineResponses _ _ _ _ (x :| _) = x


This doesn't seem right: the inlay hints request returns a list! So it's a really easy example where we can combine the responses nicely, just concatenate them.

michaelpj · 2024-05-17T11:52:54Z

hls-plugin-api/src/Ide/Types.hs

+  combineResponses _ _ _ _ (x :| _) = x
+
+instance PluginRequestMethod Method_InlayHintResolve where
+  combineResponses _ _ _ _ (x :| _) = x


Include the reference to the note explaining why there should only be one resolve handler. We can also consider implementing resolve later.

michaelpj · 2024-06-16T12:03:22Z

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

-          -- This plugin provides code actions
+        -- This plugin provides inlay hints
+        <> mkPluginHandler SMethod_TextDocumentInlayHint (inlayHintProvider recorder)
+        -- <> mkResolveHandler SMethod_InlayHintResolve (inlayHintResolveProvider recorder)


I think just delete this for now, we can worry about resolve later.

michaelpj · 2024-06-16T12:04:13Z

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

+    (ImportActionsResult {forLens, forResolve}, pm) <- runActionE "ImportActions" state $ useWithStaleE ImportActions nfp
+    let inlayHints = [ generateInlayHints newRange ie
+                     | (range, int) <- forLens
+                     , range < visibleRange


I think you want isSubRangeOf, I'm not sure this does what you want.

michaelpj · 2024-06-16T12:07:48Z

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

+      InlayHint { _position = _end
+                , _label = mkLabel ie
+                , _kind = Nothing
+                , _textEdits = Nothing


We should definitely set this, it's kind of the point!

God, the spec is weird. Inlay hints themselves only have text edits, but label parts can have commands??? I'm not even sure what you're meant to do there.

michaelpj · 2024-06-16T12:09:14Z

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

@@ -444,6 +479,10 @@ abbreviateImportTitle input =
          else actualPrefix <> suffixText
  in title

+squashedAbbreviateImportTitle :: T.Text -> T.Text


How is this different to what we do on line 170? Why are we doing different things?

I think I worked it out: the previous thing has the whole import spec in it. I think it would be better to just create the two bits of the import spec separately when we generate the title, then we can use only the pieces we need.

We'd then need to tweak abbreviateImportTitle to just abbreviate the list of imported items, and then we can glue it onto the previous part of the import or not depending on whether we're creating an inlay hint or a code lens.

Not only that, abbreviateImportTitle will retain a lot of indentation (shown as spaces after cutting by line), and inlay hints label will need remove those spaces because they are displayed at the end of each imports statement to avoid being too long.

The thing I was suggesting is that in the ImportActions rule we could compute both a) the prefix of the import (import Foo) and the suffix ((bar, baz)). Then for the code lenses we can concatenate them together and abbreviate, and for the lens we can just use the suffix and abbreviate?

michaelpj · 2024-06-16T12:12:05Z

cabal.project

@@ -16,6 +16,24 @@ benchmarks: True

 write-ghc-environment-files: never

+source-repository-package


you can use the released version now, just bump the index-state

michaelpj · 2024-06-16T12:13:40Z

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

+  where
+    generateInlayHints :: Range -> ImportEdit -> InlayHint
+    generateInlayHints Range {_end} ie =
+      InlayHint { _position = _end


This needs a comment. We are assuming that the appropriate position for the hint to begin is the end of the range for the lens, we should document that carefully.

This reverts commit 245049a.

and remove `instance PluginRequestMethod Method_InlayHintResolve` since have not decide how to combine.

…odule

michaelpj

This is looking generally good! It's also quite non-obvious to me why the failing test is failing, it doesn't look like you've changed anything there. I'll think about it. Maybe @fendor has an idea?

michaelpj · 2024-06-24T15:30:31Z

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

    }

+isInlayHintsSupported :: MonadIO m => IdeState -> m Bool
+isInlayHintsSupported state = do
+    Shake.ShakeExtras{lspEnv} <- liftIO $ runAction "" state Shake.getShakeExtras


This seems a bit complicated, compare with https://github.com/haskell/haskell-language-server/blob/master/ghcide/src/Development/IDE/Plugin/Completions.hs#L208 (I just searched for "clientCaps"!)

michaelpj · 2024-06-24T15:34:37Z

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

-                , Just newRange <- [toCurrentRange pm range]]
-    pure $ InL lens
+lensProvider _ state _ CodeLensParams {_textDocument = TextDocumentIdentifier {_uri}} = do
+    isIHSupported <- liftIO $ isInlayHintsSupported state


We need a note explaining what we're doing here.

michaelpj · 2024-06-24T15:36:42Z

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

+lensProvider _ state _ CodeLensParams {_textDocument = TextDocumentIdentifier {_uri}} = do
+    isIHSupported <- liftIO $ isInlayHintsSupported state
+    if isIHSupported
+    then do pure $ InR Null


We might be able to return a more useful error here. I think we can maybe use PluginRequestRefused? Or maybe it's just the most sensible thing to succeed but return nothing.

michaelpj · 2024-06-24T15:41:26Z

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

+           { _position = Position {_line = 3, _character = 16}
+           , _label = InL "( b1 )"
+           , _kind = Just InlayHintKind_Type
+           , _textEdits = Just [TextEdit (Range (Position 3 0) (Position 3 16)) "import ExplicitB ( b1 )"]


These text edits are arguably overly aggressive. We could just insert the export list where it needs to go, rather than replacing the entire import. That would have the advantage of preserving as much as possible of what the user wrote.

I'm not sure it makes a big difference, but we should think about it and write down why we chose this one. I think in general the expectation is that an inlay hint inserts just the text in the hint...

Yes, append is what I did first, but I did run into some problems, like this code:

import Development.IDE hiding (pluginHandlers, pluginRules)

If we just append it, it will obviously break the import statement.
So I chose the same behavior as the original code lens & code action, replacing the entire import statement, so that users only need to re-format it.

Interesting! How does the inlay hint display in that case? I guess here the "perfect" thing to do would be to append the import list and also delete any "hiding" clauses. Complicated, though.

Also, needs a comment, and maybe a test?

I guess here the "perfect" thing to do would be to append the import list and also delete any "hiding" clauses. Complicated, though.

The current approach is to append inlay hints directly at the end.
I don't think it's possible to have inlay hints "cover" a part of your code.

Also, needs a comment, and maybe a test?

Okay, I'll add both.

I don't think it's possible to have inlay hints "cover" a part of your code.

Right, that's definitely true for the hint. I meant that the text edits could only overwrite the hiding clause rather than the entire line.

michaelpj · 2024-06-24T15:41:52Z

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

+                , _kind = Just InlayHintKind_Type -- for type annotations
+                , _textEdits = fmap singleton $ toTEdit pm ie
+                , _tooltip = Nothing
+                , _paddingLeft = Just True


Did you figure out what these padding fields do? It has been very unclear to me form the spec!

In neovim and vscode, it means:

with padding

without padding

Okay, so it basically means "show an extra space before the inlay hint. I guess that makes sense.

michaelpj · 2024-06-24T15:42:32Z

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

    , codeActionOnlyGoldenTest "ExplicitOnlyThis" 3 0
    , codeActionOnlyResolveGoldenTest "ExplicitOnlyThis" 3 0
+    , inlayHintsTest "ExplicitOnlyThis" 3 $ (@=?)


We should test that we don't get code actions when we have inlay hints enabled and vice versa.

fendor · 2024-06-25T13:42:13Z

Which test is failing unexpectedly? ExplicitBreakFile code action (golden)?

jetjinser · 2024-06-25T14:03:14Z

Which test is failing unexpectedly? ExplicitBreakFile code action (golden)?

Yes. The failed started appearing after this commit. This line is where I can find the error raised.

I can't get any more information out of this error.

    ExplicitBreakFile code action (golden):                 FAIL (0.21s)
      Received an expected error in a response for id IdInt 3:
      TResponseError {_code = InL LSPErrorCodes_RequestFailed, _message = "explicitImports: Rule Failed: ImportActions", _xdata = Nothing}
      Use -p '/ExplicitBreakFile code action (golden)/' to rerun this test only.

fendor · 2024-06-26T19:43:02Z

I debugged this to the point that replacing isInlayHintsSupported with pure False makes the test succeed, but isInlayHintsSupported in the lensProvider makes it crash.
I don't understand this issue, yet. This change looks completely unrelated.

fendor · 2024-06-27T14:58:19Z

@jetjinser Change the client capabilities of codeActionBreakFile to use codeActionNoInlayHintsCaps, that worked for me locally.

However, the error message itself is extremely puzzeling.

jetjinser · 2024-06-27T20:26:08Z

@jetjinser Change the client capabilities of codeActionBreakFile to use codeActionNoInlayHintsCaps, that worked for me locally.

However, the error message itself is extremely puzzeling.

Yes, this works on my machine too.

Should I apply this modification? I'll add a comment pointing to the discussion here.

michaelpj

I do think we still need a way to turn these off at the plugin level, like we have for code lenses etc. See https://github.com/haskell/haskell-language-server/blob/master/hls-plugin-api/src/Ide/Types.hs#L262

michaelpj · 2024-06-28T15:12:15Z

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

+isInlayHintsSupported :: Applicative f => IdeState -> f Bool
+isInlayHintsSupported ideState = do
+  let clientCaps = Shake.clientCapabilities $ shakeExtras ideState
+  pure $ case clientCaps of


Could do

isJust $ clientCaps ^? textDocument . _Just . inlayHint . _Just

michaelpj · 2024-06-28T15:12:55Z

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

+    -- otherwise it will be provided as a fallback
+    isIHSupported <- liftIO $ isInlayHintsSupported state
+    if isIHSupported
+    then do pure $ InL []


Comment about why this is the right thing to return here

I'm trying to figure out what's the difference between the two, and from this comment (microsoft/language-server-protocol#1200 (comment)), it seems like [] is no different from null.

But we do need to uniformly choose a way to indicate “no information”. Do you think Right [] is better?

Yes, that seems reasonable to me.

michaelpj · 2024-06-28T15:15:22Z

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

+        pure $ InL inlayHints
+    -- When the client does not support inlay hints, fallback to the code lens,
+    -- so this is Null
+    else do pure $ InR Null


Why do we return null here but an empty list of code lenses above? It helps to be consistent!

Also, we should add: if the client doesn't support inlay hints, this method should never get called!

michaelpj · 2024-06-28T15:16:47Z

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

+                , _kind = Just InlayHintKind_Type -- for type annotations
+                , _textEdits = fmap singleton $ toTEdit pm ie
+                , _tooltip = Nothing
+                , _paddingLeft = Just True


Okay, so it basically means "show an extra space before the inlay hint. I guess that makes sense.

michaelpj · 2024-06-28T15:19:24Z

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

+    mkLabel :: ImportEdit -> T.Text
+    mkLabel (ImportEdit{ieResType, ieText}) =
+      let title ExplicitImport = abbreviateImportTitle . T.dropWhile (/= '(') $ ieText
+          title RefineImport   = "Refine imports to " <> T.intercalate ", " (T.lines ieText)


This seems odd. Do we have any tests showing this? These perhaps shouldn't be shown as inlay hints? I'm unsure.

It's possible these need to always be code lenses, since they don't represent implicit content that can be added to the document by accepting it, which is pretty much what inlay hints are supposed to be. Maybe I'm wrong though, not sure.

Well, I was going to support refine with inlay hints, it may looks like
import qualified RefineA <inlay>from RefineB</inlay> as RA

or
import RefineD <inlay>from RefineE</inlay><inlay> ( e2 )</inlay>

But this is a little tricky for me, I'm trying to locate where the module name position is. In addition, I don't know if I understand it wrong, there may be many original modules? This makes inlay hints longer and jagged.
So I want to ask your opinion.

EDIT:
Another, easier, but equally jag way to do this is to put original module name at the beginning of the import.

This may not be very good... (especially I can't think of what word to use, or not use it -- it definitely can't be from) I want to just ignore refine in inlay hints?

Yes, I think it would be pretty sensible to leave refine as a code lens and not an inlay hint. Let's do that for now and maybe open an issue to discuss what the best UX would be for refine imports?

michaelpj · 2024-06-28T15:25:02Z

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

+    generateInlayHints (Range _ end) ie pm =
+      InlayHint { _position = end
+                , _label = InL $ mkLabel ie
+                , _kind = Just InlayHintKind_Type -- for type annotations


This is surely the wrong kind. I think this should just be omitted.

michaelpj · 2024-06-28T15:26:03Z

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

+                , _label = InL $ mkLabel ie
+                , _kind = Just InlayHintKind_Type -- for type annotations
+                , _textEdits = fmap singleton $ toTEdit pm ie
+                , _tooltip = Nothing


I think we should definitely consider using this! For example, the tooltip for the minimal imports inlay hint could say "This is an import list for exactly the names used by the current module", or something. That way it's a bit more discoverable what it means. It's a nice UX feature!

Could this be as simple as "Make this import explicit"? This doesn't even require resolve, even though the LSP spec recommends that the tooltip be obtained via resolve, but it's simple enough.

edit: it may be different when refining, but this still simple enough.

Yes, I think either of those would be fine. I think we can just make it a constant string; the hint itself shows the things that are going to be inserted. So no need for resolve. Resolve is only for things that are expensive to compute.

michaelpj

One last thing then I think this is good to go!

michaelpj · 2024-07-03T08:12:47Z

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

+    -- otherwise it will be provided as a fallback
+    if isInlayHintsSupported state
+    -- `[]` is no different from `null`, we chose to use all `[]` to indicate "no information"
+    then pure $ InL []


Okay, so I think the only thing remaining here is that we were going to leave the refine import actions as code lenses, so we need to return them in both cases. And never return them for inlay hints.

Should this be checked at lens resolve? If I want to know whether ImportEdit is explicit or refine, it seems that can't do it in lensProvider.

If I can't tell whether the lensProvider should respond at the time, then every import statement actually needs to be resolved even that is not refine. I'm worried this will rely on the expensive resolve.

Hmmm, that's annoying. I would have thought we could determine what type it is beforehand? Perhaps we need to tweak the rule to include that information?

I think now should be okay.

michaelpj · 2024-07-06T20:14:05Z

Great stuff, let's merge it 🎉

jetjinser added 4 commits May 16, 2024 21:16

Provide explicit import in inlay hints

b7da1f5

Filter explict imports inlay hints by visible range

3305973

Update lsp dep by source-repository-package

245049a

to writing test before new release of haskell/lsp.

Add test for hls-explicit-imports-plugin inlay hints

0598138

jetjinser marked this pull request as ready for review June 14, 2024 14:12

jetjinser requested review from michaelpj, fendor and wz1000 as code owners June 14, 2024 14:12

michaelpj reviewed Jun 16, 2024

View reviewed changes

jetjinser added 13 commits June 17, 2024 19:59

Comment inlay hints start position

a27c5c8

Use isSubrangeOf to test if the range is visible

6b3e5aa

Remove inlayHintsResolveProvider placeholder for now

f9f1207

Use explicit InlayHintKind_Type

8e28076

Revert "Update lsp dep by source-repository-package"

d0f27c2

This reverts commit 245049a.

Combine InlayHints by sconcat them

3e9d5a1

and remove `instance PluginRequestMethod Method_InlayHintResolve` since have not decide how to combine.

Merge remote-tracking branch 'upstream/master' into inlay-hints-imports

8d94c51

Merge remote-tracking branch 'upstream/master' into inlay-hints-imports

2e869db

compress multiple spaces in abbr import tilte

dbd7508

update test to match inlay hints kind

d0fe221

rename squashedAbbreviateImportTitle to abbreviateImportTitleWithoutM…

75b0ecb

…odule

Request inlay hints with testEdits

e0543b9

ExplicitImports fallback to codelens when inlay hints not support

a6b7556

jetjinser mentioned this pull request Jun 22, 2024

bump vscode-languageclient version to 9.0.1 haskell/vscode-haskell#1108

Merged

fix explicitImports inlayHints test

2085635

michaelpj reviewed Jun 24, 2024

View reviewed changes

jetjinser added 5 commits June 25, 2024 17:39

simplify isInlayHintsSupported

ccf2d8f

comment fallback

fb52cee

empty list instead of null codeLens

6e5f746

clearify name paddingLeft

f4c2ea2

fix clientCapabilities

62a51ce

add test for inlay hints without its client caps

f70e402

use codeActionNoInlayHintsCaps to avoid error

57ef0db

michaelpj reviewed Jun 28, 2024

View reviewed changes

jetjinser added 12 commits June 30, 2024 03:35

simplify isInlayHintSupported

f8b1993

comment about paddingLeft

a50b148

use null as inlay hints kind

af635f7

add tooltip for explicit imports inlay hints to improve UX

0fab728

chore comments

4c7313d

refactor

ffdb94c

comment InL [] to indicate no info

0a876c3

ignore refine inlay hints

c11e356

add plcInlayHintsOn config

599ebcf

Merge remote-tracking branch 'upstream/master' into inlay-hints-imports

2a2b516

update func-test

3e5b88f

keep order to make Parser works

6cfafd5

jetjinser requested a review from michaelpj July 2, 2024 07:29

michaelpj reviewed Jul 3, 2024

View reviewed changes

always provide refine in code lens

ce761e7

Merge branch 'master' into inlay-hints-imports

2f3b57b

michaelpj approved these changes Jul 6, 2024

View reviewed changes

michaelpj added the merge me Label to trigger pull request merge label Jul 6, 2024

Merge branch 'master' into inlay-hints-imports

64b9533

mergify bot merged commit 763f34c into haskell:master Jul 9, 2024
38 checks passed

		@@ -16,6 +16,24 @@ benchmarks: True

		write-ghc-environment-files: never

		source-repository-package

Provide explicit import in inlay hints #4235

Provide explicit import in inlay hints #4235

Conversation

jetjinser commented May 16, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelpj Jun 17, 2024 • edited Loading

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

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

jetjinser Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fendor commented Jun 25, 2024

jetjinser commented Jun 25, 2024 • edited Loading

fendor commented Jun 26, 2024 • edited Loading

fendor commented Jun 27, 2024

jetjinser commented Jun 27, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jetjinser Jun 29, 2024 • 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

jetjinser Jun 29, 2024 • edited Loading

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

michaelpj commented Jul 6, 2024

jetjinser commented May 16, 2024 •

edited

Loading

michaelpj Jun 17, 2024 •

edited

Loading

jetjinser Jun 25, 2024 •

edited

Loading

jetjinser commented Jun 25, 2024 •

edited

Loading

fendor commented Jun 26, 2024 •

edited

Loading

jetjinser Jun 29, 2024 •

edited

Loading

jetjinser Jun 29, 2024 •

edited

Loading