Optimize rest parameter emit

[charlespierce]

Fixes #19182

Updates rest parameter initialization by implementing the first two bullet points in @RyanCavanaugh comment on the issue:

• Skip outputting completely if the rest parameter is not referenced anywhere in the function body.
• Insert initialization immediately before the first top-level statement that contains a reference to the rest parameter.
• Additionally, if there is both a reference to the rest parameter and a re-assignment of the arguments variable, the initialization will be placed before either of those, to ensure that it doesn't try to read from an arguments that has been re-assigned.

There are a large number of changes to the baselines, however a majority of them are simply the result of the rest parameter initialization being skipped on unrelated tests. The tests that were fundamentally changed / added are:

• emitRestParameters*.ts
• collisionRestParameter*.ts
• restParameters.ts
• sourceMapValidationFunctions.ts

[charlespierce]

@RyanCavanaugh @rbuckton @mhegazy @sheetalkamat Any outstanding issues with this PR?

[charlespierce]

Updated to fix tests that broke with recent changes in master

[rbuckton]

This results in a recursive walk of the entire subtree of every statement of a function or method, which we try to avoid in the transforms. It seems like this is best handled in the checker when checking an Identifier. At that point you can look up its declaration, determine whether the declaration is a rest parameter, and walk up from there to the topmost containing statement and set something like a NodeCheckFlags.ContainsRestParameterReference on the statement. Walking up the spine is far cheaper than walking down to each descendant.

Note that you would also need to verify here in the transformer that the rest parameter was type checked, as its possible that a transformation added a previously non-existent rest-parameter. In that case, I would err on the side of caution and emit at the top of the function.

[charlespierce]

Thanks, I wasn't aware of trying to avoid recursive walks in transforms. I'll take a look at flagging this info (and the below walk as well) during type checking.

[rbuckton]

While a tree walk is generally cheap, each transform is run in succession resulting in multiple tree walks per file. In the compiler we attempt to limit this as much as possible through various optimizations. One optimization we regularly employ (though it doesn't apply here) are transformation flags that we calculate during our bind step (since bind is a guaranteed top-down full walk of the tree), or check flags that we calculate in the checker (which can randomly access the tree). These flags allow us to avoid full walks of the tree because we can use that information to determine whether a tree walk is necessary for each case.

[rbuckton]

As above, this is yet another expensive recursive walk of the entire subtree and is better handled in the checker.

[rbuckton]

• Insert initialization immediately before the first top-level statement that contains a reference to the rest parameter.

It doesn't look like you handle indirect references to the rest parameter:

function f(...args) {
  g();
  function g() {
    console.log(args);
  }
}

Currently it looks like it would insert the rest parameter emit after g();, since its the function declaration that contains the reference. However, function declarations are hoisted to the top of the lexical scope and as such you should ensure the rest parameter is initialized before any statements are executed.

[charlespierce]

For indirect references, I'm not sure of a good way to detect that scenario. A first-pass would be to check if any of the top-level statements that contain a reference to the rest parameter are hoisted, and in that case emit the initialization at the top of the body.

However, I'm not sure if that covers every scenario, since (I believe) it's possible for the hoisted declaration to not be the top-level statement, in which case it would be missed. It might be doable during type-checking by setting a flag on the function or rest parameter indicating that the use is hoisted. That could still potentially miss if a previous transform created the hoisted statement after the flag is set in type-checking.

If we can't correctly detect that case, then it might not be worth doing the minor optimization. And if we can't do the optimization, then the only thing left is skipping the initialization when the rest parameter isn't used, which is a very minor edge case.

[rbuckton]

For indirect references, I'm not sure of a good way to detect that scenario. A first-pass would be to check if any of the top-level statements that contain a reference to the rest parameter are hoisted, and in that case emit the initialization at the top of the body.

The only declaration that can trigger this is a function declaration, so only a function declaration that contains a reference to the rest args would need to indicate emit at the top of the enclosing function. It may not matter if the function declaration isn't at the top.

For example:

"use strict";
/*a*/ function f(/*b*/ ...args) {
  {
    /*c*/ function g() {
      /*d*/ args;
    }
  }
}

1. In checkIdentifier in the checker we would see that args (d) is a reference to the args rest parameter (b) for function f (a).
2. Walk up the spine to (a), encountering the function declaration g (c).
3. Add a check flag to args (b) indicating that the rest args must be emit at the top of the body.

If we want we can choose to walk further up the tree from (c) in case it turns out g is contained in an arrow function, in which case we would treat the usage of args as part of the arrow function's containing top-level statement.

[charlespierce]

Closing as I haven't had time to work on this. I hope to revisit this issue when I have more time, but at that point I'll open a new PR. Thanks for the advice about how to approach it!