[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
Mofei Zhang committed
Jan 31, 2024 at 10:59 UTC
d0bb1fed61cba004f8014337488b3e8c398ab880
6 files changed
+58
-20
compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts
+28
-17
@@ -333,14 +333,9 @@ function lowerStatement(
333
const hoistableIdentifiers: Set<t.Identifier> = new Set();
334
335
for (const [, binding] of Object.entries(stmt.scope.bindings)) {
336
- // TODO: support other kinds of bindings
337
- if (binding.kind === "const") {
338
- if (
339
- binding.path.isVariableDeclarator() &&
340
- binding.path.get("id").isIdentifier()
341
- ) {
342
- hoistableIdentifiers.add(binding.identifier);
343
- }
336
+ // refs to params are always valid / never need to be hoisted
337
+ if (binding.kind !== "param") {
338
+ hoistableIdentifiers.add(binding.identifier);
339
}
340
}
341
@@ -349,7 +344,6 @@ function lowerStatement(
344
/*
345
* If we see a hoistable identifier before its declaration, it should be hoisted just
346
* before the statement that references it.
352
- * Identifier can only be hoisted if the reference occurs within an inner function
347
*/
348
let fnDepth = s.isFunctionDeclaration() ? 1 : 0;
349
const withFunctionContext = {
@@ -366,15 +360,20 @@ function lowerStatement(
360
ArrowFunctionExpression: withFunctionContext,
361
ObjectMethod: withFunctionContext,
362
Identifier(id: NodePath<t.Identifier>) {
369
- if (!id.isReferencedIdentifier() || fnDepth === 0) {
363
+ if (!id.isReferencedIdentifier()) {
364
return;
365
}
372
- const bindingIdentifier = id.scope.getBindingIdentifier(
373
- id.node.name
374
- );
366
+ const binding = id.scope.getBinding(id.node.name);
367
+ /**
368
+ * We can only hoist an identifier decl if
369
+ * 1. the reference occurs within an inner function
370
+ * or
371
+ * 2. the declaration itself is hoistable
372
+ */
373
if (
376
- bindingIdentifier != null &&
377
- hoistableIdentifiers.has(bindingIdentifier)
374
+ binding != null &&
375
+ hoistableIdentifiers.has(binding.identifier) &&
376
+ (fnDepth > 0 || binding.kind === "hoisted")
377
) {
378
willHoist.add(id);
379
}
@@ -408,7 +407,7 @@ function lowerStatement(
407
builder.errors.push({
408
severity: ErrorSeverity.Todo,
409
reason: "Unsupported declaration type for hoisting",
411
- description: `${id.parentPath.type}`,
410
+ description: `variable "${binding.identifier.name}" declared with ${binding.path.type}`,
411
suggestions: null,
412
loc: id.parentPath.node.loc ?? GeneratedSource,
413
});
@@ -417,7 +416,19 @@ function lowerStatement(
416
builder.errors.push({
417
severity: ErrorSeverity.Todo,
418
reason: "Unsupported variable declaration type for hoisting",
420
- description: `${binding.path.get("id").type}`,
419
+ description: `variable "${
420
+ binding.identifier.name
421
+ }" declared with ${binding.path.get("id").type}`,
422
+ suggestions: null,
423
+ loc: id.parentPath.node.loc ?? GeneratedSource,
424
+ });
425
+ continue;
426
+ } else if (binding.kind !== "const" && binding.kind !== "var") {
427
+ // Avoid double errors on var declarations, which we do not plan to support anyways
428
+ builder.errors.push({
429
+ severity: ErrorSeverity.Todo,
430
+ reason: "Handle non-const declarations for hoisting",
431
+ description: `variable "${binding.identifier.name}" declared with ${binding.kind}`,
432
suggestions: null,
433
loc: id.parentPath.node.loc ?? GeneratedSource,
434
});
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.hoisted-function-declaration.expect.md
+1
-1
@@ -17,7 +17,7 @@ function component(a) {
17
## Error
18
19
```
20
-[ReactForget] Todo: [hoisting] EnterSSA: Expected identifier to be defined before being used. Identifier x$5 is undefined (4:6)
20
+[ReactForget] Todo: Unsupported declaration type for hoisting. variable "x" declared with FunctionDeclaration (3:3)
21
```
22
23
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.hoisting-simple-function-declaration.expect.md
+1
-1
@@ -24,7 +24,7 @@ export const FIXTURE_ENTRYPOINT = {
24
## Error
25
26
```
27
-[ReactForget] Invariant: [hoisting] Expected value for identifier to be initialized. baz$5 (5:5)
27
+[ReactForget] Todo: Unsupported declaration type for hoisting. variable "baz" declared with FunctionDeclaration (5:5)
28
```
29
30
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.mutate-captured-arg-separately.expect.md
+1
-1
@@ -19,7 +19,7 @@ function component(a) {
19
## Error
20
21
```
22
-[ReactForget] Todo: [hoisting] EnterSSA: Expected identifier to be defined before being used. Identifier x$1 is undefined (7:7)
22
+[ReactForget] Todo: Handle non-const declarations for hoisting. variable "x" declared with let (4:4)
23
```
24
25
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-hoist-function-decls.expect.md
new
+21
@@ -0,0 +1,21 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function Component() {
6
+ return get2();
7
+ function get2() {
8
+ return 2;
9
+ }
10
+}
11
+
12
+```
13
+
14
+
15
+## Error
16
+
17
+```
18
+[ReactForget] Todo: Unsupported declaration type for hoisting. variable "get2" declared with FunctionDeclaration (2:2)
19
+```
20
+
21
+
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-hoist-function-decls.js
new
+6
@@ -0,0 +1,6 @@
1
+function Component() {
2
+ return get2();
3
+ function get2() {
4
+ return 2;
5
+ }
6
+}