From 1e0eb25cd07ef8a1aef6cf6acba04e2a0439e834 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Sun, 4 Jun 2023 21:26:01 -0400 Subject: [PATCH] Use sequence terminal, fix most remaining order-of-evaluation bugs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Changes the lowering for sequence expressions to use the new terminal. When converting to a ReactiveFunction, we convert these terminals into ReactiveSequenceValues, which nests the instructions and preserves order of evaluation in the output. The only catch is constant propagation — constant propagation breaks order-of-evaluation because it can effectively copy the final value of a sequence elsewhere, leaving the original sequence in the wrong place. I'll address that in a follow-up. --- compiler/forget/src/HIR/BuildHIR.ts | 59 +++++++++++++------ .../forget/src/HIR/MergeConsecutiveBlocks.ts | 2 +- .../ReactiveScopes/BuildReactiveFunction.ts | 9 +++ ...g.computed-call-evaluation-order.expect.md | 7 +-- ...-tag-evaluation-order-non-global.expect.md | 3 +- .../jsx-tag-evaluation-order.expect.md | 18 +++--- ... property-call-evaluation-order.expect.md} | 5 +- ...r.js => property-call-evaluation-order.js} | 0 .../compiler/sequence-expression.expect.md | 21 ++----- 9 files changed, 72 insertions(+), 52 deletions(-) rename compiler/forget/src/__tests__/fixtures/compiler/{_bug.property-call-evaluation-order.expect.md => property-call-evaluation-order.expect.md} (92%) rename compiler/forget/src/__tests__/fixtures/compiler/{_bug.property-call-evaluation-order.js => property-call-evaluation-order.js} (100%) diff --git a/compiler/forget/src/HIR/BuildHIR.ts b/compiler/forget/src/HIR/BuildHIR.ts index a81ebd2180..74d80d3119 100644 --- a/compiler/forget/src/HIR/BuildHIR.ts +++ b/compiler/forget/src/HIR/BuildHIR.ts @@ -1168,23 +1168,48 @@ function lowerExpression( const expr = exprPath as NodePath; const exprLoc = expr.node.loc ?? GeneratedSource; - let last: Place | null = null; - for (const item of expr.get("expressions")) { - last = lowerExpressionToTemporary(builder, item); - } - if (last === null) { - builder.errors.push({ - reason: `(BuildHIR::lowerExpression) Expected SequenceExpression to have at least one expression`, - severity: ErrorSeverity.InvalidInput, - nodePath: expr, - }); - return { kind: "UnsupportedNode", node: expr.node, loc: exprLoc }; - } - return { - kind: "LoadLocal", // TODO: LoadTemp - place: last, - loc: last.loc, - }; + const continuationBlock = builder.reserve(builder.currentBlockKind()); + const place = buildTemporaryPlace(builder, exprLoc); + + const sequenceBlock = builder.enter("value", (_) => { + let last: Place | null = null; + for (const item of expr.get("expressions")) { + last = lowerExpressionToTemporary(builder, item); + } + if (last === null) { + builder.errors.push({ + reason: `(BuildHIR::lowerExpression) Expected SequenceExpression to have at least one expression`, + severity: ErrorSeverity.InvalidInput, + nodePath: expr, + }); + } else { + lowerValueToTemporary(builder, { + kind: "StoreLocal", + lvalue: { kind: InstructionKind.Const, place: { ...place } }, + value: last, + loc: exprLoc, + }); + } + return { + kind: "goto", + id: makeInstructionId(0), + block: continuationBlock.id, + loc: exprLoc, + variant: GotoVariant.Break, + }; + }); + + builder.terminateWithContinuation( + { + kind: "sequence", + block: sequenceBlock, + fallthrough: continuationBlock.id, + id: makeInstructionId(0), + loc: exprLoc, + }, + continuationBlock + ); + return { kind: "LoadLocal", place, loc: place.loc }; } case "ConditionalExpression": { const expr = exprPath as NodePath; diff --git a/compiler/forget/src/HIR/MergeConsecutiveBlocks.ts b/compiler/forget/src/HIR/MergeConsecutiveBlocks.ts index 51c1350772..2af7ff32e3 100644 --- a/compiler/forget/src/HIR/MergeConsecutiveBlocks.ts +++ b/compiler/forget/src/HIR/MergeConsecutiveBlocks.ts @@ -44,7 +44,7 @@ export function mergeConsecutiveBlocks(fn: HIRFunction): void { "Expected predecessor %s to exist", predecessorId ); - if (predecessor.terminal.kind !== "goto") { + if (predecessor.terminal.kind !== "goto" || predecessor.kind !== "block") { // The predecessor is not guaranteed to transfer control to this block, // they aren't consecutive. continue; diff --git a/compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts b/compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts index f3be13fa34..bc4598d6ad 100644 --- a/compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts +++ b/compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts @@ -771,6 +771,15 @@ class Driver { id: InstructionId; } { switch (terminal.kind) { + case "sequence": { + const block = this.visitValueBlock(terminal.block, terminal.loc); + return { + value: block.value, + place: block.place, + fallthrough: terminal.fallthrough, + id: terminal.id, + }; + } case "optional": { const test = this.visitValueBlock(terminal.test, terminal.loc); const testBlock = this.cx.ir.blocks.get(test.block)!; diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.computed-call-evaluation-order.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/_bug.computed-call-evaluation-order.expect.md index ad0432beb6..868e6c1784 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/_bug.computed-call-evaluation-order.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/_bug.computed-call-evaluation-order.expect.md @@ -46,11 +46,8 @@ function Component() { if ($[2] === Symbol.for("react.memo_cache_sentinel")) { x = { f: t1 }; - console.log("A"); - console.log("B"); - changeF(x); - console.log("arg"); - x.f(1); + console.log("B"), "f"; + (console.log("A"), x).f((changeF(x), console.log("arg"), 1)); $[2] = x; } else { x = $[2]; diff --git a/compiler/forget/src/__tests__/fixtures/compiler/jsx-tag-evaluation-order-non-global.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/jsx-tag-evaluation-order-non-global.expect.md index 4915da4d97..70d0e4e036 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/jsx-tag-evaluation-order-non-global.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/jsx-tag-evaluation-order-non-global.expect.md @@ -36,8 +36,7 @@ function Component(props) { T0 = Tag; t1 = "\n "; - Tag = props.alternateComponent; - t2 = maybeMutate(maybeMutable); + t2 = ((Tag = props.alternateComponent), maybeMutate(maybeMutable)); $[0] = props.component; $[1] = props.alternateComponent; $[2] = Tag; diff --git a/compiler/forget/src/__tests__/fixtures/compiler/jsx-tag-evaluation-order.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/jsx-tag-evaluation-order.expect.md index 1aaa9e0419..6cabcdc23c 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/jsx-tag-evaluation-order.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/jsx-tag-evaluation-order.expect.md @@ -20,6 +20,8 @@ function Component(props) { import { unstable_useMemoCache as useMemoCache } from "react"; function Component(props) { const $ = useMemoCache(3); + + const t1 = props.value; let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { t0 = ; @@ -27,21 +29,21 @@ function Component(props) { } else { t0 = $[0]; } - const c_1 = $[1] !== props.value; - let t1; + const c_1 = $[1] !== t1; + let t2; if (c_1) { - t1 = ( + t2 = ( - {props.value} + {t1} {t0} ); - $[1] = props.value; - $[2] = t1; + $[1] = t1; + $[2] = t2; } else { - t1 = $[2]; + t2 = $[2]; } - return t1; + return t2; } ``` diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.property-call-evaluation-order.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/property-call-evaluation-order.expect.md similarity index 92% rename from compiler/forget/src/__tests__/fixtures/compiler/_bug.property-call-evaluation-order.expect.md rename to compiler/forget/src/__tests__/fixtures/compiler/property-call-evaluation-order.expect.md index 13620d95d3..7dc1bab2f3 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/_bug.property-call-evaluation-order.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/property-call-evaluation-order.expect.md @@ -46,10 +46,7 @@ function Component() { if ($[2] === Symbol.for("react.memo_cache_sentinel")) { x = { f: t1 }; - console.log("A"); - changeF(x); - console.log("arg"); - x.f(1); + (console.log("A"), x).f((changeF(x), console.log("arg"), 1)); $[2] = x; } else { x = $[2]; diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.property-call-evaluation-order.js b/compiler/forget/src/__tests__/fixtures/compiler/property-call-evaluation-order.js similarity index 100% rename from compiler/forget/src/__tests__/fixtures/compiler/_bug.property-call-evaluation-order.js rename to compiler/forget/src/__tests__/fixtures/compiler/property-call-evaluation-order.js diff --git a/compiler/forget/src/__tests__/fixtures/compiler/sequence-expression.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/sequence-expression.expect.md index 6bfd0623f5..7c7325627b 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/sequence-expression.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/sequence-expression.expect.md @@ -19,25 +19,16 @@ function foo() {} ```javascript import { unstable_useMemoCache as useMemoCache } from "react"; function sequence(props) { - const $ = useMemoCache(2); - Math.max(1, 2); - let t0; - if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - t0 = foo(); - $[0] = t0; - } else { - t0 = $[0]; - } + const $ = useMemoCache(1); let x; - if ($[1] === Symbol.for("react.memo_cache_sentinel")) { - x = t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + x = (Math.max(1, 2), foo()); while ((foo(), true)) { - foo(); - x = 2; + x = (foo(), 2); } - $[1] = x; + $[0] = x; } else { - x = $[1]; + x = $[0]; } return x; }