From d0bb1fed61cba004f8014337488b3e8c398ab880 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Wed, 31 Jan 2024 10:59:25 -0500 Subject: [PATCH] [be] Explicit todo diagnostics for hoisting --- Currently, we error on non-hoisted identifiers in EnterSSA with a somewhat cryptic message. This PR changes `BuildHIR` hoisting logic to find ALL hoistable bindings, then error when we try to lower hoisting for unsupported declaration types. Two benefits to this refactoring: - Dedups "unhandled identifier declaration" logic (previous to #2552 and this PR, we did this check in three places). - More explicit todo diagnostic messages when we cannot hoist a declaration --- .../src/HIR/BuildHIR.ts | 45 ++++++++++++------- ...ror.hoisted-function-declaration.expect.md | 2 +- ...ting-simple-function-declaration.expect.md | 2 +- ...r.mutate-captured-arg-separately.expect.md | 2 +- .../error.todo-hoist-function-decls.expect.md | 21 +++++++++ .../error.todo-hoist-function-decls.js | 6 +++ 6 files changed, 58 insertions(+), 20 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-hoist-function-decls.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-hoist-function-decls.js diff --git a/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts b/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts index 47da04b87b..ae526f94f6 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts @@ -333,14 +333,9 @@ function lowerStatement( const hoistableIdentifiers: Set = new Set(); for (const [, binding] of Object.entries(stmt.scope.bindings)) { - // TODO: support other kinds of bindings - if (binding.kind === "const") { - if ( - binding.path.isVariableDeclarator() && - binding.path.get("id").isIdentifier() - ) { - hoistableIdentifiers.add(binding.identifier); - } + // refs to params are always valid / never need to be hoisted + if (binding.kind !== "param") { + hoistableIdentifiers.add(binding.identifier); } } @@ -349,7 +344,6 @@ function lowerStatement( /* * If we see a hoistable identifier before its declaration, it should be hoisted just * before the statement that references it. - * Identifier can only be hoisted if the reference occurs within an inner function */ let fnDepth = s.isFunctionDeclaration() ? 1 : 0; const withFunctionContext = { @@ -366,15 +360,20 @@ function lowerStatement( ArrowFunctionExpression: withFunctionContext, ObjectMethod: withFunctionContext, Identifier(id: NodePath) { - if (!id.isReferencedIdentifier() || fnDepth === 0) { + if (!id.isReferencedIdentifier()) { return; } - const bindingIdentifier = id.scope.getBindingIdentifier( - id.node.name - ); + const binding = id.scope.getBinding(id.node.name); + /** + * We can only hoist an identifier decl if + * 1. the reference occurs within an inner function + * or + * 2. the declaration itself is hoistable + */ if ( - bindingIdentifier != null && - hoistableIdentifiers.has(bindingIdentifier) + binding != null && + hoistableIdentifiers.has(binding.identifier) && + (fnDepth > 0 || binding.kind === "hoisted") ) { willHoist.add(id); } @@ -408,7 +407,7 @@ function lowerStatement( builder.errors.push({ severity: ErrorSeverity.Todo, reason: "Unsupported declaration type for hoisting", - description: `${id.parentPath.type}`, + description: `variable "${binding.identifier.name}" declared with ${binding.path.type}`, suggestions: null, loc: id.parentPath.node.loc ?? GeneratedSource, }); @@ -417,7 +416,19 @@ function lowerStatement( builder.errors.push({ severity: ErrorSeverity.Todo, reason: "Unsupported variable declaration type for hoisting", - description: `${binding.path.get("id").type}`, + description: `variable "${ + binding.identifier.name + }" declared with ${binding.path.get("id").type}`, + suggestions: null, + loc: id.parentPath.node.loc ?? GeneratedSource, + }); + continue; + } else if (binding.kind !== "const" && binding.kind !== "var") { + // Avoid double errors on var declarations, which we do not plan to support anyways + builder.errors.push({ + severity: ErrorSeverity.Todo, + reason: "Handle non-const declarations for hoisting", + description: `variable "${binding.identifier.name}" declared with ${binding.kind}`, suggestions: null, loc: id.parentPath.node.loc ?? GeneratedSource, }); diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.hoisted-function-declaration.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.hoisted-function-declaration.expect.md index e8570429d7..f202e41608 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.hoisted-function-declaration.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.hoisted-function-declaration.expect.md @@ -17,7 +17,7 @@ function component(a) { ## Error ``` -[ReactForget] Todo: [hoisting] EnterSSA: Expected identifier to be defined before being used. Identifier x$5 is undefined (4:6) +[ReactForget] Todo: Unsupported declaration type for hoisting. variable "x" declared with FunctionDeclaration (3:3) ``` \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.hoisting-simple-function-declaration.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.hoisting-simple-function-declaration.expect.md index 535cfe3ec1..1f01ec4dbf 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.hoisting-simple-function-declaration.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.hoisting-simple-function-declaration.expect.md @@ -24,7 +24,7 @@ export const FIXTURE_ENTRYPOINT = { ## Error ``` -[ReactForget] Invariant: [hoisting] Expected value for identifier to be initialized. baz$5 (5:5) +[ReactForget] Todo: Unsupported declaration type for hoisting. variable "baz" declared with FunctionDeclaration (5:5) ``` \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.mutate-captured-arg-separately.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.mutate-captured-arg-separately.expect.md index b850019b53..280ee5dcde 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.mutate-captured-arg-separately.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.mutate-captured-arg-separately.expect.md @@ -19,7 +19,7 @@ function component(a) { ## Error ``` -[ReactForget] Todo: [hoisting] EnterSSA: Expected identifier to be defined before being used. Identifier x$1 is undefined (7:7) +[ReactForget] Todo: Handle non-const declarations for hoisting. variable "x" declared with let (4:4) ``` \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-hoist-function-decls.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-hoist-function-decls.expect.md new file mode 100644 index 0000000000..e5fd027e08 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-hoist-function-decls.expect.md @@ -0,0 +1,21 @@ + +## Input + +```javascript +function Component() { + return get2(); + function get2() { + return 2; + } +} + +``` + + +## Error + +``` +[ReactForget] Todo: Unsupported declaration type for hoisting. variable "get2" declared with FunctionDeclaration (2:2) +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-hoist-function-decls.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-hoist-function-decls.js new file mode 100644 index 0000000000..385c716fbb --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-hoist-function-decls.js @@ -0,0 +1,6 @@ +function Component() { + return get2(); + function get2() { + return 2; + } +}