allow LSP insert text to replace non-matching prefixes (#5469)

[Taywee]

closes #5469

Most LSPs will complete case-insensitive matches, particularly from lowercase to uppercase. In some cases, notably Pyright, this is given as a simple insert text instead of TextEdit. When this happens, the prefix text was left unedited.

This change seems to fix it for me. I'm not sure if there are other side effects to doing it this way, though. If another LSP gave an insert text that was just the rest to be inserted, I imagine this would still cause problems. But the previous method would also fail in that case with repeated text. If you typed su and the suggestion was surration, it would be ambiguous in that case whether that was to be inserted (making susurration) or replaced entirely (making surration).

[gabydd]

In my brief testing this seems to work really well, It think it would be good to test this on a wide range of lsp's. Also it should fix #4131 and maybe some of the related pr's, the issues that have the completion inserted in between text like #4851 probably isn't fixed by this.

[Taywee]

In a quick test, this does not seem to fix #4131. It looks a bit to me like gopls has bad completion in the first place.

Given this input:

input

2023-01-10T13:43:53.835 helix_lsp::transport [INFO] -> {"jsonrpc":"2.0","method":"textDocument/didChange","params":{"contentChanges":[{"range":{"end":{"character":8,"line":3},"start":{"character":8,"line":3}},"text":"["}],"textDocument":{"uri":"file:///tmp/tmp.APmRKN78A3/hxtest.go","version":14}}}
2023-01-10T13:43:54.283 helix_lsp::transport [INFO] -> {"jsonrpc":"2.0","method":"textDocument/didChange","params":{"contentChanges":[{"range":{"end":{"character":9,"line":3},"start":{"character":9,"line":3}},"text":"s"}],"textDocument":{"uri":"file:///tmp/tmp.APmRKN78A3/hxtest.go","version":15}}}
2023-01-10T13:43:54.459 helix_lsp::transport [INFO] -> {"jsonrpc":"2.0","method":"textDocument/didChange","params":{"contentChanges":[{"range":{"end":{"character":10,"line":3},"start":{"character":10,"line":3}},"text":"t"}],"textDocument":{"uri":"file:///tmp/tmp.APmRKN78A3/hxtest.go","version":16}}}
2023-01-10T13:43:54.471 helix_lsp::transport [INFO] -> {"jsonrpc":"2.0","method":"textDocument/completion","params":{"position":{"character":11,"line":3},"textDocument":{"uri":"file:///tmp/tmp.APmRKN78A3/hxtest.go"}},"id":6}

The response contains this:

response

      {
        "label": "string",
        "labelDetails": {},
        "kind": 7,
        "sortText": "00029",
        "filterText": "string",
        "insertTextFormat": 1,
        "textEdit": {
          "range": {
            "start": {
              "line": 3,
              "character": 11
            },
            "end": {
              "line": 3,
              "character": 11
            }
          },
          "newText": "string"
        }
      }

I'm not sure at the moment how other client LSPs handle it. I'll give it a shot with nvim real quick.

[Taywee]

In nvim, the gopls suggested response is:

{ filterText = "string", insertTextFormat = 2, kind = 7, label = "string", labelDetails = vim.empty_dict(), sortText = "00029", textEdit = { newText = "string", range = { ["end"] = { character = 11, line = 3 }, start = { character = 11, line = 3 } } } }

So nvim handles the textEdit differently, too. The gopls issue seems unrelated to what this PR addresses.

[pascalkuthe]

I looked at how nvim-cmp handles this and the LSP spec again and I am pretty sure this is correct. To quote the spec:

Completion item provides an insertText / label without a text edit: in the model the client should filter against what the user has already typed using the word boundary rules of the language (e.g. resolving the word under the cursor position). The reason for this mode is that it makes it extremely easy for a server to implement a basic completion list and get it filtered on the client.

start_offset is the start of the previous word so everything up to that should be replaced.

This will technically generate transactions that are a bit larger then they need to be but I doubt that will realistically ever matter as completions are usually pretty small.

[pascalkuthe]

The golps issue is a similar but unrelated issue as they do send an text edit

[Taywee]

Thanks for the spec reference. I'm still curious how nvim is handling the gopls case, because the textEdit range appears to not be correct, and nvim still gets the right result. According to the next section in the spec, a TextEdit with the same start and endpoints should be a plain insert. I might be simply missing something, but I guess that's not totally relevant to this PR or issue anyway.

[pascalkuthe]

Thanks for the spec reference. I'm still curious how nvim is handling the gopls case, because the textEdit range appears to not be correct, and nvim still gets the right result. According to the next section in the spec, a TextEdit with the same start and endpoints should be a plain insert. I might be simply missing something, but I guess that's not totally relevant to this PR or issue anyway.

I have been digging intot his and at this point I am pretty sure its actually an upstream issue with the language server. Clearly sending and empty edit range is supposed to be an insertion and they have been receiving and (sometimes) fixing issues for that too. In particular I found the following commdnt:
golang/go#57384 (comment)

I think the response here is odd. They are quoting the lsp standard and say that what they do.is not standard compliant and that is the reason for the bug but then they go on to state that vscode handels this fine and they don't see a reason it shouldn't work so it's ok? Why even have a standard then?

We could add some kind of workaround (we should port it from vscode if anybody can find where it's happening) although I am in favor of reporting it upstream instead of.bending.over backwards for non standard compliant severs.

I also looked at nvim-cmp and Laplace (gave up with vscode):

• Laplace always extends the removed text to the full word no matter what the edit range says (not spec compliant)
• nvim-cmp used to accidently do the same but got.patched, after that people complained and somebody posted a weird fix thathaopens to work but it seem like it directly.violates the spec and they just didn't notice because the result is fine most of.the time.regardless. There is also no explanation od how/why this wokrs. During PR review the maintainer asked his/why it works, the author said he doesn't know and it got merged anyway? :D

[Taywee]

Wow. So no major LSP client is actually spec compliant on TextEdit completions, including VSCode. That's surprising. I'd rather be correct and complain upstream than violate the spec and paper over the difference.

Based on the linked issue, it does seem like they might have fixed the issue in gopls in the main branch as of a few weeks ago: golang/go#57384

[pascalkuthe]

Emacs LSP actually also has an open issue about it. They handle it the same way helix does and there are others too (for nvim alone there are like 4? LSP completions clients or more and some of those gopls issue were filed by nvim people so some of them probably handle this correctly). I didn't bother to look for more implementations tough.

I do agree that we should stick to the standard.
I hope gpls fixes this upstream. It sounded like that the issue I linked was only about some completions for tests tough. I am not sure if they solved it in general or just for that specific case as that issue you mentioned about the general case was closed as duplicate

[Taywee]

Right, I'm mostly just shocked about VSCode. If one implementation should be fully spec compliant, it should be the editor that the standard was created for.

[archseer]

This probably also solves #1819 since we replace everything from the starting offset onwards

[pascalkuthe]

This probably also solves #1819 since we replace everything from the starting offset onwards

This change only affects LSP servers that do not send a TextEdit with their completions and leave it up to the editor what text to replace. Most LSPs do send an edit (according to the spec the option of not sending an exit only exists to allow quick imolemtmstion of basic servers) and are unaffectd by this change. So at least to those servers the problem persists.

[pascalkuthe]

I missed something while reviewing this PR: It removes the ability to complete at multiple cursors at once. I included this PR in #5728 as I was already making multiple changes to the same code that would cause conflicts and fixed that problem. I kept the commit from this PR to give credit.

[archseer]

Was included in #5728