From aea0fcd599d1b67e39a57a374bc3d7c4fd414ef1 Mon Sep 17 00:00:00 2001 From: Vedant Roy Date: Tue, 7 Apr 2020 15:06:43 -0400 Subject: [PATCH] Correctly transpile when cross-param refs with obj rest (#11326) * Transform initializers with ids in rest elements Fix issue 11281. Transform parameters with default initializers that have ids that are also in a parameter with a rest element. Before, these parameters were not transformed. * Add plugin-transform-parameters as dependency * Remove outdated comment * Use set instead of array for paramsWithRestElement * Skip when encounter "Scope" Previously, f({...R}, f = R => R) would be incorrectly transformed. * Pass in loose mode option instead of false * Address review and re-organize tests Checking the RHS of an assignment pattern/checking the parent of an identifier node fails on cases like "({...R}, a = f(R))" or "({...R}, {[R.key]: a = 42})". Also refactor tests by removing unecessary tests and separating "should transform" from "should not transform" tests. --- .../package.json | 3 +- .../src/index.js | 80 +++++++++++++++++-- .../object-rest/parameters-extra/input.js | 9 +++ .../object-rest/parameters-extra/output.js | 61 ++++++++++++++ .../fixtures/object-rest/parameters/output.js | 10 +-- .../src/index.js | 1 + .../src/params.js | 22 ++++- 7 files changed, 171 insertions(+), 15 deletions(-) create mode 100644 packages/babel-plugin-proposal-object-rest-spread/test/fixtures/object-rest/parameters-extra/input.js create mode 100644 packages/babel-plugin-proposal-object-rest-spread/test/fixtures/object-rest/parameters-extra/output.js diff --git a/packages/babel-plugin-proposal-object-rest-spread/package.json b/packages/babel-plugin-proposal-object-rest-spread/package.json index a158b5bd83a3..cd083b8f7344 100644 --- a/packages/babel-plugin-proposal-object-rest-spread/package.json +++ b/packages/babel-plugin-proposal-object-rest-spread/package.json @@ -13,7 +13,8 @@ ], "dependencies": { "@babel/helper-plugin-utils": "^7.8.3", - "@babel/plugin-syntax-object-rest-spread": "^7.8.0" + "@babel/plugin-syntax-object-rest-spread": "^7.8.0", + "@babel/plugin-transform-parameters": "^7.9.3" }, "peerDependencies": { "@babel/core": "^7.0.0-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 index 4fcd3c1be608..7c25ac399242 100644 --- a/packages/babel-plugin-proposal-object-rest-spread/src/index.js +++ b/packages/babel-plugin-proposal-object-rest-spread/src/index.js @@ -1,6 +1,7 @@ import { declare } from "@babel/helper-plugin-utils"; import syntaxObjectRestSpread from "@babel/plugin-syntax-object-rest-spread"; import { types as t } from "@babel/core"; +import { convertFunctionParams } from "@babel/plugin-transform-parameters"; // TODO: Remove in Babel 8 // @babel/types <=7.3.3 counts FOO as referenced in var { x: FOO }. @@ -177,9 +178,9 @@ export default declare((api, opts) => { ]; } - function replaceRestElement(parentPath, paramPath) { + function replaceRestElement(parentPath, paramPath, container) { if (paramPath.isAssignmentPattern()) { - replaceRestElement(parentPath, paramPath.get("left")); + replaceRestElement(parentPath, paramPath.get("left"), container); return; } @@ -187,7 +188,7 @@ export default declare((api, opts) => { const elements = paramPath.get("elements"); for (let i = 0; i < elements.length; i++) { - replaceRestElement(parentPath, elements[i]); + replaceRestElement(parentPath, elements[i], container); } } @@ -198,8 +199,12 @@ export default declare((api, opts) => { t.variableDeclarator(paramPath.node, uid), ]); - parentPath.ensureBlock(); - parentPath.get("body").unshiftContainer("body", declar); + if (container) { + container.push(declar); + } else { + parentPath.ensureBlock(); + parentPath.get("body").unshiftContainer("body", declar); + } paramPath.replaceWith(t.cloneNode(uid)); } } @@ -209,12 +214,71 @@ export default declare((api, opts) => { inherits: syntaxObjectRestSpread, visitor: { - // taken from transform-parameters/src/destructuring.js // function a({ b, ...c }) {} Function(path) { const params = path.get("params"); - for (let i = params.length - 1; i >= 0; i--) { - replaceRestElement(params[i].parentPath, params[i]); + const paramsWithRestElement = new Set(); + const idsInRestParams = new Set(); + for (let i = 0; i < params.length; ++i) { + const param = params[i]; + if (hasRestElement(param)) { + paramsWithRestElement.add(i); + for (const name of Object.keys(param.getBindingIdentifiers())) { + idsInRestParams.add(name); + } + } + } + + // if true, a parameter exists that has an id in its initializer + // that is also an id bound in a rest parameter + // example: f({...R}, a = R) + let idInRest = false; + + const IdentifierHandler = function(path, functionScope) { + const name = path.node.name; + if ( + path.scope.getBinding(name) === functionScope.getBinding(name) && + idsInRestParams.has(name) + ) { + idInRest = true; + path.stop(); + } + }; + + let i; + for (i = 0; i < params.length && !idInRest; ++i) { + const param = params[i]; + if (!paramsWithRestElement.has(i)) { + if (param.isReferencedIdentifier() || param.isBindingIdentifier()) { + IdentifierHandler(path, path.scope); + } else { + param.traverse( + { + "Scope|TypeAnnotation|TSTypeAnnotation": path => path.skip(), + "ReferencedIdentifier|BindingIdentifier": IdentifierHandler, + }, + path.scope, + ); + } + } + } + + if (!idInRest) { + for (let i = 0; i < params.length; ++i) { + const param = params[i]; + if (paramsWithRestElement.has(i)) { + replaceRestElement(param.parentPath, param); + } + } + } else { + const shouldTransformParam = idx => + idx >= i - 1 || paramsWithRestElement.has(idx); + convertFunctionParams( + path, + loose, + shouldTransformParam, + replaceRestElement, + ); } }, // adapted from transform-destructuring/src/index.js#pushObjectRest diff --git a/packages/babel-plugin-proposal-object-rest-spread/test/fixtures/object-rest/parameters-extra/input.js b/packages/babel-plugin-proposal-object-rest-spread/test/fixtures/object-rest/parameters-extra/input.js new file mode 100644 index 000000000000..b32b0596f42c --- /dev/null +++ b/packages/babel-plugin-proposal-object-rest-spread/test/fixtures/object-rest/parameters-extra/input.js @@ -0,0 +1,9 @@ +({...R}, a = R) => {} +({X: Y,...R}, {a = {Y}}) => {} +(a = R, {...R}) => {} +({...R}, e, c = 2, a = R, f = q) => { let q; } +({...R}, a = f(R)) => {} +({...R}, { [R.key]: a = 42 }) => {} + +({...R}, {a = {R: b}}) => {} +({...R}, {a = R => R} = {b: R => R}) => {} diff --git a/packages/babel-plugin-proposal-object-rest-spread/test/fixtures/object-rest/parameters-extra/output.js b/packages/babel-plugin-proposal-object-rest-spread/test/fixtures/object-rest/parameters-extra/output.js new file mode 100644 index 000000000000..8033a4d76105 --- /dev/null +++ b/packages/babel-plugin-proposal-object-rest-spread/test/fixtures/object-rest/parameters-extra/output.js @@ -0,0 +1,61 @@ +(_ref) => { + let R = babelHelpers.extends({}, _ref); + let a = arguments.length > 1 && arguments[1] !== undefined ? arguments[1] : R; +}; + +(_ref2, _ref3) => { + let { + X: Y + } = _ref2, + R = babelHelpers.objectWithoutProperties(_ref2, ["X"]); + let { + a = { + Y + } + } = _ref3; +}; + +() => { + let a = arguments.length > 0 && arguments[0] !== undefined ? arguments[0] : R; + + let _ref4 = arguments.length > 1 ? arguments[1] : undefined; + + let R = babelHelpers.extends({}, _ref4); +}; + +(_ref5, e, c = 2) => { + let R = babelHelpers.extends({}, _ref5); + let a = arguments.length > 3 && arguments[3] !== undefined ? arguments[3] : R; + let f = arguments.length > 4 && arguments[4] !== undefined ? arguments[4] : q; + return function () { + let q; + }(); +}; + +(_ref6) => { + let R = babelHelpers.extends({}, _ref6); + let a = arguments.length > 1 && arguments[1] !== undefined ? arguments[1] : f(R); +}; + +(_ref7, _ref8) => { + let R = babelHelpers.extends({}, _ref7); + let { + [R.key]: a = 42 + } = _ref8; +}; + +(_ref9, { + a = { + R: b + } +}) => { + let R = babelHelpers.extends({}, _ref9); +}; + +(_ref10, { + a = R => R +} = { + b: R => R +}) => { + let R = babelHelpers.extends({}, _ref10); +}; diff --git a/packages/babel-plugin-proposal-object-rest-spread/test/fixtures/object-rest/parameters/output.js b/packages/babel-plugin-proposal-object-rest-spread/test/fixtures/object-rest/parameters/output.js index 3fcf29325f96..ba6a49ecdba1 100644 --- a/packages/babel-plugin-proposal-object-rest-spread/test/fixtures/object-rest/parameters/output.js +++ b/packages/babel-plugin-proposal-object-rest-spread/test/fixtures/object-rest/parameters/output.js @@ -17,15 +17,15 @@ function a3(_ref3) { c2 = babelHelpers.objectWithoutProperties(_ref3, ["a2", "b2"]); } -function a4(_ref5, _ref4) { +function a4(_ref4, _ref5) { let { - a3 + a5 } = _ref5, - c3 = babelHelpers.objectWithoutProperties(_ref5, ["a3"]); + c5 = babelHelpers.objectWithoutProperties(_ref5, ["a5"]); let { - a5 + a3 } = _ref4, - c5 = babelHelpers.objectWithoutProperties(_ref4, ["a5"]); + c3 = babelHelpers.objectWithoutProperties(_ref4, ["a3"]); } function a5(_ref6) { diff --git a/packages/babel-plugin-transform-parameters/src/index.js b/packages/babel-plugin-transform-parameters/src/index.js index 5ba89ba9eec1..1f5cb7184b04 100644 --- a/packages/babel-plugin-transform-parameters/src/index.js +++ b/packages/babel-plugin-transform-parameters/src/index.js @@ -1,6 +1,7 @@ import { declare } from "@babel/helper-plugin-utils"; import convertFunctionParams from "./params"; import convertFunctionRest from "./rest"; +export { convertFunctionParams }; export default declare((api, options) => { api.assertVersion(7); diff --git a/packages/babel-plugin-transform-parameters/src/params.js b/packages/babel-plugin-transform-parameters/src/params.js index b0463f79a878..51901e1a26ba 100644 --- a/packages/babel-plugin-transform-parameters/src/params.js +++ b/packages/babel-plugin-transform-parameters/src/params.js @@ -41,7 +41,13 @@ const iifeVisitor = { path.skip(), }; -export default function convertFunctionParams(path, loose) { +// last 2 parameters are optional -- they are used by proposal-object-rest-spread/src/index.js +export default function convertFunctionParams( + path, + loose, + shouldTransformParam, + replaceRestElement, +) { const params = path.get("params"); const isSimpleParameterList = params.every(param => param.isIdentifier()); @@ -108,6 +114,14 @@ export default function convertFunctionParams(path, loose) { for (let i = 0; i < params.length; i++) { const param = params[i]; + if (shouldTransformParam && !shouldTransformParam(i)) { + continue; + } + const transformedRestNodes = []; + if (replaceRestElement) { + replaceRestElement(param.parentPath, param, transformedRestNodes); + } + const paramIsAssignmentPattern = param.isAssignmentPattern(); if (paramIsAssignmentPattern && (loose || node.kind === "set")) { const left = param.get("left"); @@ -164,6 +178,12 @@ export default function convertFunctionParams(path, loose) { param.replaceWith(t.cloneNode(uid)); } + + if (transformedRestNodes) { + for (const transformedNode of transformedRestNodes) { + body.push(transformedNode); + } + } } // we need to cut off all trailing parameters