Completing 3-component module names doesn't work

[ndmitchell]

If you type in import Control.Concurrent.C then the suggested completion is Concurrent.Chan. If you select that completion you end up with import Control.Concurrent.Concurrent.Chan, which is definitely not what you expected. I run into this all the time since the Extra modules are typically 3 components long.

[ndmitchell]

I also note that there are no tests of module completions, but lots of others for different types of completion.

[mirokuratczyk]

Hello I took a quick look at this and these were my findings.

It seems that the issue occurs when code completion is requested upon
a completion trigger character being pressed -- "." in our case.

Subsequently a stale mapping is read from Shake and the cursor position is moved back one character.

Using the completion of Control.Monad. as an example (when the "." character is pressed).

This PosPrefixInfo value is supplied to getCompletions:

PosPrefixInfo {
  fullLine = "import Control.Monad.",
  prefixModule = "Control", 
  prefixText = "Monad", 
  cursorPos = Position {_line = 4, _character = 20}
} 

When I would expect the correct value to be:

PosPrefixInfo {
  fullLine = "import Control.Monad.",
  prefixModule = "Control.Monad",
  prefixText = "",
  cursorPos = Position {_line = 4, _character = 21}
}

The position is mapped back from Position {_line = 4, _character = 21} to Position {_line = 4, _character = 20} here.

From debugging a bit further I could see that Shake is reading the PositionMapping for one document version behind the current one in this scenario (TextDocumentVersion). This seems to only occur when a completion trigger character is pressed.

I also noticed that when a completion trigger character is pressed the completion request is received immediately (perhaps causing a race condition):

import Control.Monad
                   ^

NotificationMessage {_jsonrpc = "2.0", _method = TextDocumentDidChange, _params = DidChangeTextDocumentParams {_textDocument = VersionedTextDocumentIdentifier {_uri = Uri {getUri = "file://.../ghcide-test/src/Lib.hs"}, _version = Just 23}, _contentChanges = List [TextDocumentContentChangeEvent {_range = Just (Range {_start = Position {_line = 4, _character = 19}, _end = Position {_line = 4, _character = 19}}), _rangeLength = Just 0, _text = "d"}]}}
[DEBUG] Finishing build session(exception: AsyncCancelled)
[DEBUG] Restarting build session (aborting the previous one took 0.00s)
[INFO] Modified text document: file:///.../ghcide-test/src/Lib.hs
(req) RequestMessage {_jsonrpc = "2.0", _id = IdInt 21, _method = TextDocumentCompletion, _params = CompletionParams {_textDocument = TextDocumentIdentifier {_uri = Uri {getUri = "file:///.../ghcide-test/src/Lib.hs"}}, _position = Position {_line = 4, _character = 20}, _context = Just (CompletionContext {_triggerKind = CtInvoked, _triggerCharacter = Nothing}), _workDoneToken = Nothing}}
[DEBUG] completion request in file: .../ghcide-test/src/Lib.hs took 0.01s
(rsp) RspCompletion (ResponseMessage {_jsonrpc = "2.0", ...})

import Control.Monad.
                    ^

NotificationMessage {_jsonrpc = "2.0", _method = TextDocumentDidChange, _params = DidChangeTextDocumentParams {_textDocument = VersionedTextDocumentIdentifier {_uri = Uri {getUri = "file:///.../ghcide-test/src/Lib.hs"}, _version = Just 24}, _contentChanges = List [TextDocumentContentChangeEvent {_range = Just (Range {_start = Position {_line = 4, _character = 20}, _end = Position {_line = 4, _character = 20}}), _rangeLength = Just 0, _text = "."}]}}
(req) RequestMessage {_jsonrpc = "2.0", _id = IdInt 25, _method = TextDocumentCompletion, _params = CompletionParams {_textDocument = TextDocumentIdentifier {_uri = Uri {getUri = "file:///.../ghcide-test/src/Lib.hs"}}, _position = Position {_line = 4, _character = 21}, _context = Just (CompletionContext {_triggerKind = CtTriggerCharacter, _triggerCharacter = Just "."}), _workDoneToken = Nothing}}
[DEBUG] Finishing build session(exception: AsyncCancelled)
[DEBUG] Restarting build session (aborting the previous one took 0.00s)
[INFO] Modified text document: file:///.../src/Lib.hs
[DEBUG] completion request in file: /.../ghcide-test/src/Lib.hs took 0.00s
(rsp) RspCompletion (ResponseMessage {_jsonrpc = "2.0", ...})
w

