fix: Duplicate function call in variable destructuring #13711

dan-kez · 2021-08-28T17:50:54Z

Q	A
Fixed Issues?	Fixes #13710
Patch: Bug Fix?	Yes
Tests Added + Pass?	Yes
Any Dependency Changes?	No
License	MIT

I am new to babel's code base but I believe I have a fix here for an issue where the RHS of an assignment will be called duplicate times.

I've added a few test cases to validate against this potential issue and have confirmed the output looks as I would expect.

If there are any suggestions please let me know! I'm happy to make updates.

Thanks for reviewing!

Short summary

This PR adds a simple recursive check so that any multi property case with a spread operator on assignment will not duplicate the RHS of the assignment.

Prior to this PR there was a guard for this case but it only looked one level deep. Example cases of this are shown in #13710 (comment)

babel-bot · 2021-08-28T17:52:57Z

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

codesandbox-ci · 2021-08-28T17:55:45Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 4be2624:

Sandbox	Source
babel-repl-custom-plugin	Configuration
babel-plugin-multi-config	Configuration

nicolo-ribaudo · 2021-08-30T10:29:46Z

...plugin-proposal-object-rest-spread/test/fixtures/object-rest/variable-destructuring/input.js

+let { x6: { w6: { a6, ...y6 } } } = z();
+let { x7: { e7, r7 }, q7: { w7: { a7, ...y7 } } } = z();
+let { x8, ...y8 } = z();
+let { x9: { w9: { a9, ...y9 } }, x10: { a10, ...y10 },  } = z();


Thanks!

Can you add a test case with mixed arrays?

Suggested change

let { x9: { w9: { a9, ...y9 } }, x10: { a10, ...y10 }, } = z();

let { x9: { w9: { a9, ...y9 } }, x10: { a10, ...y10 }, } = z();

let { x10: [{ w10, ...z10 }] } = z();

let { x11: [{ a11, b11 }, { c11, ...d11 }] } = z();

let { x12: [, { c12, ...d12 }] } = z();

Just pushed up a commit. Thanks for the recommendation, it caught an edge case with the implementation.

Can you add a new test case?

const { x: [...{ x, ...y }] } = z();

A RestElement's argument can be a pattern so we have to recurse into it.

Great point! I added the test case and confirmed the output looks as I'd expect.

We can not return early when properties is falsy, instead we should invoke the recursive function on .argument. For example,

const { x: [...{ ...y }] } = z();

has only one binding identifier but doesAnyChildNodeHaveMoreThanOneProperty returns true.

Hey @JLHwung - thanks for the the detailed case. I looked through https://astexplorer.net/ and believe I was able to diagnose the issue you called out with more robust test cases for array destructuring.

Happy to make any other adjustments. Thank you for the thorough review and prompt responses!

packages/babel-plugin-proposal-object-rest-spread/src/index.js

JLHwung · 2021-08-30T19:27:43Z

packages/babel-plugin-proposal-object-rest-spread/src/index.js

