Snippets should be passed to snippet managers instead of completion plugins

andreyorst commented 6 years ago

Summary

LC Detects UltiSnips snippet manager https://github.com/autozimu/LanguageClient-neovim/blob/d690d2b62fb161266ccce15c3160760530f3c085/autoload/LanguageClient.vim#L36-L40 and creates snippets, that are listed in completion popup via deoplete, but upon completion it just inserts placeholders as text. See screenshots below: Just text completed function name: Now I selected [LC] source in the list: After completion confirm I left with plaintext placeholders inside function definition:

Reproduction

neovim/vim version: NVIM v0.2.3-987-g2cbeb7ca5
This plugin version: d690d2b
This plugin's binary version: languageclient 0.1.68
Minimal vimrc: minimalrc.txt
File to try to reproduce: main.rs.txt
Language server link and version: rls-preview 0.125.1-stable (cebf188 2018-03-19)
Steps to reproduce the issue from clean state
1. Download minimalrc.txt and main.rs.txt (you will need vundle to install plugins (sorry), and :UpdateRemotePlugins)
2. Rename main.rs.txt to main.rs (damn github)
3. nvim -u /path/to/minimalrc.txt /path/to/main.rs
4. Try to call vaiv() inside main's body
  - type va, completion menu by deoplete pops up.
  - press Tab to complete vaiv's body
  - look at placeholders: vaiv(${1:a}, ${2:b})
Logs. Paste or attach /tmp/LanguageClient.log and /tmp/LanguageServer.log. LanguageClient.log LanguageServer.log

Current Behavior

Expected Behavior

There were some disscussion about it here: https://github.com/Shougo/deoplete.nvim/issues/724#issuecomment-381312149 There currently discussion abut supporting LSP snippets in ultisnips here: https://github.com/SirVer/ultisnips/issues/964

andreyorst commented 6 years ago

Also happens with cquery v20180302-6 (2, 1.35) build from AUR

I'm even not sure that such snippet even make sence, because it should be just vaiv(${1:a}, ${2:b})$0, because having types inside braces isn't make any sence on function call.

Edit: I've seen bunch of issues here, that are referring to neovim-completion-manager, neosnippet, etc. But they are all closed, however the issue is still presists

andreyorst commented 6 years ago

With fresh install of nvim-completion-manager I got even more interesting results:

However I won't plan to use ncm anyway, but still. This need fixing.

autozimu commented 6 years ago

I believe in all those cases, you can jump to snippet placeholders with your defined shortcut <c-j>, right?

andreyorst commented 6 years ago

No. It's plain text. Not a snippet placeholder.

YaLTeR commented 6 years ago

It works with nvim-completion-manager and UltiSnips but it requires a rather complex setup.

andreyorst commented 6 years ago

@YaLTeR what you refering to as "works" is just a workaround of the problem. LC inserts snippet placeholders as text, instead of integrating with snippet managers. I already have a complex setup to create seampless completion and jumping via one key for 3 plugins (deoplete, ultisnips, delimitmate), and I'd better avoid another one for another plugin. The main Idea that if plugin searches for snippet managers, means that plugin supports them. Without complex configurations. Like NCM + NCM Clang + Ultisnips do. It just works. Parameter expansion like no problem. Plug and play.

But I don't want to use NCM because it has LOT of problems. For example it perceives c style // comments as path completion. Wich is stupid. Plain text completion has lesser priority then file completion, and if I want for snippet the first item will be lost + found found somewhere in the path, wich is stupid too. And many other stupid bugs like displaying completion on scrolling. Even fuzzy matching algorithms are poor. None of theese problems occur when using deoplete, so I prefer to stick with good tools. Sorry for offtopic.

I'd like to replace ALE in my current configuration with LC because LC is much faster, and has more features, like renaming symbols, good semantic completion, etc etc, but I don't want to loose snippets too.

autozimu commented 6 years ago

Right now, the right completion (kind of a dynamic snippet) is provided to completion engine, completion engine indeed inserts requested completion item in the right place, and one can jump to snippet placeholder(s) with predefined shortcut(s) after completion.

I do appreciate if the whole experience is smoother, i.e., it should automatically jump to the first placehoder after completion if the completed item is a snippet. But there are a few concerns about making the described behaviour happen in this repo,

