From d51c68dcf7a418bde4d02f62345710f298168328 Mon Sep 17 00:00:00 2001 From: Fendor Date: Mon, 9 Dec 2024 16:24:00 +0100 Subject: [PATCH 1/2] Always handle "completion/resolve" requests Resolves the LSP "completion/resolve" requests, even if there is nothing more to resolve. If there is nothing to resolve, we simply return the initial completion item. If we reject "completion/resolve" requests, then some LSP clients show our rejection as pop-ups which are hugely distracting. We change completion tests to always resolve completions, regardless of the existence of `_data_`, as this seems to be the behaviour of VSCode. --- .../src/Development/IDE/Plugin/Completions.hs | 1 + .../IDE/Plugin/Completions/Logic.hs | 6 ++++-- .../IDE/Plugin/Completions/Types.hs | 19 ++++++++++++++----- ghcide/test/exe/CompletionTests.hs | 19 ++++++++++++------- 4 files changed, 31 insertions(+), 14 deletions(-) diff --git a/ghcide/src/Development/IDE/Plugin/Completions.hs b/ghcide/src/Development/IDE/Plugin/Completions.hs index 0564855177..2482dc3903 100644 --- a/ghcide/src/Development/IDE/Plugin/Completions.hs +++ b/ghcide/src/Development/IDE/Plugin/Completions.hs @@ -127,6 +127,7 @@ dropListFromImportDecl iDecl = let in f <$> iDecl resolveCompletion :: ResolveFunction IdeState CompletionResolveData Method_CompletionItemResolve +resolveCompletion _ide _pid comp _uri NothingToResolve = pure comp resolveCompletion ide _pid comp@CompletionItem{_detail,_documentation,_data_} uri (CompletionResolveData _ needType (NameDetails mod occ)) = do file <- getNormalizedFilePathE uri diff --git a/ghcide/src/Development/IDE/Plugin/Completions/Logic.hs b/ghcide/src/Development/IDE/Plugin/Completions/Logic.hs index 9fdc196cd5..cc21daca54 100644 --- a/ghcide/src/Development/IDE/Plugin/Completions/Logic.hs +++ b/ghcide/src/Development/IDE/Plugin/Completions/Logic.hs @@ -229,7 +229,9 @@ mkCompl _additionalTextEdits = Nothing, _commitCharacters = Nothing, _command = mbCommand, - _data_ = toJSON <$> fmap (CompletionResolveData uri (isNothing typeText)) nameDetails, + _data_ = Just $ toJSON $ case nameDetails of + Nothing -> NothingToResolve + Just nameDets -> CompletionResolveData uri (isNothing typeText) nameDets, _labelDetails = Nothing, _textEditText = Nothing} removeSnippetsWhen (isJust isInfix) ci @@ -309,7 +311,7 @@ mkExtCompl label = defaultCompletionItemWithLabel :: T.Text -> CompletionItem defaultCompletionItemWithLabel label = CompletionItem label def def def def def def def def def - def def def def def def def def def + def def def def def def def def (Just $ toJSON NothingToResolve) fromIdentInfo :: Uri -> IdentInfo -> Maybe T.Text -> CompItem fromIdentInfo doc identInfo@IdentInfo{..} q = CI diff --git a/ghcide/src/Development/IDE/Plugin/Completions/Types.hs b/ghcide/src/Development/IDE/Plugin/Completions/Types.hs index 2d950d66a9..ef0cb896b0 100644 --- a/ghcide/src/Development/IDE/Plugin/Completions/Types.hs +++ b/ghcide/src/Development/IDE/Plugin/Completions/Types.hs @@ -199,10 +199,19 @@ instance Show NameDetails where -- | The data that is actually sent for resolve support -- We need the URI to be able to reconstruct the GHC environment -- in the file the completion was triggered in. -data CompletionResolveData = CompletionResolveData - { itemFile :: Uri - , itemNeedsType :: Bool -- ^ Do we need to lookup a type for this item? - , itemName :: NameDetails - } +data CompletionResolveData + = NothingToResolve + -- ^ The client requested to resolve a completion, but there is nothing to resolve, + -- as we can't add any additional information, such as docs. + -- We still handle these requests, otherwise HLS will reject "completion/resolve" requests + -- which present on some clients (e.g., emacs) as an error message. + -- See https://github.com/haskell/haskell-language-server/issues/4451 for the issue that + -- triggered this change. + | CompletionResolveData + -- ^ Data that we use to handle "completion/resolve" requests. + { itemFile :: Uri + , itemNeedsType :: Bool -- ^ Do we need to lookup a type for this item? + , itemName :: NameDetails + } deriving stock Generic deriving anyclass (FromJSON, ToJSON) diff --git a/ghcide/test/exe/CompletionTests.hs b/ghcide/test/exe/CompletionTests.hs index 8b90244b76..bafc0ad8ac 100644 --- a/ghcide/test/exe/CompletionTests.hs +++ b/ghcide/test/exe/CompletionTests.hs @@ -557,19 +557,24 @@ completionDocTests = ] let expected = "*Imported from 'Prelude'*\n" test doc (Position 1 7) "id" (Just $ T.length expected) [expected] + , testSessionEmpty "defined in where clause" $ do + doc <- createDoc "A.hs" "haskell" $ T.unlines + [ "module A where" + , "bar = foo" + , " where foobar = 5" + ] + let expected = "*Defined at line 3, column" + test doc (Position 1 9) "foobar" (Just $ T.length expected) [expected] ] where test doc pos label mn expected = do _ <- waitForDiagnostics compls <- getCompletions doc pos rcompls <- forM compls $ \item -> do - if isJust (item ^. L.data_) - then do - rsp <- request SMethod_CompletionItemResolve item - case rsp ^. L.result of - Left err -> liftIO $ assertFailure ("completionItem/resolve failed with: " <> show err) - Right x -> pure x - else pure item + rsp <- request SMethod_CompletionItemResolve item + case rsp ^. L.result of + Left err -> liftIO $ assertFailure ("completionItem/resolve failed with: " <> show err) + Right x -> pure x let compls' = [ -- We ignore doc uris since it points to the local path which determined by specific machines case mn of From 19387b4716a03b3c097dc9f84c51a0a6a6dd93d4 Mon Sep 17 00:00:00 2001 From: Fendor Date: Tue, 10 Dec 2024 09:11:03 +0100 Subject: [PATCH 2/2] Expand documentation for 'CompletionResolveData' Add explanations and justification for 'NothingToResolve' constructor. Also, include historical note to explain how we end up with this 'CompletionResolveData' version. --- .../src/Development/IDE/Plugin/Completions.hs | 8 ++++++- .../IDE/Plugin/Completions/Types.hs | 24 +++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/ghcide/src/Development/IDE/Plugin/Completions.hs b/ghcide/src/Development/IDE/Plugin/Completions.hs index 2482dc3903..903779b335 100644 --- a/ghcide/src/Development/IDE/Plugin/Completions.hs +++ b/ghcide/src/Development/IDE/Plugin/Completions.hs @@ -127,7 +127,13 @@ dropListFromImportDecl iDecl = let in f <$> iDecl resolveCompletion :: ResolveFunction IdeState CompletionResolveData Method_CompletionItemResolve -resolveCompletion _ide _pid comp _uri NothingToResolve = pure comp +resolveCompletion _ide _pid comp _uri NothingToResolve = + -- See docs of 'NothingToResolve' for the reasoning of this + -- NO-OP handler. + -- + -- Handle "completion/resolve" requests, even when we don't want to add + -- any additional information to the 'CompletionItem'. + pure comp resolveCompletion ide _pid comp@CompletionItem{_detail,_documentation,_data_} uri (CompletionResolveData _ needType (NameDetails mod occ)) = do file <- getNormalizedFilePathE uri diff --git a/ghcide/src/Development/IDE/Plugin/Completions/Types.hs b/ghcide/src/Development/IDE/Plugin/Completions/Types.hs index ef0cb896b0..d0899943b4 100644 --- a/ghcide/src/Development/IDE/Plugin/Completions/Types.hs +++ b/ghcide/src/Development/IDE/Plugin/Completions/Types.hs @@ -207,6 +207,30 @@ data CompletionResolveData -- which present on some clients (e.g., emacs) as an error message. -- See https://github.com/haskell/haskell-language-server/issues/4451 for the issue that -- triggered this change. + -- + -- Clients (i.e., VSCode) request completion items to be resolved if + -- we advertise capabilities for it. + -- However, HLS used to resolve completion items for only a subset of completion items, + -- and reject requests for which there is no additional info available, + -- e.g. function local variables don't have any docs or similar, so we didn't + -- respond to "completion/resolve" requests for these completion items. + -- We rejected these requests by *not* adding `_data_` (i.e., `_data_ = Nothing), + -- which caused HLS to reject "completion/resolve" requests, as we can only thread + -- requests to the appropriate plugin if this `_data_` field is populated and + -- can be deserialised. + -- Since this `_data_` was missing, we rejected the request with: + -- + -- @ + -- Error processing message (error "No plugins are available to handle this SMethod_CompletionItemResolve request. + -- Plugins installed for this method, but not available to handle this request are: + -- ghcide-completions does not handle resolve requests for (unable to determine resolve owner))."). + -- @ + -- + -- However, this proved to be annoying as some clients (i.e., emacs) display + -- our request rejection prominently in the user interface. + -- As this is annoying, we insert this "dummy" data 'NothingToResolve', + -- which allows HLS to thread the "completion/resolve" request to this plugin. + -- The plugin then simply returns the original 'CompletionItem', | CompletionResolveData -- ^ Data that we use to handle "completion/resolve" requests. { itemFile :: Uri