Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an API to mark specific signature help parameters as active, replace top-level activeParameter #94637

Closed
jakebailey opened this issue Apr 7, 2020 · 10 comments
Assignees
Labels
api api-finalization api-proposal editor-parameter-hints Function, method and class parameter hint widget feature-request Request for new features or functionality verified Verification succeeded
Milestone

Comments

@jakebailey
Copy link
Member

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. Because current signature help API (and thus its use in the LSP) only lets you specify one active parameter for the entire signature help response (versus one active parameter per signature), this is difficult to model.

VS Code and the LSP introduced a signature help context with retriggering, which can provide context about which signature the user has selected. However, a retrigger does not occur on a simple up/down navigation in the UI, so there is no way to use retriggers to fully allow a signature provider to change the active parameter to match the active signature. In microsoft/vscode-languageserver-node#608, I suggested having VS Code retrigger on this type of navigation as it does for left/right cursor movements, but this presumably may be too much of a behavior change (though I don't personally think it's any more expensive than when the cursor is moved left/right).

What I am really looking for is a way to be able to specify per-signature which parameter is the active one. This means that during navigation the UI would not need to do any extra requests to know which parameter to show as active when an alternate signature is selected; the signature already contains this info.

There are two ways I can see this being done. Either add activeParameter?: number to the SignatureInformation class, like:

class SignatureInformation {
	label: string;
	documentation?: string | MarkdownString;
	parameters: ParameterInformation[];
	activeParameter?: number; // NEW: an index into the above parameters array
}

Or to add a property to ParameterInformation:

class ParameterInformation {
	label: string | [number, number];
	documentation?: string | MarkdownString;
	active?: boolean; // NEW: true if the UI should highlight this as active
}

I am unsure which is more natural, but my assumption is the former given it's close to the current implementation indexes, and how it disallows multiple parameters from being active at once.

I find this to be a clearer API than having a single active parameter for the entire signature help. To maintain backwards compatibility, if the new activeParameter property is set on a signature, then it could take precedence over the old top-level value (which I believe is stuck as a non-optional number and cannot be omitted).


To support this, here is an example usecase.

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. I will highlight each signature below with their intended active parameter 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 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. With the new API, each signature would state which parameter is the one that'd be active, and things work.

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.


This was split off of microsoft/vscode-languageserver-node#608. cc @mjbvz, @jrieken, @dbaeumer

@vscodebot
Copy link

vscodebot bot commented Apr 7, 2020

(Experimental duplicate detection)
Thanks for submitting this issue. Please also check if it is already covered by an existing one, like:

@heejaechang
Copy link

current protocol looks like it assumes there will be only 1 signature that matches what user is writing and supposed to set that signature as active signature and use active parameter only for active signature.

meaning, if users uses up and down key to cycle through signatures, it should not use active parameter info except the active signature (no highlight except active signature in another word)

but current behavior seems a bit off since it uses active parameter info for other signatures making it to highlight wrong parameters.

and that seems like a bug that needed to be fixed.

..

now, a feature request. when user is typing, it is very common the partially written code is ambiguous and could be matched to multiple signatures. so rather than having just one active signatures, it feels like we should have multiple active signature candidates with its own active parameter. and when user cycle through signatures, editor highlight possible matching signatures and active parameters on those signatures among all signatures.

how it gets actually implemented or added to protocol is implementation detail, but the experience seems to be something can be considered?

@jrieken jrieken added editor-parameter-hints Function, method and class parameter hint widget feature-request Request for new features or functionality api labels Apr 8, 2020
@jakebailey
Copy link
Member Author

The bot removed extra users you assigned, I'm not sure if that was intentional or not.

@jrieken
Copy link
Member

jrieken commented Apr 9, 2020

Yeah, that's weird. @JacksonKearl why is that? bot bug?

@JacksonKearl
Copy link
Contributor

JacksonKearl commented Apr 9, 2020

@jrieken yes, that's a bug.

microsoft/vscode-github-triage-actions#1

@mjbvz mjbvz modified the milestones: Backlog Candidates, April 2020 Apr 15, 2020
mjbvz added a commit that referenced this issue Apr 16, 2020
For #94637

Allows the active paramter to be be specified per signature instead of for all signatures. If provided, this overrides `SignatureHelp.activeParamter`
@mjbvz
Copy link
Contributor

mjbvz commented Apr 16, 2020

be6b1df adds the VS Code api proposal for this

@mjbvz mjbvz added api-finalization verification-needed Verification of issue is requested labels Apr 21, 2020
@mjbvz
Copy link
Contributor

mjbvz commented Apr 21, 2020

We also just decided to finalize it this iteration. @dbaeumer Can you please follow up for the language server?

@dbaeumer
Copy link
Member

Will do as soon as we have the next stable API vscode.d.ts file.

@aeschli aeschli added verified Verification succeeded and removed verification-needed Verification of issue is requested labels Apr 29, 2020
@aeschli
Copy link
Contributor

aeschli commented Apr 29, 2020

Verified by code sample, setting a activeParameter per signature worked

@github-actions github-actions bot locked and limited conversation to collaborators Jun 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api api-finalization api-proposal editor-parameter-hints Function, method and class parameter hint widget feature-request Request for new features or functionality verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

8 participants
@heejaechang @jrieken @dbaeumer @jakebailey @aeschli @JacksonKearl @mjbvz and others