Allow signature help to explicitly specify "no parameters highlighted"

[jakebailey]

Given code like this:

def foo(a, b): ...

foo(1, 2, |)

If the cursor is on the last parameter to the call, we want to specify that nothing should be highlighted. Additionally, the function may be overloaded, in which case we specify per-signature active parameters (microsoft/vscode#94637). Some of those overloads may not match what the user has written, and so none of the parameters should be highlighted.

The spec says:

	/**
	 * 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?: uinteger;

And for the the signature itself:

	/**
	 * The index of the active parameter.
	 *
	 * If provided, this is used in place of `SignatureHelp.activeParameter`.
	 *
	 * @since 3.16.0
	 */
	activeParameter?: uinteger;

This means there is no spec-compliant way to say "don't highlight any parameters"; omitting the active parameter on the signature defers to the old-style active parameter field, which will be zero (the first parameter) if not provided or specified with an out of bounds value.

We can work around this in pyright/pylance by making use of the fact that the VS Code LSP client doesn't verify the values its given when they are numbers:

https://github.com/microsoft/vscode-languageserver-node/blob/2645fb54ea1e764aff71dee0ecc8aceff3aabf56/client/src/common/protocolConverter.ts#L587-L598

VS Code doesn't implement the "if out of bounds, assume zero" part of the spec, instead not highlighting a parameter, which is what we want, so it works. We've been using -1 as the out of bounds value: https://github.com/microsoft/pyright/blob/48a5f1284055e61f9887a6e0fbfb6176bae730bf/packages/pyright-internal/src/languageServerBase.ts#L702

However, the specs were retroactively changed to specify uinteger for many values, and some clients strictly check that the value is non-negative (microsoft/pyright#1783), so we will likely need to move to some other out of bounds value to maintain this hack. Looking at other languages, I can see that gopls is also relying on this behavior, providing indexes past the end of the parameter list to avoid highlighting, so we can do that too.

But, swapping one hack for another doesn't really address the original need of being able to explicitly specify that no parameter is selected (for the single signature or for all).

Can the spec be modified in some way to explicitly allow something like this? E.g.:

• Treat "index past end of parameter list" as "no highlight"; this may match the behavior of existing language servers.
• Allow explicitly setting null or something to say "no highlight" behind a capability.

[dbaeumer]

We changed the values in the spec from number -> integer and uinteger to remove some under specification. Given this concrete example I think it was already clear from the comment that passing in -1 is non spec conformant :-)

The right fix is to support null as a value for activeParameter to indicate that no active parameter should be shown.

[dbaeumer]

@jakebailey would you be willing to work on a PR?

[jakebailey]

Sure, I can propose something, though I'm not personally a fan of using null where undefined exists, just knowing how difficult that can be to implement for languages like Go.

If anything, I'd be more interested in a survey of client implementations to see if they've all just been treating "out of bounds" as "nothing highlighted" already, such that that'd be a simpler choice than a new sentinel value and client capability.

[dbaeumer]

We can't use undefined since the property was optional and we can't overload this right now.

An alternative is making the value an integer and then using -1. But that needs to have a capability as well.

In general null is easier for other programming language than undefined. We have other cases were we explicitly allow null

[puremourning]

I agree that allowing both null and undefined for the same item is a bit squirrely.

Currently:

• undefined means the same as 0
• null is invalid, breach of protocol
• Negative is invalid, breach of protocol
• Out of bounds is invalid, breach of protocol

Handling both undefined and null with different semantics is a little tricky in some clients/servers, and handling out of range requires logic to range check and it a little fiddly (maybe).

So I suspect that it’s actually easier for clients to support -1 as the sentinel so that:

• undefined means 0
• null is invalid, breach of protocol
• -1 means ‘nothing selected’
• Out of range is still invalid, breach of protocol

at least I think in my client that would be simplest and clearest to handle. It’s a bit of a shame to have a capability for this though, so maybe another alternative is to add an optional field like:

• don’t supply selected parameter (implies 0, but …)
• Supply ‘nothingSelected: true ‘ (overrides the above)

just a thought.
*

[jakebailey]

I'll just point out that out of bounds values are not a "breach of protocol"; the spec defines what the client should do in this case. It's just that VS Code doesn't do that, and the behavior it has is what I want.

I don't think -1 is ever going to be possible as a sentinel value; the spec says uinteger and it's be even more awkward to say "uinteger or -1", especially given my linked thread where some rust client has used uint.

Note that I've already made the change in Pylance to use out of bounds the other way (past the end of the parameter list), and this is what gopls does as well, so I'm comfortable retaining it for now.

[puremourning]

Sorry yes you’re right out of bounds already means 0 so you can’t change it to mean something else. That only leaves null or -ve (with a type change) or a backward compat api change like I suggested.

FWIW De facto behaviour like what vscode does is actually worse than just retcon the protocol IMO.

[jakebailey]

Well, I also think that the current spec has already been retconned already; back when I wrote this code originally, the spec didn't have any of this uinteger stuff in it (and I don't recall it defining what out of bounds did). I was really surprised to need to change things to re-conform to a spec I thought was set.

I'll have to think about this more tomorrow when I'm not just haphazardly typing on my phone. 🙂

[dbaeumer]

When we started the spec everything was a number without further details which was not very spec friendly. We then introduced the uinteger and integer and it clearly made a lot more sense to spec activeParameter as uinteger since the spec already stated at that time that values outside the valid array bounds default to 0 (VS Code didn't honor that part hence you were able to achieve what you wanted). I can make VS Code spec conformant but that might not make you happy right now :-).

From all my experience with this the best is to allow null and to have a client capability guarding this.

[jakebailey]

We then introduced the uinteger and integer and it clearly made a lot more sense to spec activeParameter as uinteger since the spec already stated at that time that values outside the valid array bounds default to 0

This is something I disagree with; the fact that the spec gave a defined behavior to an out of bounds value (which to me includes negative values) implies that the right type in the protocol itself would have been integer, not uinteger. A response that was previously valid is no longer valid, and newer libraries that try and take the new types as truth complain, leading to having to retroactively conform to the spec.

(VS Code didn't honor that part hence you were able to achieve what you wanted). I can make VS Code spec conformant but that might not make you happy right now :-)

Yes, I've honestly been avoiding mentioning it in hopes nobody notices and it doesn't get "fixed" into the bad state that makes everyone unhappy. There are certainly other servers than Pylance/pyright that benefit from the current UX.

Regarding null, I don't see in the current spec any place where undefined and null are both valid but semantically different (besides "data" properties where every TS type is listed). I'd think this would be the first one, and I don't feel that it's a good idea to change the spec to be even more TS-centric (in that TS can distinguish these types, where Go or Python or others have a really hard time).

[mbovel]

We also stumbled across this issue for the Scala LSP (Metals).

I am wondering, what was the rational for defaulting to 0 when activeParameter is undefined to begin with. Does any language server rely on this?

We can't use undefined since the property was optional and we can't overload this right now.

In general null is easier for other programming language than undefined. We have other cases were we explicitly allow null.

Is there any way implementations that can't differentiate null and undefined could just interpret both as "no active parameter" and still be conformant?

I am not familiar with how backward compatibility and deprecation work in LSP—but would that be possible to use null to mean "no active parameter" as suggested, and also to deprecate undefined and out of range to avoid confusions in the future? (Or can only properties be deprecated, but not certain types for a given property?)