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

Nested snippet expansion breaks initial snippet placeholder navigation #156

Closed
oblitum opened this issue Nov 10, 2018 · 78 comments
Closed

Comments

@oblitum
Copy link
Member

oblitum commented Nov 10, 2018

Is your feature request related to a problem? Please describe.
I'm in general not liking the current implementations for function argument snippets that are being provided by several code completers because they're very limited. For example, for int foo(int a, int b), if I accept the snippet for foo call with another snippet completion for a inner foo call as argument, this happens: foo(foo(4, 2), <placeholder jumps of first snippet don't work anymore!>int b), which means, I'll have to edit and remove the remaining int b by hand, without snippet's jump and selection. That's very bad.

Describe the solution you'd like
I can't say whether there's a best solution but I'd like at least to disable these snippets for function arguments so that I can just stick to echodoc.vim for example. LanguageClient-neovim provides g:LanguageClient_hasSnippetSupport = 0 to help on that, for example.

Describe alternatives you've considered
Alternative means of dealing with arguments are described here:

@chemzqm
Copy link
Member

chemzqm commented Nov 11, 2018

The language server could provide an option to disable snippet feature, but I don't know if there's such option for clangd.

There is signature help support with coc, you can use something like:

autocmd CursorHoldI,CursorMovedI * silent! call CocActionAsync('showSignatureHelp')

@oblitum
Copy link
Member Author

oblitum commented Nov 11, 2018

The language server could provide an option to disable snippet feature, but I don't know if there's such option for clangd.

The issue is not the server providing the parameter information but how the client is doing it in the user interface. The problem I've mentioned is solely related to client implementation, which in this case is coc.

@oblitum
Copy link
Member Author

oblitum commented Nov 11, 2018

There is signature help support with coc, you can use something like:

autocmd CursorHoldI,CursorMovedI * silent! call CocActionAsync('showSignatureHelp')

Thanks for this info.

@chemzqm
Copy link
Member

chemzqm commented Nov 11, 2018

It's not additionalTextEdit actually, it's snippet returned from language server, coc just expand the snippet on completion confirm.
I think that clangd should not return label with function parameters so that user could just insert function name by use <C-n>, and it should return one item for function overloads, it's not possible for neovim/vim to resolve different complete item with same label as expected for now. And that's how tsserver works like.

@oblitum oblitum changed the title Optional additionalTextEdit Optional snippet support Nov 11, 2018
@chemzqm
Copy link
Member

chemzqm commented Nov 11, 2018

You can't disable snippet feature of coc, language server like coc-json requires snippet to work.

@oblitum
Copy link
Member Author

oblitum commented Nov 11, 2018

It's not additionalTextEdit actually, it's snippet returned from language server, coc just expand the snippet on completion confirm.

Changed title, I may have used the wrong term.

I think that clangd should not return label with function parameters so that user could just insert function name by use <C-n>, and it should return one item for function overloads, it's not possible for neovim/vim to resolve different complete item with same label as expected for now. And that's how tsserver works like.

In the list of links I've provided at the issue, some of them do that. Clang provides per parameter info and prototypes after ( and ,, this is implemented in their signatureHelp support. It's an option of the client to insert the whole function prototype with argument placeholders (and display the multiple prototypes) or just display and insert the function name and provide the compatible overloads, show the current argument, etc, while the user inserts the arguments.

You can't disable snippet feature of coc, language server like coc-json requires snippet to work.

That's sad, I can use LanguageClient-neovim (g:LanguageClient_hasSnippetSupport = 0) and vim-lsp fine coupled with ncm2, because I'm not obligated to use snippets. As I proved at my example, argument snippets are very limited since they work just for the first expansion, after that they get broken and I have to edit function calls removing the snippet text arguments that come with it.

@chemzqm
Copy link
Member

chemzqm commented Nov 11, 2018

That's sad, I can use LanguageClient-neovim (g:LanguageClient_hasSnippetSupport = 0)

As I've said, that would break other language server from working, clangd should provide the option to disable snippet or not provide params in label of complete items, they should be exists in detail field of completion item.

The signatureHelp could be improved, trigger character is not respected for now, it's a bug.

Another solution is create a coc extension with completion middlewire to fix this issue with clangd, the hack of get function name from label would possibly break other language server used by coc.

@oblitum
Copy link
Member Author

oblitum commented Nov 11, 2018

As I've said, that would break other language server from working, clangd should provide the option to disable snippet or not provide params in label of complete items, they should be exists in detail field of completion item.

OK, I didn't verify the source code and implementation of clangd yet to see whether it's providing full prototype where should just be the function name. It's weird though because other LSP completers are working fine at providing just a function name for the insertion text. I've tried with ncm2 both vim-lsp and LanguageClient-neovim, using clangd and ccls, which is another server. Currently I'm not even using clangd, I'm using ccls, but the problem, when using coc, still remains.

@chemzqm
Copy link
Member

chemzqm commented Nov 11, 2018

They just spilt the label to get the part of function name, coc not doing that since coc does completion resolve on select change of completion item and it requires the word of vim completion item to be unique (until vim could emit selected complete item on completion item change).

If I do the split, dup should be true for vim completion item and it's possible of wrong resolved complete item.

@oblitum oblitum changed the title Optional snippet support Nested snippet expansion breaks initial snippet placeholder navigation Nov 11, 2018
@oblitum
Copy link
Member Author

oblitum commented Nov 11, 2018

I've changed the title to point to the actual issue. If it can be fixed while still having snippets, then OK.

@chemzqm
Copy link
Member

chemzqm commented Nov 11, 2018

It's possible to improve nested snippet, but would require some time, should be easier if you can use an option from clangd to disable snippet https://reviews.llvm.org/D51214?id=162370.

@oblitum
Copy link
Member Author

oblitum commented Nov 11, 2018

Nice, and thanks for pointing that issue. Sadly ccls should then be suffering from same problem maybe?...

@oblitum
Copy link
Member Author

oblitum commented Nov 11, 2018

I'm a long time YouCompleteMe user but I'm looking for options now, hence I'm trying these several new ones, but I'm doing my tests just with a minimal setup just to give the completion plugins a try. That's the reason I've picked just one language for now, c++, and I'm trying with both clangd and ccls.

I've hit this issue with them and I'm assuming you're right about it involving server implementation, so I'm expecting that for other languages I won't experience the same issue of the title? Or that at least the issue can be avoided somehow, full prototype not being inserted for other languages being one behavior that would solve it.

@chemzqm
Copy link
Member

chemzqm commented Nov 11, 2018

You may or may not, there's no guarantee.

Different language server could have different problems when working with vim, and that's why coc support extensions which could use middleware to fix those problems.

@oblitum
Copy link
Member Author

oblitum commented Nov 11, 2018

Oh well, OK... I suspect I'll hit this for all languages and few will have servers where I can disable this, and it's a bad route in any way because I would then need to do it (configuration) server per server.

@chemzqm
Copy link
Member

chemzqm commented Nov 11, 2018

You don't need to disable snippet as long as the function params not exists in label, which should be in detail field as tsserver.

Or wait for vim/neovim support emit selected completion item, then we can do split for function name.

@oblitum
Copy link
Member Author

oblitum commented Nov 11, 2018

They just spilt the label to get the part of function name, coc not doing that since coc does completion resolve on select change of completion item and it requires the word of vim completion item to be unique (until vim could emit selected complete item on completion item change).

If I do the split, dup should be true for vim completion item and it's possible of wrong resolved complete item.

OK. So I'm understanding that it's also a problem due to how coc has its completion resolve dependent upon the (neo)vim limitations on item selection. Due to how (neo)vim is contrived on information on "item emit", I'd rely the design and uniqueness of items on another data and event rather than selection and completion-item word. This constrains the core of the completion engine based on a detail of the editor, if I'm understanding it right.

But, anyway, that just an opinion.

@chemzqm
Copy link
Member

chemzqm commented Nov 11, 2018

There's no event for completion item select in vim/neovim, so coc have to rely on unique word of complete item for correct completion resolve on select change.

I've just added the option coc.preferences.splitLabelForWord which make coc split label to get function name, just like other vim's language client, which also disable completion resolve on completion item change for that source. Build from source to use it until new release is available.

@oblitum
Copy link
Member Author

oblitum commented Nov 11, 2018

OK, thanks. Sadly, as you said, there's an issue when that's true now, not all overloads are displayed and signatureHelp on the command line (from autocmd CursorMovedI * silent! call CocActionAsync('showSignatureHelp') ) will just show the prototype for the single overloaded that gets displayed in the popup menu. I don't have the same problem with echodoc and ncm2, the popup menu show all prototypes and echodoc helps with permanently displaying the signature for the item that was selected (works when nested too), even though the word inserted for the overloads being duplicated.

@chemzqm
Copy link
Member

chemzqm commented Nov 11, 2018

Just fixed that, forget to add dup property.

@oblitum
Copy link
Member Author

oblitum commented Nov 11, 2018

@chemzqm great! that's almost near echodoc, but still echodoc is doing better because it's showing the correct selected overload, CocActionAsync('showSignatureHelp') solution just show the first, not one different I select from the menu when there are multiple overloads for a function.

@oblitum
Copy link
Member Author

oblitum commented Nov 11, 2018

And another thing is that for C++ what's shown in the command line is a bit strange than echodoc, I get foo(int x, int y) -> int(int x, int y) there, where I'd expect just foo(int x, int y) -> int or foo(int x, int y).

@chemzqm
Copy link
Member

chemzqm commented Nov 11, 2018

echodoc using abbr field of vim's completion item on complete done, but coc using the result from language server, the server should return all overload signatures and coc could show them all.

@oblitum
Copy link
Member Author

oblitum commented Nov 11, 2018

Could coc remember and associate which overload is the one I've selected in the menu and use that information to just show in the command line the one related with my selection? (This is just one solution, on my YouCompleteMe fork and on vim-clangd it's completely different approach that uses the preview window for overload display and signatureHelp on popup).

