Correctly associate hoisted variables with parameters

[lukastaegert]

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

List any relevant issue numbers:
Resolves #2451

Description

This was a tricky one. Apparently, hoisted variables in function copes are identified with parameters if the names match. The difficulty comes from the fact that those variables are not available for parameter defaults while other parameters are:

var a = 1;
var d = 4;
function fn(a, b = a, c = d) {
  var a; // is associated with the parameter a and thus the default for b
  var d; // is not associated with the default
  console.log(a, b, c, d);
}
fn(1); // logs "1 1 4 undefined"

Another difficulty arose from the fact that Rollup's logic first initializes child AST nodes + declared variables before initializing their parents thus we cannot check for hoisted variables if there already is a parameter of the same name.

The solution I went with is to collect all hoisted variables in a special scope which is available to parameters when they are initialized. If they find there exists a hoisted variable of the same name, they reuse this variable instead of creating a new one.

To make this possible, there also is no longer a distinction between parameters and other local variables which also simplifies the code.

[robpalme]

Thank you for pursuing this tough bug. This looks great.

I ran into the same ordering issues you mentioned in the description; children initialized before the parameter variables are known. The hoistedBodyVarScope solution seems reasonable.

Two minor style comments attached.

One general comment: given you've identified that var-declarations are not truly hoisted into the parameter scope and therefore are unavailable as default parameters, maybe we should add a test case for this in case later refactoring hits it? Something like:

function throwsReferenceError (a = b) {
   var b;
}

function test () {
  try {
    throwsReferenceError();
  } catch (e) {
    return;
  }
  throw Error("Failed to throw ReferenceError");
}

test();

[lukastaegert]

Thanks for the detailed review, this is a huge help and always highly appreciated!

given you've identified that var-declarations are not truly hoisted into the parameter scope and therefore are unavailable as default parameters, maybe we should add a test case for this in case later refactoring hits it?

I am not sure about the value of such a test because this would mostly test that the runtime environment behaves correctly. At least at the moment I fail to envision a situation where optimizations done by rollup could brake this. For rollup, in order to make wrong variable associations visible, the most effective way is to trigger a deconflicting situation via scope-hoisting, so instead I added a test that does just that and checks the combinatorics around parameters used as defaults for other parameters. Which promptly delivered a test case for your other comment.

[robpalme]

The test changes look good.

Your test for deconflicting parameter defaults make a lot more sense than my suggestions. Thanks for adding!