fix: Duplicate function call in variable destructuring (#13711)

* Add test case demonstrating invalid behavior * Add conditional check for child properties so RHS is not duplicated * Add recursive check to find any nested non single-property case * Add case for array destructuring * Add test case for a nested rest element * More safely recurse through array destructuring * Traverse assignment and nested rest operations * Refactor to be more clear which case statement we are executing against * Update missed snapshots * Update packages/babel-plugin-proposal-object-rest-spread/src/index.js Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com> * Filter null array elements, add early return, remove optional chaining * Use stronger type assertions * Update fall through to be false; add early return to RestElement case * Move hasMoreThanOneBinding to its own file with distinct tests * Rename testing helper to more appropriately match business logic * Yarn Dedup & rename hasMoreThanOneBinding to shouldStoreRHSInTemporaryVariable Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com>
babel · Sep 7, 2021 · 40e43f5 · 40e43f5
1 parent d87a3d9
commit 40e43f5
Show file tree

Hide file tree

Showing 10 changed files with 295 additions and 64 deletions.
diff --git a/packages/babel-plugin-proposal-object-rest-spread/package.json b/packages/babel-plugin-proposal-object-rest-spread/package.json
@@ -28,7 +28,8 @@
   },
   "devDependencies": {
     "@babel/core": "workspace:*",
-    "@babel/helper-plugin-test-runner": "workspace:*"
+    "@babel/helper-plugin-test-runner": "workspace:*",
+    "@babel/parser": "workspace:*"
   },
   "engines": {
     "node": ">=6.9.0"

diff --git a/packages/babel-plugin-proposal-object-rest-spread/src/index.js b/packages/babel-plugin-proposal-object-rest-spread/src/index.js
@@ -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 shouldStoreRHSInTemporaryVariable from "./shouldStoreRHSInTemporaryVariable";
 
 // TODO: Remove in Babel 8
 // @babel/types <=7.3.3 counts FOO as referenced in var { x: FOO }.
@@ -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 &&
+            shouldStoreRHSInTemporaryVariable(originalPath.node.id) &&
             !t.isIdentifier(originalPath.node.init)
           ) {
             // const { a, ...b } = foo();

diff --git a/packages/babel-plugin-proposal-object-rest-spread/src/shouldStoreRHSInTemporaryVariable.js b/packages/babel-plugin-proposal-object-rest-spread/src/shouldStoreRHSInTemporaryVariable.js
@@ -0,0 +1,30 @@
+import { types as t } from "@babel/core";
+
+/**
+ * This is a helper function to determine if we should create an intermediate variable
+ * such that the RHS of an assignment is not duplicated.
+ *
+ * See https://github.com/babel/babel/pull/13711#issuecomment-914388382 for discussion
+ * on further optimizations.
+ */
+export default function shouldStoreRHSInTemporaryVariable(node) {
+  if (t.isArrayPattern(node)) {
+    const nonNullElements = node.elements.filter(element => element !== null);
+    if (nonNullElements.length > 1) return true;
+    else return shouldStoreRHSInTemporaryVariable(nonNullElements[0]);
+  } else if (t.isObjectPattern(node)) {
+    if (node.properties.length > 1) return true;
+    else if (node.properties.length === 0) return false;
+    else return shouldStoreRHSInTemporaryVariable(node.properties[0]);
+  } else if (t.isObjectProperty(node)) {
+    return shouldStoreRHSInTemporaryVariable(node.value);
+  } else if (t.isAssignmentPattern(node)) {
+    return shouldStoreRHSInTemporaryVariable(node.left);
+  } else if (t.isRestElement(node)) {
+    if (t.isIdentifier(node.argument)) return true;
+    return shouldStoreRHSInTemporaryVariable(node.argument);
+  } else {
+    // node is Identifier or MemberExpression
+    return false;
+  }
+}
diff --git a/...lugin-proposal-object-rest-spread/test/fixtures/object-rest/nested-computed-key/output.js b/...lugin-proposal-object-rest-spread/test/fixtures/object-rest/nested-computed-key/output.js
@@ -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
 } = {};
diff --git a/...ugin-proposal-object-rest-spread/test/fixtures/object-rest/nested-default-value/output.js b/...ugin-proposal-object-rest-spread/test/fixtures/object-rest/nested-default-value/output.js
@@ -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)
 } = {};
diff --git a/...lugin-proposal-object-rest-spread/test/fixtures/object-rest/object-ref-computed/output.js b/...lugin-proposal-object-rest-spread/test/fixtures/object-rest/object-ref-computed/output.js
@@ -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

diff --git a/...gin-proposal-object-rest-spread/test/fixtures/object-rest/variable-destructuring/input.js b/...gin-proposal-object-rest-spread/test/fixtures/object-rest/variable-destructuring/input.js
@@ -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();
+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();
diff --git a/...in-proposal-object-rest-spread/test/fixtures/object-rest/variable-destructuring/output.js b/...in-proposal-object-rest-spread/test/fixtures/object-rest/variable-destructuring/output.js
@@ -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,
@@ -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);