@chemzqm
Copy link
Member

chemzqm commented Nov 11, 2018

I get foo(int x, int y) -> int(int x, int y) there, where I'd expect just foo(int x, int y) -> int or foo(int x, int y).

It should be just the result from language server, checkout the output channel: https://github.com/neoclide/coc.nvim/wiki/Debug-language-server#using-output-channel

@oblitum
Copy link
Member Author

oblitum commented Nov 11, 2018

It should be just the result from language server, checkout the output channel: https://github.com/neoclide/coc.nvim/wiki/Debug-language-server#using-output-channel

OK.

@chemzqm
Copy link
Member

chemzqm commented Nov 11, 2018

Could coc remember and associate which overload is the one I've selected in the menu and use that information to just show in the command line the one related with my selection?

I think the answer is no for now, since it's not standard to include function signature in complete item.

@chemzqm
Copy link
Member

chemzqm commented Nov 11, 2018

You can ask echodoc to make use of virtual text feature of neovim, so you can have better experience with it.

@oblitum
Copy link
Member Author

oblitum commented Nov 14, 2018

hi @MaskRay. The problem here is not LanguageClient, I was referring to the message and discussion just above, not the start of the issue, it's change on Coc here to output the detail data at the command line. For clangd it's displaying the member return type, but for ccls it displays the type of the variable being accessed, which is weird or not useful information in this situation.

