Fix #498: Optimize rest arguments

[begincalendar]

Fixes #498.

Inspiration of the implementation is drawn from this.

[begincalendar]

@mhegazy I left three failing baselines, because I'm not sure about them.

They are: parser509668, varArgConstructorMemberParameter and restParamModifier2.

Should the rest parameter be emitted when there is a failed attempt to use it as a parameter property?

I'm thinking it's fine for the optimization to occur (i.e. not emit the rest parameter), but I just want to make sure.

Edit 1

There is also accessorWithRestParam, which @Kovensky pointed out.

There are probably more error cases (which do not compile) that I've missed, so the generalized question is: can I still apply the optimization if the code doesn't compile?

[Jessidhia]

This looks wrong.

This test file is wrong.

class C { set X(...v) {} }, which this file is testing, is invalid syntax. set methods, on either classes or objects, only accept exactly one valid FormalParameter, which does not allow for FunctionRestParameter.

See https://tc39.github.io/ecma262/#prod-PropertySetParameterList

[begincalendar]

Right, thanks.

Yet TypeScript does not allow set to have a rest parameter anyway (i.e. the test code won't compile).

So I guess I'll revert this baseline and add it to the list above, where I ask @mhegazy about whether or not to apply the optimization in error cases.

[Jessidhia]

Oh, the transform looks correct -- the test file having a valid-looking output at all is wrong. Invalid syntax should either throw or stay as invalid syntax 😄

This is probably out of scope of this PR, though, but the change to that file helped notice it.

[begincalendar]

@Andy-MS If you get a chance and this applies to you, some input would be much appreciated.

[ghost]

This can be done without adding a new check flag.
In shouldAddRestParameter:

return !!node
    && !!node.dotDotDotToken
    && node.name.kind === SyntaxKind.Identifier
    && !inConstructorWithSynthesizedSuper
    && resolver.isRestParameterUsed(getOriginalNode(node, isParameter));

In export interface EmitResolver:

isRestParameterUsed(node: ts.ParameterDeclaration): boolean;

In checker.ts resolveNameHelper: if (isUse && result && nameNotFoundMessage && noUnusedIdentifiers && result !== lastNonBlockLocation.symbol) { --> remove && noUnusedIdentifiers
And in createResolver, add isRestParameterUsed: p => p.symbol!.isReferenced,.

@rbuckton Maybe isReferenced should be a check flag instead of its own property?

[ghost]

Is this feature actually worth adding though? What's a real-world use for having a lot of unused rest parameters?

[begincalendar]

Thanks for the feedback @Andy-MS.
I think your question regarding whether this PR is even necessary is the first priority, but while we wait for clarification on that...

1. Should the unary cast operations in shouldAddRestParameter (e.g. !!node) be done in another PR?
2. Can we just inline isRestParameterUsed (given that node internals are already being accessed within shouldAddRestParameter and the interface function would have no other callers)?

[ghost]

As for whether this is necessary, pinging @rbuckton.

1. I don't think that's necessary because it seems obvious that this was always meant to return a boolean.
2. Sure. I was expecting that it would need to access checker internals, but I guess it doesn't.

[RyanCavanaugh]

I think this PR is subsumed by #20307 (fix for #19182)