test: add parameter default test to renamer #10055

JLHwung · 2019-06-05T02:23:38Z

Q	A
Fixed Issues?	Reveal potential issue
Patch: Bug Fix?	Determined after further discussion
Major: Breaking Change?	No
Minor: New Feature?	No
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?	No
License	MIT

This is a follow-up to #10053 (review)

Expected:

Renamer should not rename the bindings in the function default parameter if it is borrowed from parent scope
Renamer should rename the bindings in the function default parameter if it is borrowed from the param binding in the function scope.

Actual:

Renamer renames the bindings in the function default parameter even if it is borrowed from parent scope
Renamer renames the bindings in the function default parameter when it is borrowed from the param binding in the function scope.

On the test example, we have renamer to rename a to z on FunctionDeclaration visitor.
In the first case,

let a = "outside";
function f(g = () => a) {
  let a = "inside";
  return g();
}

The Renamer also rename () => a to () => z. This behavior is wrong since a is defined on global scope.

In the second case,

function h(a, g = () => a) {
  return g();
}

The renamer should also () => a to () => z since a is a param binding now.

If we agree on the correctness of my proposed behavior, then this PR reveals a bug in renamer and should be fixed in later release.

Note that CI will break since the master does not conform to this behavior.

nicolo-ribaudo · 2019-06-05T21:20:33Z

I think that, in order to fix this, we need to change this line:
https://github.com/babel/babel/blob/master/packages/babel-traverse/src/scope/lib/renamer.js#L119

instead of running renameVisitor, we should first check where the binding is defined.
If the current scope is a function scope and the binding is not defined inside it's parameters, then we should only traverse its body instead of the whole declaration.

JLHwung · 2019-06-07T04:08:56Z

Hi Nicolo,

Thanks for your reply and I have drafted a WIP commit. However, after I add another test case

let a = "outside";

function j(g = a) {
  return g;
}

it successfully fails my WIP draft commit though the commit works good on those two testcase. I would work on this test case tomorrow. And I believe this case would also improve code coverage on the transform-parameters. I would sync it to #10053 later.

babel-bot · 2019-06-09T02:07:24Z

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

JLHwung · 2019-06-09T02:21:21Z

Hi Nicolo, I have polished my work and it should be ready to review.

On this example,

let a = "outside";

function j(g = a) {
  return g;
}

I think it is expected since the binding scope is not always same with the scope that invokes renamer. On this case, there is no a binding inside the Function scope, so it will rename a to z in the whole program.

nicolo-ribaudo · 2019-06-09T22:08:02Z

packages/babel-traverse/src/scope/lib/renamer.js

