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

[notpeter]

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.

sdk/pkg/analysis_server/lib/src/lsp/mapping.dart

Lines 1591 to 1599 in d38e0ff

	// TODO(dantup): The LSP spec says this value will default to 0 if it's
	// not supplied or outside of the value range. However, setting -1 results
	// in no parameters being selected in VS Code, whereas null/0 will select the first.
	// We'd like for none to be selected (since we don't support this yet) so
	// we send -1. I've made a request for LSP to support not selecting a parameter
	// (because you could also be on param 5 of an invalid call to a function
	// taking only 3 arguments) here:
	// https://github.com/Microsoft/language-server-protocol/issues/456#issuecomment-452318297
	activeParameter: -1, // activeParameter

sdk/pkg/analysis_server/lib/src/computer/computer_type_arguments_signature.dart

Lines 111 to 118 in d38e0ff

	return lsp.SignatureHelp(
	signatures: signatures,
	activeSignature: 0,
	// As with [toSignatureHelp], we don't currently use this, but need to set
	// it to something that doesn't match a parameter to avoid one being
	// selected.
	activeParameter: -1,
	);

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

• SignatureHelp.activeParameter microsoft/language-server-protocol#456 (comment)

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:

• project restore/reopen starting multiple instance of language server  zed-industries/zed#20767

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]

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]

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: