From 482008c8914fbd9cc253022141892770bbbf73f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Ribaudo?= Date: Wed, 17 Jul 2024 01:48:03 +0200 Subject: [PATCH] Simplify `helper-function-name` logic (#16652) * Simplify `helper-function-name` logic * Simplify renaming logic * Avoid custom wrapper for generators --- .../babel-helper-function-name/src/index.ts | 114 +++++------------- .../self-referential/output.js | 10 +- .../src/index.ts | 2 +- .../with-arrow-functions-transform/output.js | 6 +- .../plugins-integration/issue-15170/output.js | 22 +--- .../babel-traverse/src/path/conversion.ts | 2 +- 6 files changed, 46 insertions(+), 110 deletions(-) diff --git a/packages/babel-helper-function-name/src/index.ts b/packages/babel-helper-function-name/src/index.ts index 50cf66744542..0fb1a38b2f08 100644 --- a/packages/babel-helper-function-name/src/index.ts +++ b/packages/babel-helper-function-name/src/index.ts @@ -1,8 +1,7 @@ import template from "@babel/template"; import { - NOT_LOCAL_BINDING, - cloneNode, identifier, + inherits, isAssignmentExpression, isAssignmentPattern, isFunction, @@ -28,7 +27,7 @@ function getFunctionArity(node: t.Function): number { return count === -1 ? node.params.length : count; } -const buildPropertyMethodAssignmentWrapper = template.statement(` +const buildPropertyMethodAssignmentWrapper = template.expression(` (function (FUNCTION_KEY) { function FUNCTION_ID() { return FUNCTION_KEY.apply(this, arguments); @@ -42,46 +41,29 @@ const buildPropertyMethodAssignmentWrapper = template.statement(` })(FUNCTION) `); -const buildGeneratorPropertyMethodAssignmentWrapper = template.statement(` - (function (FUNCTION_KEY) { - function* FUNCTION_ID() { - return yield* FUNCTION_KEY.apply(this, arguments); - } - - FUNCTION_ID.toString = function () { - return FUNCTION_KEY.toString(); - }; - - return FUNCTION_ID; - })(FUNCTION) -`); - type State = { name: string; - outerDeclar: t.Identifier; - selfAssignment: boolean; - selfReference: boolean; + needsRename: boolean; }; -const visitor: Visitor = { +const refersOuterBindingVisitor: Visitor = { "ReferencedIdentifier|BindingIdentifier"( path: NodePath, state, ) { // check if this node matches our function id if (path.node.name !== state.name) return; - - // check that we don't have a local variable declared as that removes the need - // for the wrapper - const localDeclar = path.scope.getBindingIdentifier(state.name); - if (localDeclar !== state.outerDeclar) return; - - state.selfReference = true; + state.needsRename = true; path.stop(); }, + Scope(path, state) { + if (path.scope.hasOwnBinding(state.name)) { + path.skip(); + } + }, }; -function getNameFromLiteralId(id: t.Literal) { +function getNameFromLiteralId(id: t.Literal): string { if (isNullLiteral(id)) { return "null"; } @@ -102,12 +84,12 @@ function getNameFromLiteralId(id: t.Literal) { } function wrap( - state: State, + needsRename: boolean, method: t.FunctionExpression | t.Class, id: t.Identifier, scope: Scope, ) { - if (state.selfReference) { + if (needsRename) { if (scope.hasBinding(id.name) && !scope.hasGlobal(id.name)) { // we can just munge the local binding scope.rename(id.name); @@ -116,25 +98,17 @@ function wrap( if (!isFunction(method)) return; // need to add a wrapper since we can't change the references - let build = buildPropertyMethodAssignmentWrapper; - if (method.generator) { - build = buildGeneratorPropertyMethodAssignmentWrapper; - } - const template = ( - build({ - FUNCTION: method, - FUNCTION_ID: id, - FUNCTION_KEY: scope.generateUidIdentifier(id.name), - }) as t.ExpressionStatement - ).expression as t.CallExpression; + const template = buildPropertyMethodAssignmentWrapper({ + FUNCTION: method, + FUNCTION_ID: id, + FUNCTION_KEY: scope.generateUidIdentifier(id.name), + }) as t.CallExpression; // shim in dummy params to retain function arity, if you try to read the // source then you'll get the original since it's proxied so it's all good - const params = ( - (template.callee as t.FunctionExpression).body - .body[0] as any as t.FunctionExpression - ).params; + const { params } = (template.callee as t.FunctionExpression).body + .body[0] as any as t.FunctionExpression; for (let i = 0, len = getFunctionArity(method); i < len; i++) { params.push(scope.generateUidIdentifier("x")); @@ -153,18 +127,12 @@ function visit( name: string, scope: Scope, ) { - const state: State = { - selfAssignment: false, - selfReference: false, - outerDeclar: scope.getBindingIdentifier(name), - name: name, - }; + const state: State = { needsRename: false, name }; // check to see if we have a local binding of the id we're setting inside of // the function, this is important as there are caveats associated const binding = scope.getOwnBinding(name); - if (binding) { if (binding.kind === "param") { // safari will blow up in strict mode with code like: @@ -179,7 +147,7 @@ function visit( // this isn't to the spec and they've invented this behaviour which is // **extremely** annoying so we avoid setting the name if it has a param // with the same id - state.selfReference = true; + state.needsRename = true; } else { // otherwise it's defined somewhere in scope like: // @@ -190,11 +158,11 @@ function visit( // so we can safely just set the id and move along as it shadows the // bound function id } - } else if (state.outerDeclar || scope.hasGlobal(name)) { - scope.traverse(node, visitor, state); + } else if (scope.parent.hasBinding(name) || scope.hasGlobal(name)) { + scope.traverse(node, refersOuterBindingVisitor, state); } - return state; + return state.needsRename; } /** @@ -215,7 +183,7 @@ function visit( * - an IIFE when `node` contains a binding shadowing the inferred function name (e.g. `let f = function (f) {}`), * - `void` when `node` has `id` property or the helper can not inferred the name or the inferred name contains non-BMP characters that is not supported by current target */ -export default function ( +export default function nameFunction( { node, parent, @@ -231,7 +199,8 @@ export default function ( | t.NumericLiteral | t.BigIntLiteral; }, - localBinding = false, + // eslint-disable-next-line @typescript-eslint/no-unused-vars + localBinding = false, // TODO: Remove this unused param supportUnicodeId = false, ): N | t.CallExpression | void { // has an `id` so we don't need to infer one @@ -250,22 +219,6 @@ export default function ( } else if (isVariableDeclarator(parent)) { // let foo = function () {}; id = parent.id; - - // but not "let foo = () => {};" being converted to function expression - if (isIdentifier(id) && !localBinding) { - const binding = scope.parent.getBinding(id.name); - if ( - binding && - binding.constant && - scope.getBinding(id.name) === binding - ) { - // always going to reference this method - node.id = cloneNode(id); - // @ts-expect-error Fixme: avoid mutating AST nodes - node.id[NOT_LOCAL_BINDING] = true; - return; - } - } } else if (isAssignmentExpression(parent, { operator: "=" })) { // foo = function () {}; id = parent.left; @@ -290,13 +243,8 @@ export default function ( name = toBindingIdentifierName(name); const newId = identifier(name); + if (id) inherits(newId, id); - // The id shouldn't be considered a local binding to the function because - // we are simply trying to set the function name and not actually create - // a local binding. - // @ts-expect-error Fixme: avoid mutating AST nodes - newId[NOT_LOCAL_BINDING] = true; - - const state = visit(node, name, scope); - return wrap(state, node, newId, scope) || node; + const needsRename = visit(node, name, scope); + return wrap(needsRename, node, newId, scope) || node; } diff --git a/packages/babel-plugin-transform-arrow-functions/test/fixtures/arrow-functions/self-referential/output.js b/packages/babel-plugin-transform-arrow-functions/test/fixtures/arrow-functions/self-referential/output.js index ad7cfa875a66..39a13c925899 100644 --- a/packages/babel-plugin-transform-arrow-functions/test/fixtures/arrow-functions/self-referential/output.js +++ b/packages/babel-plugin-transform-arrow-functions/test/fixtures/arrow-functions/self-referential/output.js @@ -1,13 +1,13 @@ var fooCalls = []; -var jumpTable = function jumpTable(name, arg) { - if (jumpTable[name]) { - jumpTable[name](arg); +var _jumpTable = function jumpTable(name, arg) { + if (_jumpTable[name]) { + _jumpTable[name](arg); } }; -Object.assign(jumpTable, { +Object.assign(_jumpTable, { foo(arg) { fooCalls.push(arg); } }); -jumpTable('foo', 'bar'); +_jumpTable('foo', 'bar'); expect(fooCalls[0]).toBe('bar'); diff --git a/packages/babel-plugin-transform-function-name/src/index.ts b/packages/babel-plugin-transform-function-name/src/index.ts index dcecfc7a77da..d0cdb87d68b2 100644 --- a/packages/babel-plugin-transform-function-name/src/index.ts +++ b/packages/babel-plugin-transform-function-name/src/index.ts @@ -28,7 +28,7 @@ export default declare(api => { const newNode = nameFunction( // @ts-expect-error Fixme: should check ArrowFunctionExpression value, - false, + undefined, supportUnicodeId, ); if (newNode) value.replaceWith(newNode); diff --git a/packages/babel-plugin-transform-function-name/test/fixtures/function-name/with-arrow-functions-transform/output.js b/packages/babel-plugin-transform-function-name/test/fixtures/function-name/with-arrow-functions-transform/output.js index 2a8510803a31..1e80bbf19ac6 100644 --- a/packages/babel-plugin-transform-function-name/test/fixtures/function-name/with-arrow-functions-transform/output.js +++ b/packages/babel-plugin-transform-function-name/test/fixtures/function-name/with-arrow-functions-transform/output.js @@ -1,11 +1,11 @@ -const x = function x() { - return x; +const _x = function x() { + return _x; }; const y = function y(x) { return x(); }; const z = { z: function z() { - return y(x); + return y(_x); } }.z; diff --git a/packages/babel-preset-env/test/fixtures/plugins-integration/issue-15170/output.js b/packages/babel-preset-env/test/fixtures/plugins-integration/issue-15170/output.js index eb549fd35d17..fba3285b6fb8 100644 --- a/packages/babel-preset-env/test/fixtures/plugins-integration/issue-15170/output.js +++ b/packages/babel-preset-env/test/fixtures/plugins-integration/issue-15170/output.js @@ -1,34 +1,22 @@ var _this = this; x = /*#__PURE__*/function () { var _ref = function (_x) { - var _marked = /*#__PURE__*/babelHelpers.regeneratorRuntime().mark(x); function x(_x2) { - var _args = arguments; - return babelHelpers.regeneratorRuntime().wrap(function x$(_context) { - while (1) switch (_context.prev = _context.next) { - case 0: - return _context.delegateYield(_x.apply(this, _args), "t0", 1); - case 1: - return _context.abrupt("return", _context.t0); - case 2: - case "end": - return _context.stop(); - } - }, _marked, this); + return _x.apply(this, arguments); } x.toString = function () { return _x.toString(); }; return x; }( /*#__PURE__*/babelHelpers.regeneratorRuntime().mark(function _callee(x) { - return babelHelpers.regeneratorRuntime().wrap(function _callee$(_context2) { - while (1) switch (_context2.prev = _context2.next) { + return babelHelpers.regeneratorRuntime().wrap(function _callee$(_context) { + while (1) switch (_context.prev = _context.next) { case 0: babelHelpers.newArrowCheck(this, _this); - return _context2.abrupt("return", 0); + return _context.abrupt("return", 0); case 2: case "end": - return _context2.stop(); + return _context.stop(); } }, _callee, this); })); diff --git a/packages/babel-traverse/src/path/conversion.ts b/packages/babel-traverse/src/path/conversion.ts index 770dca28f211..ddcd5e6a3fb7 100644 --- a/packages/babel-traverse/src/path/conversion.ts +++ b/packages/babel-traverse/src/path/conversion.ts @@ -217,7 +217,7 @@ export function arrowFunctionToExpression( callExpression( memberExpression( // @ts-expect-error TS can't infer nameFunction returns CallExpression | ArrowFunctionExpression here - nameFunction(this, true) || fn.node, + nameFunction(this) || fn.node, identifier("bind"), ), [checkBinding ? identifier(checkBinding.name) : thisExpression()],