mini.completion: expand snippets with tabstops, variables, and also TABS?

[rmuir]

Background: trying out mini.completion and it does not play well with ansible-language-server.

Completions contain raw tabs (\t), accepting any completion causes raw tab to be added as-is to the document (instead of adjusted for indentation), breaking yaml format and causing new diagnostics.

Server Problems:

• server sends completion responses of textedits with raw tabs (\t). Does not check that client can handle this correctly :) I think this happens to work for vscode because it behaves differently with indentation, e.g. uses InsertTextMode=2
• server doesn't indicate snippet format of the textedit until 'resolve'. I could be wrong, but this seems really broken to me, but might be easy to get fixed.

Client Problem:

• imagine some of above were fixed: mini still doesn't expand textedits of snippet format unless they have variables or tabstops. This means snippets with \t but no variables or tabstops won't be expanded with correct indentation. This part also seems wrong to me?

Should the logic maybe be adjusted to also look for tabs? I checked that many friendly-snippets have \t in them. Did not check any other LSP servers, and it will not solve my issue with this language server on its own, but may improve compatibility in general.

snippet-hack.patch

[echasnovski]

I know there is already a lot of details in here, but there is something I am missing to fully understand the issue. Could you, please, provide a reproducible example of the issue you describe? It can be with ansible-language-server (in which case it requires exact file, cursor position, and sequence of key presses needed to repro). But I'd appreciate an example of an actual snippet body together with text and cursor position to where this snippet is trying to be inserted.

Quick test showed that I did not have issues with preserving indentation (although \t did insert literally and there were some other issues with matching). Here is what I just did to try to see:

• Create the '~/.config/nvim/snippets/global.json' file with the following contents:

  {
    "Just text": {
      "prefix": "ctext",
      "body": "Just some\n\tMulti\n\tLine\nText",
      "description": "Multiline text"
    }
  }

• Use the following 'mini.snippets' setup:

  local snippets, config_path = require('mini.snippets'), vim.fn.stdpath('config')
  snippets.setup({
    snippets = {
      snippets.gen_loader.from_file(config_path .. '/snippets/global.json'),
    },
  })
  MiniSnippets.start_lsp_server()

• Start new Neovim session and in an empty buffer in Insert mode type <Space><Space>abc<CR> (this would, probably, preserve two space indentation on a new line; if not - make sure that it is there).

• Press <C-Space>. This shows the single available snippet from the 'mini.snippets' LSP server. Yes, it is not marked as snippet.

• Press <C-n> to select the entry. It makes it appear in the buffer.

• Press <C-y> to accept candidate. This result in the following buffer text (before Multi and Line there are literal \t, but they are indented):

    abc
    Just some
    	Multi
    	Line
    Text
  

Server Problems:

Yes, both look like a server issues. Especially not providing insert kind before 'resolve'. This is against LSP specification and will not work in 'mini.completion'.

[echasnovski]

So the problem is not quite "This means snippets with \t but no variables or tabstops won't be expanded with correct indentation.", which I understood that there is an issue with preserving indentation of current line? It is only that 'mini.completion' inserts \t literally and not expands it into spaces? If yes, then no repro needed.

Do you think that ansible-language-server is specialized enough to "know" that it is not a good idea to use literal \t in places where only space indentation is supported?

[rmuir]

Here is example repro with a screenshot. if you name it playbook.yml and open it with ansible_ls you should be able to see it.

# Basic idea, create a new list element (like shown below) under "tasks":
#
# ```
# - name: test
#   asser
# ```
#
# After typing 'asser' you should have completion item of "ansible.builtin.assert" available.
# after accepting completion you should have:
#
# - name: test
#   ansible.builtin.assert:
#     <-- cursor indented (NO TABS). if you type 't' it should then complete 'that:' ... and so on
- name: Example
  hosts: localhost
  gather_facts: false
  tasks:
    - name: test
      ansible.builtin.assert:
        that: [ true ]

Maybe this screenshot also helps if you look closely at the listchars. You can see there is no issue with indentation, the problem is just the raw tab. It breaks the language-server's parsing:

• causes it to not suggest "that:" as an option, even though it is mandatory
• suggests other things instead that are invalid
• issue diagnostic about malformed yaml

The inserted \t also causes a treesitter parsing error and incorrect highlighting.

[rmuir]

It is only that 'mini.completion' inserts \t literally and not expands it into spaces? If yes, then no repro needed.

Yes. If i force it to be expanded as snippet (needs_snippet_insert) then there is no issue.

Do you think that ansible-language-server is specialized enough to "know" that it is not a good idea to use literal \t in places where only space indentation is supported?

It knows to emit the diagnostic about it, so I think so. Looking at the code, I think they just made it work for vscode. Language server does not even check that client supports snippets. But I had in mind that if i sent them a one-line PR to set the format as snippet in the first place, it might be the easiest fix, it would just still require a change on the mini side to ensure it is treated that way.

[echasnovski]

... it would just still require a change on the mini side to ensure it is treated that way.

At first glance, I think it makes sense here to restrict number of cases when candidate is marked as snippet but 'mini.completion' does not treat it as such. I'd go even further and make the extra check not for tabstop, but for any whitespace (word:find('%s') ~= nil). If it is marked as a snippet and contains a whitespace, then it probably needs to be treated as a snippet.

It still seems to work for the use case that prompted this workaround solution (described in the comment; like LSP server marking every candidate as snippet, even just words).

[rmuir]

Thank you for working thru it with me! I can try to work up PRs for this one, might be a couple of days.

[echasnovski]

The 'mini.completion' part of this should now be resolved on latest main. In the end, I decided to go the middle road and treat only newlines and tabs as "snippet features that require explicit snippet_insert call".

Thanks for pointing out this use case!

[rmuir]

Thank you! draft PR to the language server: ansible/vscode-ansible#2215

It works with your latest commit: will see what the maintainers say.

[echasnovski]

I don't think "Many clients (e.g. neovim mini.completion) cannot defer the insertTextFormat property until completionItem/resolve:" is the best reason for this change.

There is a dedicated resolveSupport field of client capabilities which lists all the fields that client can handle supplied in completionItem/resolve response. All other properties that are not included in that list must be provided in textDocument/completion response.

So if insertTextFormat is absent in resolveSupport property array, then it must be supplied in textDocument/completion response or never at all. Otherwise the server violates the LSP specification.

[rmuir]

I worded the text in the way I did after looking at their sources and tests :) Of course it is a violation of specification, but the server is not even tested in that way and I'm honestly not sure the project is concerned with it or even knows about it. I guess it is a bit of a gamble, I will definitely re-use your argument if needed.