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 18 commits
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
9 changes: 9 additions & 0 deletions hls-plugin-api/src/Ide/Types.hs
Original file line number Diff line number Diff line change
Expand Up @@ -511,6 +511,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 @@ -810,6 +816,9 @@ instance PluginRequestMethod Method_TextDocumentSemanticTokensFull where
instance PluginRequestMethod Method_TextDocumentSemanticTokensFullDelta where
combineResponses _ _ _ _ (x :| _) = x

instance PluginRequestMethod Method_TextDocumentInlayHint where
combineResponses _ _ _ _ x = sconcat x

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

Expand Down
7 changes: 7 additions & 0 deletions hls-test-utils/src/Test/Hls/Util.hs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ module Test.Hls.Util
( -- * Test Capabilities
codeActionResolveCaps
, codeActionNoResolveCaps
, codeActionNoInlayHintsCaps
, codeActionSupportCaps
, expectCodeAction
-- * Environment specifications
Expand Down Expand Up @@ -107,6 +108,12 @@ codeActionNoResolveCaps :: ClientCapabilities
codeActionNoResolveCaps = Test.fullLatestClientCaps
& (L.textDocument . _Just . L.codeAction . _Just . L.resolveSupport) .~ Nothing
& (L.textDocument . _Just . L.codeAction . _Just . L.dataSupport . _Just) .~ False

codeActionNoInlayHintsCaps :: ClientCapabilities
codeActionNoInlayHintsCaps = Test.fullLatestClientCaps
& (L.textDocument . _Just . L.codeAction . _Just . L.resolveSupport) .~ Nothing
& (L.textDocument . _Just . L.codeAction . _Just . L.dataSupport . _Just) .~ False
& (L.textDocument . _Just . L.inlayHint) .~ Nothing
-- ---------------------------------------------------------------------
-- Environment specification for ignoring tests
-- ---------------------------------------------------------------------
Expand Down
128 changes: 103 additions & 25 deletions plugins/hls-explicit-imports-plugin/src/Ide/Plugin/ExplicitImports.hs
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@
{-# LANGUAGE RecordWildCards #-}
{-# LANGUAGE TypeFamilies #-}
{-# LANGUAGE ViewPatterns #-}

module Ide.Plugin.ExplicitImports
( descriptor
, descriptorForModules
, abbreviateImportTitle
, abbreviateImportTitleWithoutModule
, Log(..)
) where

Expand All @@ -22,14 +24,17 @@ 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)
import Data.List (singleton)
import qualified Data.Map.Strict as Map
import Data.Maybe (isNothing, mapMaybe)
import qualified Data.Set as S
import Data.String (fromString)
import qualified Data.Text as T
import qualified Data.Text as Text
import Data.Traversable (for)
import qualified Data.Unique as U (hashUnique,
newUnique)
Expand All @@ -44,14 +49,17 @@ 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
import qualified Language.LSP.Protocol.Lens as L
import Language.LSP.Protocol.Message
import Language.LSP.Protocol.Types
import qualified Language.LSP.Protocol.Types as LSP
import qualified Language.LSP.Server as LSP

-- This plugin is named explicit-imports for historical reasons. Besides
-- providing code actions and lenses to make imports explicit it also provides
Expand Down Expand Up @@ -97,17 +105,35 @@ 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)
-- This plugin provides code actions
<> codeActionHandlers

}

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

Choose a reason for hiding this comment

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

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"!)

case lspEnv of
Just env -> liftIO $ LSP.runLspT env s
Nothing -> pure False
where
s = do
clientCapabilities <- LSP.getClientCapabilities
pure $ case () of
_ | LSP.ClientCapabilities{_textDocument} <- clientCapabilities
, Just LSP.TextDocumentClientCapabilities{_inlayHint} <- _textDocument
, Just _ <- _inlayHint
-> True
| otherwise -> False


-- | The actual command handler
runImportCommand :: Recorder (WithPriority Log) -> CommandFunction IdeState IAResolveData
runImportCommand recorder ideState _ eird@(ResolveOne _ _) = do
wedit <- resolveWTextEdit ideState eird
_ <- lift $ pluginSendRequest SMethod_WorkspaceApplyEdit (ApplyWorkspaceEditParams Nothing wedit) logErrors
return $ InR Null
return $ InR Null
where logErrors (Left re) = do
logWith recorder Error (LogWAEResponseError re)
pure ()
Expand All @@ -129,13 +155,17 @@ runImportCommand _ _ _ rd = do
-- the provider should produce one code lens associated to the import statement:
-- > Refine imports to import Control.Monad.IO.Class (liftIO)
lensProvider :: Recorder (WithPriority Log) -> PluginMethodHandler IdeState 'Method_TextDocumentCodeLens
lensProvider _ state _ CodeLensParams {_textDocument = TextDocumentIdentifier {_uri}} = do
nfp <- getNormalizedFilePathE _uri
(ImportActionsResult{forLens}, pm) <- runActionE "ImportActions" state $ useWithStaleE ImportActions nfp
let lens = [ generateLens _uri newRange int
| (range, int) <- forLens
, Just newRange <- [toCurrentRange pm range]]
pure $ InL lens
lensProvider _ state _ CodeLensParams {_textDocument = TextDocumentIdentifier {_uri}} = do
isIHSupported <- liftIO $ isInlayHintsSupported state
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

if isIHSupported
then do pure $ InR Null
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

else do
nfp <- getNormalizedFilePathE _uri
(ImportActionsResult{forLens}, pm) <- runActionE "ImportActions" state $ useWithStaleE ImportActions nfp
let lens = [ generateLens _uri newRange int
| (range, int) <- forLens
, Just newRange <- [toCurrentRange pm range]]
pure $ InL lens
where -- because these are non resolved lenses we only need the range and a
-- unique id to later resolve them with. These are for both refine
-- import lenses and for explicit import lenses.
Expand All @@ -145,12 +175,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 @@ -165,6 +196,50 @@ lensResolveProvider _ ideState plId cl uri rd@(ResolveOne _ uid) = do
lensResolveProvider _ _ _ _ _ rd = do
throwError $ PluginInvalidParams (T.pack $ "Unexpected argument for lens resolve handler: " <> show rd)


-- | Provide explicit imports in inlay hints.
-- Applying textEdits can make the import explicit.
-- There is currently no need to resolve inlay hints,
-- as no tooltips or commands are provided in the label.
inlayHintProvider :: Recorder (WithPriority Log) -> PluginMethodHandler IdeState 'Method_TextDocumentInlayHint
inlayHintProvider _ state _ InlayHintParams {_textDocument = TextDocumentIdentifier {_uri}, _range = visibleRange} = do
isIHSupported <- liftIO $ isInlayHintsSupported state
if isIHSupported
then do
nfp <- getNormalizedFilePathE _uri
(ImportActionsResult {forLens, forResolve}, pm) <- runActionE "ImportActions" state $ useWithStaleE ImportActions nfp
let inlayHints = [ generateInlayHints newRange ie pm
| (range, int) <- forLens
, Just newRange <- [toCurrentRange pm range]
, isSubrangeOf newRange visibleRange
, Just ie <- [forResolve IM.!? int]]
pure $ InL inlayHints
else do pure $ InR Null
Copy link
Collaborator

Choose a reason for hiding this comment

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

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!

where
-- The appropriate and intended position for the hint hints to begin
-- is the end of the range for the code lens.
-- import Data.Char (isSpace)
-- |--- range ----|-- IH ---|
-- |^-padding
-- ^-_position
generateInlayHints :: Range -> ImportEdit -> PositionMapping -> InlayHint
generateInlayHints (Range _ end) ie pm =
InlayHint { _position = end
, _label = InL $ mkLabel ie
, _kind = Just InlayHintKind_Type -- for type annotations
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

, _textEdits = fmap singleton $ toTEdit pm ie
, _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
mkLabel (ImportEdit{ieResType, ieText}) =
let title ExplicitImport = abbreviateImportTitle . T.dropWhile (/= '(') $ ieText
title RefineImport = "Refine imports to " <> T.intercalate ", " (T.lines ieText)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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.

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.

image

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?

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 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?

in 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 @@ -175,7 +250,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 @@ -231,12 +306,14 @@ resolveWTextEdit ideState (RefineAll uri) = do
pure $ mkWorkspaceEdit uri edits pm
mkWorkspaceEdit :: Uri -> [ImportEdit] -> PositionMapping -> WorkspaceEdit
mkWorkspaceEdit uri edits pm =
WorkspaceEdit {_changes = Just $ Map.singleton uri (mapMaybe toWEdit edits)
WorkspaceEdit {_changes = Just $ Map.singleton uri (mapMaybe (toTEdit pm) edits)
, _documentChanges = Nothing
, _changeAnnotations = Nothing}
where toWEdit ImportEdit{ieRange, ieText} =
let newRange = toCurrentRange pm ieRange
in (\r -> TextEdit r ieText) <$> newRange

toTEdit :: PositionMapping -> ImportEdit -> Maybe TextEdit
toTEdit pm ImportEdit{ieRange, ieText} =
let newRange = toCurrentRange pm ieRange
in (\r -> TextEdit r ieText) <$> newRange

data ImportActions = ImportActions
deriving (Show, Generic, Eq, Ord)
Expand Down Expand Up @@ -413,16 +490,15 @@ 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
-- after the abbreviation
abbreviateImportTitle :: T.Text -> T.Text
abbreviateImportTitle input =
let
-- For starters, we only want one line in the title
oneLineText = T.unwords $ T.lines input
-- we also need to compress multiple spaces into one
oneLineText = T.unwords $ filter (not . T.null) $ T.split isSpace input
-- Now, split at the max columns, leaving space for the summary text we're going to add
-- (conservatively assuming we won't need to print a number larger than 100)
(prefix, suffix) = T.splitAt (maxColumns - T.length (summaryText 100)) oneLineText
Expand All @@ -447,6 +523,11 @@ abbreviateImportTitle input =
else actualPrefix <> suffixText
in title

-- Create an import abbreviate title without module for inlay hints
abbreviateImportTitleWithoutModule :: Text.Text -> Text.Text
abbreviateImportTitleWithoutModule = abbreviateImportTitle . T.dropWhile (/= '(')

-- | The title of the command is ideally the minimal explicit import decl, but
--------------------------------------------------------------------------------


Expand All @@ -465,10 +546,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
63 changes: 62 additions & 1 deletion plugins/hls-explicit-imports-plugin/test/Main.hs
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,43 @@ main = defaultTestRunner $ testGroup "import-actions"
"Make imports explicit"
[ codeActionAllGoldenTest "ExplicitUsualCase" 3 0
, codeActionAllResolveGoldenTest "ExplicitUsualCase" 3 0
, inlayHintsTest "ExplicitUsualCase" 2 $ (@=?)
[InlayHint
{ _position = Position {_line = 2, _character = 16}
, _label = InL "( a1 )"
, _kind = Just InlayHintKind_Type
, _textEdits = Just [TextEdit (Range (Position 2 0) (Position 2 16)) "import ExplicitA ( a1 )"]
, _tooltip = Nothing
, _paddingLeft = Just True
, _paddingRight = Nothing
, _data_ = Nothing
}]
, codeActionOnlyGoldenTest "ExplicitOnlyThis" 3 0
, codeActionOnlyResolveGoldenTest "ExplicitOnlyThis" 3 0
, inlayHintsTest "ExplicitOnlyThis" 3 $ (@=?)
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 test that we don't get code actions when we have inlay hints enabled and vice versa.

[InlayHint
{ _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 )"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, needs a comment, and maybe a test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

, _tooltip = Nothing
, _paddingLeft = Just True
, _paddingRight = Nothing
, _data_ = Nothing
}]
, codeLensGoldenTest notRefineImports "ExplicitUsualCase" 0
, codeActionBreakFile "ExplicitBreakFile" 4 0
, inlayHintsTest "ExplicitBreakFile" 3 $ (@=?)
[InlayHint
{ _position = Position {_line = 3, _character = 16}
, _label = InL "( a1 )"
, _kind = Just InlayHintKind_Type
, _textEdits = Just [TextEdit (Range (Position 3 0) (Position 3 16)) "import ExplicitA ( a1 )"]
, _tooltip = Nothing
, _paddingLeft = Just True
, _paddingRight = Nothing
, _data_ = Nothing
}]
, codeActionStaleAction "ExplicitStaleAction" 4 0
, testCase "No CodeAction when exported" $
runSessionWithServer def explicitImportsPlugin testDataDir $ do
Expand All @@ -49,6 +82,11 @@ main = defaultTestRunner $ testGroup "import-actions"
doc <- openDoc "ExplicitExported.hs" "haskell"
lenses <- getCodeLenses doc
liftIO $ lenses @?= []
, testCase "No InlayHints when exported" $
runSessionWithServer def explicitImportsPlugin testDataDir $ do
doc <- openDoc "ExplicitExported.hs" "haskell"
inlayHints <- getInlayHints doc (pointRange 3 0)
liftIO $ inlayHints @?= []
, testGroup "Title abbreviation"
[ testCase "not abbreviated" $
let i = "import " <> T.replicate 70 "F" <> " (Athing, Bthing, Cthing)"
Expand All @@ -72,6 +110,20 @@ main = defaultTestRunner $ testGroup "import-actions"
o = "import " <> T.replicate 80 "F" <> " (Athing, Bthing, ... (3 items))"
in ExplicitImports.abbreviateImportTitle i @?= o
]
, testGroup "Title abbreviation without module"
[ testCase "not abbreviated" $
let i = "import M (" <> T.replicate 70 "F" <> ", Athing, Bthing, Cthing)"
o = "(FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF, Athing, Bthing, Cthing)"
in ExplicitImports.abbreviateImportTitleWithoutModule i @?= o
, testCase "abbreviated that drop module name" $
let i = "import " <> T.replicate 120 "F" <> " (Athing, Bthing, Cthing)"
o = "(Athing, Bthing, Cthing)"
in ExplicitImports.abbreviateImportTitleWithoutModule i @?= o
, testCase "abbreviated in import list" $
let i = "import M (Athing, Bthing, " <> T.replicate 100 "F" <> ", Cthing, Dthing, Ething)"
o = "(Athing, Bthing, ... (4 items))"
in ExplicitImports.abbreviateImportTitleWithoutModule i @?= o
]
]]

-- code action tests
Expand Down Expand Up @@ -151,7 +203,7 @@ caTitle _ = Nothing
-- code lens tests

codeLensGoldenTest :: (CodeLens -> Bool) -> FilePath -> Int -> TestTree
codeLensGoldenTest predicate fp i = goldenWithImportActions " code lens" fp codeActionNoResolveCaps $ \doc -> do
codeLensGoldenTest predicate fp i = goldenWithImportActions " code lens" fp codeActionNoInlayHintsCaps $ \doc -> do
codeLenses <- getCodeLenses doc
resolvedCodeLenses <- for codeLenses resolveCodeLens
(CodeLens {_command = Just c}) <- pure (filter predicate resolvedCodeLenses !! i)
Expand All @@ -162,6 +214,15 @@ notRefineImports (CodeLens _ (Just (Command text _ _)) _)
| "Refine imports to" `T.isPrefixOf` text = False
notRefineImports _ = True

inlayHintsTest :: FilePath -> UInt -> ([InlayHint] -> Assertion) -> TestTree
inlayHintsTest fp line assert = testCase (fp ++ " inlay hints") $ runSessionWithServer def explicitImportsPlugin testDataDir $ do
doc <- openDoc (fp ++ ".hs") "haskell"
inlayHints <- getInlayHints doc (lineRange line)
liftIO $ assert inlayHints
where
-- zero-based position
lineRange line = Range (Position line 0) (Position line 1000)

-- Execute command and wait for result
executeCmd :: Command -> Session ()
executeCmd cmd = do
Expand Down
Loading