Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UltiSnips integration #306

Merged

Conversation

thomasfaingnaert
Copy link
Collaborator

I added a new variable g:lsp_ultisnips_integration which, when set to 1, uses UltiSnips to provide LSP snippet functionality.
If g:lsp_ultisnips_integration = 0 (the default), vim-lsp should continue to function in exactly the same way as it does now, so this PR should not break anything.
This requires UltiSnips to be installed, and Vim 8.0 patch 1493 or higher (for user_data).

A couple notes:

  • Because UltiSnips deletes all tabstops when switching buffers, you have to remove preview from 'completeopt'. Alternatively, you can try Fix snippets breaking when opening preview window SirVer/ultisnips#1055.
  • If you want snippets to expand when its trigger is the only match, you need to add menuone to 'completeopt'. This is because in that case, the CompleteDone autocommand is called with v:completed_item = {} instead of the completed item. If anyone knows another fix for this, please let me know.
  • UltiSnips does not implement the complete LSP spec. For example, choice placeholders will just be displayed verbatim. However, because I am most familiar with UltiSnips as a vim snippet plugin, I decided to use it instead of other solutions. Besides, most of the important features are present.
  • I tested this with omnifunc, however because I am using CompleteDone, I believe this should also work with asyncomplete.vim.

Demo with C++ in clangd:
demo

Minimal vimrc to reproduce:

call plug#begin()

Plug 'SirVer/ultisnips'
Plug 'prabirshrestha/async.vim'
Plug 'thomasfaingnaert/vim-lsp', {'branch': 'ultisnips-integration'}

call plug#end()

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

if executable('clangd')
    augroup vim_lsp_cpp
        autocmd!
        autocmd User lsp_setup call lsp#register_server({
                    \ 'name': 'clangd',
                    \ 'cmd': {server_info->['clangd']},
                    \ 'whitelist': ['c', 'cpp', 'objc', 'objcpp', 'cc'],
                    \ })
        autocmd FileType c,cpp,objc,objcpp,cc setlocal omnifunc=lsp#complete
    augroup end
endif

set completeopt-=preview
set completeopt+=menuone

let g:lsp_ultisnips_integration = 1

@thomasfaingnaert
Copy link
Collaborator Author

Concerning the failing CI build: the issue is this snippet:

execute 'let l:user_data = ' . a:item['user_data']

let s:trigger = l:user_data[0]
let s:snippet = l:user_data[1]

which produces autoload/lsp/omni.vim:228:21: Undefined variable: l:user_data (see :help E738).

This snippet is used to convert a string back to an array, which was needed because user_data must be a string.
One way to fix this would be to add an explicit assignment let l:user_data = '' before the execute, but perhaps there is a better way to do the conversion from string to array?

@thomasfaingnaert thomasfaingnaert changed the title Ultisnips integration UltiSnips integration Feb 17, 2019
@prabirshrestha
Copy link
Owner

Because none of the vim snippets actually support lsp I'm a bit hesitant to support this out of the box. Similar to how autocomplete works with asyncomplete.vim and deplolete, we can easily make this work with an external plugin.

  1. Expose a function for to override capabilities so someone outside vim-lsp can set it.
  2. Allow lsp#omni#get_vim_completion_item to be overriden so someone outside vim-lsp can do whatever they want. They can manually set CompleteDone to handle snippet trigger.

This allows to have external plugins such as vim-lsp-ultisnips, vim-lsp-neosnippet and so on.

Copy link
Owner

@prabirshrestha prabirshrestha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add suggestions and comments.

@@ -201,7 +207,35 @@ function! lsp#omni#get_vim_completion_item(item, ...) abort
endif
endif

if exists('l:snippet')
let l:completion['user_data'] = string([l:trigger, l:snippet])
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

user_data is only available in certain version of vim. We need to check if vim has that patch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In plugin/lsp.vim, I already check if Vim has('patch-8.0.1493'). Would you prefer I set g:lsp_ultisnips_integration back to 0 there and/or add an additional has('patch-8.0.1493') to if exists('l:snippet')?

let s:trigger = l:user_data[0]
let s:snippet = l:user_data[1]

call timer_start(0, function('s:expand_snippet'))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why do we need a timer here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure why, but without a timer the tabstops in nested snippets do not work:
without-timer

It seems only the innermost snippet's tabstops are preserved.

@thomasfaingnaert
Copy link
Collaborator Author

I agree, perhaps it would be a better idea to separate the snippet plugin dependent logic in a separate plugin, and allow that plugin to change the behaviour of vim-lsp using an API.
How do you imagine such an interface might look like?

