neoclide / coc.nvim

Nodejs extension host for vim & neovim, load extensions like VSCode and host language servers.
Other
24.33k stars 956 forks source link

Nested snippet expansion breaks initial snippet placeholder navigation #156

Closed oblitum closed 5 years ago

oblitum commented 5 years ago

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 commented 5 years ago

It could be improved by using middleware when there is coc-clangd extension. You can fix the echodoc and use echodoc to give signature after complete done for now.

oblitum commented 5 years ago

@chemzqm OK thanks. I've verified, ccls is the one returning S. As you said, clangd returns the function return type (just int), which is less bad/unexpected. Dunno why ccls would be returning type of variable being accessed as detail. I'll probably poke the ccls author later on that.

oblitum commented 5 years ago

cc @MaskRay, Hi!, do you think that^^^ is an issue? It's operating differently than clangd at detail data, dunno why I'm seeing the type of the variable being accessed as detail instead of data of the actual members.

MaskRay commented 5 years ago

LanguageClient-neovim doesn't really support LSP style snippets. g:LanguageClient_hasSnippetSupport should just default to 0.

In ccls, there is an initialization option client.snippetSupport designed for such clients: if you set it to false, the server will not enable snippets even if the client announces that it supports it.

Its completion.detailedLabel is true by default: irony-mode style completion originally contributed by @Sarcasm The rationale seems to be that in many clients detail is not shown by default, so his suggestion is to make label detailed (foo(int a, int b) -> bool).

See some documentation on https://github.com/maskray/ccls/blob/master/src/config.hh#L111

oblitum commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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.

oblitum commented 5 years ago

Relevant previous messages:

MaskRay commented 5 years ago

@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 commented 5 years ago

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

oblitum commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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

chemzqm commented 5 years ago

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 388294ae9a67cd1da4eb2234af3253543bfada48, so this may not happen any more.

oblitum commented 5 years ago

@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 commented 5 years ago

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

oblitum commented 5 years ago

@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 commented 5 years ago

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:

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 commented 5 years ago

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

oblitum commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

oblitum commented 5 years ago

Agreed.

oblitum commented 5 years ago

Thanks!

oblitum commented 5 years ago

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).