The jumping after completion is more closely related to completion engine, such as deoplete and NCM. They should be able to recognize if the completed item is a snippet and act accordingly.
There are multiple completion engines and multiple snippet managers out there.

To be frank, I don't feel too motivated to work on this area because of those previously mentioned concerns.

Maybe https://github.com/Shougo/deoppet.nvim will make the situation better.

andreyorst commented 6 years ago

@autozimu You can define a snippet and pass it to snippet manager instead of completion manager. Because snippets that are provided by snippet manager are displayed in completion manager popup, you can expand it via snippet manager plugin mappings, because it is it's work. Not a work of completion plugin.

So there is no need to pass snippet to completion system. You need to pass a trigger only to completion engine (or even nothing, because trigger is contained in snippet definition, so it will be listed in completion list automatically), wich in this case will be function name only (vaiv in this case). Then completion engine will find a snippet that you have passed to snippet engine. And user will desire, does he want to trigger a snippet with his snippet manager, or does he want just complete the name of the function. That's how it done in NCM for example.

So

The jumping after completion is more closely related to completion engine

Wrong. It is related to Snippet engine only.

There are multiple completion engines and multiple snippet managers out there

And most completion managers are already well integrated with most of snippet managers.

So only implementation of passing snippets to snippet managers is needed. Most of snippet engines provide a way to pass a snippet definition to them in realtime. And in case that your plugin already searches for snippet managers, and defines a snippet, you need only to change where to pass it. Because passing it to completion system is wrong in first place.

autozimu commented 6 years ago

As I said, I won't be actively working on this.

PR is always welcome.

andreyorst commented 6 years ago

Then disable searching for snippet plugins because it impossible to use LCN with it. Or make a setting to disable it, because It is absolutely unusable when you need to edit every completion to get rid of placeholders that are just plain text.

andreyorst commented 6 years ago

IIUUC your current implementation looks like this:

Lang Server creates a snippet → LCN → Completion plugin → Snippet plugin

So if user doesn't use any completion plugin, but uses snippet plugin (as it doesn't need a completion system, because it only displays the triggers in it) User simply can't use snippets because they are passed to nowhere.

If your plugin will pass it directly to snippet plugin (wich is make sence, because snippet should be passed to snippet manager, and completion should be passed to completion manager) any user, with or without completion plugin will be able to use such snippets. I'm proposing a change to:

Lang Server creates a snippet → LCN → Snippet Manager

The rest is up to snippet manager, it will pass snippet's trigger to completion system automatically, because it is usual behavior. And user will decide if he want to expand it, or just plain complete the trigger. I'ts not only makes it easier to everyone to implemet and maintain this, but more logical too, because each plugin will to the work it was designed to do.

You may wonder why you would decide to expand function name only? There are some situations like so:

// fancy function pointer typedef
typedef int (*lambda)();

// lets use this function via pointer in main()
int vaiv(int a, int b, int c)
{
    return a + b + c;
}

So for example in main i would like to have function name only, to pass it's address to function pointer:

int main()
{
    lambda ptr_to_vaiv = vaiv; // Imagine I've completed name only
    return ptr_to_vaiv(1, 2, 3); // Now I expand the snippet defined by Lang Server
}

But with your current realisation I will have:

int main()
{
    lambda ptr_to_vaiv = vaiv(${1:int a}, ${2:int b}, ${3:int c})$0| // Jeez...
}

Where | is cursor position at the end of the string right after completion.

andreyorst commented 6 years ago

i.e., it should automatically jump to the first placehoder after completion

And this is not what i;m asking for at all. Because it is snippet manager work too. And it will work out of the box with proposed change, because user will trigger a snippet himself. Even without completion plugin.

YaLTeR commented 6 years ago

This does make sense actually, typing the whole name of the function and hitting the expand snippet key doesn't seem to involve a completion manager plugin in any way. It's kinda tied together though because language servers return snippets as part of completion responses, not by themselves.

andreyorst commented 6 years ago

@YaLTeR Yes, you even don't need to type the whole name of the function, because you can complete it with vim's builtin completion, wich doesn't even need to integrate with LC, and then hit expand snippet button, because basicaly you completed a trigger's text

