Skip to content

Provide explicit import in inlay hints #4235

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

Merged
merged 40 commits into from
Jul 9, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
b7da1f5
Provide explicit import in inlay hints
jetjinser May 16, 2024
3305973
Filter explict imports inlay hints by visible range
jetjinser Jun 14, 2024
245049a
Update lsp dep by source-repository-package
jetjinser Jun 14, 2024
0598138
Add test for hls-explicit-imports-plugin inlay hints
jetjinser Jun 14, 2024
a27c5c8
Comment inlay hints start position
jetjinser Jun 17, 2024
6b3e5aa
Use `isSubrangeOf` to test if the range is visible
jetjinser Jun 17, 2024
f9f1207
Remove inlayHintsResolveProvider placeholder for now
jetjinser Jun 17, 2024
8e28076
Use explicit InlayHintKind_Type
jetjinser Jun 17, 2024
d0f27c2
Revert "Update lsp dep by source-repository-package"
jetjinser Jun 17, 2024
3e9d5a1
Combine InlayHints by sconcat them
jetjinser Jun 17, 2024
8d94c51
Merge remote-tracking branch 'upstream/master' into inlay-hints-imports
jetjinser Jun 21, 2024
2e869db
Merge remote-tracking branch 'upstream/master' into inlay-hints-imports
jetjinser Jun 21, 2024
dbd7508
compress multiple spaces in abbr import tilte
jetjinser Jun 22, 2024
d0fe221
update test to match inlay hints kind
jetjinser Jun 22, 2024
75b0ecb
rename squashedAbbreviateImportTitle to abbreviateImportTitleWithoutM…
jetjinser Jun 22, 2024
e0543b9
Request inlay hints with testEdits
jetjinser Jun 22, 2024
a6b7556
ExplicitImports fallback to codelens when inlay hints not support
jetjinser Jun 22, 2024
2085635
fix explicitImports inlayHints test
jetjinser Jun 23, 2024
ccf2d8f
simplify isInlayHintsSupported
jetjinser Jun 25, 2024
fb52cee
comment fallback
jetjinser Jun 25, 2024
6e5f746
empty list instead of null codeLens
jetjinser Jun 25, 2024
f4c2ea2
clearify name `paddingLeft`
jetjinser Jun 25, 2024
62a51ce
fix clientCapabilities
jetjinser Jun 25, 2024
f70e402
add test for inlay hints without its client caps
jetjinser Jun 25, 2024
57ef0db
use codeActionNoInlayHintsCaps to avoid error
jetjinser Jun 28, 2024
f8b1993
simplify isInlayHintSupported
jetjinser Jun 29, 2024
a50b148
comment about paddingLeft
jetjinser Jun 29, 2024
af635f7
use null as inlay hints kind
jetjinser Jun 29, 2024
0fab728
add tooltip for explicit imports inlay hints to improve UX
jetjinser Jun 29, 2024
4c7313d
chore comments
jetjinser Jun 29, 2024
ffdb94c
refactor
jetjinser Jun 29, 2024
0a876c3
comment InL [] to indicate no info
jetjinser Jul 1, 2024
c11e356
ignore refine inlay hints
jetjinser Jul 1, 2024
599ebcf
add plcInlayHintsOn config
jetjinser Jul 1, 2024
2a2b516
Merge remote-tracking branch 'upstream/master' into inlay-hints-imports
jetjinser Jul 2, 2024
3e5b88f
update func-test
jetjinser Jul 2, 2024
6cfafd5
keep order to make Parser works
jetjinser Jul 2, 2024
ce761e7
always provide refine in code lens
jetjinser Jul 5, 2024
2f3b57b
Merge branch 'master' into inlay-hints-imports
michaelpj Jul 6, 2024
64b9533
Merge branch 'master' into inlay-hints-imports
mergify[bot] Jul 8, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions hls-plugin-api/src/Ide/Types.hs
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,12 @@ instance PluginMethod Request Method_WorkspaceSymbol where
-- Unconditionally enabled, but should it really be?
handlesRequest _ _ _ _ = HandlesRequest

instance PluginMethod Request Method_TextDocumentInlayHint where
handlesRequest _ _ _ _ = HandlesRequest

instance PluginMethod Request Method_InlayHintResolve where
handlesRequest _ _ _ _ = HandlesRequest

instance PluginMethod Request Method_TextDocumentCodeLens where
handlesRequest = pluginEnabledWithFeature plcCodeLensOn

Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.


instance PluginRequestMethod Method_InlayHintResolve where
combineResponses _ _ _ _ (x :| _) = x
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


