Retrigger signatureHelp on new signature selection

[jakebailey]

I'm working on better selecting an active parameter in the signature help for Python code. Python differs from many languages in that you can generally specify parameters in any order you'd like by naming them, or to have them be put into *args or **kwargs completely independently of their passed position. Since the LSP specification only lets you specify one active parameter for the entire signature help response (versus one active parameter per signature), this is difficult to model.

LSP 3.15 introduced the signature help context, where the active signature (what the user has selected) is passed back on a retrigger. I've used this to some success to update the active parameter to the selected signature in lieu of per-signature active parameters (which is awkward compared to just allowing an active boolean on each parameter, but it's too late).

The specification says:

The activeSignatureHelp has its SignatureHelp.activeSignature field updated based on the user navigating through available signatures.

Which I take to include movement up and down in the signature tooltip, as it's "the user navigating through available signatures". But, I only get sent a request with an updated context when moving left and right (the actual editor cursor) or when editing the file, which prevents me from changing the active parameter index when the user is just browsing the list. I've confirmed this by setting a breakpoint in the provideSignatureHelp function in the client and observing it is only called once.

I'm unsure if this is intended behavior, or just a bug. If the current behavior is as-designed, can this extra retrigger be done in addition to the current behavior? I would think that navigating the signature menu would be beneficial and be within specification.

I'm uncertain if I should be opening this on VS Code (since this library's just a passthrough to provideSignatureHelp and the implementation likely in VS Code proper), but figured I would try asking here first as it's the overlap between the specification and VS Code's behavior.

[dbaeumer]

Since the LSP client doesn't do any filtering of user input I guess this is already a behavior in VS Code. Looping in @jrieken. Is there a reason why the arrow up / down key doesn't retrigger this.

[jrieken]

@mjbvz made these changes

[dbaeumer]

@jrieken thanks!

@mjbvz can you please comment.

[mjbvz]

activeSignatureHelp tracks the selected signature when the next retrigger occurs (such as when you continue typing). It does not track the live, currently selected signature in the UI. We do not retrigger when the user moves though signatures in the UI and I don't think this would be desirable behavior in general as it would allow the list of signatures to change as they navigate about that list

Also, the activeParameter is specific to the active signature, so you should not need to update it unless the document changes. Can you explain in more detail what you are trying to accomplish.

[jakebailey]

I don't think this would be desirable behavior in general as it would allow the list of signatures to change as they navigate about that list

Within the same parameter you should end up with the same result. A provider which is changing the list of signatures on retriggers that are pure navigation at the same cursor position would seem like a bad provider more than anything.

Also, the activeParameter is specific to the active signature, so you should not need to update it unless the document changes.

This is not entirely the case in Python, where the active parameter's index can be different per signature. You can largely specify parameters in any order you'd like so long as you're naming them, and different overloads can have different signatures with entirely different mappings.

Can you explain in more detail what you are trying to accomplish.

Sure. Take the following Python code, which uses typing annotations to hint the user on valid signatures (and for potential type checking in addition to a better editor experience):

from typing import Any, Optional, overload

@overload
def func(a: int) -> int: ...

@overload
def func(a: int, b: int, c: int = 1234, d: Optional[str] = None, **kwargs: Any) -> str: ...

def func(*args, **kwargs):
    # Real implementation here.
    # This might be in a .py file while the above is in a stub distributed separately.

Assume the user is typing out a call, like:

func(1234|)

Where | is the cursor. For this, any of the three known signatures are reasonable candidates. In an ideal world, we'd mark each separately with their active parameter (which I'll highlight below with <>):

(<a: int>) -> int
(<a: int>, b: int, c: int = 1234, d: Optional[str] = None, **kwargs: Any) -> str
(<*args>, **kwargs)

There's no problem yet; they all have the same active parameter index, 0. Navigation without retrigger will appear to work; the first parameter is selected in all cases.

Now, change the user's code to:

func(1234, d=|)

The signatures should look like this:

(a: int) -> int
(a: int, b: int, c: int = 1234, <d: Optional[str] = None>, **kwargs: Any) -> str
(*args, <**kwargs>)

The active parameter indexes are different. d will go to the d=... of the second signature, but the **kwargs of the third. The first has no active parameter, as the call no longer fits.

Currently, we can only indicate a single active signature (index 1) and its active parameter (index 3). But if the user navigates down to the next signature, there's nothing we can do to change the parameter index and say the UI should be highlighting **kwargs instead.

If the client were to retrigger on this navigation, we'd see that the user is now selecting a different signature and be able to update the parameter index to work within this API's constraints.

This is not a problem in a language like TypeScript, where optional parameters are just syntactic sugar and you must provide them in order left to right. In that case, the index always works for every signature; signatures where the index goes out of range simply don't have their parameter highlighted and it all works out implicitly.

(Note that this all is working around the problem that we can't just mark for each signature its active parameter; if we could do that then the UI would have all of the information it needs without any queries.)

[mjbvz]

Thanks. So if you could set different active parameters for each signature, that would fix the issue? Can you please file an issue against VS Code for that?

[jakebailey]

Yes and no; we're in a language server and cannot use things that aren't in the LSP specification. Adding it to VS Code will sort of solve it, but we won't be able to make use of it, hence why I'm asking for the retriggering on navigation where the client resends the same request (the same trigger kind; Invoked) just with the context set. I believe this will still be in-spec, and very similar to the existing VS Code behavior where moving the cursor left and right retriggers and updates the UI when the request completes.

Also, if a per-parameter active property were added, I think the entire activeParameter property on the signature help could be ignored. I think that's great, since you can fully specify via the list of signatures and an active signature index, but I think this will also have implications for LSP users and VS Code users (as it's yet another way to mark things active). I'm not sure how that all works out.

[mjbvz]

VS Code is usually the best place to start since we often test new language API before moving it on the LSP. Either that or file an issue against the LSP: https://github.com/Microsoft/language-server-protocol/issues

Again, having VS Code re-trigger sounds like a workaround that will impact all existing providers. We should have proper API to support your use case

[jakebailey]

I have opened microsoft/vscode#94637.

[dbaeumer]

@jakebailey please ping me when this lands in VS Code and I can make a proposed version of it in the LSP which you then can consume.

[jakebailey]

I will do that, thanks. Should this issue be retitled to capture the new API, since my original focus was on adding more retriggering (and not a whole new API)?

[dbaeumer]

I think it is fine until we know what the new API will look like.

[dbaeumer]

See microsoft/vscode#94637 for the new API.

[dbaeumer]

Code and spec updated.