It's kinda tied together though because language servers return snippets as part of completion responses, not by themselves.

Well I guess that language client, wich recieves the completion from language server, should decide where to pass it, as it client's job to handle interfaces between vim, pugins and lang server. If it is snippet - to snippet manager. If it is semantic completion - to completion manager.

andreyorst commented 6 years ago

@autozimu @YaLTeR I've made a simple snippet manager, and developed a function that should allow other plugins to communicate with it. Take a look: adding flash snippet

Here I define a snippet by hand, by calling a function: SimpleSnippets#addFlashSnippet('some_func', 'some_func(${1:a}, ${0:b})') which accepts a trigger and a snippet's body as a parameters (i've removed last line count parameter, as I decided that it is function who need to calculate that). Current example shows, that completion plugin doesn't involved at all.

I'm not asking of implementation of usage of this function in LCN, but just want to illustrate how I think it should work:

LCN calls a SimpleSnippets#addFlashSnippet() function
LCN tells completion plugin that there is a trigger avalible (optional step, most snippet managers also support this. If not, LCN can help here)
User is typing a trigger and hits his expand key - SimpleSnippets looks for defined Flash Snippets, and expands one if found.

chemzqm commented 6 years ago

I think there's need to clarify the responsibility, LCN should better not bundled to specified function/plugin, it could detect the complete item text format from server response and just pass the complete item info with text format to complete plugin. It's complete plugin's job to make the snippet works.

chemzqm commented 6 years ago

@autozimu snippetSupport should better not set true for now with initialize params

03:16:25 INFO Handler-main src/vim.rs:135 => {"jsonrpc":"2.0","method":"initialize","params":{"capabilities":{"textDocument":{"completion":{"completionItem":{"commitCharactersSupport":null,"documentationFormat":null,"snippetSupport":true},"dynamicRegistration":null}}},"initializationOptions":null,"processId":83753,"rootPath":"/Users/chemzqm/wechat-dev/xinjian","rootUri":"file:///Users/chemzqm/wechat-dev/xinjian","trace":"off"},"id":9}

since it's not working.

andreyorst commented 6 years ago

It's complete plugin's job to make the snippet works.

Snippet managers don't need completion plugins at all. You're adding an unneeded route in snippet path from LCN to snippet manager, wich will make snippets unawailible for people who simply not use completion plugins, and are just use lang server for linting and other features like renaming symbol.

how it works now without LCN in UltiSnips for example: Ultisnips sends deoplete information about trigger, and user can complete it and then expand. So the path is:

UltiSnips->deoplete->user

lets add LCN. I'm proposing this path: LCN sends snippet to Ultisnips,Ultisnipssends triggerto deoplete,usercompletes trigger, and expands snippet. The path is:

LCN->Ultisnips->deoplete->user

Now your proposed way of doing it: LCN passes snippet to deoplete, deoplete passes snippet to ultisnips, ultosnips passes trigger to deoplete, user completes trigger, and expands snippet. Deoplete involved 2 times. Without deopletr snippet will never arrave to ultisnips.

LCN->deoplete->ultisnips->deoplete->user

Everyone can use ultisnips without completion plugin. Why someone should add a completion plugin as a dependency to LCN snippets?

chemzqm commented 6 years ago

@andreyorst the snippet of LSP is one kind of complete item in LSP side, it could comes with other plain text complete items, how could you make user to choose complete item if different kinds of complete item goes to different plugin? The snippet plugin could just provide an extra API for complete plugin to eval the snippet complete item, or the complete plugin eval the snippet itself, since the snippet complete item contains every snippet information needed.

Everyone can use ultisnips without completion plugin. Why someone should add a completion plugin as a dependency to LCN snippets?

It's not LCN snippets, snippets of LSP comes from complete, but snippet manager doesn't provide complete feature

YaLTeR commented 6 years ago

I suppose one way of handling this would be to feed the snippets from the language server to the snippet plugin, then strip them from any snippet parts and feed to the completion plugin. This way the user would select the completion from the completion plugin (if they have one) and then expand it with the snippet plugin (if they have one).

chemzqm commented 6 years ago

