From a97c55dc5da37c0d0358fee660be4343e7528c38 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 4 Apr 2023 13:29:04 -0700 Subject: [PATCH] Support for with empty update expression Adds support for `for` statements with an empty or unreachable update expression. In both cases, reversePostorderBlocks() will remove the empty/unreachable update block, leaving the ForTerminal.update pointing to a non-existent block. We explicitly rewrite this (much like we null out unreachable fallthroughs after shrink). When transforming to ReactiveFunction, we emit the update block as null if it was the same as the test block. --- compiler/forget/src/HIR/BuildHIR.ts | 48 +++++++++---------- compiler/forget/src/HIR/HIR.ts | 2 +- compiler/forget/src/HIR/HIRBuilder.ts | 13 +++++ compiler/forget/src/HIR/visitors.ts | 2 +- .../src/Optimization/ConstantPropagation.ts | 6 ++- .../ReactiveScopes/BuildReactiveFunction.ts | 10 ++-- compiler/forget/src/SSA/LeaveSSA.ts | 2 +- .../forget/src/Utils/VisualizeHIRMermaid.ts | 4 +- .../compiler/error.for-return.expect.md | 20 -------- ...r.leave-ssa-handle-return-in-for.expect.md | 20 -------- .../error.leave-ssa-handle-return-in-for.js | 5 -- .../compiler/error.todo-kitchensink.expect.md | 18 ------- .../for-empty-update-with-continue.expect.md | 30 ++++++++++++ .../for-empty-update-with-continue.js | 9 ++++ .../compiler/for-empty-update.expect.md | 33 +++++++++++++ .../fixtures/compiler/for-empty-update.js | 10 ++++ .../fixtures/compiler/for-return.expect.md | 23 +++++++++ .../{error.for-return.js => for-return.js} | 0 18 files changed, 158 insertions(+), 97 deletions(-) delete mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.for-return.expect.md delete mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.leave-ssa-handle-return-in-for.expect.md delete mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.leave-ssa-handle-return-in-for.js create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/for-empty-update-with-continue.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/for-empty-update-with-continue.js create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/for-empty-update.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/for-empty-update.js create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/for-return.expect.md rename compiler/forget/src/__tests__/fixtures/compiler/{error.for-return.js => for-return.js} (100%) diff --git a/compiler/forget/src/HIR/BuildHIR.ts b/compiler/forget/src/HIR/BuildHIR.ts index a6abeb9bf7..755b287edc 100644 --- a/compiler/forget/src/HIR/BuildHIR.ts +++ b/compiler/forget/src/HIR/BuildHIR.ts @@ -307,35 +307,35 @@ function lowerStatement( }; }); - const updateBlock = builder.enter("loop", (_blockId) => { - const update = stmt.get("update"); - if (update.node == null) { - builder.errors.push({ - reason: `(BuildHIR::lowerStatement) Handle empty update in ForStatement`, - severity: ErrorSeverity.Todo, - nodePath: stmt, - }); - return { kind: "unsupported", id: makeInstructionId(0) }; - } - lowerExpressionToTemporary(builder, update as NodePath); - return { - kind: "goto", - block: testBlock.id, - variant: GotoVariant.Break, - id: makeInstructionId(0), - }; - }); - - const bodyBlock = builder.enter("block", (_blockId) => { - return builder.loop(label, updateBlock, continuationBlock.id, () => { - lowerStatement(builder, stmt.get("body")); + let updateBlock: BlockId | null = null; + const update = stmt.get("update"); + if (update.node != null) { + updateBlock = builder.enter("loop", (_blockId) => { + lowerExpressionToTemporary(builder, update as NodePath); return { kind: "goto", - block: updateBlock, - variant: GotoVariant.Continue, + block: testBlock.id, + variant: GotoVariant.Break, id: makeInstructionId(0), }; }); + } + + const bodyBlock = builder.enter("block", (_blockId) => { + return builder.loop( + label, + updateBlock ?? testBlock.id, + continuationBlock.id, + () => { + lowerStatement(builder, stmt.get("body")); + return { + kind: "goto", + block: updateBlock ?? testBlock.id, + variant: GotoVariant.Continue, + id: makeInstructionId(0), + }; + } + ); }); builder.terminateWithContinuation( diff --git a/compiler/forget/src/HIR/HIR.ts b/compiler/forget/src/HIR/HIR.ts index 91f746b23c..5e56f3b0a7 100644 --- a/compiler/forget/src/HIR/HIR.ts +++ b/compiler/forget/src/HIR/HIR.ts @@ -347,7 +347,7 @@ export type ForTerminal = { loc: SourceLocation; init: BlockId; test: BlockId; - update: BlockId; + update: BlockId | null; loop: BlockId; fallthrough: BlockId; id: InstructionId; diff --git a/compiler/forget/src/HIR/HIRBuilder.ts b/compiler/forget/src/HIR/HIRBuilder.ts index 3fc17f7c08..40689fc7a8 100644 --- a/compiler/forget/src/HIR/HIRBuilder.ts +++ b/compiler/forget/src/HIR/HIRBuilder.ts @@ -282,6 +282,7 @@ export default class HIRBuilder { logHIR("Build (shrunk)", ir); // then convert to reverse postorder reversePostorderBlocks(ir); + removeUnreachableForUpdates(ir); removeUnreachableFallthroughs(ir); removeDeadDoWhileStatements(ir); markInstructionIds(ir); @@ -528,6 +529,18 @@ export function shrink(func: HIR): void { } } +export function removeUnreachableForUpdates(fn: HIR): void { + for (const [, block] of fn.blocks) { + if ( + block.terminal.kind === "for" && + block.terminal.update !== null && + !fn.blocks.has(block.terminal.update) + ) { + block.terminal.update = null; + } + } +} + export function removeUnreachableFallthroughs(func: HIR): void { const visited: Set = new Set(); for (const [_, block] of func.blocks) { diff --git a/compiler/forget/src/HIR/visitors.ts b/compiler/forget/src/HIR/visitors.ts index 9256a6c4bc..93a2d2be3a 100644 --- a/compiler/forget/src/HIR/visitors.ts +++ b/compiler/forget/src/HIR/visitors.ts @@ -632,7 +632,7 @@ export function mapTerminalSuccessors( case "for": { const init = fn(terminal.init); const test = fn(terminal.test); - const update = fn(terminal.update); + const update = terminal.update !== null ? fn(terminal.update) : null; const loop = fn(terminal.loop); const fallthrough = fn(terminal.fallthrough); return { diff --git a/compiler/forget/src/Optimization/ConstantPropagation.ts b/compiler/forget/src/Optimization/ConstantPropagation.ts index 3f087cfe16..baabb64899 100644 --- a/compiler/forget/src/Optimization/ConstantPropagation.ts +++ b/compiler/forget/src/Optimization/ConstantPropagation.ts @@ -24,7 +24,10 @@ import { validateConsistentIdentifiers, validateTerminalSuccessors, } from "../HIR"; -import { removeDeadDoWhileStatements } from "../HIR/HIRBuilder"; +import { + removeDeadDoWhileStatements, + removeUnreachableForUpdates, +} from "../HIR/HIRBuilder"; import { eliminateRedundantPhi } from "../SSA"; /** @@ -52,6 +55,7 @@ export function constantPropagation(fn: HIRFunction): void { shrink(fn.body); reversePostorderBlocks(fn.body); removeUnreachableFallthroughs(fn.body); + removeUnreachableForUpdates(fn.body); removeDeadDoWhileStatements(fn.body); markInstructionIds(fn.body); markPredecessors(fn.body); diff --git a/compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts b/compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts index f35d065085..32a26ffa7b 100644 --- a/compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts +++ b/compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts @@ -347,7 +347,7 @@ class Driver { const scheduleId = this.cx.scheduleLoop( terminal.fallthrough, - terminal.update, + terminal.update ?? terminal.test, terminal.loop ); scheduleIds.push(scheduleId); @@ -382,10 +382,10 @@ class Driver { terminal.loc ).value; - const updateValue = this.visitValueBlock( - terminal.update, - terminal.loc - ).value; + const updateValue = + terminal.update !== null + ? this.visitValueBlock(terminal.update, terminal.loc).value + : null; let loopBody: ReactiveBlock; if (loopId) { diff --git a/compiler/forget/src/SSA/LeaveSSA.ts b/compiler/forget/src/SSA/LeaveSSA.ts index 3c25a07b90..c5b059343f 100644 --- a/compiler/forget/src/SSA/LeaveSSA.ts +++ b/compiler/forget/src/SSA/LeaveSSA.ts @@ -335,7 +335,7 @@ export function leaveSSA(fn: HIRFunction): void { } } - if (terminal.kind === "for") { + if (terminal.kind === "for" && terminal.update !== null) { const update = fn.body.blocks.get(terminal.update)!; pushPhis(update); } diff --git a/compiler/forget/src/Utils/VisualizeHIRMermaid.ts b/compiler/forget/src/Utils/VisualizeHIRMermaid.ts index fe74d151ce..4954bfdc24 100644 --- a/compiler/forget/src/Utils/VisualizeHIRMermaid.ts +++ b/compiler/forget/src/Utils/VisualizeHIRMermaid.ts @@ -205,7 +205,9 @@ function printTerminalArrows(blockId: BlockId, terminal: Terminal): string { case "for": { buffer.push(printJumpArrow(blockId, terminal.init, "init")); buffer.push(printJumpArrow(blockId, terminal.test, "test")); - buffer.push(printJumpArrow(blockId, terminal.update, "update")); + if (terminal.update !== null) { + buffer.push(printJumpArrow(blockId, terminal.update, "update")); + } buffer.push(printJumpArrow(blockId, terminal.loop, "loop")); buffer.push(printJumpArrow(blockId, terminal.fallthrough, "fallthrough")); break; diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.for-return.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.for-return.expect.md deleted file mode 100644 index 851361dd9b..0000000000 --- a/compiler/forget/src/__tests__/fixtures/compiler/error.for-return.expect.md +++ /dev/null @@ -1,20 +0,0 @@ - -## Input - -```javascript -function Component(props) { - for (let i = 0; i < props.count; i++) { - return; - } -} - -``` - - -## Error - -``` -[ReactForget] Invariant: Block bb4 does not exist for terminal '[1] For init=bb3 test=bb1 loop=bb5 update=bb4 fallthrough=bb2' (2:4) -``` - - \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.leave-ssa-handle-return-in-for.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.leave-ssa-handle-return-in-for.expect.md deleted file mode 100644 index 5faf9a3cc9..0000000000 --- a/compiler/forget/src/__tests__/fixtures/compiler/error.leave-ssa-handle-return-in-for.expect.md +++ /dev/null @@ -1,20 +0,0 @@ - -## Input - -```javascript -function foo(props) { - for (let i = 0; i < 10; i += 1) { - return; - } -} - -``` - - -## Error - -``` -Cannot read properties of undefined (reading 'phis') -``` - - \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.leave-ssa-handle-return-in-for.js b/compiler/forget/src/__tests__/fixtures/compiler/error.leave-ssa-handle-return-in-for.js deleted file mode 100644 index 9f652923ee..0000000000 --- a/compiler/forget/src/__tests__/fixtures/compiler/error.leave-ssa-handle-return-in-for.js +++ /dev/null @@ -1,5 +0,0 @@ -function foo(props) { - for (let i = 0; i < 10; i += 1) { - return; - } -} diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.todo-kitchensink.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.todo-kitchensink.expect.md index 76492789a5..4fe768ca39 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/error.todo-kitchensink.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.todo-kitchensink.expect.md @@ -144,15 +144,6 @@ let moduleLocal = false; 25 | } 26 | for (;;) { -[ReactForget] TodoError: (BuildHIR::lowerStatement) Handle empty update in ForStatement - 21 | x.push(i); - 22 | } -> 23 | for (; i < 3; ) { - | ^ - 24 | break; - 25 | } - 26 | for (;;) { - [ReactForget] TodoError: (BuildHIR::lowerStatement) Handle non-variable initialization in ForStatement 24 | break; 25 | } @@ -162,15 +153,6 @@ let moduleLocal = false; 28 | } 29 | -[ReactForget] TodoError: (BuildHIR::lowerStatement) Handle empty update in ForStatement - 24 | break; - 25 | } -> 26 | for (;;) { - | ^ - 27 | break; - 28 | } - 29 | - [ReactForget] TodoError: (BuildHIR::lowerStatement) Handle empty test in ForStatement 24 | break; 25 | } diff --git a/compiler/forget/src/__tests__/fixtures/compiler/for-empty-update-with-continue.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/for-empty-update-with-continue.expect.md new file mode 100644 index 0000000000..8319c32082 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/for-empty-update-with-continue.expect.md @@ -0,0 +1,30 @@ + +## Input + +```javascript +function Component(props) { + let x = 0; + for (let i = 0; i < props.count; ) { + x += i; + i += 1; + continue; + } + return x; +} + +``` + +## Code + +```javascript +function Component(props) { + let x = 0; + for (let i = 0; i < props.count; ) { + x = x + i; + i = i + 1; + } + return x; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/for-empty-update-with-continue.js b/compiler/forget/src/__tests__/fixtures/compiler/for-empty-update-with-continue.js new file mode 100644 index 0000000000..8ad3eeeb16 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/for-empty-update-with-continue.js @@ -0,0 +1,9 @@ +function Component(props) { + let x = 0; + for (let i = 0; i < props.count; ) { + x += i; + i += 1; + continue; + } + return x; +} diff --git a/compiler/forget/src/__tests__/fixtures/compiler/for-empty-update.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/for-empty-update.expect.md new file mode 100644 index 0000000000..cd1e17f761 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/for-empty-update.expect.md @@ -0,0 +1,33 @@ + +## Input + +```javascript +function Component(props) { + let x = 0; + for (let i = 0; i < props.count; ) { + x += i; + if (x > 10) { + break; + } + } + return x; +} + +``` + +## Code + +```javascript +function Component(props) { + let x = 0; + for (const i = 0; 0 < props.count; ) { + x = x + 0; + if (x > 10) { + break; + } + } + return x; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/for-empty-update.js b/compiler/forget/src/__tests__/fixtures/compiler/for-empty-update.js new file mode 100644 index 0000000000..d1411e8498 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/for-empty-update.js @@ -0,0 +1,10 @@ +function Component(props) { + let x = 0; + for (let i = 0; i < props.count; ) { + x += i; + if (x > 10) { + break; + } + } + return x; +} diff --git a/compiler/forget/src/__tests__/fixtures/compiler/for-return.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/for-return.expect.md new file mode 100644 index 0000000000..5670e40374 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/for-return.expect.md @@ -0,0 +1,23 @@ + +## Input + +```javascript +function Component(props) { + for (let i = 0; i < props.count; i++) { + return; + } +} + +``` + +## Code + +```javascript +function Component(props) { + for (const i = 0; 0 < props.count; ) { + return; + } +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.for-return.js b/compiler/forget/src/__tests__/fixtures/compiler/for-return.js similarity index 100% rename from compiler/forget/src/__tests__/fixtures/compiler/error.for-return.js rename to compiler/forget/src/__tests__/fixtures/compiler/for-return.js