Fix weird behavior of OPTIONS_GHC completions (fixes #3908)

[jhrcek]

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.

1. modify getCompletionPrefix to accept larger prefix (seems too dangerous, as it's used by a lot of code already)?
2. create custom version of "prefix finder" tailored to ghc options (my plan)?

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

[michaelpj]

getCompletionPrefix is mis-named: it finds completion prefixes for Haskell identifiers. So yeah, we need a custom one. Maybe we can make it more generic, not sure?

Also note that it's moved to Logic.hs in ghcide, I'm going to get rid of the one in lsp at some point, it should never have been there.

[michaelpj]

The use in the cabal plugin is likely also wrong for this reason!

[jhrcek]

Here's how it works after the fix in this PR:

[jhrcek]

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

[michaelpj]

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

[fendor]

The use in the cabal plugin is likely also wrong for this reason!

IIRC, hls-cabal-plugin introduces its own completion prefix function due to / in filepaths.

[michaelpj]

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

[michaelpj]

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?

[jhrcek]

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

[michaelpj]

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.

[michaelpj]

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.

[jhrcek]

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 😄

[jhrcek]

I did my best to reuse the results of getCompletionPrefix and avoid repeated work.
Let me know if it looks good. I'll merge this as is in a day or two if there's no response.