Fix weird behavior of OPTIONS_GHC completions (fixes #3908)#4031
Conversation
|
Also note that it's moved to |
|
The use in the cabal plugin is likely also wrong for this reason! |
f76ca86 to
1835e21
Compare
1835e21 to
0c81fb2
Compare
|
Here's how it works after the fix in this PR:
|
|
|
||
| -- grab the entire line the cursor is at | ||
| curLine <- headMaybe $ T.lines $ Rope.toText | ||
| curLine <- headMaybe $ Rope.lines |
There was a problem hiding this comment.
This is supposed to be faster than Text.lines, so why not use it here as well..
https://hackage.haskell.org/package/text-rope-0.2/docs/Data-Text-Lines.html#v:lines
There was a problem hiding this comment.
Yes! We really need to audit all uses of Rope.toText, maybe even give it a hlint hint. It's generally unnecessarily expensive and does work proportional to the length of the document...
IIRC, hls-cabal-plugin introduces its own completion prefix function due to |
|
|
||
| -- grab the entire line the cursor is at | ||
| curLine <- headMaybe $ T.lines $ Rope.toText | ||
| curLine <- headMaybe $ Rope.lines |
There was a problem hiding this comment.
Yes! We really need to audit all uses of Rope.toText, maybe even give it a hlint hint. It's generally unnecessarily expensive and does work proportional to the length of the document...
| | "{-# options_ghc" `T.isPrefixOf` line | ||
| = map buildCompletion | ||
| (Fuzzy.simpleFilter (VFS.prefixText pfix) flags) | ||
| = let flagPrefix = getGhcOptionPrefix cursorPos cnts |
There was a problem hiding this comment.
The logic here feels a little bad; we're passing in pfix here and no longer using it at all. Maybe we can refactor it a bit?
There was a problem hiding this comment.
What do you mean by "refactor a bit"?
I did localized change in fear of breaking existing functionality.
I can look into rewriting the whole thing so that the pragmas plugin doesn't use VFS.getCompletionPrefix from lsp package at all. I actually didn't notice there were two versions of that (one in ghcide and one in lsp).
Or did you mean something like pulling out this options_ghc branch from the result helper and have structure like this?
- result <$> VFS.getCompletionPrefix position cnts
+
+ (result <$> VFS.getCompletionPrefix cursorPos cnts)
+ <|>
+ (mkGhcOptionCompletions <$> getGhcOptionCompletionPrefix cursorPos cnts))and treating option prefixes
There was a problem hiding this comment.
Yes, that's what I meant. I didn't meant to say that the pragmas plugin shouldn't be using the generic getCompletionPrefix function (although that's almost certainly true, for the same reasons!), just that the particular branch that you're changing now sits oddly.
I actually didn't notice there were two versions of that (one in ghcide and one in lsp).
I'm going to kill the lsp one, so ignore that.
There was a problem hiding this comment.
On reflection, I don't want to block this on doing the refactoring, so I'm happy to leave it up to you whether you want to do that now or not. If not, maybe leave a note explaining the weirdness.
There was a problem hiding this comment.
I'll get back to this and try if I can clean it up. It just needs a bit more focus time that I'm unlikely to get during workdays 😄
0c81fb2 to
bfabdef
Compare
1c74f2a to
8b02ab4
Compare
8b02ab4 to
7942f99
Compare
|
I did my best to reuse the results of getCompletionPrefix and avoid repeated work. |
Reproducer for #3908
The issue is that getCompletionPrefix on this line doesn't correctly identify the "already typed prefix of the ghc option".
It thinks the prefix is only the part up to '-' (e.g. in the reproducer it tries to fuzzy match prefix "fo" instead of "dsuppress-constant-fo" against existing flags), which can be also seen in the suggestion dropdown (note only "fo" is highlighted).
I'll take some time over weekend to try to fix this. In the meantime I welcome any suggestions as to what would be the proper way to fix this.
NOTE: although the tests seem to pass, it's due to issue on master due to which most tests are not running - this is being resolved in #4028