signatureHelp returns activeParameter = -1

[mikehaertl]

System.out.println( |);

Response from LS:

{
  activeParameter = -1,
  activeSignature = 0,
  signatures = { {
      label = "println() : void",
      parameters = {}
    }, {
      label = "println(Object x) : void",
      parameters = { {
          label = "Object x"
        } }
    }, {
      label = "println(String x) : void",
      parameters = { {
          label = "String x"
        } }
    },
   [ ... ]
}

This breaks signature help with neovim (0.8.2) and cmp-nvim-lsp. For context: I also use nvim-jdtls, mason.nvim, mason-lspconfig.nvim and nvim-cmp.

I use eclipse.jdt.ls 1.19.0.

[mikehaertl]

This issue can probably be fixed in cmp-nvim-lsp (see PR above).

I'm still interested in feedback if this behavior is expected. The LSP specs are a bit unclear to me in this regard:

The active parameter of the active signature. If omitted or the value lies outside the range of signatures[activeSignature].parameters defaults to 0 if the active signature has parameters. If the active signature has no parameters it is ignored. In future version of the protocol this property might become mandatory to better express the active parameter if the active signature does have any.

I don't quite get the 2nd sentence. How can it be omitted/outside the range and at the same time default to 0?

[rgrunber]

If you look at the protocol spec, you'll see activeParameter?: uinteger; . The question mark implies property is optional, and so it's perfectly valid to provide SignatureHelp response that doesn't contain it. However, you're absolutely right that uinteger rules out negative values.

[rgrunber]

https://github.com/eclipse/eclipse.jdt.ls/blob/fecf51c318e766dc81acc414a1544ccd79ef4604/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/SignatureHelpUtils.java#L195 😐

[jdneo]

See the discussion here: microsoft/vscode#145851.

If we set it to 0, it could cause another problem: when the cursor is out of the range of the brackets, the first parameter will still be highlighted. (This is what I've observed when I implemented this feature, not sure if this is still true now).

There is a contribution that allows null active parameters to the spec, which is exactly what we want.

I'm okay to follow the spec - return 0 instead of -1 now. - The tradeoff is that we lose some correctness.

[rgrunber]

You're right. If activeParameter is not defined it seems to imply it treats it as the 1st (0) parameter being the active one. We definitely don't want to break this in clients, assuming they would want a way to avoid having a parameter as active when out of the invocation range.

I guess we depend on the spec being updated to properly support this.

[mikehaertl]

Doesn't the specs already include that case (outside range...)? But TBH I still struggle to parse this sentence:

If omitted or the value lies outside the range of signatures[activeSignature].parameters defaults to 0 if the active signature has parameters

To me this reads like:

if value==nil or value==(outside range) then value==0

which makes no sense. This sentence looks like it's missing a part in the middle or something.

[jdneo]

FYI, looks like this PR is close to be merged. We can adopt the new spec once it's ready.

microsoft/language-server-protocol#1597

[rgrunber]

If omitted or the value lies outside the range of signatures[activeSignature].parameters defaults to 0 if the active signature has parameters

Looking back, it is confusing. I just intepreted the final if as a condition that must be satisfied for the entire initial "if" to apply.

if (
     ( 
       (value == null/undefined) || value < 0 || value > signatures[activeSignature].parameters)
     ) && signatures[activeSignature].parameters > 0
   ) => 0

Here's what the behaviour looks like. We'd lose the ability to have the signature help popup shown with no active parameter if we stopped returning -1. The new capability introduced should support the new behaviour while respecting the spec.

current behaviour

return null where we would return -1

I think the problem is you've joined different phrases together, while in the documentation, the whitespace was used to separate the different conditions :

[jdneo]

The upstream PR is merged in the next spec version. We can consider adopting it sooner.

[notpeter]

I know this issue has been stale for a while, but I wanted to give it a bump.

Currently Zed does not properly handle activeParameter being -1 because our lsp bindings (gluon-lang/lsp-types) expect it be a unsigned integer because that's how it's defined in v3.16 of the LSP spec (previously it was underspecified as number, but now it's uinteger).

The Dart LSP was doing the exact same thing, setting activeParameter = -1 for the same reason -- to avoid highlighting the first parameter. The way they opted to fix it is instead of using -1, they return the number the number of parameters. Since this is 0-indexed it is always out of range and none of the parameters will be highlighted. Thought y'all might find this to be an acceptable solution.

Here's the commit: dart-lang/sdk@5681cfa which fixed dart-lang/sdk#58577

Thanks for making a cool thing.