+    const params = path.get("params");
+    for (let i = 0; i < params.length; i++) {
+      const param = params[i];
+      if (param.isAssignmentPattern()) {


This is not enough; someone could also do this: (which is currently broken anyway)

let name; function fn({ [name]: value }) { let name; }

or

let name; function fn({ key = name }) { let name; }

nicolo-ribaudo · 2019-06-09T22:09:09Z

packages/babel-traverse/src/scope/lib/renamer.js

+        if (!state.paramDefaultOuterBinding) {
+          if (
+            right.isIdentifier() &&
+            right.node.body.name === oldName &&


Suggested change

right.node.body.name === oldName &&

right.node.name === oldName &&

Identifiers don't have body, right?
Also, this should have been caught by a test.

Ahh, good catch. Fixed and testcase is added.

nicolo-ribaudo · 2019-06-09T22:09:48Z

packages/babel-traverse/src/scope/lib/renamer.js

+          if (
+            right.isIdentifier() &&
+            right.node.body.name === oldName &&
+            !isSafeBinding(scope, right.node.body)


Suggested change

!isSafeBinding(scope, right.node.body)

!isSafeBinding(scope, right.node)

Fixed in 840a26e.

nicolo-ribaudo · 2019-06-09T22:11:18Z

Please write a few more tests, to ensure that everything works 🙂

I think it is expected since the binding scope is not always same with the scope that invokes renamer. On this case, there is no a binding inside the Function scope, so it will rename a to z in the whole program.

Yeah, it is ok 👍

JLHwung · 2019-06-17T03:09:48Z

Hi Nicolo,

If the current scope is a function scope and the binding is not defined inside it's parameters, then we should only traverse its body instead of the whole declaration.

I have crafted some examples and it turns out this "body/whole" approach would not cover some edge cases. In principal, what's missing here is that babel-traverse has not modeled the implicit intermediate scope for function parameters.

Examples of different categories

isolated scope

On the following example,

let a = "outside"
f(g = () => a) {
  let a = "inside";
}

() => a is a new scope, but the path.scope.bindingIdentifierEquals("a", state.binding.identifier) falsely returns true since path.scope.getBinding() returns let a = "inside" binding. If the implicit scope is modeled, () => a would not have binding in g = () => a scope and will pass on to the outer scope. Note that this is a separate bug in scope.getBinding and the "body/whole" approach does not fix it.

same scope

Besides new scope, another category is expressions in various binding patterns. For example

// computed property in `ObjectBindingPattern`
function l([{
  [a]: g
}]) {}

// expression in `KeyedBindingInitializer`
function k([{
  g = a
}]) {
  let a = "inside";
  return g;
}

These expressions do not introduce a new scope. So the renameVisistor will treat it as same as any other ReferencedIdentifier. If the parameter scope is modeled, the Scope visitor will kick in and check if could be skipped.

nested binding patterns

Perhaps the most complicated examples comes from nested binding patterns. On this example

function p(a, g = (a = a) => a) {
  g(a);
}

There are two a bindings in function p. One is param binding of p, the other is param binding of default parameter of g. When scope.getBinding() called on (a = a) => a, it returns the latter binding so the Scope visitor skip the whole path even though there is an = a binding, whose identifier exactly the param binding a of p.

The "body/whole" assumes that if a is defined as parameter in scope p, it should be safe to replace all bindings given that those in a new scope, who has different binding of a, should be skipped. However, the binding patterns can easily break across the scopes.

New approach

So today I have tried a different approach other than "body/whole" to solve these. For those in separated scope, I patched the scope.getBinding so that it can returns correct binding when it is in function parameter scope.

For those expressions in the same scope, I added logic in ReferencedIdentifier visitor to check ancestry if it is in function parameter scope.

For the third cases, I think we should should do a recursive revised renameVisitor in the Scope visitor to pick up those possible nested binding patterns. It will be triggered only when the new scope is inside the parameter scope. It is a rough idea and I suggest to move this to a separate PR.

I have pushed a working edition of my new approach, it is still proof-of-concept and, of course need refactoring before ready to merge. I would like to discuss this idea before I go too far.

Thank you for your patience.

JLHwung · 2019-06-28T16:18:06Z

Ping @nicolo-ribaudo. :)

nicolo-ribaudo · 2019-07-15T19:20:51Z

packages/babel-traverse/src/scope/index.js

@@ -45,6 +45,20 @@ function gatherNodeParts(node: Object, parts: Array) {
  }
 }

+function isScopeInFunctionParameter(scope) {
+  //todo: check arrow function expression or any util method?
+  if (scope.parent && scope.parent.block.type === "FunctionDeclaration") {


This could be any function kind. It should be t.isFunction(scope.parent.block)

nicolo-ribaudo · 2019-07-15T19:21:18Z

packages/babel-traverse/src/scope/index.js

+  //todo: check arrow function expression or any util method?
+  if (scope.parent && scope.parent.block.type === "FunctionDeclaration") {
+    const fdPath = scope.path.findParent(
+      path => path.type === "FunctionDeclaration",


nicolo-ribaudo · 2019-07-15T19:22:20Z

packages/babel-traverse/src/scope/index.js

+    );
+    if (fdPath) {
+      return !!fdPath.findParent(path => {
+        return fdPath.node.params.includes(path.node);


I'm not sure that I understand this. If path.node must be inside the parameters of fdPath, why are we searching it in the parents of fdPath?

nicolo-ribaudo · 2019-07-15T19:23:50Z

packages/babel-traverse/src/scope/index.js

+      if (!scope) {
+        break;
+      }
+    } while (true);


Can we keep this as while ((scope = scope.parent))?

True, I have refactored this part.

nicolo-ribaudo · 2019-07-15T19:24:05Z

packages/babel-traverse/src/scope/lib/renamer.js

+      (path.type === "ObjectProperty" && path.node.computed)
+    );
+  });
+  const fdPath = path.findParent(path => path.type === "FunctionDeclaration");


We should only compute this is computableNode has been found

Can we share lines 12-17 with the same code in the isScopeInFunctionParameter function?

nicolo-ribaudo · 2019-07-15T19:27:24Z

packages/babel-traverse/src/scope/lib/renamer.js

+function isInFunctionParameterScope(path) {
+  const computableNode = path.findParent(path => {
+    return (
+      path.type === "AssignmentPattern" ||


WDYT about using path.parentPath.isAssignmentPattern({ right: path.node }) instead?
I don't think that it would change anything, but the left of an assignment isn't computable anyway.

WDYT about using path.parentPath.isAssignmentPattern({ right: path.node }) instead?

Good point. I have refactored this part and feel horrible on what I was writing a month ago.

When the scope path is a function declaration, check possible outer bindings in the default parameter assignment expression. They should not be renamed in this case.

…an be checked

nicolo-ribaudo · 2019-10-13T23:10:12Z

packages/babel-traverse/src/scope/lib/renamer.js

+      while (
+        p.parentPath &&
+        !p.parentPath.isAssignmentPattern({ right: p.node }) &&
+        !p.isObjectProperty({ computed: true })


We should only stop if we were inside the key, not if we were inside the "value"/pattern of a computed property.

I reworked on #10912 to get rid of the spaghetti. 😝

JLHwung · 2019-12-23T14:27:23Z

Closing this PR in favor of #10912.

JLHwung mentioned this pull request Jun 5, 2019

Check shadow variable to identifier in default parameters #10053

Merged

nicolo-ribaudo added the pkg: traverse (scope) label Jun 5, 2019

JLHwung force-pushed the renamer-parameter-default branch from fa11361 to 07bd17e Compare June 9, 2019 02:01

nicolo-ribaudo reviewed Jun 9, 2019

View reviewed changes

nicolo-ribaudo added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Jun 9, 2019

nicolo-ribaudo self-requested a review June 29, 2019 12:43

nicolo-ribaudo reviewed Jul 15, 2019

View reviewed changes

JLHwung added 13 commits July 16, 2019 19:03

test: add parameter default test to renamer

db27df0

test: another edge case test

7e90c7d

fix(renamer): check parameter default outer binding

46d6c14

When the scope path is a function declaration, check possible outer bindings in the default parameter assignment expression. They should not be renamed in this case.

fix(renamer): add test on parameter default identifier case

4f99c9d

fix(renamer): traverse param for assignment pattern

49ddbd9

fix(renamer): check computedProperty in paramHasOuterBinding

5053042

test: add mixed cases

34fb047

fix: do not skip new scope so that the nested referenced identifier c…

9742b0e

…an be checked

test: two true negative case introduced from last commit

a8ff216

test: add case where rename happens on isolated scope default parameter

90c0151

fix: getBinding return incorrect binding on parameter scope

9972a5d

fix: check default parameter in rename visitor

ae0e42b

refactor: simplify logic

4151066

JLHwung force-pushed the renamer-parameter-default branch from 0e71dc3 to 4151066 Compare July 17, 2019 04:01

nicolo-ribaudo reviewed Oct 13, 2019

View reviewed changes

JLHwung mentioned this pull request Dec 23, 2019

Fix parameter expression get binding #10912

Merged

JLHwung closed this Dec 23, 2019

JLHwung deleted the renamer-parameter-default branch December 23, 2019 14:27

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Mar 24, 2020

github-actions bot locked as resolved and limited conversation to collaborators Mar 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add parameter default test to renamer #10055

test: add parameter default test to renamer #10055

JLHwung commented Jun 5, 2019 •

edited

nicolo-ribaudo commented Jun 5, 2019

JLHwung commented Jun 7, 2019

babel-bot commented Jun 9, 2019 •

edited

JLHwung commented Jun 9, 2019

nicolo-ribaudo Jun 9, 2019

nicolo-ribaudo Jun 9, 2019

JLHwung Jun 10, 2019

nicolo-ribaudo Jun 9, 2019

JLHwung Jun 10, 2019

nicolo-ribaudo commented Jun 9, 2019 •

edited

JLHwung commented Jun 17, 2019

JLHwung commented Jun 28, 2019

nicolo-ribaudo Jul 15, 2019

nicolo-ribaudo Jul 15, 2019

nicolo-ribaudo Jul 15, 2019

nicolo-ribaudo Jul 15, 2019

JLHwung Jul 17, 2019

nicolo-ribaudo Jul 15, 2019

nicolo-ribaudo Jul 15, 2019

JLHwung Jul 17, 2019

nicolo-ribaudo Oct 13, 2019

JLHwung Dec 23, 2019

JLHwung commented Dec 23, 2019

	right.node.body.name === oldName &&
	right.node.name === oldName &&

	!isSafeBinding(scope, right.node.body)
	!isSafeBinding(scope, right.node)

test: add parameter default test to renamer #10055

test: add parameter default test to renamer #10055

Conversation

JLHwung commented Jun 5, 2019 • edited

nicolo-ribaudo commented Jun 5, 2019

JLHwung commented Jun 7, 2019

babel-bot commented Jun 9, 2019 • edited

JLHwung commented Jun 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolo-ribaudo commented Jun 9, 2019 • edited

JLHwung commented Jun 17, 2019

Examples of different categories

isolated scope

same scope

nested binding patterns

New approach

JLHwung commented Jun 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JLHwung commented Dec 23, 2019

JLHwung commented Jun 5, 2019 •

edited

babel-bot commented Jun 9, 2019 •

edited

nicolo-ribaudo commented Jun 9, 2019 •

edited