eclipse.jdt.ls

eclipse.jdt.ls copied to clipboard

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.

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?

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.

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 :neutral_face:

See the discussion here: https://github.com/microsoft/vscode/issues/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.

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.

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.

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

https://github.com/microsoft/language-server-protocol/pull/1597

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 :

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