From 812e0ce7010ca9475a10981609467a9f5b6b5eac Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 3 Feb 2023 14:16:17 -0800 Subject: [PATCH] [valueblocks] Join blocks instead of replacing operands (fix edge case) There's a bug in the HIR->ReactiveFunction conversion for certain categories of compound value blocks where we replace operands (which must be a Place) with a ReactiveValue. This approach worked in practice for lots of cases so I thought a type coercion was safe, but then I found a case where this assumption breaks (see new test). The updated logic fixes the bug and is simpler. When a value block gets split up (because there was a nested value block), instead of replacing the earlier value in the later instructions, we append the instructions together. This can result in some extra nesting (which if we wanted we could flatten away) but ensures that we maintain type-safety. --- .../ReactiveScopes/BuildReactiveFunction.ts | 177 ++++++++---------- .../ReactiveScopes/PrintReactiveFunction.ts | 2 +- .../fixtures/hir/for-logical.expect.md | 44 +++++ .../src/__tests__/fixtures/hir/for-logical.js | 1 - 4 files changed, 128 insertions(+), 96 deletions(-) create mode 100644 compiler/forget/src/__tests__/fixtures/hir/for-logical.expect.md diff --git a/compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts b/compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts index eff01759f2..f17a9b4e17 100644 --- a/compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts +++ b/compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts @@ -18,12 +18,10 @@ import { } from "../HIR"; import { HIRFunction, - Instruction, InstructionKind, ReactiveBreakTerminal, ReactiveContinueTerminal, ReactiveFunction, - ReactiveInstruction, ReactiveLogicalValue, ReactiveSequenceValue, ReactiveTerminalStatement, @@ -31,7 +29,6 @@ import { ReactiveValue, Terminal, } from "../HIR/HIR"; -import { mapInstructionOperands } from "../HIR/visitors"; import { assertExhaustive } from "../Utils/utils"; /** @@ -471,62 +468,74 @@ class Driver { visitValueBlock( id: BlockId, loc: SourceLocation - ): { block: BlockId; value: ReactiveValue; place: Place } { - let block: BasicBlock; - let value: ReactiveValue | null = null; - let place: Place | null = null; + ): { block: BlockId; value: ReactiveValue; place: Place; id: InstructionId } { const defaultBlock = this.cx.ir.blocks.get(id)!; if ( defaultBlock.terminal.kind === "goto" || defaultBlock.terminal.kind === "branch" ) { - block = defaultBlock; + const instructions = defaultBlock.instructions; + if (instructions.length === 0) { + invariant( + defaultBlock.terminal.kind === "branch", + "Expected instructions for non-branch terminal" + ); + return { + block: defaultBlock.id, + place: defaultBlock.terminal.test, + value: defaultBlock.terminal.test, + id: defaultBlock.terminal.id, + }; + } else if (defaultBlock.instructions.length === 1) { + const instr = defaultBlock.instructions[0]!; + return { + block: defaultBlock.id, + place: instr.lvalue!.place, + value: instr.value, + id: instr.id, + }; + } else { + const instr = defaultBlock.instructions.at(-1)!; + const sequence: ReactiveSequenceValue = { + kind: "SequenceExpression", + instructions: defaultBlock.instructions.slice(0, -1), + id: instr.id, + value: instr.value, + loc: loc, + }; + return { + block: defaultBlock.id, + place: instr.lvalue!.place, + value: sequence, + id: instr.id, + }; + } } else { - const result = this.visitValueBlockTerminal(defaultBlock.terminal); - block = this.cx.ir.blocks.get(result.fallthrough)!; - place = result.place; - value = result.value; - } - const instructions: Array = block.instructions; - if (place !== null && value !== null) { - instructions.forEach((instr) => - mapInstructionOperands(instr as Instruction, (place) => { - return place.identifier === place.identifier - ? (value as Place) - : place; - }) - ); - } - if (instructions.length === 0) { - invariant( - block.terminal.kind === "branch", - "Expected instructions for non-branch terminal" - ); - return { - block: block.id, - place: block.terminal.test, - value: value ?? block.terminal.test, - }; - } else if (instructions.length === 1) { - const instr = instructions[0]!; - return { - block: block.id, - place: instr.lvalue!.place, - value: instr.value, - }; - } else { - const instr = instructions.at(-1)!; + // The value block ended in a value terminal, recurse to get the value + // of that terminal + const init = this.visitValueBlockTerminal(defaultBlock.terminal); + // Code following the logical terminal + const final = this.visitValueBlock(init.fallthrough, loc); + // Stitch the two together... const sequence: ReactiveSequenceValue = { kind: "SequenceExpression", - instructions: instructions.slice(0, -1), - id: instr.id, - value: instr.value, - loc: loc, + instructions: [ + { + id: init.id, + loc, + lvalue: { kind: InstructionKind.Const, place: init.place }, + value: init.value, + }, + ], + id: final.id, + value: final.value, + loc, }; return { - block: block.id, - place: instr.lvalue!.place, + block: init.fallthrough, value: sequence, + place: final.place, + id: final.id, }; } } @@ -535,77 +544,56 @@ class Driver { value: ReactiveValue; place: Place; fallthrough: BlockId; + id: InstructionId; } { switch (terminal.kind) { case "logical": { - let testBlock: BasicBlock; - let leftPlace: Place | null = null; - let leftValue: ReactiveValue | null = null; - const defaultTestBlock = this.cx.ir.blocks.get(terminal.test)!; - if (defaultTestBlock.terminal.kind === "branch") { - testBlock = defaultTestBlock; - } else { - const leftResult = this.visitValueBlockTerminal( - defaultTestBlock.terminal - ); - testBlock = this.cx.ir.blocks.get(leftResult.fallthrough)!; - leftPlace = leftResult.place; - leftValue = leftResult.value; - } - + const test = this.visitValueBlock(terminal.test, terminal.loc); + const testBlock = this.cx.ir.blocks.get(test.block)!; invariant( testBlock.terminal.kind === "branch", "Unexpected terminal kind '%s' for logical test block", testBlock.terminal.kind ); - const leftInstructions: Array = - testBlock.instructions; - const leftBlock = this.cx.ir.blocks.get(testBlock.terminal.consequent)!; - leftInstructions.push(...leftBlock.instructions); - if (leftPlace !== null && leftValue !== null) { - leftInstructions.forEach((instr) => - mapInstructionOperands(instr as Instruction, (place) => { - return place.identifier === leftPlace!.identifier - ? (leftValue as Place) - : place; - }) - ); - } - const lastInstruction = leftInstructions.at(-1)!; - const place = lastInstruction.lvalue!.place; - let left: ReactiveValue; - if (leftInstructions.length === 1) { - left = leftInstructions[0]!.value; - } else { - const sequence: ReactiveSequenceValue = { - kind: "SequenceExpression", - instructions: leftInstructions.slice(0, -1), - id: lastInstruction.id, - value: lastInstruction.value, - loc: terminal.loc, - }; - left = sequence; - } + const leftFinal = this.visitValueBlock( + testBlock.terminal.consequent, + terminal.loc + ); + const left: ReactiveSequenceValue = { + kind: "SequenceExpression", + instructions: [ + { + id: test.id, + loc: terminal.loc, + lvalue: { kind: InstructionKind.Const, place: test.place }, + value: test.value, + }, + ], + id: leftFinal.id, + value: leftFinal.value, + loc: terminal.loc, + }; const right = this.visitValueBlock( testBlock.terminal.alternate, terminal.loc ); invariant( - place.identifier === right.place.identifier, + leftFinal.place.identifier === right.place.identifier, "Expected the left and right side of a logical expression to store a value to the same place" ); const value: ReactiveLogicalValue = { kind: "LogicalExpression", operator: terminal.operator, - left, + left: left, right: right.value, loc: terminal.loc, }; return { - place: { ...place }, + place: { ...leftFinal.place }, value, fallthrough: terminal.fallthrough, + id: terminal.id, }; } case "ternary": { @@ -639,6 +627,7 @@ class Driver { place: { ...consequent.place }, value, fallthrough: terminal.fallthrough, + id: terminal.id, }; } default: { diff --git a/compiler/forget/src/ReactiveScopes/PrintReactiveFunction.ts b/compiler/forget/src/ReactiveScopes/PrintReactiveFunction.ts index 537dd1103c..d066232603 100644 --- a/compiler/forget/src/ReactiveScopes/PrintReactiveFunction.ts +++ b/compiler/forget/src/ReactiveScopes/PrintReactiveFunction.ts @@ -218,7 +218,7 @@ function printTerminal(writer: Writer, terminal: ReactiveTerminal): void { break; } case "for": { - writer.writeLine("[${terminal.id}] for ("); + writer.writeLine(`[${terminal.id}] for (`); printReactiveValue(writer, terminal.init); writer.writeLine(";"); printReactiveValue(writer, terminal.test); diff --git a/compiler/forget/src/__tests__/fixtures/hir/for-logical.expect.md b/compiler/forget/src/__tests__/fixtures/hir/for-logical.expect.md new file mode 100644 index 0000000000..6545db8a51 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/hir/for-logical.expect.md @@ -0,0 +1,44 @@ + +## Input + +```javascript +function foo(props) { + let y = 0; + for ( + let x = 0; + x > props.min && x < props.max; + x += props.cond ? props.increment : 2 + ) { + x *= 2; + y += x; + } + return y; +} + +``` + +## Code + +```javascript +function foo(props) { + const $ = React.useMemoCache(); + let y; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + y = 0; + $[0] = y; + } else { + y = $[0]; + } + for ( + let x = 0; + x > props.min && x < props.max; + x = x$0 + (props.cond ? props.increment : (2, 2)), x + ) { + const x$0 = x * 2; + const y$1 = y + x$0; + } + return y; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/hir/for-logical.js b/compiler/forget/src/__tests__/fixtures/hir/for-logical.js index e5a2d6a352..b0e5a09fac 100644 --- a/compiler/forget/src/__tests__/fixtures/hir/for-logical.js +++ b/compiler/forget/src/__tests__/fixtures/hir/for-logical.js @@ -1,4 +1,3 @@ -// @skip function foo(props) { let y = 0; for (