@prabirshrestha
Copy link
Owner

My thought was to make it simple as this.

let g:lsp_get_vim_completion_item = get(g:, 'myplugin#utilsnips#get_vim_completion_item', 'lsp#omni#get_vim_completion_item')

We might want to rename existing lsp#omni#get_vim_completion_item to lsp#omni#get_default_vim_completion_item so get_vim_completion_item can actually call g:lsp_get_vim_completion_item internally. Note: we do need to make sure we don't break asyncomplete-lsp.vim.

@thomasfaingnaert
Copy link
Collaborator Author

So if I understand correctly, you're proposing the following:

In vim-lsp:

  • Move all existing logic to lsp#omni#get_default_vim_completion_item(item, ...)
  • Introduce a global variable g:lsp_get_vim_completion_item that points to the completion item provider function that should be used
  • Change lsp#omni#get_vim_completion_item(item, ...) to just forward its arguments to a call to g:lsp_get_vim_completion_item
  • Add let g:lsp_get_vim_completion_item = get(g:, 'myplugin#ultisnips#get_vim_completion_item', 'lsp#omni#get_default_vim_completion_item') to vim-lsp for all snippet plugin integrations
  • Add snippet support to the server's capabilities, e.g. depending on the value of a boolean variable g:lsp_enable_snippets

In vim-lsp-ultisnips:

  • Let myplugin#ultisnips#get_vim_completion_item(item, ...) call lsp#omni#get_default_vim_completion_item(item, ...), and change the user_data of its return value
  • Register a CompleteDone autocmd to expand the snippet

If so, would it not be better to just set g:lsp_get_vim_completion_item to lsp#omni#get_default_vim_completion_item in vim-lsp, and let vim-lsp-ultisnips change g:lsp_get_vim_completion_item to its own provider myplugin#... when it is loaded? That way, writing a new snippet plugin integration does not require changing vim-lsp's source.

@prabirshrestha
Copy link
Owner

That is true. It is possible but there is quite a logic that could be reused by others from get_default_vim_completion_item, unless you want to copy paste the code.

@prabirshrestha
Copy link
Owner

Instead of g:lsp_enable_snippets might be wroth adding that as function too so people can add other custom capabilities but that does mean a bit more work but a snippet plugin can handle it so others don't have to worry about it,

@natebosch
Copy link

Some snippet managers, like neosnippet, allow using CompleteDone and reading from user_data on a completed item in order to expand a snippet. I don't know if UltiSnips has this built in but I'd imagine it would be feasible to build something similar. You might consider going this route so that it's up to the snipper plugin to handle and read user_data however it wants, and the implementation here is not tied to any one plugin.

You can see an example of this approach and some discussion here: natebosch/vim-lsc#142

@thomasfaingnaert
Copy link
Collaborator Author

@natebosch Thanks for the info! The way I'm rewriting it now is to allow external plugins to provide their own implementation of get_vim_completion_item and to allow them to specify the client capabilities. That way, an external plugin can customize get_vim_completion_item to set user_data to whatever it desires, add snippets to the capabilities, and register its own CompleteDone autocmd.
Of course, the custom versions of these functions will be able to call the default vim-lsp version of these functions (to avoid code duplication).
I think this would be the most general solution to provide snippet integration.

@prabirshrestha
Copy link
Owner

I would still prefer func instead of setting snippet in user_data as the snippet format is not defined.
Asyncomplete supports other completion besides LSP so we need a way to differentiate it.

@thomasfaingnaert
Copy link
Collaborator Author

@prabirshrestha I'm not entirely sure what you mean exactly. I don't change textEdit support, and I set textDocument.completion.completionItem.snippetSupport to true in both vim-lsp-ultisnips and vim-lsp-neosnippet.

@thomasfaingnaert
Copy link
Collaborator Author

@prabirshrestha Any update on this?

@prabirshrestha
Copy link
Owner

@thomasfaingnaert Seems to be conflicting a bit with this PR to enable textEdits since both use user data and seems to be conflicting. #318

I do want both the PRs to be merged and enable both use case. Looking at both PRs we might have to merge the text edit PR first since it doesn't expose dependencies to others.

//cc @mikoto2000

@prabirshrestha
Copy link
Owner

Now that TextEdit for omnicomplete is supported can you try this against the latest master. #318

plugin/lsp.vim Outdated Show resolved Hide resolved
@prabirshrestha
Copy link
Owner

@thomasfaingnaert Do you have a minimal vimrc as well as a langserver that I can try.

@thomasfaingnaert
Copy link
Collaborator Author

