Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Duplicate function call in variable destructuring #13711

Merged
merged 17 commits into from Sep 7, 2021
Merged

fix: Duplicate function call in variable destructuring #13711

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
51c520d
Add test case demonstrating invalid behavior
dan-kez Aug 28, 2021
b74dfb9
Add conditional check for child properties so RHS is not duplicated
dan-kez Aug 28, 2021
cb530b1
Add recursive check to find any nested non single-property case
dan-kez Aug 28, 2021
698b776
Add case for array destructuring
dan-kez Aug 30, 2021
b1c393e
Add test case for a nested rest element
dan-kez Aug 30, 2021
87a02d1
More safely recurse through array destructuring
dan-kez Aug 30, 2021
84c2300
Traverse assignment and nested rest operations
dan-kez Aug 31, 2021
3c851b4
Refactor to be more clear which case statement we are executing against
dan-kez Aug 31, 2021
af17b56
Update missed snapshots
dan-kez Sep 1, 2021
8ace7f1
Update packages/babel-plugin-proposal-object-rest-spread/src/index.js
dan-kez Sep 1, 2021
ab3b603
Filter null array elements, add early return, remove optional chaining
dan-kez Sep 1, 2021
80fb872
Use stronger type assertions
dan-kez Sep 2, 2021
60cabe8
Update fall through to be false; add early return to RestElement case
dan-kez Sep 2, 2021
df3d149
Move hasMoreThanOneBinding to its own file with distinct tests
dan-kez Sep 3, 2021
80b1692
Rename testing helper to more appropriately match business logic
dan-kez Sep 3, 2021
317065d
Merge branch 'babel:main' into duplicate_function_call_in_variable_de…
dan-kez Sep 7, 2021
4be2624
Yarn Dedup & rename hasMoreThanOneBinding to shouldStoreRHSInTemporar…
dan-kez Sep 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 29 additions & 1 deletion packages/babel-plugin-proposal-object-rest-spread/src/index.js
View file
Open in desktop
Expand Up @@ -240,6 +240,34 @@ export default declare((api, opts) => {
}
}

function hasMoreThanOneBinding(node) {
dan-kez marked this conversation as resolved.
Show resolved Hide resolved
// ArrayPattern
if (node.elements) {
dan-kez marked this conversation as resolved.
Show resolved Hide resolved
const nonNullElements = node.elements.filter(element => element !== null);
if (nonNullElements.length > 1) return true;
else return hasMoreThanOneBinding(nonNullElements[0]);
// ObjectPattern
} else if (node.properties) {
if (node.properties.length > 1) return true;
// Case of `const {} = {}`. This shouldn't be hit as
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

// we currently call `hasMoreThanOneBinding` only under visitRestElements.
dan-kez marked this conversation as resolved.
Show resolved Hide resolved
else if (node.properties.length === 0) return false;
else return hasMoreThanOneBinding(node.properties[0]);
dan-kez marked this conversation as resolved.
Show resolved Hide resolved
// ObjectProperty
} else if (node.value) {
return hasMoreThanOneBinding(node.value);
// AssignmentPattern
} else if (node.left) {
return hasMoreThanOneBinding(node.left);
// RestElement
} else if (node.argument) {
return hasMoreThanOneBinding(node.argument);
} else {
// node is Identifier or MemberExpression
return true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's ok!

}
}