@YaLTeR that could work, but then LanguageClient-neovim would have to implement adapters of each complete plugin for feed the snippet, and the user have to manually trigger complete shortcut to complete the snippet instead of just <C-y> or <enter> to complete the snippet.

For user experience and simplification, the snippet support of LSP item should comes from completion plugin or vim/neovim natively.

chrisduerr commented 6 years ago

I don't feel like putting this job on completion managers is the right choice. I'd love to see this working with LCN because otherwise the placeholders are kinda useless, but I don't think it will work out nicely if we just expect completion engines to suddenly handle snippets too.

chemzqm commented 6 years ago

There's no easy way for vim plugin to support this feature, so I created a feature request for neovim: https://github.com/neovim/neovim/issues/8334

chrisduerr commented 6 years ago

Thanks @chemzqm, I'd love to see this work with LCN. It would truly step it up to another level of convenience for me.

lisongmin commented 6 years ago

I workaround the problem in UltiSnips side as follow in vimrc. Hope it can help somebody who urgently need a temporary solution.

function! ExpandLspSnippet()
    call UltiSnips#ExpandSnippetOrJump()
    if !pumvisible() || empty(v:completed_item)
        return ''
    endif

    " only expand Lsp if UltiSnips#ExpandSnippetOrJump not effect.
    let l:value = v:completed_item['word']
    let l:kind = v:completed_item['kind']
    let l:abbr = v:completed_item['abbr']

    " remove inserted chars before expand snippet
    let l:end = col('.')
    let l:line = 0
    let l:start = 0
    for l:match in [l:abbr . '(', l:abbr, l:value]
        let [l:line, l:start] = searchpos(l:match, 'b', line('.'))
        if l:line != 0 || l:start != 0
            break
        endif
    endfor
    if l:line == 0 && l:start == 0
        return ''
    endif

    let l:matched = l:end - l:start
    if l:matched <= 0
        return ''
    endif

    exec 'normal! ' . l:matched . 'x'

    if col('.') == col('$') - 1
        " move to $ if at the end of line.
        call cursor(l:line, col('$'))
    endif

    " expand snippet now.
    call UltiSnips#Anon(l:value)
    return ''
endfunction

imap <C-k> <C-R>=ExpandLspSnippet()<CR>

I'm not familiar with vimL and UltiSnips, it should much simple than what i written. I hope there is an official solution, either LCN or snippet side.

butterflyfish commented 6 years ago

@lisongmin, this workaround duplicated func name. example, exec.Command(${1:name string}, ${2:arg ...string})$0 after kick it will be changed to exec.CommandCommand(name string, arg ...string)

lisongmin commented 6 years ago

How to reproduce? the patch work fine with me:

ctrl + n selecet complettion
ctrl + k will auto snippet like this:

20180602t160817_1130x78_scrot

by the way, I just test it under linux.

butterflyfish commented 6 years ago

steps are the same as yours, but language is golang. I found it works well in c function, e.g.

    fcntl(${1:int}, ${2:int, ...})$0
will be changed to 
    fcntl(int, int, ...)

is it possible caused by prefix exec. ?

lisongmin commented 6 years ago

@butterflyfish can your try this version? I remove the inserted chars with len() now, and also work fine with me.

function! ExpandLspSnippet()
    call UltiSnips#ExpandSnippetOrJump()
    if !pumvisible() || empty(v:completed_item)
        return ''
    endif

    " only expand Lsp if UltiSnips#ExpandSnippetOrJump not effect.
    let l:value = v:completed_item['word']
    let l:matched = len(l:value)
    if l:matched <= 0
        return ''
    endif

    " remove inserted chars before expand snippet
    if col('.') == col('$')
        let l:matched -= 1
        exec 'normal! ' . l:matched . 'Xx'
    else
        exec 'normal! ' . l:matched . 'X'
    endif

    if col('.') == col('$') - 1
        " move to $ if at the end of line.
        call cursor(line('.'), col('$'))
    endif

    " expand snippet now.
    call UltiSnips#Anon(l:value)
    return ''
endfunction

imap <C-k> <C-R>=ExpandLspSnippet()<CR>

butterflyfish commented 6 years ago

perfectly. works now. thank you very much!

butterflyfish commented 6 years ago

@lisongmin how about sending MR to upstream UltiSnips?