@prabirshrestha I've mainly been using clangd, which you can install from http://releases.llvm.org/download.html or from your package manager if you're on *NIX (clang-7 and clang-tools-7 on Debian based distros).

Minimal vimrc
call plug#begin()

Plug 'SirVer/ultisnips'
Plug 'prabirshrestha/async.vim'
Plug 'thomasfaingnaert/vim-lsp', {'branch': 'ultisnips-integration'}
Plug 'thomasfaingnaert/vim-lsp-ultisnips'

call plug#end()

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

if executable('clangd')
    augroup vim_lsp_cpp
        autocmd!
        autocmd User lsp_setup call lsp#register_server({
                    \ 'name': 'clangd',
                    \ 'cmd': {server_info->['clangd']},
                    \ 'whitelist': ['c', 'cpp', 'objc', 'objcpp', 'cc'],
                    \ })
	autocmd FileType c,cpp,objc,objcpp,cc setlocal omnifunc=lsp#complete
    augroup end
endif

set completeopt+=menuone
Example test.cpp
#include <iostream>
#include <vector>

int main(int argc, char **argv)
{
	// type: std::co<C-x><C-o>
	// to get: std::cout
	
	// type: std::vec<C-x><C-o><C-y>int<Tab> v;
	// to get: std::vector<int> v;

	// type: vect.pu<C-x><C-o><C-y>30<Tab>;
	// to get: vect.push_back(30);
	std::vector<int> vect;

	// type: for<C-x><C-o><C-y>int i = 0<Tab>i<10<Tab>i++<Tab>// ...
	// to get: for(int i = 0;i<10;i++){
	// //...
	// }
}

@prabirshrestha
Copy link
Owner

Thanks for the instructions. I will be out on vacation for 2 weeks, will give it a try when I come back.

@mikoto2000
Copy link
Contributor

What do you think adding snippet plugin integration support API?
I think more easily implement snippet plugin integration if this API added.

For example:

Difficulty 2 points of snippet integration

1. can not call snippet function after s:apply_text_edits

I want call search_first_placeholder_and_move function after s:apply_text_edits(called by CompleteDone).
But autocmd CompleteDone * call <SID>apply_text_edits() is autoload, so I can not register easily search_first_placeholder_and_move after s:apply_text_edits.

augroup lsp_completion_item_text_edit
autocmd!
autocmd CompleteDone * call <SID>apply_text_edit()
augroup END

e.g. :augroup of vim-lsp and vim-lsp-ultisnips.
image

2. v:completed_item is empty, after call lsp#utils#text_edit#apply_text_edits.

v:completed_item is empty after lsp#utils#text_edit#apply_text_edits, so after called CompleteDone function can not reference v:completed_item.
v:completed_item is needed for move cursor to tabstop or placeholder.

    call lsp#log(v:completed_item) " {"word": "...(snip)} (not empty)

    " apply textEdits
    if !empty(l:all_text_edits)
        call lsp#utils#text_edit#apply_text_edits(lsp#utils#get_buffer_uri(), l:all_text_edits)
    endif

    call lsp#log(v:completed_item) " {} (empty)

@thomasfaingnaert
Copy link
Collaborator Author

@mikoto2000
Sorry for the late response. For your first issue, how about I use a User autocmd that is called at the end of s:apply_text_edit, so users/other plugins can register their own callbacks as well?

For the second issue: I did some research, and I think the problem is https://github.com/vim/vim/blob/8055d17388736421d875dd4933c4c93d49a2ab58/src/insexpand.c#L1512, which is called in edit: https://github.com/vim/vim/blob/8055d17388736421d875dd4933c4c93d49a2ab58/src/edit.c#L204.

Because the apply_text_edit function uses c, i and a, and these commands enter insert mode, v:completed_item is cleared. x does not have this problem because you stay in normal mode. One way to solve this is to use p and P instead of c, i and a, which I have implemented in thomasfaingnaert@1371a1e.

From my limited testing, that indeed retains the value of v:completed_item, without breaking anything.
If it's OK with @prabirshrestha, I'll create a PR for this as well.

@mikoto2000
Copy link
Contributor

@thomasfaingnaert
Thank you for your examination and comments.

I use a User autocmd that is called at the end of s:apply_text_edit,

use p and P instead of c, i and a,

That makes sense.
I think those fixes are smart approach.

@thomasfaingnaert
Copy link
Collaborator Author