@MaskRay
Copy link

MaskRay commented Nov 14, 2018

@oblitum Can you upload a screencast?

For clangd it returns LSP.detail = BundleSize > 1 ? formatv("[{0} overloads]", BundleSize) : ReturnType; where ResultType is ultimately derived from CodeCompletionString::CK_ResultType.

In ccls label has this piece of information:

  if (result_type.size())
    for (auto i = first; i < out.size(); ++i) {
      // ' : ' for variables,
      // ' -> ' (trailing return type-like) for functions
      out[i].label += (out[i].label == out[i].filterText ? " : " : " -> ");
      out[i].label += result_type;
    }

@oblitum
Copy link
Member Author

oblitum commented Nov 14, 2018

In case the discussion is too confuse I'll produce a testcase later. To sum it up, Coc is getting LSP detail info from server and showing it. For clangd it shows one thing, for ccls it shows another. For example, for:

struct S {
    int foo(int a);
    int foo(int x, int y);
};

int main() {
    S s;
    s.<browse the menu here>
}

When browsing the popup there, Coc shows detail data, there are two functions, both return int, for clangd, that's what is shown, but for ccls the type S is shown for all members.

@MaskRay
Copy link

MaskRay commented Nov 14, 2018

@oblitum I have mentioned in my previous comments: it is irony-mode style completion and I find it useful.

      ls_item.detail = CCS->getParentContextName().str();

