Fix missing declaration invariant for "for" within try/catch
Fixes T180509722. What happened is that the logic in LeaveSSA to find declarations within for initializers wasn't working with try/catch because the initializer block gets broken up with a maybe-throw after every instruction that can throw. These maybe-throws can then get turned into gotos by PruneMaybeThrows, so LeaveSSA has to handle both cases. The new logic scans from the start of the init block until reaching the end, and creates declarations for all StoreLocals. Note that we don't yet support maybe-throw in value blocks — that's already a todo — so the change here simply allows us to compile farther until reaching that other todo. But i've double-checked the HIR and it looks correct for this case, so it should just work once we fix that todo. I've also added a comment to help us remember (and of course, we'd have to add a snap fixture too)
Joe Savona committed
Mar 13, 2024 at 13:52 UTC
dc7ed065c0d77e5e3df689f906f3a941032257b5
3 files changed
+81
-20
compiler/packages/babel-plugin-react-forget/src/SSA/LeaveSSA.ts
+46
-20
@@ -18,7 +18,7 @@ import {
18
Phi,
19
Place,
20
} from "../HIR/HIR";
21
-import { printPlace } from "../HIR/PrintHIR";
21
+import { printIdentifier, printPlace } from "../HIR/PrintHIR";
22
import {
23
eachInstructionLValue,
24
eachInstructionValueOperand,
@@ -375,31 +375,57 @@ export function leaveSSA(fn: HIRFunction): void {
375
terminal.kind === "for-of" ||
376
terminal.kind === "for-in"
377
) {
378
- const init = fn.body.blocks.get(terminal.init)!;
378
+ let init = fn.body.blocks.get(terminal.init)!;
379
pushPhis(init);
380
381
+ // The first block after the end of the init
382
+ let initContinuation =
383
+ terminal.kind === "for" ? terminal.test : terminal.loop;
384
/*
385
* To avoid generating a let binding for the initializer prior to the loop,
383
- * check to see if the for declares an iterator variable
386
+ * check to see if the for declares an iterator variable.
387
*/
385
- const initIdentifier = init.instructions.at(-1);
386
- if (
387
- initIdentifier !== undefined &&
388
- initIdentifier.value.kind === "StoreLocal"
389
- ) {
390
- const value = initIdentifier.value;
391
- if (value.lvalue.place.identifier.name !== null) {
392
- const originalLVal = declarations.get(
393
- value.lvalue.place.identifier.name.value
394
- );
395
- if (originalLVal === undefined) {
396
- declarations.set(value.lvalue.place.identifier.name.value, {
397
- lvalue: value.lvalue,
398
- place: value.lvalue.place,
399
- });
400
- value.lvalue.kind = InstructionKind.Const;
388
+ while (init.id !== initContinuation) {
389
+ for (const instr of init.instructions) {
390
+ if (
391
+ instr.value.kind === "StoreLocal" &&
392
+ instr.value.lvalue.kind !== InstructionKind.Reassign
393
+ ) {
394
+ const value = instr.value;
395
+ if (value.lvalue.place.identifier.name !== null) {
396
+ const originalLVal = declarations.get(
397
+ value.lvalue.place.identifier.name.value
398
+ );
399
+ if (originalLVal === undefined) {
400
+ declarations.set(value.lvalue.place.identifier.name.value, {
401
+ lvalue: value.lvalue,
402
+ place: value.lvalue.place,
403
+ });
404
+ value.lvalue.kind = InstructionKind.Const;
405
+ }
406
+ }
407
}
408
}
409
+
410
+ let next: BlockId | null = null;
411
+ switch (init.terminal.kind) {
412
+ case "maybe-throw": {
413
+ next = init.terminal.continuation;
414
+ break;
415
+ }
416
+ case "goto": {
417
+ next = init.terminal.block;
418
+ break;
419
+ }
420
+ default: {
421
+ break;
422
+ }
423
+ }
424
+ if (next === null) {
425
+ break;
426
+ }
427
+
428
+ init = fn.body.blocks.get(next)!;
429
}
430
431
if (terminal.kind === "for" && terminal.update !== null) {
@@ -446,7 +472,7 @@ export function leaveSSA(fn: HIRFunction): void {
472
CompilerError.invariant(declaration != null, {
473
loc: null,
474
reason: "Expected a declaration for all variables",
449
- description: null,
475
+ description: `${printIdentifier(phi.id)} in block bb${phiBlock.id}`,
476
suggestions: null,
477
});
478
if (isPhiMutatedAfterCreation) {
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-repro-declaration-for-all-identifiers.expect.md
new
+28
@@ -0,0 +1,28 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function Foo() {
6
+ try {
7
+ // NOTE: this fixture previously failed during LeaveSSA;
8
+ // double-check this code when supporting value blocks in try/catch
9
+ for (let i = 0; i < 2; i++) {}
10
+ } catch {}
11
+}
12
+
13
+```
14
+
15
+
16
+## Error
17
+
18
+```
19
+ 3 | // NOTE: this fixture previously failed during LeaveSSA;
20
+ 4 | // double-check this code when supporting value blocks in try/catch
21
+> 5 | for (let i = 0; i < 2; i++) {}
22
+ | ^ [ReactForget] Todo: Support value blocks (conditional, logical, optional chaining, etc) within a try/catch statement (5:5)
23
+ 6 | } catch {}
24
+ 7 | }
25
+ 8 |
26
+```
27
+
28
+
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-repro-declaration-for-all-identifiers.js
new
+7
@@ -0,0 +1,7 @@
1
+function Foo() {
2
+ try {
3
+ // NOTE: this fixture previously failed during LeaveSSA;
4
+ // double-check this code when supporting value blocks in try/catch
5
+ for (let i = 0; i < 2; i++) {}
6
+ } catch {}
7
+}