From bca170ad79e8297ee29391a2dec93d563c975173 Mon Sep 17 00:00:00 2001 From: Erik Desjardins Date: Wed, 18 Jan 2017 21:32:44 -0500 Subject: [PATCH] Avoid duplicating impure expressions in object rest destructuring (#5151) * avoid duplicating impure initializers in object rest destructuring * reuse existing VariableDeclarations in object rest destructuring, to fix two issues: 1. inserting an additional VariableDeclaration after the current one may change order of operations, which is unsafe if a future VariableDeclarator refers to a destructured variable. 2. The entire VariableDeclaration is removed when all properties are rest properties, indiscriminately removing other variables --- .../src/index.js | 39 +++++++++++++------ .../object-rest/catch-clause/expected.js | 12 +++--- .../fixtures/object-rest/export/expected.js | 6 +-- .../fixtures/object-rest/for-x/expected.js | 14 +++---- .../object-rest/parameters/expected.js | 28 ++++++------- .../variable-destructuring/expected.js | 31 +++++++-------- .../fixtures/regression/gh-4904/actual.js | 7 ++++ .../fixtures/regression/gh-4904/expected.js | 15 +++++++ .../fixtures/regression/gh-5151/actual.js | 10 +++++ .../fixtures/regression/gh-5151/expected.js | 14 +++++++ .../test/fixtures/regression/options.json | 6 +++ 11 files changed, 124 insertions(+), 58 deletions(-) create mode 100644 packages/babel-plugin-transform-object-rest-spread/test/fixtures/regression/gh-4904/actual.js create mode 100644 packages/babel-plugin-transform-object-rest-spread/test/fixtures/regression/gh-4904/expected.js create mode 100644 packages/babel-plugin-transform-object-rest-spread/test/fixtures/regression/gh-5151/actual.js create mode 100644 packages/babel-plugin-transform-object-rest-spread/test/fixtures/regression/gh-5151/expected.js create mode 100644 packages/babel-plugin-transform-object-rest-spread/test/fixtures/regression/options.json diff --git a/packages/babel-plugin-transform-object-rest-spread/src/index.js b/packages/babel-plugin-transform-object-rest-spread/src/index.js index 4e2be6de80..8de5e5eea2 100644 --- a/packages/babel-plugin-transform-object-rest-spread/src/index.js +++ b/packages/babel-plugin-transform-object-rest-spread/src/index.js @@ -78,11 +78,31 @@ export default function ({ types: t }) { // const { a, ...b } = c; VariableDeclarator(path, file) { if (!path.get("id").isObjectPattern()) { return; } - const kind = path.parentPath.node.kind; - const nodes = []; - path.traverse({ + let insertionPath = path; + + path.get("id").traverse({ RestProperty(path) { + if ( + // skip single-property case, e.g. + // const { ...x } = foo(); + // since the RHS will not be duplicated + this.originalPath.node.id.properties.length > 1 && + !t.isIdentifier(this.originalPath.node.init) + ) { + // const { a, ...b } = foo(); + // to avoid calling foo() twice, as a first step convert it to: + // const _foo = foo(), + // { a, ...b } = _foo; + const initRef = path.scope.generateUidIdentifierBasedOnNode(this.originalPath.node.init, "ref"); + // insert _foo = foo() + this.originalPath.insertBefore(t.variableDeclarator(initRef, this.originalPath.node.init)); + // replace foo() with _foo + this.originalPath.replaceWith(t.variableDeclarator(this.originalPath.node.id, initRef)); + + return; + } + let ref = this.originalPath.node.init; path.findParent((path) => { @@ -99,29 +119,24 @@ export default function ({ types: t }) { ref ); - nodes.push( + insertionPath.insertAfter( t.variableDeclarator( argument, callExpression ) ); + insertionPath = insertionPath.getSibling(insertionPath.key + 1); + if (path.parentPath.node.properties.length === 0) { path.findParent( - (path) => path.isObjectProperty() || path.isVariableDeclaration() + (path) => path.isObjectProperty() || path.isVariableDeclarator() ).remove(); } } }, { originalPath: path }); - - if (nodes.length > 0) { - path.parentPath.getSibling(path.parentPath.key + 1) - .insertBefore( - t.variableDeclaration(kind, nodes) - ); - } }, // taken from transform-es2015-destructuring/src/index.js#visitor // export var { a, ...b } = c; diff --git a/packages/babel-plugin-transform-object-rest-spread/test/fixtures/object-rest/catch-clause/expected.js b/packages/babel-plugin-transform-object-rest-spread/test/fixtures/object-rest/catch-clause/expected.js index c1695cf2ed..a311c3e537 100644 --- a/packages/babel-plugin-transform-object-rest-spread/test/fixtures/object-rest/catch-clause/expected.js +++ b/packages/babel-plugin-transform-object-rest-spread/test/fixtures/object-rest/catch-clause/expected.js @@ -2,16 +2,16 @@ try {} catch (_ref) { let a34 = babelHelpers.objectWithoutProperties(_ref, []); } try {} catch (_ref2) { - let { a1 } = _ref2; - let b1 = babelHelpers.objectWithoutProperties(_ref2, ["a1"]); + let { a1 } = _ref2, + b1 = babelHelpers.objectWithoutProperties(_ref2, ["a1"]); } try {} catch (_ref3) { - let { a2, b2 } = _ref3; - let c2 = babelHelpers.objectWithoutProperties(_ref3, ["a2", "b2"]); + let { a2, b2 } = _ref3, + c2 = babelHelpers.objectWithoutProperties(_ref3, ["a2", "b2"]); } try {} catch (_ref4) { - let { a2, b2, c2: { c3 } } = _ref4; - let c4 = babelHelpers.objectWithoutProperties(_ref4.c2, ["c3"]); + let { a2, b2, c2: { c3 } } = _ref4, + c4 = babelHelpers.objectWithoutProperties(_ref4.c2, ["c3"]); } // Unchanged diff --git a/packages/babel-plugin-transform-object-rest-spread/test/fixtures/object-rest/export/expected.js b/packages/babel-plugin-transform-object-rest-spread/test/fixtures/object-rest/export/expected.js index d3d2967d6e..95af7b2702 100644 --- a/packages/babel-plugin-transform-object-rest-spread/test/fixtures/object-rest/export/expected.js +++ b/packages/babel-plugin-transform-object-rest-spread/test/fixtures/object-rest/export/expected.js @@ -1,7 +1,7 @@ // ExportNamedDeclaration -var { b } = asdf2; +var { b } = asdf2, + c = babelHelpers.objectWithoutProperties(asdf2, ["b"]); // Skip -var c = babelHelpers.objectWithoutProperties(asdf2, ["b"]); export { b, c }; export var { bb, cc } = ads; -export var [dd, ee] = ads; \ No newline at end of file +export var [dd, ee] = ads; diff --git a/packages/babel-plugin-transform-object-rest-spread/test/fixtures/object-rest/for-x/expected.js b/packages/babel-plugin-transform-object-rest-spread/test/fixtures/object-rest/for-x/expected.js index 59b38ec57f..9617d7b78f 100644 --- a/packages/babel-plugin-transform-object-rest-spread/test/fixtures/object-rest/for-x/expected.js +++ b/packages/babel-plugin-transform-object-rest-spread/test/fixtures/object-rest/for-x/expected.js @@ -1,16 +1,16 @@ // ForXStatement for (var _ref of []) { - var { a } = _ref; - var b = babelHelpers.objectWithoutProperties(_ref, ["a"]); + var { a } = _ref, + b = babelHelpers.objectWithoutProperties(_ref, ["a"]); } for (var _ref2 of []) { - var { a } = _ref2; - var b = babelHelpers.objectWithoutProperties(_ref2, ["a"]); + var { a } = _ref2, + b = babelHelpers.objectWithoutProperties(_ref2, ["a"]); } async function a() { for await (var _ref3 of []) { - var { a } = _ref3; - var b = babelHelpers.objectWithoutProperties(_ref3, ["a"]); + var { a } = _ref3, + b = babelHelpers.objectWithoutProperties(_ref3, ["a"]); } } @@ -25,4 +25,4 @@ for (a in {}) {} for (a of []) {} async function a() { for (a of []) {} -} \ No newline at end of file +} diff --git a/packages/babel-plugin-transform-object-rest-spread/test/fixtures/object-rest/parameters/expected.js b/packages/babel-plugin-transform-object-rest-spread/test/fixtures/object-rest/parameters/expected.js index 71c7876f13..abaf34d76d 100644 --- a/packages/babel-plugin-transform-object-rest-spread/test/fixtures/object-rest/parameters/expected.js +++ b/packages/babel-plugin-transform-object-rest-spread/test/fixtures/object-rest/parameters/expected.js @@ -2,31 +2,31 @@ function a(_ref) { let a34 = babelHelpers.objectWithoutProperties(_ref, []); } function a2(_ref2) { - let { a1 } = _ref2; - let b1 = babelHelpers.objectWithoutProperties(_ref2, ["a1"]); + let { a1 } = _ref2, + b1 = babelHelpers.objectWithoutProperties(_ref2, ["a1"]); } function a3(_ref3) { - let { a2, b2 } = _ref3; - let c2 = babelHelpers.objectWithoutProperties(_ref3, ["a2", "b2"]); + let { a2, b2 } = _ref3, + c2 = babelHelpers.objectWithoutProperties(_ref3, ["a2", "b2"]); } function a4(_ref4, _ref5) { - let { a5 } = _ref5; - let c5 = babelHelpers.objectWithoutProperties(_ref5, ["a5"]); - let { a3 } = _ref4; - let c3 = babelHelpers.objectWithoutProperties(_ref4, ["a3"]); + let { a5 } = _ref5, + c5 = babelHelpers.objectWithoutProperties(_ref5, ["a5"]); + let { a3 } = _ref4, + c3 = babelHelpers.objectWithoutProperties(_ref4, ["a3"]); } function a5(_ref6) { - let { a3, b2: { ba1 } } = _ref6; - let ba2 = babelHelpers.objectWithoutProperties(_ref6.b2, ["ba1"]), + let { a3, b2: { ba1 } } = _ref6, + ba2 = babelHelpers.objectWithoutProperties(_ref6.b2, ["ba1"]), c3 = babelHelpers.objectWithoutProperties(_ref6, ["a3", "b2"]); } function a6(_ref7) { - let { a3, b2: { ba1 } } = _ref7; - let ba2 = babelHelpers.objectWithoutProperties(_ref7.b2, ["ba1"]); + let { a3, b2: { ba1 } } = _ref7, + ba2 = babelHelpers.objectWithoutProperties(_ref7.b2, ["ba1"]); } function a7(_ref8 = {}) { - let { a1 = 1 } = _ref8; - let b1 = babelHelpers.objectWithoutProperties(_ref8, ["a1"]); + let { a1 = 1 } = _ref8, + b1 = babelHelpers.objectWithoutProperties(_ref8, ["a1"]); } // Unchanged function b(a) {} diff --git a/packages/babel-plugin-transform-object-rest-spread/test/fixtures/object-rest/variable-destructuring/expected.js b/packages/babel-plugin-transform-object-rest-spread/test/fixtures/object-rest/variable-destructuring/expected.js index c7d81cdf99..b9b3cfe2d3 100644 --- a/packages/babel-plugin-transform-object-rest-spread/test/fixtures/object-rest/variable-destructuring/expected.js +++ b/packages/babel-plugin-transform-object-rest-spread/test/fixtures/object-rest/variable-destructuring/expected.js @@ -3,25 +3,24 @@ var x = babelHelpers.objectWithoutProperties(z, []); var a = babelHelpers.objectWithoutProperties({ a: 1 }, []); var x = babelHelpers.objectWithoutProperties(a.b, []); var x = babelHelpers.objectWithoutProperties(a(), []); - -var { x1 } = z; -var y1 = babelHelpers.objectWithoutProperties(z, ["x1"]); +var { x1 } = z, + y1 = babelHelpers.objectWithoutProperties(z, ["x1"]); x1++; -var { [a]: b } = z; -var c = babelHelpers.objectWithoutProperties(z, [a]); -var { x1 } = z; -var y1 = babelHelpers.objectWithoutProperties(z, ["x1"]); -let { x2, y2 } = z; -let z2 = babelHelpers.objectWithoutProperties(z, ["x2", "y2"]); -const { w3, x3, y3 } = z; +var { [a]: b } = z, + c = babelHelpers.objectWithoutProperties(z, [a]); +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 z4 = babelHelpers.objectWithoutProperties(z, ["w3", "x3", "y3"]); let { x: { a: xa, [d]: f } -} = complex; - -let asdf = babelHelpers.objectWithoutProperties(complex.x, ["a", d]), +} = complex, + asdf = babelHelpers.objectWithoutProperties(complex.x, ["a", d]), d = babelHelpers.objectWithoutProperties(complex.y, []), g = babelHelpers.objectWithoutProperties(complex, ["x"]); -let {} = z; -let y4 = babelHelpers.objectWithoutProperties(z.x4, []); + +let {} = z, + y4 = babelHelpers.objectWithoutProperties(z.x4, []); diff --git a/packages/babel-plugin-transform-object-rest-spread/test/fixtures/regression/gh-4904/actual.js b/packages/babel-plugin-transform-object-rest-spread/test/fixtures/regression/gh-4904/actual.js new file mode 100644 index 0000000000..261723058d --- /dev/null +++ b/packages/babel-plugin-transform-object-rest-spread/test/fixtures/regression/gh-4904/actual.js @@ -0,0 +1,7 @@ +const { s, ...t } = foo(); + +const { s: { q1, ...q2 }, ...q3 } = bar(); + +const { a } = foo(({ b, ...c }) => { + console.log(b, c); +}); diff --git a/packages/babel-plugin-transform-object-rest-spread/test/fixtures/regression/gh-4904/expected.js b/packages/babel-plugin-transform-object-rest-spread/test/fixtures/regression/gh-4904/expected.js new file mode 100644 index 0000000000..a07001af1b --- /dev/null +++ b/packages/babel-plugin-transform-object-rest-spread/test/fixtures/regression/gh-4904/expected.js @@ -0,0 +1,15 @@ +const _foo = foo(), + { s } = _foo, + t = babelHelpers.objectWithoutProperties(_foo, ["s"]); + +const _bar = bar(), + { s: { q1 } } = _bar, + q2 = babelHelpers.objectWithoutProperties(_bar.s, ["q1"]), + q3 = babelHelpers.objectWithoutProperties(_bar, ["s"]); + +const { a } = foo((_ref) => { + let { b } = _ref, + c = babelHelpers.objectWithoutProperties(_ref, ["b"]); + + console.log(b, c); +}); diff --git a/packages/babel-plugin-transform-object-rest-spread/test/fixtures/regression/gh-5151/actual.js b/packages/babel-plugin-transform-object-rest-spread/test/fixtures/regression/gh-5151/actual.js new file mode 100644 index 0000000000..a57f76549a --- /dev/null +++ b/packages/babel-plugin-transform-object-rest-spread/test/fixtures/regression/gh-5151/actual.js @@ -0,0 +1,10 @@ +const { x, ...y } = a, + z = foo(y); + +const { ...s } = r, + t = foo(s); + +// ordering is preserved +var l = foo(), + { m: { n, ...o }, ...p } = bar(), + q = baz(); diff --git a/packages/babel-plugin-transform-object-rest-spread/test/fixtures/regression/gh-5151/expected.js b/packages/babel-plugin-transform-object-rest-spread/test/fixtures/regression/gh-5151/expected.js new file mode 100644 index 0000000000..ef6147befd --- /dev/null +++ b/packages/babel-plugin-transform-object-rest-spread/test/fixtures/regression/gh-5151/expected.js @@ -0,0 +1,14 @@ +const { x } = a, + y = babelHelpers.objectWithoutProperties(a, ["x"]), + z = foo(y); + +const s = babelHelpers.objectWithoutProperties(r, []), + t = foo(s); + +// ordering is preserved +var l = foo(), + _bar = bar(), + { m: { n } } = _bar, + o = babelHelpers.objectWithoutProperties(_bar.m, ["n"]), + p = babelHelpers.objectWithoutProperties(_bar, ["m"]), + q = baz(); diff --git a/packages/babel-plugin-transform-object-rest-spread/test/fixtures/regression/options.json b/packages/babel-plugin-transform-object-rest-spread/test/fixtures/regression/options.json new file mode 100644 index 0000000000..e155c0bd07 --- /dev/null +++ b/packages/babel-plugin-transform-object-rest-spread/test/fixtures/regression/options.json @@ -0,0 +1,6 @@ +{ + "plugins": [ + "transform-object-rest-spread", + "external-helpers" + ] +}