signatureHelp doesn't work for similar parameters

[oblitum]

Result from CocInfo
Run :CocInfo command and paste the content below.

## versions

vim version: NVIM v0.3.1
node version: v11.4.0
coc.nvim version: 0.0.39
term: tmux-256color
platform: linux

## Error messages

## Output channel: prettier

Describe the bug
A clear and concise description of what the bug is.

Two issues identified when prototypes have parameters without names:

1. completion suggestions for prototypes won't get refreshed on remotion of parameter names, only when names are changed
   • signatureHelp gets refreshed though
2. "JumpPlaceholder" signatureHelp only works when parameters have name
   • using autocmd User CocJumpPlaceholder call CocActionAsync('showSignatureHelp')

To Reproduce
Steps to reproduce the behavior:

1. ccls configuration
2. See video

Screenshots
If applicable, add screenshots to help explain your problem.

• https://i.imgur.com/IJiZ2E7.mp4

[chemzqm]

completion suggestions for prototypes won't get refreshed on remotion of parameter names, only when names are changed.

It's just what returns by ccls, should be from cache of ccls.

"JumpPlaceholder" only works when parameters have name

The autocmd triggered as expected, you can try it with other command, ccls doesn't give correct active param (always 0), but clangd does.

[oblitum]

Ops, ok, sorry, I was betting this was some regex matching issue on parameters without name.

cc @MaskRay

[oblitum]

The autocmd triggered as expected, you can try it with other command, ccls doesn't give correct active param (always 0), but clangd does.

Hi @chemzqm, I just tried clangd (same configuration as the wiki), and it provides the same behavior as in the video for the part of not highlighting current parameter (it does refresh the signature correctly though).

[oblitum]

This gif is simpler for showing that issue because it doesn't involve snippet usage:

[chemzqm]

Just tried with clangd and ccls, they both work as expected for me.

[oblitum]

I was thinking whether it's a clang version issue, I'm on 7.0.0 (official numbering, not Apple Clang one).

[oblitum]

clangd

Server seems to be responding correctly:

• coc-clangd-signaturehelp.log

On file open:

void foo(int, int) {}

int main() {
}

On end of session:

void foo(int, int) {}

