Completion result causing error when navigating over a completion with a snippet

[ckipp01]

I hit on this today which I believe used to work previously 🤔, but let's say that I'm going to go through the list of completions returned and one of those completions is a snippet, it seems to put the actual new text in the buffer including the $0 which ends up not being rendered correctly as you filtering through them, which causes an error in this case. I'll try to demonstrate exactly what I mean.

Given the following small Scala snippet:

val option: Option[Int] = Some(3)

option matc@@

If I look at the completion results at that point while attempting to completion the match, there is the following completion item being returned from the server (Metals).

...
    {
      "label": "match",
      "kind": 12,
      "detail": "",
      "preselect": true,
      "sortText": "00000",
      "filterText": "match",
      "insertTextFormat": 2,
      "textEdit": {
        "range": {
          "start": {
            "line": 31,
            "character": 6
          },
          "end": {
            "line": 31,
            "character": 7
          }
        },
        "newText": "match {\n\tcase$0\n}"
      },
      "data": {
        "symbol": "local1",
        "target": "file:/Users/ckipp/Documents/scala-workspace/tester/?id\u003droot"
      }
    },
...

When you hit on this option, the following is put into the buffer:

option match {^@ case Some(value) =>

The server then complains since \u0000 is not valid for it and was sent to the server during the textDocument/didChange. Also, you then see the following message in the message window:

Error executing vim.schedule lua callback: Vim:E731: using Dictionary as a String

I hope this is enough to give you an idea of what may be going on, but if not, don't hesitate to have me dive into something to help. In other clients (coc.nvim), it seems that instead of showing the newText here as you are going through the completion items, they just show the label, and then when you select it, then it enters the new text. I've also included a gif of what I'm talking about up above down below:

My testing minimal init.vim Here is an init.vim to use

call plug#begin('~/.vim/plugged') 
    Plug 'neovim/nvim-lsp'
    Plug 'haorenW1025/completion-nvim'
    Plug 'scalameta/nvim-metals'
call plug#end()
lua require'nvim_lsp'.metals.setup{on_attach=require'completion'.on_attach}
au Filetype scala setl omnifunc=v:lua.vim.lsp.omnifunc
" Use <Tab> and <S-Tab> to navigate through popup menu
inoremap <expr> <Tab>   pumvisible() ? "\<C-n>" : "\<Tab>"
inoremap <expr> <S-Tab> pumvisible() ? "\<C-p>" : "\<S-Tab>"

" Set completeopt to have a better completion experience
set completeopt=menuone,noinsert,noselect

How to reproduce

1. I've included an example repo here for you to use. There is a file in there called CompletionError.scala that you can try the above completion on.
2. Install metals via :LspInstall metals
3. Open the build.sbt at the root and do a :BuildImport
4. Once completion go to CompletionError.scala and attempt the completion referenced above.

[haorenW1025]

Yeah I can reproduce. I'll look into it.

[haorenW1025]

Hmm I spent some time look into it and I can't find what went wrong... but changing the label instead of newText seems to fix this, maybe I'll fix this that way.

[ckipp01]

Hmm I spent some time look into it and I can't find what went wrong... but changing the label instead of newText seems to fix this, maybe I'll fix this that way.

I think that's a sensible fix.

[haorenW1025]

I think I find the issue. When inserting a complete items with line break in it, vim actually think it is a list instead of a string. Substitute out the line break seems to fix that (at least on my side). Can you try the PR and see if it's fixed?

[ckipp01]

Thanks for the quick work. I just tried and it no, it doesn't seem to fix it. It fixes the error from appearing, but it no longer applies the edit correctly. It seems that it replaces the new line, but then cuts off the rest. This is worsened when there are multiple new lines. For example, trying to complete the following:

   {
      "label": "match (exhaustive)",
      "kind": 7,
      "detail": " Option[Int] (2 cases)",
      "sortText": "00001",
      "filterText": "match (exhaustive)",
      "insertTextFormat": 2,
      "textEdit": {
        "range": {
          "start": {
            "line": 31,
            "character": 9
          },
          "end": {
            "line": 31,
            "character": 12
          }
        },
        "newText": "match {\n\tcase Some(value) \u003d\u003e $0\n\tcase None \u003d\u003e\n}"
      },
      "data": {
        "symbol": "scala/Option#",
        "target": "file:/Users/ckipp/Documents/scala-workspace/tester/?id\u003droot"
      }
    }

This is what happens:

So it no longer actually applies the new line and also cuts off the rest.

[haorenW1025]

Ahh that's because I use a lsp snippet expansion plugins(vim-vsnip) so everything work smooth on my side...So I think the proper solution will be use label as insert word if there's no snippet expansion tools available, otherwise use newText and trimmed off new line.

[haorenW1025]

@ckipp01 I've made some update. Right now it should be using label if you don't have vim-vsnip install. Can you try again?

[ckipp01]

Hi @haorenW1025 I just gave it another try and I still get the same behavior as shown in the gif above.

[haorenW1025]

I've been trying to use label first and then apply textEdit in completeDone. However one thing that bothers me is the cursor position after text edit is off. I've tried to set cursor position to textEdit.range.end but it's not correct place. @ckipp01 Any thought on that?

[ckipp01]

I've tried to set cursor position to textEdit.range.end but it's not correct place. @ckipp01 Any thought on that?

Well this isn't always the case though right? For example, given the following which originally caused the issue:

{
      "label": "match",
      "kind": 12,
      "detail": "",
      "preselect": true,
      "sortText": "00000",
      "filterText": "match",
      "insertTextFormat": 2,
      "textEdit": {
        "range": {
          "start": {
            "line": 31,
            "character": 6
          },
          "end": {
            "line": 31,
            "character": 7
          }
        },
        "newText": "match {\n\tcase$0\n}"
      },
      "data": {
        "symbol": "local1",
        "target": "file:/Users/ckipp/Documents/scala-workspace/tester/?id\u003droot"
      }
    }

If you would end with the cursor at the range.end it would be at the end of the actual full textEdit, but instead it should end up where the $0 is correct? So I'm actually not fully sure how other clients handle this, but I'm assuming it needs to detect the position of the $0 and end the cursor there.

[haorenW1025]

Yeah that's definitely correct. Currently completion-nvim don't have the ability to parse the lsp snippets' placeholder and stuff..so I'll have to look into how to get that. But considering another case, for example, I get this completion from metals when typing op

   newText = "optManifest",
   range = {
     end = {
       character = 5,
       line = 5
     },
     start = {
       character = 2,
       line = 5
     }
   }

It confuse me because putting cursor in newText.range.end is definitely wrong.

[ckipp01]

🤔 hmmm, yea that doesn't look correct. I'll look at this on the Metals side.

[clason]

@haorenW1025 Would it be possible to disable snippets (i.e., filter out snippet candidates) via a configuration option for those people that don't have a compatible snippet engine installed? That would eliminate the annoying error message -- and prevent useless results in the candidate list -- without (hopefully) much work.

[ckipp01]

don't have a compatible snippet engine installed

Well this is interesting I guess. This is a wider conversation than completion-nvim, but when initializing nvim is saying capabilities.textDocument.completion.snippetSupport: true. So the question is whether or not that's valid and if it's fair to assume that with the client passing this in, snippets are indeed supported whether or not a user has a "compatible snippet engine installed". Shouldn't there be an assumption that they should just work irregardless of third party extensions if that's what the client is passing in?

[clason]

@ckipp01 You're right, that would be the better place to do this -- I forgot about this capability. Do you recall off-hand how to (or whether it is even possible to) override this in nvim-lsp's setup function?

(There's an argument to be made for this to be opt-in rather than opt-out, i.e., default to false.)

[ckipp01]

(There's an argument to be made for this to be opt-in rather than opt-out, i.e., default to false.)

Well imo I sort of disagree. Capabilities are just that, stating whether or not the client has the capability to do something, not whether the user wants it. I don't think a users preference should have any effect on the stated capabilities. If a user wants to disable them for example, then that should be a setting in the editor and or server, but it shouldn't affect what the client is saying it's capable of doing.

[clason]

I agree with you there; my point was that without separately installing a suitable snippet plugin, it's arguable that neovim does not (fully) support snippets and should therefore not advertise the capability.

I don't have a strong opinion on this and just wanted to raise the point for discussion. (It seems not every server respects that flag anyway...)

[ckipp01]

I agree with you there; my point was that without separately installing a suitable snippet plugin, it's arguable that neovim does not (fully) support snippets and should therefore not advertise the capability.

Ah, gotcha, yea that's true and a good point.

I don't have a strong opinion on this and just wanted to raise the point for discussion. (It seems not every server respects that flag anyway...)

Again true. The only reason I somewhat do have a strong opinion on it is that in Metals we use simple snippets quite it a bit in various multi-line completions, so it doesn't take long for a user to hit on this.

(It seems not every server respects that flag anyway...)

True, but that's a server issue, not a client one 😏

[clason]

True, but that's a server issue, not a client one 😏

Indeed. But you can either throw up your hands and grudgingly accept this, or look for a client-side workaround ;) (which brings us back to my initial suggestion).

And clearly you should not penalize servers (and client setups) that do work!

[haorenW1025]

I've tried filtering out non snippets item out before, but the problem is most of the server doesn't respect the spec...so the insertFormat is always 2 (which means that it should be a snippet items)..So filtering out base on that will cause more trouble and more issue. I'm considering adding some basic snippet expansion which doesn't fully support jumping between placeholder but will do the expansion stuff.