So at this point the issue seems to be reading a stale mapping, but i'm not sure if the solution is:

• Do not use position mappings because the position supplied to getCompletionsLSP is already correct
• Use the position mapping for ProduceCompletions ¹
• There is an underlying issue with reading stale values that needs to be fixed

This is my first time looking at GHCIDE (and Shake) so I'm still a bit unsure about the relationship between Shake and the language server. Any guidance would be much appreciated!

---

¹
Reading the position mapping from useWithStale ProduceCompletions npath does seem to fix this issue, but i'm not sure if it causes any regressions.

diff --git a/src/Development/IDE/Plugin/Completions.hs b/src/Development/IDE/Plugin/Completions.hs
index 6111900..b8b9d04 100644
--- a/src/Development/IDE/Plugin/Completions.hs
+++ b/src/Development/IDE/Plugin/Completions.hs
@@ -87,7 +87,7 @@ getCompletionsLSP lsp ide
             pm <- useWithStale GetParsedModule npath
             pure (opts, liftA2 (,) compls pm)
         case compls of
-          Just ((cci', _), (pm, mapping)) -> do
+          Just ((cci', mapping), (pm, _)) -> do
             let !position' = fromCurrentPosition mapping position
             pfix <- maybe (return Nothing) (flip VFS.getCompletionPrefix cnts) position'
             case (pfix, completionContext) of

[ndmitchell]

Thanks for the diagnosis! I've not done much work in this area, but if any other maintainer has, it would be great to get some feedback.

My guess from your explanation is that the Shake bits and stale info aren't relevant. We got the correct full line from PosPrefixInfo. My guess is that PosPrefixInfo is misparsing the full line, and you are right about what it should be in this circumstance, and then we'd be correct. If so, that's a bug with the upstream haskell-lsp, and probably one missing case in a string splitting routine. But I'm totally guessing.

[mirokuratczyk]

If so, that's a bug with the upstream haskell-lsp, and probably one missing case in a string splitting routine.

The issue is not upstream in haskell-lsp, but in the cursor position provided to Language.Haskell.LSP.VFS.getCompletionPrefix.

Continuing with the previous example, the position argument (Position {_line = 4, _character = 20}) does not include the trailing character '.' in "import Control.Monad.", so getCompletionPrefix only processes import Control.Monad: https://github.com/alanz/haskell-lsp/blob/9447831943a21b2f6f593e5f7bf47cae41b7b486/src/Language/Haskell/LSP/VFS.hs#L284.

Here is the flow I have observed:

-- Latest document version is N
-- position = Position {_line = 4, _character = 21}
-- Line 4 of document version N contains `import Control.Monad.`
(ideOpts, compls) <- runAction ide $ do
  opts <- getIdeOptions
  compls <- useWithStale ProduceCompletions npath
  -- reads mapping for document version N-1
  pm <- useWithStale GetParsedModule npath
  pure (opts, liftA2 (,) compls pm)
case compls of
  Just ((cci', _), (pm, mapping)) -> do
    let !position' = fromCurrentPosition mapping position
    -- After applying mapping:
    -- position' = Position {_line = 4, _character = 20}
    pfix <- maybe (return Nothing) (flip VFS.getCompletionPrefix cnts) position'

[ndmitchell]

Is it possible we're confusing 0-based and 1-based indicies at some point leading to an off by one error? (I'll admit, I'm quite out of my depth here)

[poscat0x04]

I think this also affects modules with more segments, such as Control.Monad.IO.Class