takeLefts :: [a |? b] -> [a]
takeLefts = mapMaybe (\x -> [res | (InL res) <- Just x])

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
{-# LANGUAGE RecordWildCards #-}
{-# LANGUAGE TypeFamilies #-}
{-# LANGUAGE ViewPatterns #-}

module Ide.Plugin.ExplicitImports
( descriptor
, descriptorForModules
Expand All @@ -22,6 +23,7 @@ import Control.Monad.Trans.Except (ExceptT)
import Control.Monad.Trans.Maybe
import qualified Data.Aeson as A (ToJSON (toJSON))
import Data.Aeson.Types (FromJSON)
import Data.Char (isSpace)
import qualified Data.IntMap as IM (IntMap, elems,
fromList, (!?))
import Data.IORef (readIORef)
Expand All @@ -44,8 +46,9 @@ import GHC.Generics (Generic)
import Ide.Plugin.Error (PluginError (..),
getNormalizedFilePathE,
handleMaybe)
import Ide.Plugin.RangeMap (filterByRange)
import qualified Ide.Plugin.RangeMap as RM (RangeMap, fromList)
import qualified Ide.Plugin.RangeMap as RM (RangeMap,
filterByRange,
fromList)
import Ide.Plugin.Resolve
import Ide.PluginUtils
import Ide.Types
Expand Down Expand Up @@ -98,9 +101,11 @@ descriptorForModules recorder modFilter plId =
-- This plugin provides code lenses
mkPluginHandler SMethod_TextDocumentCodeLens (lensProvider recorder)
<> mkResolveHandler SMethod_CodeLensResolve (lensResolveProvider recorder)
-- This plugin provides code actions
-- This plugin provides inlay hints
<> mkPluginHandler SMethod_TextDocumentInlayHint (inlayHintProvider recorder)
-- <> mkResolveHandler SMethod_InlayHintResolve (inlayHintResolveProvider recorder)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

-- This plugin provides code actions
<> codeActionHandlers

}

-- | The actual command handler
Expand Down Expand Up @@ -146,12 +151,13 @@ lensProvider _ state _ CodeLensParams {_textDocument = TextDocumentIdentifier {
, _range = range
, _command = Nothing }


lensResolveProvider :: Recorder (WithPriority Log) -> ResolveFunction IdeState IAResolveData 'Method_CodeLensResolve
lensResolveProvider _ ideState plId cl uri rd@(ResolveOne _ uid) = do
nfp <- getNormalizedFilePathE uri
(ImportActionsResult{forResolve}, _) <- runActionE "ImportActions" ideState $ useWithStaleE ImportActions nfp
target <- handleMaybe PluginStaleResolve $ forResolve IM.!? uid
let updatedCodeLens = cl & L.command ?~ mkCommand plId target
let updatedCodeLens = cl & L.command ?~ mkCommand plId target
pure updatedCodeLens
where mkCommand :: PluginId -> ImportEdit -> Command
mkCommand pId (ImportEdit{ieResType, ieText}) =
Expand All @@ -166,6 +172,35 @@ lensResolveProvider _ ideState plId cl uri rd@(ResolveOne _ uid) = do
lensResolveProvider _ _ _ _ _ rd = do
throwError $ PluginInvalidParams (T.pack $ "Unexpected argument for lens resolve handler: " <> show rd)


inlayHintProvider :: Recorder (WithPriority Log) -> PluginMethodHandler IdeState 'Method_TextDocumentInlayHint
inlayHintProvider _ state _ InlayHintParams {_textDocument = TextDocumentIdentifier {_uri}} = do
nfp <- getNormalizedFilePathE _uri
(ImportActionsResult {forLens, forResolve}, pm) <- runActionE "ImportActions" state $ useWithStaleE ImportActions nfp
let inlayHints = [ generateInlayHints newRange ie
| (range, int) <- forLens
, Just newRange <- [toCurrentRange pm range]
, Just ie <- [forResolve IM.!? int]]
pure $ InL inlayHints
where
generateInlayHints :: Range -> ImportEdit -> InlayHint
generateInlayHints Range {_end} ie =
InlayHint { _position = _end
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

, _label = mkLabel ie
, _kind = Nothing
, _textEdits = Nothing
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

, _tooltip = Nothing
Copy link
Collaborator

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

@jetjinser jetjinser Jun 29, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

, _paddingLeft = Just True
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jetjinser jetjinser Jun 25, 2024

Choose a reason for hiding this comment

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

In neovim and vscode, it means:

with padding
image

without padding
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

, _paddingRight = Nothing
, _data_ = Nothing
}
mkLabel :: ImportEdit -> T.Text |? [InlayHintLabelPart]
mkLabel (ImportEdit{ieResType, ieText}) =
let title ExplicitImport = abbreviateImportTitle $ (T.intercalate " " . filter (not . T.null) . T.split isSpace . T.dropWhile (/= '(')) ieText
title RefineImport = T.intercalate ", " (T.lines ieText)
in InL $ title ieResType


-- |For explicit imports: If there are any implicit imports, provide both one
-- code action per import to make that specific import explicit, and one code
-- action to turn them all into explicit imports. For refine imports: If there
Expand All @@ -176,7 +211,7 @@ codeActionProvider _ ideState _pId (CodeActionParams _ _ TextDocumentIdentifier
nfp <- getNormalizedFilePathE _uri
(ImportActionsResult{forCodeActions}, pm) <- runActionE "ImportActions" ideState $ useWithStaleE ImportActions nfp
newRange <- toCurrentRangeE pm range
let relevantCodeActions = filterByRange newRange forCodeActions
let relevantCodeActions = RM.filterByRange newRange forCodeActions
allExplicit =
[InR $ mkCodeAction "Make all imports explicit" (Just $ A.toJSON $ ExplicitAll _uri)
-- We should only provide this code action if there are any code
Expand Down Expand Up @@ -410,7 +445,6 @@ isExplicitImport _ = False
maxColumns :: Int
maxColumns = 120


-- | The title of the command is ideally the minimal explicit import decl, but
-- we don't want to create a really massive code lens (and the decl can be extremely large!).
-- So we abbreviate it to fit a max column size, and indicate how many more items are in the list
Expand Down Expand Up @@ -462,10 +496,7 @@ filterByImport (ImportDecl{ideclHiding = Just (_, L _ names)})
else Nothing
where importedNames = S.fromList $ map (ieName . unLoc) names
res = flip Map.filter avails $ \a ->
any (`S.member` importedNames)
$ concatMap
getAvailNames
a
any (any (`S.member` importedNames) . getAvailNames) a
allFilteredAvailsNames = S.fromList
$ concatMap getAvailNames
$ mconcat
Expand Down
Loading