From 4a70aa7faf82bc20e09bca99b230c50a181378b3 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 17 Jan 2023 09:40:48 -0800 Subject: [PATCH] Make implicit break/continue explicit in ReactiveFunction Previously when converting from HIR -> ReactiveFunction we elided break/continue terminals in places where control would implicitly transfer to the break/continue target and therefore nothing has to be emitted. The one downside of this approach is that it makes scope analysis a bit trickier. We want to close scopes once we see an instruction id past the end of the scope's range, but these implicit breaks were causing us to miss some instruction ids. We compensated for this, but it's helpful to keep the representation explicit and discard these terminals later in codegen. --- compiler/forget/src/HIR/HIR.ts | 112 ++++++++++++------ .../ReactiveScopes/BuildReactiveFunction.ts | 49 ++++++-- .../ReactiveScopes/CodegenReactiveFunction.ts | 14 ++- .../fixtures/hir/conditional-break.expect.md | 31 ----- .../fixtures/hir/conditional-break.js | 16 --- ...error.conditional-break-labeled.expect.md} | 16 +-- ....js => error.conditional-break-labeled.js} | 0 ...f.expect.md => error.ssa-for-of.expect.md} | 9 +- .../{ssa-for-of.js => error.ssa-for-of.js} | 0 .../fixtures/hir/inverted-if.expect.md | 18 ++- .../src/__tests__/fixtures/hir/inverted-if.js | 3 +- 11 files changed, 147 insertions(+), 121 deletions(-) rename compiler/forget/src/__tests__/fixtures/hir/{_bug_conditional-break-labeled.expect.md => error.conditional-break-labeled.expect.md} (61%) rename compiler/forget/src/__tests__/fixtures/hir/{_bug_conditional-break-labeled.js => error.conditional-break-labeled.js} (100%) rename compiler/forget/src/__tests__/fixtures/hir/{ssa-for-of.expect.md => error.ssa-for-of.expect.md} (73%) rename compiler/forget/src/__tests__/fixtures/hir/{ssa-for-of.js => error.ssa-for-of.js} (100%) diff --git a/compiler/forget/src/HIR/HIR.ts b/compiler/forget/src/HIR/HIR.ts index e1fbeebb19..15b1f26822 100644 --- a/compiler/forget/src/HIR/HIR.ts +++ b/compiler/forget/src/HIR/HIR.ts @@ -68,10 +68,23 @@ export type ReactiveValueBlock = { }; export type ReactiveStatement = - | { kind: "instruction"; instruction: ReactiveInstruction } - | { kind: "terminal"; terminal: ReactiveTerminal; label: BlockId | null } + | ReactiveInstructionStatement + | ReactiveTerminalStatement | ReactiveScopeBlock; +export type ReactiveInstructionStatement = { + kind: "instruction"; + instruction: ReactiveInstruction; +}; + +export type ReactiveTerminalStatement< + Tterminal extends ReactiveTerminal = ReactiveTerminal +> = { + kind: "terminal"; + terminal: Tterminal; + label: BlockId | null; +}; + export type ReactiveInstruction = { id: InstructionId; lvalue: LValue | null; @@ -80,40 +93,67 @@ export type ReactiveInstruction = { }; export type ReactiveTerminal = - | { kind: "break"; label: BlockId | null; id: InstructionId | null } - | { kind: "continue"; label: BlockId | null; id: InstructionId } - | { kind: "return"; value: Place | null; id: InstructionId } - | { kind: "throw"; value: Place; id: InstructionId } - | { - kind: "switch"; - test: Place; - cases: Array<{ - test: Place | null; - block: ReactiveBlock | void; - }>; - id: InstructionId; - } - | { - kind: "while"; - test: ReactiveValueBlock; - loop: ReactiveBlock; - id: InstructionId; - } - | { - kind: "for"; - init: ReactiveValueBlock; - test: ReactiveValueBlock; - update: ReactiveValueBlock; - loop: ReactiveBlock; - id: InstructionId; - } - | { - kind: "if"; - test: Place; - consequent: ReactiveBlock; - alternate: ReactiveBlock | null; - id: InstructionId; - }; + | ReactiveBreakTerminal + | ReactiveContinueTerminal + | ReactiveReturnTerminal + | ReactiveThrowTerminal + | ReactiveSwitchTerminal + | ReactiveWhileTerminal + | ReactiveForTerminal + | ReactiveIfTerminal; + +export type ReactiveBreakTerminal = { + kind: "break"; + label: BlockId | null; + id: InstructionId | null; + implicit: boolean; +}; +export type ReactiveContinueTerminal = { + kind: "continue"; + label: BlockId | null; + id: InstructionId; + implicit: boolean; +}; +export type ReactiveReturnTerminal = { + kind: "return"; + value: Place | null; + id: InstructionId; +}; +export type ReactiveThrowTerminal = { + kind: "throw"; + value: Place; + id: InstructionId; +}; +export type ReactiveSwitchTerminal = { + kind: "switch"; + test: Place; + cases: Array<{ + test: Place | null; + block: ReactiveBlock | void; + }>; + id: InstructionId; +}; +export type ReactiveWhileTerminal = { + kind: "while"; + test: ReactiveValueBlock; + loop: ReactiveBlock; + id: InstructionId; +}; +export type ReactiveForTerminal = { + kind: "for"; + init: ReactiveValueBlock; + test: ReactiveValueBlock; + update: ReactiveValueBlock; + loop: ReactiveBlock; + id: InstructionId; +}; +export type ReactiveIfTerminal = { + kind: "if"; + test: Place; + consequent: ReactiveBlock; + alternate: ReactiveBlock | null; + id: InstructionId; +}; /** * A function lowered to HIR form, ie where its body is lowered to an HIR control-flow graph diff --git a/compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts b/compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts index 545a56172e..53eb712ba2 100644 --- a/compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts +++ b/compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts @@ -15,10 +15,15 @@ import { InstructionValue, Place, ReactiveBlock, - ReactiveStatement, ReactiveValueBlock, } from "../HIR"; -import { HIRFunction, ReactiveFunction } from "../HIR/HIR"; +import { + HIRFunction, + ReactiveBreakTerminal, + ReactiveContinueTerminal, + ReactiveFunction, + ReactiveTerminalStatement, +} from "../HIR/HIR"; import todo from "../Utils/todo"; import { assertExhaustive } from "../Utils/utils"; @@ -180,7 +185,7 @@ class Driver { const break_ = this.visitBreak(case_.block, null); if ( index === 0 && - break_ === null && + break_.terminal.implicit && case_.block === terminal.fallthrough && case_.test === null ) { @@ -383,6 +388,9 @@ class Driver { } break; } + case "error": { + invariant(false, "Unexpected error terminal"); + } default: { assertExhaustive(terminal, "Unexpected terminal"); } @@ -434,27 +442,30 @@ class Driver { visitBreak( block: BlockId, id: InstructionId | null - ): ReactiveStatement | null { + ): ReactiveTerminalStatement { const target = this.cx.getBreakTarget(block); if (target === null) { - // TODO: we should always have a target - return null; + invariant(false, "Expected a break target"); } switch (target.type) { case "implicit": { - return null; + return { + kind: "terminal", + terminal: { kind: "break", label: null, id, implicit: true }, + label: null, + }; } case "labeled": { return { kind: "terminal", - terminal: { kind: "break", label: target.block, id }, + terminal: { kind: "break", label: target.block, id, implicit: false }, label: null, }; } case "unlabeled": { return { kind: "terminal", - terminal: { kind: "break", label: null, id }, + terminal: { kind: "break", label: null, id, implicit: false }, label: null, }; } @@ -467,7 +478,10 @@ class Driver { } } - visitContinue(block: BlockId, id: InstructionId): ReactiveStatement | null { + visitContinue( + block: BlockId, + id: InstructionId + ): ReactiveTerminalStatement { const target = this.cx.getContinueTarget(block); invariant( target !== null, @@ -475,19 +489,28 @@ class Driver { ); switch (target.type) { case "implicit": { - return null; + return { + kind: "terminal", + terminal: { kind: "continue", label: null, id, implicit: true }, + label: null, + }; } case "labeled": { return { kind: "terminal", - terminal: { kind: "continue", label: target.block, id }, + terminal: { + kind: "continue", + label: target.block, + id, + implicit: false, + }, label: null, }; } case "unlabeled": { return { kind: "terminal", - terminal: { kind: "continue", label: null, id }, + terminal: { kind: "continue", label: null, id, implicit: false }, label: null, }; } diff --git a/compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts index aeff29922d..c73a2c0810 100644 --- a/compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -104,6 +104,9 @@ function codegenBlock(cx: Context, block: ReactiveBlock): t.BlockStatement { } case "terminal": { const statement = codegenTerminal(cx, item.terminal); + if (statement === null) { + break; + } if (item.label !== null) { statements.push( t.labeledStatement( @@ -232,9 +235,15 @@ function codegenReactiveScope( statements.push(t.ifStatement(testCondition, computationBlock, memoBlock)); } -function codegenTerminal(cx: Context, terminal: ReactiveTerminal): t.Statement { +function codegenTerminal( + cx: Context, + terminal: ReactiveTerminal +): t.Statement | null { switch (terminal.kind) { case "break": { + if (terminal.implicit) { + return null; + } return t.breakStatement( terminal.label !== null ? t.identifier(codegenLabel(terminal.label)) @@ -242,6 +251,9 @@ function codegenTerminal(cx: Context, terminal: ReactiveTerminal): t.Statement { ); } case "continue": { + if (terminal.implicit) { + return null; + } return t.continueStatement( terminal.label !== null ? t.identifier(codegenLabel(terminal.label)) diff --git a/compiler/forget/src/__tests__/fixtures/hir/conditional-break.expect.md b/compiler/forget/src/__tests__/fixtures/hir/conditional-break.expect.md index bb3d9693ac..5d02d80260 100644 --- a/compiler/forget/src/__tests__/fixtures/hir/conditional-break.expect.md +++ b/compiler/forget/src/__tests__/fixtures/hir/conditional-break.expect.md @@ -56,22 +56,6 @@ function Component(props) { return a; } -/** - * props.b *does* influence `a` - */ -function Component(props) { - const a = []; - a.push(props.a); - label: { - if (props.b) { - break label; - } - a.push(props.c); - } - a.push(props.d); - return a; -} - ``` ## Code @@ -202,19 +186,4 @@ function Component(props) { } ``` -## Code - -```javascript -function Component(props) { - const a = []; - a.push(props.a); - if (props.b) { - a.push(props.d); - return a; - } - - a.push(props.c); -} - -``` \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/hir/conditional-break.js b/compiler/forget/src/__tests__/fixtures/hir/conditional-break.js index a4ad55cd05..297a2db070 100644 --- a/compiler/forget/src/__tests__/fixtures/hir/conditional-break.js +++ b/compiler/forget/src/__tests__/fixtures/hir/conditional-break.js @@ -51,19 +51,3 @@ function Component(props) { a.push(props.d); return a; } - -/** - * props.b *does* influence `a` - */ -function Component(props) { - const a = []; - a.push(props.a); - label: { - if (props.b) { - break label; - } - a.push(props.c); - } - a.push(props.d); - return a; -} diff --git a/compiler/forget/src/__tests__/fixtures/hir/_bug_conditional-break-labeled.expect.md b/compiler/forget/src/__tests__/fixtures/hir/error.conditional-break-labeled.expect.md similarity index 61% rename from compiler/forget/src/__tests__/fixtures/hir/_bug_conditional-break-labeled.expect.md rename to compiler/forget/src/__tests__/fixtures/hir/error.conditional-break-labeled.expect.md index 2605d96223..4bf3feaec6 100644 --- a/compiler/forget/src/__tests__/fixtures/hir/_bug_conditional-break-labeled.expect.md +++ b/compiler/forget/src/__tests__/fixtures/hir/error.conditional-break-labeled.expect.md @@ -20,19 +20,11 @@ function Component(props) { ``` -## Code -```javascript -function Component(props) { - const a = []; - a.push(props.a); - if (props.b) { - a.push(props.d); - return a; - } - - a.push(props.c); -} +## Error ``` +Expected a break target +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/hir/_bug_conditional-break-labeled.js b/compiler/forget/src/__tests__/fixtures/hir/error.conditional-break-labeled.js similarity index 100% rename from compiler/forget/src/__tests__/fixtures/hir/_bug_conditional-break-labeled.js rename to compiler/forget/src/__tests__/fixtures/hir/error.conditional-break-labeled.js diff --git a/compiler/forget/src/__tests__/fixtures/hir/ssa-for-of.expect.md b/compiler/forget/src/__tests__/fixtures/hir/error.ssa-for-of.expect.md similarity index 73% rename from compiler/forget/src/__tests__/fixtures/hir/ssa-for-of.expect.md rename to compiler/forget/src/__tests__/fixtures/hir/error.ssa-for-of.expect.md index 7e060dd4c9..45eb946d18 100644 --- a/compiler/forget/src/__tests__/fixtures/hir/ssa-for-of.expect.md +++ b/compiler/forget/src/__tests__/fixtures/hir/error.ssa-for-of.expect.md @@ -15,12 +15,11 @@ function foo(cond) { ``` -## Code -```javascript -function foo(cond) { - const items = []; -} +## Error ``` +Expected a break target +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/hir/ssa-for-of.js b/compiler/forget/src/__tests__/fixtures/hir/error.ssa-for-of.js similarity index 100% rename from compiler/forget/src/__tests__/fixtures/hir/ssa-for-of.js rename to compiler/forget/src/__tests__/fixtures/hir/error.ssa-for-of.js diff --git a/compiler/forget/src/__tests__/fixtures/hir/inverted-if.expect.md b/compiler/forget/src/__tests__/fixtures/hir/inverted-if.expect.md index 62bbd1a2f0..2bb8951bb4 100644 --- a/compiler/forget/src/__tests__/fixtures/hir/inverted-if.expect.md +++ b/compiler/forget/src/__tests__/fixtures/hir/inverted-if.expect.md @@ -2,13 +2,14 @@ ## Input ```javascript -function foo(a, b, c) { +function foo(a, b, c, d) { let y = []; label: if (a) { if (b) { y.push(c); break label; } + y.push(d); } return y; } @@ -18,27 +19,32 @@ function foo(a, b, c) { ## Code ```javascript -function foo(a, b, c) { +function foo(a, b, c, d) { const $ = React.useMemoCache(); const c_0 = $[0] !== a; const c_1 = $[1] !== b; const c_2 = $[2] !== c; + const c_3 = $[3] !== d; let y; - if (c_0 || c_1 || c_2) { + if (c_0 || c_1 || c_2 || c_3) { y = []; - if (a) { + bb1: if (a) { if (b) { y.push(c); + break bb1; } + + y.push(d); } $[0] = a; $[1] = b; $[2] = c; - $[3] = y; + $[3] = d; + $[4] = y; } else { - y = $[3]; + y = $[4]; } return y; diff --git a/compiler/forget/src/__tests__/fixtures/hir/inverted-if.js b/compiler/forget/src/__tests__/fixtures/hir/inverted-if.js index 0555787fce..97cd1fbff2 100644 --- a/compiler/forget/src/__tests__/fixtures/hir/inverted-if.js +++ b/compiler/forget/src/__tests__/fixtures/hir/inverted-if.js @@ -1,10 +1,11 @@ -function foo(a, b, c) { +function foo(a, b, c, d) { let y = []; label: if (a) { if (b) { y.push(c); break label; } + y.push(d); } return y; }