You can set completion.detailedLabel to false to disable the behavior.

@oblitum
Copy link
Member Author

oblitum commented Nov 14, 2018

@MaskRay ah, ok then, I didn't get it. Thanks for the info.

@oblitum
Copy link
Member Author

oblitum commented Nov 14, 2018

@MaskRay I've made completion.detailedLabel=false and I'm now getting full prototype as detail (unsure whether that's OK since the popup already shows it), but now strangely enough that also makes the popup show just one overload of the two in the example code I've given previously.

@MaskRay
Copy link

MaskRay commented Nov 14, 2018

That is client issue. It probably uniquifies completion items with label. What ccls may learn from clangd is to support its LSP.detail = BundleSize > 1 ? formatv("[{0} overloads]", BundleSize) : ReturnType;

But I'm unsure how useful it is.

@oblitum
Copy link
Member Author

oblitum commented Nov 14, 2018

@MaskRay ok, I just thought the same, that it was client decision... Anyway, that's it. At least you captured what's the situation.

@oblitum
Copy link
Member Author

oblitum commented Nov 14, 2018

@MaskRay regarding usability, I dunno what's more useful as detail, I think return value is better than the type of the variable I'm accessing, because it's the same detail for all members. I just don't have a formed opinion on what to make out of detail. Just S for everything looked weird, for me at least.

@MaskRay
Copy link

MaskRay commented Nov 14, 2018

Parent context in detail. It is an issue @Sarcasm is also aware of. I hope he can also give some insights here.

@chemzqm
Copy link
Member

chemzqm commented Nov 14, 2018

but now strangely enough that also makes the popup show just one overload of the two in the example code I've given previously.

I've improved coc to extra word from snippet body of snippet complete item at 388294a, so this may not happen any more.

@oblitum
Copy link
Member Author

oblitum commented Nov 14, 2018

@chemzqm yeah, it shows the two prototypes at the popup now, but the detail for both is the same prototype, if I try snippet expand for both, I just get the same one expanded. So I prefer it with "detailedLabel": true.

@chemzqm
Copy link
Member

chemzqm commented Nov 14, 2018

The resolve on completion done using abbr, so I think I should add uuid to complete item to avoid this issue.

@oblitum
Copy link
Member Author

oblitum commented Nov 14, 2018

@MaskRay I think I just realized why type for parent context is more useful. I was thinking too naively, my testcase also didn't help. There's two things, return type of member is already available from prototype in popup, so, it would just get duplicated in detail, like clangd does. Having the type of parent context becomes clearly more useful when you're member-accessing expressions whose type isn't readily visible/known, like in foo().bar().baz().

@Sarcasm
Copy link

Sarcasm commented Nov 14, 2018

Indeed, the parent information starts to get useful on non-toy examples.
Also, I think I decided to display this information because that's what I saw for some VS Code client for other language AND/OR because this once the detailLabel was enabled, this was the only extra information I could see to be available.

FYI, in my mind, the ideal completion for C++ would look like this:

ideal_cpp_completion

With <f> being a nicer Method icon, instead of displaying the string Method.

For this to work, I need the CompletionItem fields (https://microsoft.github.io/language-server-protocol/specification#textDocument_completion) to be as follow:

  • label is detailed overloaded(int x) -> void
  • detail is parent
  • filterText is overloaded
  • insertText is a snippet, something like overloaded($1)$0

There is something special with the Emacs completion backend, which I tried to work around by making the filterText be a prefix of label.

Not sure if this is of any help.

Another thing, regarding the snippet issue.
I'm not sure I understood all the issue, so sorry if I'm saying gibberish.
Isn't the original issue that the snippet tool you use is not recursive?

With Emacs' yasnippet, it's possible to enter snippet edits recursively.

So you can do:

foo(${1:int a}, ${2:int b})$0
    ~~~~~~~~~~              # initial snippet with expansion 1/2
foo(bar(${1:int x}), ${2:int b})$0
        ~~~~~~~~~~          # recursive snippet expansion 1/1
foo(bar(42), ${2:int b})$0
             ~~~~~~~~~~     # initial snippet with expansion 2/2
foo(bar(42, 42))$0
                ~           # final position of initial snippet

And never be left with an unexpanded snippet int b, that you have to erase manually.

@oblitum
Copy link
Member Author

oblitum commented Nov 14, 2018

@Sarcasm thanks for your input. Yes that's the main theme of this issue, reason why it's still open.

@oblitum
Copy link
Member Author

oblitum commented Nov 14, 2018

@Sarcasm the issue with snippets for me, even if they were working in nested form, is that they would still break/miss the placeholders if I decide to go about formatting the function call prior to inputting arguments, like displacing placeholders/args over multiple lines or reaching their end. 99% of implementations of snippets I know of in Vim at least will make the placeholders vanish and the snippet turned into plain text you have to manually patch. I had one very old implementation of snippets once whose placeholders were based on textual marks around formal parameters, the marks were concealed (Vim conceal feature) but used to define placeholder's highlighting and their location. With this I could have snippets that didn't simply vanish if I pressed enter or simply jumped placeholders too much hitting end-of-snippet. The drawback is that such markers were actual text that could be left in source or interfere with diagnostics. Old gif without conceal:

Out of that experience I started to be inclined to like more param hints than snippets because I'd be free to edit as I wished.

With Coc + echodoc at least I'm having the snippets expanded at my confirmation, and I get the per param info expanding them or not (not fully whitespace agnostic though).

@oblitum
Copy link
Member Author

oblitum commented Nov 14, 2018

The drawback is that such markers were actual text that could be left in source or interfere with diagnostics.

I think if I'd implement that today I would use special whitespace chars as delimiters, plus obligatory highlighting: fewer chance of producing any effect to compilation, and still having the placeholders highlighted/present in case they're forgot in source.

@oblitum
Copy link
Member Author

oblitum commented Nov 14, 2018

The resolve on completion done using abbr, so I think I should add uuid to complete item to avoid this issue.

Just tested b5827b3, working fine. Only thing is that "[LS]" is glued to the item's text, so it also goes to echodoc.

@chemzqm
Copy link
Member

chemzqm commented Nov 14, 2018

echodoc is triky at extracting signature, it may provide hooks so you can fix the wrong signature.

@oblitum
Copy link
Member Author

oblitum commented Nov 14, 2018

Agreed.

@oblitum
Copy link
Member Author

oblitum commented Nov 15, 2018

Thanks!

@oblitum
Copy link
Member Author

oblitum commented Nov 24, 2018

Just for anyone interested reading this, to overcome the issues with wrong prototype (meaning non selected and/or exquisite declaration) on the command line, echodoc still works as expected with coc, one just needs now to avoid signatureHelp triggers using "coc.preferences.triggerSignatureHelp":false.

At tip (I'm on b4a2bb6 currently), this isn't necessary anymore. Even with "coc.preferences.triggerSignatureHelp":true all seem to be working fine, I can still chose to expand snippet or not at my option with <c-y> and echodoc signature is always correct regardless of this option ("coc.preferences.echodocSupport" has no interference here).

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

No branches or pull requests

4 participants