int main() {
    foo(4,
}

[oblitum]

• These changes had no effect:

  • 2e93b1f#diff-4b0287e58feb91e5735437c5854f2a15

• Issue is possibly due to assuming parameter strings can't repeat, then just indexing the first occurrence:

  • coc.nvim/src/handler/index.ts

    Line 623 in 2e93b1f

    let idx = ms ? ms.index : after.indexOf(active.label)

• Proof it's not related to parameters being unnamed:
  

[MaskRay]

It is one of my complaints https://github.com/MaskRay/ccls/wiki/LSP

textDocument/signatureHelp: the position of ParameterInformation::label is unspecified: it is difficult to highlight ParameterInformation::label in SignatureInformation::label

[oblitum]

With just the following, it indeed requires some extra logic on client's side:

[Trace - 11:19:20 PM] Received response 'textDocument/signatureHelp - (4)' in 6ms.
Result: {
    "activeParameter": 1,
    "activeSignature": 0,
    "signatures": [
        {
            "label": "foo(int, int) -> void",
            "parameters": [
                {
                    "label": "int"
                },
                {
                    "label": "int"
                }
            ]
        }
    ]
}

[oblitum]

Instead of matching the parameter labels with the content of the signature label, one could simply reconstruct the signature out of the parameter labels. It's easier at least.

[chemzqm]

Server could send indexes as label to resolve this problem, it's new signature.

[oblitum]

Server could send indexes as label to resolve this problem, it's new signature.

That's starts being problematic when Unicode is involved.

[chemzqm]

It's unicode index, same as character in position.

[MaskRay]

That's starts being problematic when Unicode is involved.

Unicode is problematic because in LSP interface Position uses UTF-16 counting for character. clangd succumbed but I refuse to use UTF-16.

[oblitum]

It is problematic because in LSP interface Position uses UTF-16 counting for character. clangd succumbed but I refuse to use UTF-16.

Ugh, that's bad. Didn't know that.

[chemzqm]

The easiest way to fix this problem is never define function with identical parameters.

[chemzqm]

Server could send indexes as label to resolve this problem, it's new signature.

That's starts being problematic when Unicode is involved.

I think it's OK, it's rare case that user use unicode for define functions, and the acuracy is not important here.

[oblitum]

The easiest way to fix this problem is never define function with identical parameters.

There are entire APIs in this form:

• http://git.savannah.gnu.org/cgit/readline.git/tree/readline.h?h=devel#n82

• A few other examples from my /usr/include: https://gist.github.com/oblitum/2ebbb0b774fb354e193d2fb6fd896ecf

• In fact, it's not limited to identical parameters, it also fails for void foo(int *, int), void foo(int [], int), etc.

[oblitum]

Instead of matching the parameter labels with the content of the signature label, one could simply reconstruct the signature out of the parameter labels. It's easier at least.

A general problem with current solution, even with a correct implementation (getting indexes of the correct submatch), is the assumption that parameter labels will match signature label content. Does LSP provide such guarantees that compliant clients need to conform? In my view signature label could possibly contain terser form for parameters, if there's freedom for that, with parameter labels containing full form of each one. Type signatures can be rather long, so that's one reason to have that, it could even be as neat as foo(short_form, current_parameter_in_full_form_and_highlighted, short_form, short_form) -> short_form_return.

[chemzqm]

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

[oblitum]

Thanks, there's some mention of indexes and constrains, but I don't understand why there's two ParameterInformation.label members.

[oblitum]

Thanks, there's some mention of indexes and constrains, but I don't understand why there's two ParameterInformation.label members.

Web page bug, there's just one on vscode implementation.

Possibly an artifact of version 2.x: https://github.com/Microsoft/language-server-protocol/blob/master/versions/protocol-2-x.md#signature-help-request

[oblitum]

So, according to spec, must be substrings. Sad, I thought current parameter full form expansion was neat idea. Version 2.x didn't have this limitation, so it was better.

[oblitum]

So, according to spec, must be substrings. Sad, I thought current parameter full form expansion was neat idea. Version 2.x didn't have this limitation, so it was better.

This has been added 3 days back o_O.

[MaskRay]

LSP 3.14 allows ParameterInformation.label to be [number, number] (was: string):

interface ParameterInformation {

	/**
	 * The label of this parameter information.
	 *
	 * Either a string or an inclusive start and exclusive end offsets within its containing
	 * signature label. (see SignatureInformation.label). *Note*: A label of type string must be
	 * a substring of its containing signature label.
	 */
	label: string | [number, number];
...
}

Pushed a commit to ccls:

ParameterInformation: use label: [number, number]

Don't bother checking signatureHelp.signatureInformationparameterInformation.labelOffsetSupport

[oblitum]

@MaskRay nice you're using the new [number, number] form, though I think other servers will take time to migrate, if ever. I think coc should still support them correctly by using as first scan resolve to try to match sequentially each ParameterInformation.label provided, over SignatureInformation.label, up to the activeParameter, to find their highlight range, without removing any special chars.

[oblitum]

match sequentially each ParameterInformation.label provided, over SignatureInformation.label, up to the activeParameter, to find their highlight range, without removing any special chars

In fact, this is the correct way to follow the current spec to support string as sub-labels. There's no other correct way. The correct match should be found, otherwise it's the language server to blame.

activeParameter should simply be viewed as the nth match, after matching each of the ParameterInformation.label once, sequentially, after parameter start.

[oblitum]

There's no other correct way. The correct match should be found, otherwise it's the language server to blame.

I mean, if you start matching where parameters starts. A signature with return type and the function name itself, coming first, can still confuse that resolve.

[oblitum]

FYI, I had long discussion over this here, IMO they just messed up naming and intention on this part, reason why clients and servers have been struggling here: microsoft/language-server-protocol#640.

[oblitum]

activeParameter should simply be viewed as the nth match, after matching each of the ParameterInformation.label once, sequentially, after parameter start.

With basic literal textual match, no need for regex, or escaping for regex. At least according to spec.