Check shadow variable to identifier in default parameters

[JLHwung]

Q	A
Fixed Issues?	Fixes #9947
Patch: Bug Fix?	Yes
Major: Breaking Change?	No
Minor: New Feature?	No
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?	No
License	MIT

When there is a variable declaration inside the function body, which shares its name to a referenced identifier in default parameter expression, the function body should be wrapped into iife, otherwise the binding in default parameter scope will be shadowed by function body.

On the example,

// scope P
let x = "outside";
function outer(a = () => x // scope B) { // scope A
  let x = "inside";
  return a();
}
outer();

As is illustrated, there are 3 scopes in this program. Variable x in scope B will borrow from scope P. However, since there is x declared in scope A, we should wrap body in scope A to IIFE, otherwise the transformed code will have x in scope B borrowed from scope A, which is incorrect.

Here we check if it is a safe binding with respect to the function scope. Note that unlike the path scope changing within the traverse, this scope reference is invariant and always refers to the function declaration scope (in our example, it is scope A).

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/10901/

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/10901/

[nicolo-ribaudo]

I think that a better approach would be to rename the inner x binding using path.scope.rename("x"), to avoid the IIFE:

let x = "outside";
function outer(a = () => x) {
  let x = "inside";
  return a();
}


let x = "outside";
function outer(a = () => x) {
  let x = "inside";
  return a();
}
outer();

// -->

var x = "outside";

function outer() {
  var a = arguments.length > 0 && arguments[0] !== undefined ? arguments[0] : function () {
    return x;
  };
  var _x = "inside";
  return a();
}

outer();

If it turns out to be difficult/buggy, I'm ok with the behavior implemented by this PR since this is a very rare edge case.

[nicolo-ribaudo]

Related: #2471

[JLHwung]

Hi Nicolò,

Thanks for the quick response. The issue 2471 is still reproducible on master. I would try to look into this in couple of days. At first glance, renaming the inner binding looks better as it prevents from IIFE performance penalties.

Where could I know all the possible values and the meaning of binding.kind? It would greatly help list every binding cases and evaluate whether it is safe to rename.

[JLHwung]

Hi Nicolò,

I have tried to use path.scope.rename but it seems that the renamer will also rename the referenced identifier borrowed from parent scope. I have added extra test to showcase this potential issue. (PR is here) The renamer approach is blocked by this issue.

Though issue #2471 is related, I would suggest move things forward and try to fix them on different PR. Any thoughts?

[JLHwung]

This should be considered a workaround hotfix because the ReferencedIdentifier in params initializers should point to the parent of the function scope. Since path.scope is not accurate here, we have to do a hotfix and check state.scope again.

The ideal fix should construct a separate scope for parameter initializer.

[JLHwung]

✅