@@ -335,7 +368,7 @@ export default declare((api, opts) => {
            // skip single-property case, e.g.
            // const { ...x } = foo();
            // since the RHS will not be duplicated
-            originalPath.node.id.properties.length > 1 &&
+            doesAnyChildNodeHaveMoreThanOneProperty(originalPath.node.id) &&
            !t.isIdentifier(originalPath.node.init)


Unrelated to this PR: I think we should use originalPath.scope.isStatic(originalPath.node.init) here, because

A ThisExpression is safe to reuse, e.g.

const { ...x } = this;

Even if node.init is an identifier, it can not be reused if not a constant binding. e.g.

let z = { v: 1 }; function sideEffect() { z = { v: 2 }; } const { x = sideEffect(), ...y } = z; y // { v: 1 }

y becomes { v: 2 } after transformed because z can be modified in sideEffect().

These two issues will be fixed by scope.isStatic, they can be addressed in a subsequent PR.

JLHwung · 2021-08-31T15:06:46Z

packages/babel-plugin-proposal-object-rest-spread/src/index.js

+      // If this is an AssignmentPattern, always return true else the RHS will be duplicated
+      if (innerNode.type === "AssignmentPattern" && innerNode.left) {
+        return true;
+      }


Similar to RestElement, we need to recurse into node.left.

innerNode ??= node.left

So I originally added that but the output did not end up as I expected. Specifically the RHS was duplicated. I believe the way that we handle assignment patterns also duplicates the RHS.

See this diff that I branched off this branch for: https://github.com/dan-kez/babel/pull/1/files

Oh I was meant to replace the whole "if" branch with innerNode ??= node.left, which is similar to how we handle the array / object patterns (note that in your posted branch it was changed to innerNode = innerNode.left). The innerNode is essentially the arguments of next recursive call, so the algorithm can be summarized as

function hasMoreThanOneBinding(node) { if (node is ArrayPattern ) { return true if node has more than one elements else return hasMoreThanOneBinding(node's first element) } else if (node is ObjectPattern) { return true if node has more than one properties else return hasMoreThanOneBinding(node's first property value) } else if (node is AssignmentPattern) { return hasMoreThanOneBinding(node's left) } else if (node is RestElement) { return hasMoreThanOneBinding(node's argument) } else { // node is Identifier or MemberExpression return true } }

Ah I understand now. Apologies for all the back and forth here - I really appreciate you taking the time on this PR!

I've refactored the function to be more in line with you example. I did add one additional case ObjectProperty. This was because an ObjectPattern could have the first element a RestElement (for example const { x15: { ...y15 } } = z();). In this case node.properties[0].value would be undefined which would fall through the final else clause.

To be more specific I added an ObjectProperty case where I test on the presence of node?.value and then recurse on the given value.

If this is wrong please let me know - I'm happy to change it.

I also used optional chaining just in case I missed a case were node could be falsey (this was how I found the above issue when stepping through the test case). I am also happy to add an early return if node is undefined to return true or other cases that could be present in this LHS assignment.

packages/babel-plugin-proposal-object-rest-spread/src/index.js

JLHwung · 2021-09-02T13:03:51Z

packages/babel-plugin-proposal-object-rest-spread/src/index.js

+      // ObjectPattern
+    } else if (node.properties) {
+      if (node.properties.length > 1) return true;
+      // Case of `const {} = {}`. This shouldn't be hit as


This branch is reachable because visitRestElements applies to rest elements in array patterns, too. E.g.

const { x: [ ...{} ] } = { x: [] }

JLHwung

Great work!

dan-kez · 2021-09-02T13:49:41Z

Thanks for the review @JLHwung !! I was following along with the CI steps and noticed [CI / E2E (create-react-app) (pull_request) was failing]https://github.com/babel/babel/runs/3495970358?check_suite_focus=true

It looks like there is in issue in how we're invoking the build command incorrectly

$ npm run build:prod -w react-error-overlay
npm ERR! missing script: build:prod
npm ERR! 
npm ERR! Did you mean this?
npm ERR!     build

I'm not sure if this error is blocking to have this fixed merged in or how I can help fixing this error.

JLHwung · 2021-09-02T13:51:27Z

The react e2e CI error has been fixed in #13724, you can rebase this PR on latest main.

dan-kez · 2021-09-02T14:33:45Z

The react e2e CI error has been fixed in #13724, you can rebase this PR on latest main.

Perfect - thank you! Just rebased

packages/babel-plugin-proposal-object-rest-spread/src/index.js

nicolo-ribaudo · 2021-09-02T17:25:35Z

packages/babel-plugin-proposal-object-rest-spread/src/index.js

+      return hasMoreThanOneBinding(node.argument);
+    } else {
+      // node is Identifier or MemberExpression
+      return true;


Q: Since this is a single identifier, shouldn't it return false? (it's one binding)

It looks like this test case fails if I make this false

input:

const { x15: [...{ ...y15 }] } = z();

output:

const {} = z(), y15 = babelHelpers.extends({}, z().x15);

Ah I believe I have a fix. In the event node.argument is an Identifier then we can early return true and this these test cases work out.

Actually, I think we don't need to return true for rest but the fix needs to live somewhere else.

In the const { x15: [...{ ...y15 }] } = z(); case, we don't need to cache z(): we can just skip the {} = z() one.

Would it be ok to consider making this change in a separate PR as an optimization? My main goal for this is to mitigate duplicating the right hand assignment. I'd prefer avoiding doing a larger refactor of the greater plugin if possible.

Yes, that's ok!

packages/babel-plugin-proposal-object-rest-spread/src/index.js

JLHwung · 2021-09-03T18:16:26Z