Signature help turns off current-parameter display for non-trailing rest parameters #42592

sandersn · 2021-02-01T22:52:29Z

With tuple types as rest parameters, you can now create non-trailing rest parameters. However, the simplistic current-parameter index matching just increments right past them. This PR turns off the current-parameter highlight for non-trailing rest parameters.

declare function loading(...args: [...names: string[], allCaps: boolean, extra: boolean]): void;

leading(/**/
leading('one', /**/
leading('one', 'two', /**/

For these calls, there is no current parameter, because signature help doesn't have any code to map multiple arguments to a single rest parameter. But at least it's never wrong!

Fixes #42292

Scope Creep Time!

I improved current-parameter highlighting for trailing rest parameters as well: the highlight now stops at the end.

In signature help, the first rest parameter will always be the *last* 'current' parameter (disregarding types). Previously, the signature help current-parameter highlight was only correct for trailing rest parameters. However, with tuple types, you can now create non-trailing rest parameters. This PR now correctly highlights non-trailing rest parameters as the last 'current' parameter. For example, `names` should be the current parameter in all the calls below: ```ts declare function loading(...args: [...names: string[], allCaps: boolean, extra: boolean]): void; leading(/**/ leading('one', /**/ leading('one', 'two', /**/ ``` And, because signature help doesn't do real overload resolution, `names` is also the current parameter for other calls: ```ts leading(1, 2, 3, 'ill-typed', /**/ leading('fine', true, /**/ ```

andrewbranch · 2021-02-01T23:02:34Z

Wait, we don't look at types here? So in a valid usage like

loading("name1", "name2", /*allCaps*/ true, /**/

you’d always be stuck on names?

sandersn · 2021-02-01T23:07:30Z

Yeah, previously, we'd just do overload resolution and then highlight whichever parameter matched the count of arguments. This is more clever, but only a little. I don't think it's worthwhile to be too clever here.

sandersn · 2021-02-01T23:10:59Z

The checker code that iterates arguments and gets a matching parameter is buried pretty deep. It might be possible to expose that to the services layer, but I'm not sure how much scaffolding I'd have to make to make it work.

andrewbranch · 2021-02-01T23:11:32Z

What if we just don’t set a current argument since we know we won’t get it right?

DanielRosenwasser · 2021-02-02T00:45:49Z

@typescript-bot pack this

typescript-bot · 2021-02-02T00:45:53Z

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 692c590. You can monitor the build here.

DanielRosenwasser · 2021-02-02T00:47:20Z

Another alternative might be to not flatten the signature if the tuple has a non-trailing rest element.

sandersn · 2021-02-02T00:49:12Z

@andrewbranch argumentIndex is required. =(
@DanielRosenwasser That's not a bad idea. Not sure how hard it is to do that. I'll go look.

sandersn · 2021-02-02T01:34:10Z

Well, uh, we don't even try to highlight the current parameter correctly for the last argument of this simple example:

declare function mcduff(x, y, ...args)
mcduff(1, 2, 3, 4)

So I think it's fine to fall back to that behaviour for now. I've got fairly simple code that deactivates the tuple expansion in case of non-trailing rests, which I'll push shortly.

DanielRosenwasser · 2021-02-02T01:56:22Z

Well, uh, we don't even try to highlight the current parameter correctly for the last argument of this simple example:

!!!

DanielRosenwasser · 2021-02-02T19:58:12Z

src/services/signatureHelp.ts

-        const lists = checker.getExpandedParameters(candidateSignature);
-        return lists.map(parameterList => {
+        const expansions = checker.getExpandedParameters(candidateSignature);
+        const hasNonTrailingRest = expansions.some(e => e.some((p,i) => (p as TransientSymbol).checkFlags & CheckFlags.RestParameter && i !== e.length - 1));


Suggested change

const hasNonTrailingRest = expansions.some(e => e.some((p,i) => (p as TransientSymbol).checkFlags & CheckFlags.RestParameter && i !== e.length - 1));

const hasNonTrailingRest = expansions.some(e => e.some((p, i) => (p as TransientSymbol).checkFlags & CheckFlags.RestParameter && i !== e.length - 1));

DanielRosenwasser

Can we get an example with a middle spread too?

DanielRosenwasser · 2021-02-02T20:01:57Z

I still feel a bit conflicted because this displays more poorly. Maybe argumentIndex can just be set to -1 or something? Ideally argumentIndex could be split into argumentRange or something like that.

andrewbranch · 2021-02-02T20:05:59Z

I was wondering about setting argumentIndex to -1, but it seems like whether that works will be editor-dependent. If it works in VS and VS Code, do we call that good enough?

DanielRosenwasser · 2021-02-02T21:49:59Z

Well, editors today have to sort of handle the case where there's too many parentheses, right? I don't exactly know what the expected behavior is. Maybe @CyrusNajmabadi has some thoughts from the Roslyn side.

sandersn · 2021-02-02T23:56:10Z

It already works fine to have argumentIndex > parameters.length if you want to turn off current-parameter highlighting. I think that's a pretty good option.

This reverts commit f0896f3.

1. Trailing rest keeps highlight at end instead of going off the end. 2. Non-trailing rest disable highlight entirely (by putting the index one past the end).

sandersn · 2021-02-04T17:14:47Z

The latest commit:

Skips highlighting non-trailing rests by setting argumentIndex = parameters.length, which is one past the end.
Stops the highlight at the end for trailing rests. (This is new! I can't believe we never tested this before!)

andrewbranch · 2021-02-04T17:39:54Z

src/services/signatureHelp.ts

+            const firstRest = findIndex(selected.parameters, p => !!p.isRest);
+            if (-1 < firstRest && firstRest < selected.parameters.length - 1) {
+                // We don't have any code to get this correct; instead, don't highlight a current parameter AT ALL
+                help.argumentIndex = selected.parameters.length;


We could also make this conditional upon argumentIndex >= firstRest if we wanted parameter highlighting to work up until the first rest, where the ambiguity begins. But maybe that would be a worse experience than consistently seeing no highlighting. I personally feel like parameter highlighting is pretty useful when parameters have JSDoc description, and it might be worthwhile to show them as often as we can. But it might be pretty confusing for it to stop working halfway through typing call arguments. Don’t have a super strong opinion here.

sandersn added 2 commits February 1, 2021 14:42

Change 'variadic' to 'rest'

e55212e

typescript-bot assigned sandersn Feb 1, 2021

sandersn requested a review from sheetalkamat February 1, 2021 22:52

typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Feb 1, 2021

sandersn requested review from andrewbranch and armanio123 February 1, 2021 22:52

fix missed rename

692c590

use single, original tuple instead

f0896f3

sandersn changed the title ~~Signature help's current-parameter supports non-trailing rest parameters~~ Signature help displays non-trailing rest parameters in its original tuple form Feb 2, 2021

andrewbranch approved these changes Feb 2, 2021

View reviewed changes

DanielRosenwasser reviewed Feb 2, 2021

View reviewed changes

sandersn added 2 commits February 3, 2021 17:14

Revert "use single, original tuple instead"

3351ccb

This reverts commit f0896f3.

Improve sig help of trailing rest too

2e32638

1. Trailing rest keeps highlight at end instead of going off the end. 2. Non-trailing rest disable highlight entirely (by putting the index one past the end).

sandersn changed the title ~~Signature help displays non-trailing rest parameters in its original tuple form~~ Signature help turns off current-paremeter display for non-trailing rest parameters Feb 4, 2021

sandersn changed the title ~~Signature help turns off current-paremeter display for non-trailing rest parameters~~ Signature help turns off current-parameter display for non-trailing rest parameters Feb 4, 2021

update API baselines

88e83d1

andrewbranch approved these changes Feb 4, 2021

View reviewed changes

sandersn merged commit f1583f0 into master Feb 5, 2021

sandersn deleted the signature-help-support-non-trailing-variadic-parameters branch February 5, 2021 17:37

	const hasNonTrailingRest = expansions.some(e => e.some((p,i) => (p as TransientSymbol).checkFlags & CheckFlags.RestParameter && i !== e.length - 1));
	const hasNonTrailingRest = expansions.some(e => e.some((p, i) => (p as TransientSymbol).checkFlags & CheckFlags.RestParameter && i !== e.length - 1));

Signature help turns off current-parameter display for non-trailing rest parameters #42592

Signature help turns off current-parameter display for non-trailing rest parameters #42592

Uh oh!

Conversation

sandersn commented Feb 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Scope Creep Time!

Uh oh!

andrewbranch commented Feb 1, 2021

Uh oh!

sandersn commented Feb 1, 2021

Uh oh!

sandersn commented Feb 1, 2021

Uh oh!

andrewbranch commented Feb 1, 2021

Uh oh!

DanielRosenwasser commented Feb 2, 2021

Uh oh!

typescript-bot commented Feb 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanielRosenwasser commented Feb 2, 2021

Uh oh!

sandersn commented Feb 2, 2021

Uh oh!

sandersn commented Feb 2, 2021

Uh oh!

DanielRosenwasser commented Feb 2, 2021

Uh oh!

DanielRosenwasser Feb 2, 2021

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser left a comment

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser commented Feb 2, 2021

Uh oh!

andrewbranch commented Feb 2, 2021

Uh oh!

DanielRosenwasser commented Feb 2, 2021

Uh oh!

sandersn commented Feb 2, 2021

Uh oh!

sandersn commented Feb 4, 2021

Uh oh!

andrewbranch Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sandersn commented Feb 1, 2021 •

edited

Loading

typescript-bot commented Feb 2, 2021 •

edited

Loading