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.

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

fix: Duplicate function call in variable destructuring #13711

Merged
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
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
Expand Up @@ -28,6 +28,7 @@
},
"devDependencies": {
"@babel/core": "workspace:*",
"@babel/parser": "workspace:*",
"@babel/helper-plugin-test-runner": "workspace:*"
},
"engines": {
Expand Down
@@ -0,0 +1,23 @@
import { types as t } from "@babel/core";

export default function hasMoreThanOneBinding(node) {
if (t.isArrayPattern(node)) {
const nonNullElements = node.elements.filter(element => element !== null);
if (nonNullElements.length > 1) return true;
else return hasMoreThanOneBinding(nonNullElements[0]);
} else if (t.isObjectPattern(node)) {
if (node.properties.length > 1) return true;
else if (node.properties.length === 0) return false;
else return hasMoreThanOneBinding(node.properties[0]);
} else if (t.isObjectProperty(node)) {
return hasMoreThanOneBinding(node.value);
} else if (t.isAssignmentPattern(node)) {
return hasMoreThanOneBinding(node.left);
} else if (t.isRestElement(node)) {
if (t.isIdentifier(node.argument)) return true;
return hasMoreThanOneBinding(node.argument);
} else {
// node is Identifier or MemberExpression
return false;
}
}
Expand Up @@ -4,6 +4,7 @@ import { types as t } from "@babel/core";
import { convertFunctionParams } from "@babel/plugin-transform-parameters";
import { isRequired } from "@babel/helper-compilation-targets";
import compatData from "@babel/compat-data/corejs2-built-ins";
import hasMoreThanOneBinding from "./hasMoreThanOneBinding";

// TODO: Remove in Babel 8
// @babel/types <=7.3.3 counts FOO as referenced in var { x: FOO }.
Expand Down Expand Up @@ -335,7 +336,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

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
@@ -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
} = {};
@@ -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)
} = {};
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
@@ -1,19 +1,39 @@
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 },
y: { ...d },
...g
} = complex;

let { x4: { ...y4 } } = z;
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

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

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();
const { ...y33 } = z();
@@ -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 @@ -38,5 +11,166 @@ let {
asdf = babelHelpers.objectWithoutProperties(complex.x, ["a", d].map(babelHelpers.toPropertyKey)),
d = babelHelpers.extends({}, complex.y),
g = babelHelpers.objectWithoutProperties(complex, ["x"]);
let {} = z,
y4 = babelHelpers.extends({}, z.x4);

let _z2 = z(),
{} = _z2,
y4 = babelHelpers.extends({}, _z2.x4);

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

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

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

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

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

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

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

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

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

const _z12 = z(),
{} = _z12,
y16 = babelHelpers.extends({}, _z12.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 _z13 = z(),
{} = _z13,
y20 = babelHelpers.extends({}, _z13.x20);

const _z14 = z(),
{
x22: {
q22
} = {}
} = _z14,
y22 = babelHelpers.objectWithoutProperties(_z14.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 _z15 = z(),
{} = _z15,
y28 = babelHelpers.extends({}, _z15.x28);

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

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

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

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

const _z18 = z(),
y33 = babelHelpers.extends({}, _z18);