lisongmin commented 6 years ago

I'd like to do so, but i don't known how to remove inserted candidate words in UltiSnips way. It should simpler than the workaround.

hkmix commented 6 years ago

Here's a slight tweak to @lisongmin's tweak to allow the use of the Enter key:

Change the initial return to return a newline:

    if !pumvisible() || empty(v:completed_item)
        return '^M'
    endif

where ^M is the output of <C-v><C-m>. Then you can map <CR>:

imap <silent> <CR> <C-r>=ExpandLspSnippet()<CR>

And after selecting a completion you can hit Enter to start the expansion.

languitar commented 6 years ago

Is there a chance to get first placeholder to work as well? If I use the proposed "hack", the cursor is correctly placed on the first placeholder, but it is not selected and thus cannot be replace by starting to type.

butterflyfish commented 6 years ago

it's recommended to use https://github.com/ncm2/ncm2-ultisnips

languitar commented 6 years ago

@butterflyfish How does that relate to deoplete, which I have been using so far?

andreyorst commented 6 years ago

@autozimu the conversation went off topic. Should I close the issue, or you'll lock the conversation?

TylerHorth commented 6 years ago

I made an autocommand based on the CompleteDone event which some of you may find useful.

let g:ulti_expand_res = 0 "default value, just set once
function! CompleteSnippet()
  if empty(v:completed_item)
    return
  endif

  call UltiSnips#ExpandSnippet()
  if g:ulti_expand_res > 0
    return
  endif

  let l:complete = type(v:completed_item) == v:t_dict ? v:completed_item.word : v:completed_item
  let l:comp_len = len(l:complete)

  let l:cur_col = mode() == 'i' ? col('.') - 2 : col('.') - 1
  let l:cur_line = getline('.')

  let l:start = l:comp_len <= l:cur_col ? l:cur_line[:l:cur_col - l:comp_len] : ''
  let l:end = l:cur_col < len(l:cur_line) ? l:cur_line[l:cur_col + 1 :] : ''

  call setline('.', l:start . l:end)
  call cursor('.', l:cur_col - l:comp_len + 2)

  call UltiSnips#Anon(l:complete)
endfunction

autocmd CompleteDone * call CompleteSnippet()

These are the mappings I use, although any mappings should work with the autocommand:

imap <silent><expr> <tab> pumvisible() ? "\<c-y>" : "\<tab>"

let g:UltiSnipsExpandTrigger="<NUL>"
let g:UltiSnipsListSnippets="<NUL>"
let g:UltiSnipsJumpForwardTrigger="<tab>"
let g:UltiSnipsJumpBackwardTrigger="<s-tab>"

yshui commented 6 years ago

Why are we parsing the placeholders in vimL when we can do it in LanguageClient-neovim....

MaskRay commented 5 years ago

g:LanguageClient_hasClientSupport should just default to 0 as LSP-style snippets are not supported.

For these completion/snippet manager, when they claim they have LSP/LanguageClient-neovim integration, they should make clear if

https://microsoft.github.io/language-server-protocol/specification

InsertTextFormat.Snippet is supported and if they can interpret

interface CompletionItem insertText?: string; (deprecated) or textEdit?: TextEdit; (used by ccls and clangd)

roxma commented 5 years ago

FWIW, there're some differences between ultisnips/neosnippet/snipmate syntax and lsp snippet syntax.

The best way for integration, I believe, is to first parse the lsp snippet, and then convert it to your native snippet code. I have written a small parser, and as a result, ultisnips/neosnippet/snipmate can integrate perfectly with ncm2.

https://github.com/ncm2/ncm2-neosnippet https://github.com/ncm2/ncm2-ultisnips https://github.com/ncm2/ncm2-snipmate

The parser and the plugins listed above are all written from scratch, and MIT licensed. Feel free to reuse some of the code for your own use case.

Shougo commented 5 years ago

It is fixed in the latest version of neosnippet.

@autozimu The issue can be closed.

chrisduerr commented 5 years ago

I just tested this with Rust and it works amazingly well.

Thanks for your work @Shougo!

autozimu commented 5 years ago

@Shougo Thanks!

autozimu commented 5 years ago

Verified. Closing.

autozimu / LanguageClient-neovim