Incorrect completion text when using DuplicateRecordFields

[expipiplus1]

Your environment

Output of haskell-language-server --probe-tools or haskell-language-server-wrapper --probe-tools:

❯ haskell-language-server --probe-tools
haskell-language-server version: 0.7.0.0 (GHC: 8.10.2) (PATH: /nix/store/r7bgp8sh4hm7x4r7nz3y3if9n74hv5zx-haskell-language-server-0.7.0.0/bin/haskell-language-server)
Tool versions found on the $PATH
cabal:          3.2.0.0
stack:          Not found
ghc:            8.10.2

Which lsp-client do you use:

This happens with nvim/coc and vscode

Steps to reproduce

In Foo.hs:

{-# language DuplicateRecordFields #-}
module Foo
  where

newtype Foo = Foo { member :: () }
newtype Bar = Bar { member :: () }

in Bar.hs:

module Bar where

import Foo

foo = memb

• Trigger a completion for memb
• Observe that member is suggested twice (not useful)
• Accept either one of the suggestions for member
• Observe that the text :member:Foo is inserted instead of member

I suppose that this is some incorrect demangling of the $sel:Foo:member names that GHC uses.

[jneira]

As #680 and #681 i would fix the new completion plugin about to be added with #670

[expipiplus1]

This patch to ghcide fixes things:

diff --git a/src/Development/IDE/Plugin/Completions/Logic.hs b/src/Development/IDE/Plugin/Completions/Logic.hs
index f91a3f7..19f5a78 100644
--- a/src/Development/IDE/Plugin/Completions/Logic.hs
+++ b/src/Development/IDE/Plugin/Completions/Logic.hs
@@ -647,7 +647,7 @@ openingBacktick line prefixModule prefixText Position { _character }
 -- don't want in the autocompleted symbols
 stripAutoGenerated :: CompItem -> CompItem
 stripAutoGenerated ci =
-    ci {label = stripPrefix (label ci)}
+    ci {label = stripPrefix (label ci), insertText = stripPrefix (insertText ci)}
     {- When e.g. DuplicateRecordFields is enabled, compiler generates
     names like "$sel:accessor:One" and "$sel:accessor:Two" to disambiguate record selectors
     https://ghc.haskell.org/trac/ghc/wiki/Records/OverloadedRecordFields/DuplicateRecordFields#Implementation

This does sadly break the record completion snippet though.

I'm not sure what caused this regression, but the recent change nearby is 36a2f0027e73a756d10eb72b2dc36a1711c6802f. @gdevanla what are your thoughts?

[jneira]

@expipiplus1 if #685 fixes it too, i hope it keeps record completion snippet ok

[gdevanla]

@expipiplus1 It breaks the record completion snippet, since the completion snippets that are values in insertText also have : in them. I think we need to address this issue while constructing CompItem.

[gdevanla]

I think this fixes it. But, we need to make sure it does not break any other cases:

diff --git a/src/Development/IDE/Plugin/Completions/Logic.hs b/src/Development/IDE/Plugin/Completions/Logic.hs
index 42f91ad..ab52074 100644
--- a/src/Development/IDE/Plugin/Completions/Logic.hs
+++ b/src/Development/IDE/Plugin/Completions/Logic.hs
@@ -191,7 +191,7 @@ mkNameCompItem origName origMod thingType isInfix docs !imp = CI{..}
     insertText = case isInfix of
             Nothing -> case getArgText <$> thingType of
                             Nothing -> label
-                            Just argText -> label <> " " <> argText
+                            Just argText -> (stripPrefix label) <> " " <> argText
             Just LeftSide -> label <> "`"

@expipiplus1 Do you plan on working on this fix?

[expipiplus1]

I think this fixes it. But, we need to make sure it does not break any other cases:

...

Checking this out now, surely it leaves things less broken than they are currently; and isn't that what tests are for :D

@expipiplus1 Do you plan on working on this fix?

Not sure I can spare the cycles at this busy time of year I'm afraid, also there seems to be other work going on in similar places at the moment.

[gdevanla]

Ok. Thanks. I can work on a quick fix on this issue.

[gdevanla]

@expipiplus1 One other question on your initial comment:

Observe that member is suggested twice (not useful)

I am curious why you did not find this useful. Each of the entries are qualified with respective record declarations as you see here.

[expipiplus1]

Ah, It's because I don't display that information and forgot that that might be the default, so I just see a bunch of repeated lines with the same purpose (to insert cczz)

Ok. Thanks. I can work on a quick fix on this issue.

Thanks! FWIW the patch you posted earlier is working very well.

[gdevanla]

I think this issue is fixed and can be closed.

[expipiplus1]

Thanks!

[jneira]

@gdevanla @expipiplus1 thanks for confirming it, closing!

[yan-sh]

This problem still exists(

[jneira]

Ugh, we have to revise this then, a regression test would be definitely needed
//cc @gdevanla