@prabirshrestha
I have extracted duplicated code in vim-lsp-ultisnips and vim-lsp-neosnippet in a separate repo vim-lsp-snippets, which is now a dependency as well.
The new installation instructions are very similar, but I'll repeat them here:

  1. Install a language server that supports snippets, e.g. clangd (Windows: http://releases.llvm.org/7.0.1/LLVM-7.0.1-win64.exe, Ubuntu: sudo apt install -y clang-7 clang-tools-7). For completeness clangd --version gives clangd version 7.0.1 (tags/RELEASE_701/final) on my system.

  2. Install vim-plug: https://github.com/junegunn/vim-plug#vim

  3. Use the following minimal vimrc and run :PlugInstall:

call plug#begin()

Plug 'SirVer/ultisnips'
Plug 'prabirshrestha/async.vim'
Plug 'thomasfaingnaert/vim-lsp', {'branch': 'ultisnips-integration'}
Plug 'thomasfaingnaert/vim-lsp-snippets'
Plug 'thomasfaingnaert/vim-lsp-ultisnips'

call plug#end()

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

if executable('clangd')
    augroup vim_lsp_cpp
        autocmd!
        autocmd User lsp_setup call lsp#register_server({
                    \ 'name': 'clangd',
                    \ 'cmd': {server_info->['clangd']},
                    \ 'whitelist': ['c', 'cpp', 'objc', 'objcpp', 'cc'],
                    \ })
	autocmd FileType c,cpp,objc,objcpp,cc setlocal omnifunc=lsp#complete
    augroup end
endif

set completeopt+=menuone
  1. Create a file test.cpp with the following contents, and open it in Vim:
#include <iostream>
#include <vector>

int main(int argc, char **argv)
{
	// type: std::co<C-x><C-o>
	// to get: std::cout
	
	// type: std::vec<C-x><C-o><C-y>int<Tab> v;
	// to get: std::vector<int> v;

	// type: vect.pu<C-x><C-o><C-y>30<Tab>;
	// to get: vect.push_back(30);
	std::vector<int> vect;

	// type: for<C-x><C-o><C-y>int i = 0<Tab>i<10<Tab>i++<Tab>// ...
	// to get: for(int i = 0;i<10;i++){
	// //...
	// }
}

I have also tested this with a language server that uses text edits (I've been using #318 (comment)), and this works as well, provided #389 is merged.

@thomasfaingnaert
Copy link
Collaborator Author

@prabirshrestha
Now that #389 is merged, you can test this with text edits as well, if you want to.

Minimal vimrc
call plug#begin()

Plug 'SirVer/ultisnips'
Plug 'prabirshrestha/async.vim'
Plug 'thomasfaingnaert/vim-lsp', {'branch': 'ultisnips-integration'}
Plug 'thomasfaingnaert/vim-lsp-snippets'
Plug 'thomasfaingnaert/vim-lsp-ultisnips'

call plug#end()

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

autocmd User lsp_setup call lsp#register_server({
\ 'name': 'lsp4xml',
\ 'cmd': {server_info->[
\     'java',
\     '-noverify',
\     '-Xmx1G',
\     '-XX:+UseStringDeduplication',
\     '-Dfile.encoding=UTF-8',
\     '-jar',
\     '/path/to/lsp4xml/org.eclipse.lsp4xml/target/org.eclipse.lsp4xml-uber.jar'
\ ]},
\ 'whitelist': ['xml', 'arxml'],
\ })

autocmd FileType xml setlocal omnifunc=lsp#complete

set completeopt+=menuone

And using the files from #318 (comment):

test.xsd
<?xml version="1.0" encoding="UTF-8" ?>
<xsd:schema xmlns:xsd="http://www.w3.org/2001/XMLSchema">
  <xsd:element name="books">
    <xsd:complexType>
      <xsd:sequence>
        <xsd:element name="book">
          <xsd:complexType>
            <xsd:sequence>
              <xsd:element name="title" type="xsd:string" />
              <xsd:element name="price" type="xsd:decimal" />
            </xsd:sequence>
          </xsd:complexType>
        </xsd:element>
      </xsd:sequence>
    </xsd:complexType>
  </xsd:element>
</xsd:schema>
test.xml
<?xml version="1.0" encoding="UTF-8"?>
<books xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="./test.xsd">
	<!-- type <C-x> <C-o> and select book, <C-y> -->
</books>

@prabirshrestha prabirshrestha merged commit 20f5b75 into prabirshrestha:master Jun 15, 2019
@prabirshrestha
Copy link
Owner

Merged. Thanks to all for the PR feedback as well as other fixes.

@thomasfaingnaert thomasfaingnaert deleted the ultisnips-integration branch June 18, 2019 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants