search

dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.28k stars 1.58k forks source link

analysis server: SignatureHelp.activeParameter == -1 violates LSP spec #58577

Closed notpeter closed 16 hours ago

notpeter commented 2 days ago

Apologies if this is not the right place to file this.

The analysis server will generate SignatureHelp objects which violate the LSP by including a -1 for activeParameter -- this value is supposed to be an unsigned integer and you are returning -1. spec link.

image

https://github.com/dart-lang/sdk/blob/d38e0ff14818cbfa851186e9fdc0876b042e3048/pkg/analysis_server/lib/src/lsp/mapping.dart#L1591-L1599

https://github.com/dart-lang/sdk/blob/d38e0ff14818cbfa851186e9fdc0876b042e3048/pkg/analysis_server/lib/src/computer/computer_type_arguments_signature.dart#L111-L118

There is a link in the code to a discussion by @DanTup from 5 years ago:

Which was resolved 9 months later, but the code link with explanation has since gone stale and the code remains.

I am not a dart developer and did not personally experience this, but a discovered a user was having a bad time in Zed because that field is supposed to be an unsigned integer:

The user encountered this on NixOS on aarch64. I'm not sure the version they were running, but looking at the git blame in main it appears this has been the behavior for the last five years. The log in that issue includes the complete response where the user encountered this value of -1.

We can probably fix this on our end in Zed, but I wanted to mention the issue since I saw it in the wild. I haven't dug into how this behaves on neovim, helix or other LSP consumers but out of the box it's broken for Zed.

Thanks!

DanTup commented 2 days ago

The analysis server will generate SignatureHelp objects which violate the LSP by including a -1 for activeParameter -- this value is supposed to be an unsigned integer and you are returning -1. spec link.

It looks like the type of this changed in LSP 3.16. In 3.15 of the spec (the version when the issue above was resolved), it was just number:

    /**
     * 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.
     */
    activeParameter?: number;

I didn't notice the change and we don't currently handle uinteger any differently to other integers here. We should fix this, although depending on the behaviour in VS Code, we might end up setting this to just some high (but out-of-range) number. The problem with using 0 is that VS Code highlights the first parameter as being active when it's not, but because of named arguments there isn't always a valid active parameter index.

DanTup commented 23 hours ago

Working on a fix at https://dart-review.googlesource.com/c/sdk/+/396840

As well as changing -1 to just be the first out-of-bounds value, I added support for providing the right index when we can easily compute it (that is, we know the correspondingParameter, or we can count positional args+params to match them up). This means in VS Code we do now see parameters lit up where appropriate:

image