return {
name: "proposal-object-rest-spread",
inherits: syntaxObjectRestSpread.default,
Expand Down Expand Up @@ -335,7 +363,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 &&
hasMoreThanOneBinding(originalPath.node.id) &&
!t.isIdentifier(originalPath.node.init)
Copy link
Contributor

@JLHwung JLHwung Aug 30, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. A ThisExpression is safe to reuse, e.g.
const { ...x } = this;
  1. 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.

) {
// const { a, ...b } = foo();
Expand Down
7 changes: 4 additions & 3 deletions ...lugin-proposal-object-rest-spread/test/fixtures/object-rest/nested-computed-key/output.js
View file
Open in desktop
@@ -1,9 +1,10 @@
var _ref2;
var _ref3;

const {
[_ref => {
let rest = babelHelpers.extends({}, _ref);
let b = babelHelpers.extends({}, {});
let _ref2 = {},
b = babelHelpers.extends({}, _ref2);
}]: a,
[(_ref2 = {}, ({} = _ref2), d = babelHelpers.extends({}, _ref2), _ref2)]: c
[(_ref3 = {}, ({} = _ref3), d = babelHelpers.extends({}, _ref3), _ref3)]: c
} = {};
7 changes: 4 additions & 3 deletions ...ugin-proposal-object-rest-spread/test/fixtures/object-rest/nested-default-value/output.js
View file
Open in desktop
@@ -1,9 +1,10 @@
var _ref2;
var _ref3;

const {
a = _ref => {
let rest = babelHelpers.extends({}, _ref);
let b = babelHelpers.extends({}, {});
let _ref2 = {},
b = babelHelpers.extends({}, _ref2);
},
c = (_ref2 = {}, ({} = _ref2), d = babelHelpers.extends({}, _ref2), _ref2)
c = (_ref3 = {}, ({} = _ref3), d = babelHelpers.extends({}, _ref3), _ref3)
} = {};
18 changes: 7 additions & 11 deletions ...lugin-proposal-object-rest-spread/test/fixtures/object-rest/object-ref-computed/output.js
View file
Open in desktop
Expand Up @@ -2,23 +2,19 @@ var key, x, y, z; // impure

key = 1;

var _key = key++,
{
[_key]: {
y
}
} = {
var _ = {
1: {
a: 1,
y: 1
}
},
x = babelHelpers.objectWithoutProperties({
1: {
a: 1,
y: 1
_key = key++,
{
[_key]: {
y
}
}[_key], ["y"]);
} = _,
x = babelHelpers.objectWithoutProperties(_[_key], ["y"]);

expect(x).toEqual({
a: 1
Expand Down
41 changes: 30 additions & 11 deletions ...gin-proposal-object-rest-spread/test/fixtures/object-rest/variable-destructuring/input.js
View file
Open in desktop
@@ -1,14 +1,4 @@
var z = {};
var { ...x } = z;
var { ...a } = { a: 1 };
var { ...x } = a.b;
var { ...x } = a();
var {x1, ...y1} = z;
x1++;
var { [a]: b, ...c } = z;
var {x1, ...y1} = z;
let {x2, y2, ...z2} = z;
const {w3, x3, y3, ...z4} = z;
const { x15: [...{ ...y15 }] } = z();

let {
x: { a: xa, [d]: f, ...asdf },
Expand All @@ -17,3 +7,32 @@ let {
} = complex;

let { x4: { ...y4 } } = z;

let { x5: { w5, ...y5 } } = z();
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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@JLHwung JLHwung Aug 30, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dan-kez dan-kez Aug 30, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

let { x11: [{ w11, ...z11 }] } = z();
let { x12: [{ a12, b12 }, { c12, ...d12 }] } = z();
let { x13: [, { c13, ...d13 }] } = z();
const { x14: [...{ q14, ...y14 }] } = z();
const { x15: [...{ ...y16 }] } = z();
const { x16: [] } = z();
const [...[ ...y17 ]] = z();
const [...{ ...y18 }] = z();
const [...{ a19, ...y19 }] = z();
const { x20: { ...y20 } = { } } = z();
const { x22: { q22, ...y22 } = {} } = z();
const [[ ...y23 ] = []] = z();
const [{ ...y24 } = []] = z();
const { x25: [ ...y25 ] = []} = z();
const { x26: [ q26, ...y26 ] = []} = z();
const {} = {};
const [,,x27] = z();
const {x28: [,,{...y28}]} = z();
const {x29: [,,{q29, ...y29}]} = z();
const [,,{y30, ...x30}] = z();
const [,,{...x31}] = z();
const { x32: { }, w32: { ...y32 } } = z();
const [,,{}, {...q32}] = z();
191 changes: 160 additions & 31 deletions ...in-proposal-object-rest-spread/test/fixtures/object-rest/variable-destructuring/output.js
View file
Open in desktop
@@ -1,34 +1,7 @@
var z = {};
var x = babelHelpers.extends({}, z);
var a = babelHelpers.extends({}, {
a: 1
});
var x = babelHelpers.extends({}, a.b);
var x = babelHelpers.extends({}, a());
var {
x1
} = z,
y1 = babelHelpers.objectWithoutProperties(z, ["x1"]);
x1++;
var {
[a]: b
} = z,
c = babelHelpers.objectWithoutProperties(z, [a].map(babelHelpers.toPropertyKey));
var {
x1
} = z,
y1 = babelHelpers.objectWithoutProperties(z, ["x1"]);
let {
x2,
y2
} = z,
z2 = babelHelpers.objectWithoutProperties(z, ["x2", "y2"]);
const {
w3,
x3,
y3
} = z,
z4 = babelHelpers.objectWithoutProperties(z, ["w3", "x3", "y3"]);
const _z = z(),
{} = _z,
y15 = babelHelpers.extends({}, _z.x15);

let {
x: {
a: xa,
Expand All @@ -40,3 +13,159 @@ let {
g = babelHelpers.objectWithoutProperties(complex, ["x"]);
let {} = z,
y4 = babelHelpers.extends({}, z.x4);

let _z2 = z(),
{
x5: {
w5
}
} = _z2,
y5 = babelHelpers.objectWithoutProperties(_z2.x5, ["w5"]);

let _z3 = z(),
{
x6: {
w6: {
a6
}
}
} = _z3,
y6 = babelHelpers.objectWithoutProperties(_z3.x6.w6, ["a6"]);

let _z4 = z(),
{
x7: {
e7,
r7
},
q7: {
w7: {
a7
}
}
} = _z4,
y7 = babelHelpers.objectWithoutProperties(_z4.q7.w7, ["a7"]);

let _z5 = z(),
{
x8
} = _z5,
y8 = babelHelpers.objectWithoutProperties(_z5, ["x8"]);

let _z6 = z(),
{
x9: {
w9: {
a9
}
},
x10: {
a10
}
} = _z6,
y9 = babelHelpers.objectWithoutProperties(_z6.x9.w9, ["a9"]),
y10 = babelHelpers.objectWithoutProperties(_z6.x10, ["a10"]);

let _z7 = z(),
{
x11: [{
w11
}]
} = _z7,
z11 = babelHelpers.objectWithoutProperties(_z7.x11, ["w11"]);

let _z8 = z(),
{
x12: [{
a12,
b12
}, {
c12
}]
} = _z8,
d12 = babelHelpers.objectWithoutProperties(_z8.x12, ["c12"]);

let _z9 = z(),
{
x13: [, {
c13
}]
} = _z9,
d13 = babelHelpers.objectWithoutProperties(_z9.x13, ["c13"]);

const _z10 = z(),
{
x14: [...{
q14
}]
} = _z10,
y14 = babelHelpers.objectWithoutProperties(_z10.x14, ["q14"]);

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

const {
x16: []
} = z();
const [...[...y17]] = z();
const [..._ref] = z();
const y18 = babelHelpers.extends({}, _ref);
const [..._ref2] = z();
const {
a19
} = _ref2,
y19 = babelHelpers.objectWithoutProperties(_ref2, ["a19"]);

const _z12 = z(),
{} = _z12,
y20 = babelHelpers.extends({}, _z12.x20);

const _z13 = z(),
{
x22: {
q22
} = {}
} = _z13,
y22 = babelHelpers.objectWithoutProperties(_z13.x22, ["q22"]);

const [[...y23] = []] = z();
const [_ref3 = []] = z();
const y24 = babelHelpers.extends({}, _ref3);
const {
x25: [...y25] = []
} = z();
const {
x26: [q26, ...y26] = []
} = z();
const {} = {};
const [,, x27] = z();

const _z14 = z(),
{} = _z14,
y28 = babelHelpers.extends({}, _z14.x28);

const _z15 = z(),
{
x29: [,, {
q29
}]
} = _z15,
y29 = babelHelpers.objectWithoutProperties(_z15.x29, ["q29"]);

const [,, _ref4] = z();
const {
y30
} = _ref4,
x30 = babelHelpers.objectWithoutProperties(_ref4, ["y30"]);
const [,, _ref5] = z();
const x31 = babelHelpers.extends({}, _ref5);

const _z16 = z(),
{
x32: {}
} = _z16,
y32 = babelHelpers.extends({}, _z16.w32);

const [,, {}, _ref6] = z();
const q32 = babelHelpers.extends({}, _ref6);