From 95360bbb4552f365beae2cfe5cd29834e6ecd509 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 28 Aug 2025 22:08:10 -0700 Subject: [PATCH] [compiler] Fix for scopes with unreachable fallthroughs Fixes #34108. If a scope ends with with a conditional where some/all branches exit via labeled break, we currently compile in a way that works but bypasses memoization. We end up with a shape like ```js let t0; label: { if (changed) { ... if (cond) { t0 = ...; break label; } // we don't save the output if the break happens! t0 = ...; $[0] = t0; } else { t0 = $[0]; } ``` The fix here is to update AlignReactiveScopesToBlockScopes to take account of breaks that don't go to the natural fallthrough. In this case, we take any active scopes and extend them to start at least as early as the label, and extend at least to the label fallthrough. Thus we produce the correct: ```js let t0; if (changed) { label: { ... if (cond) { t0 = ...; break label; } t0 = ...; } // now the break jumps here, and we cache the value $[0] = t0; } else { t0 = $[0]; } ``` Note that this messes with one situation: conditional return inside a useMemo. That desugars to a labeled break, and the fix here causes the scope to extend unnecessarily. So we likely want to be a bit more targeted than what i'm doing here. WIP. --- .../AlignReactiveScopesToBlockScopesHIR.ts | 20 +++ ...copes-reactive-scope-overlaps-if.expect.md | 16 +-- .../mutation-within-jsx-and-break.expect.md | 19 +-- .../useMemo-multiple-if-else.expect.md | 37 +++--- ...ble-code-early-return-in-useMemo.expect.md | 30 +++-- ...seMemo-if-else-both-early-return.expect.md | 118 ++++++++++++++++++ ...repro-useMemo-if-else-both-early-return.js | 35 ++++++ .../useMemo-if-else-multiple-return.expect.md | 36 +++--- .../useMemo-inlining-block-return.expect.md | 25 ++-- .../useMemo-multiple-if-else.expect.md | 37 +++--- 10 files changed, 270 insertions(+), 103 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-useMemo-if-else-both-early-return.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-useMemo-if-else-both-early-return.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/AlignReactiveScopesToBlockScopesHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/AlignReactiveScopesToBlockScopesHIR.ts index 2b4e890a40..6b13a5f9c8 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/AlignReactiveScopesToBlockScopesHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/AlignReactiveScopesToBlockScopesHIR.ts @@ -6,8 +6,10 @@ */ import {CompilerError} from '..'; +import {printFunction} from '../HIR'; import { BlockId, + GotoVariant, HIRFunction, InstructionId, MutableRange, @@ -16,6 +18,7 @@ import { getPlaceScope, makeInstructionId, } from '../HIR/HIR'; +import {printTerminal} from '../HIR/PrintHIR'; import { eachInstructionLValue, eachInstructionValueOperand, @@ -175,6 +178,23 @@ export function alignReactiveScopesToBlockScopesHIR(fn: HIRFunction): void { if (node != null) { valueBlockNodes.set(fallthrough, node); } + } else if (terminal.kind === 'goto') { + const start = activeBlockFallthroughRanges.find( + range => range.fallthrough === terminal.block, + ); + if (start != null && start !== activeBlockFallthroughRanges.at(-1)) { + const fallthroughBlock = fn.body.blocks.get(start.fallthrough)!; + const firstId = + fallthroughBlock.instructions[0]?.id ?? fallthroughBlock.terminal.id; + for (const scope of activeScopes) { + scope.range.start = makeInstructionId( + Math.min(start.range.start, scope.range.start), + ); + scope.range.end = makeInstructionId( + Math.max(firstId, scope.range.end), + ); + } + } } /* diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-reactive-scope-overlaps-if.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-reactive-scope-overlaps-if.expect.md index 7136b3a173..03939d16d6 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-reactive-scope-overlaps-if.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-reactive-scope-overlaps-if.expect.md @@ -46,14 +46,16 @@ function useFoo(t0) { t1 = $[0]; } let items = t1; - bb0: if ($[1] !== cond) { - if (cond) { - items = []; - } else { - break bb0; - } + if ($[1] !== cond) { + bb0: { + if (cond) { + items = []; + } else { + break bb0; + } - items.push(2); + items.push(2); + } $[1] = cond; $[2] = items; } else { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mutation-within-jsx-and-break.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mutation-within-jsx-and-break.expect.md index 1d0f40e29f..17a8524eee 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mutation-within-jsx-and-break.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mutation-within-jsx-and-break.expect.md @@ -49,12 +49,12 @@ import { } from "shared-runtime"; function useFoo(t0) { - const $ = _c(3); + const $ = _c(4); const { data } = t0; let obj; let myDiv = null; - bb0: if (data.cond) { - if ($[0] !== data.cond1) { + if ($[0] !== data.cond || $[1] !== data.cond1) { + bb0: if (data.cond) { obj = makeObject_Primitives(); if (data.cond1) { myDiv = ; @@ -62,13 +62,14 @@ function useFoo(t0) { } mutate(obj); - $[0] = data.cond1; - $[1] = obj; - $[2] = myDiv; - } else { - obj = $[1]; - myDiv = $[2]; } + $[0] = data.cond; + $[1] = data.cond1; + $[2] = obj; + $[3] = myDiv; + } else { + obj = $[2]; + myDiv = $[3]; } return myDiv; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/useMemo-multiple-if-else.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/useMemo-multiple-if-else.expect.md index a75b592b83..4ce8ef5802 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/useMemo-multiple-if-else.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/useMemo-multiple-if-else.expect.md @@ -34,17 +34,16 @@ import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR import { useMemo } from "react"; function Component(props) { - const $ = _c(6); + const $ = _c(5); let t0; - bb0: { - let y; - if ( - $[0] !== props.a || - $[1] !== props.b || - $[2] !== props.cond || - $[3] !== props.cond2 - ) { - y = []; + if ( + $[0] !== props.a || + $[1] !== props.b || + $[2] !== props.cond || + $[3] !== props.cond2 + ) { + bb0: { + const y = []; if (props.cond) { y.push(props.a); } @@ -54,17 +53,15 @@ function Component(props) { } y.push(props.b); - $[0] = props.a; - $[1] = props.b; - $[2] = props.cond; - $[3] = props.cond2; - $[4] = y; - $[5] = t0; - } else { - y = $[4]; - t0 = $[5]; + t0 = y; } - t0 = y; + $[0] = props.a; + $[1] = props.b; + $[2] = props.cond; + $[3] = props.cond2; + $[4] = t0; + } else { + t0 = $[4]; } const x = t0; return x; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-unreachable-code-early-return-in-useMemo.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-unreachable-code-early-return-in-useMemo.expect.md index 496d80d52b..4cdb2ff2d7 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-unreachable-code-early-return-in-useMemo.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-unreachable-code-early-return-in-useMemo.expect.md @@ -48,24 +48,22 @@ function Component(t0) { const $ = _c(7); const { value } = t0; let t1; - bb0: { - if (value == null) { - t1 = null; - break bb0; - } - try { - let t3; - if ($[0] !== value) { - t3 = { value }; - $[0] = value; - $[1] = t3; - } else { - t3 = $[1]; + if ($[0] !== value) { + bb0: { + if (value == null) { + t1 = null; + break bb0; + } + try { + t1 = { value }; + } catch (t2) { + t1 = null; } - t1 = t3; - } catch (t2) { - t1 = null; } + $[0] = value; + $[1] = t1; + } else { + t1 = $[1]; } const result = t1; let t2; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-useMemo-if-else-both-early-return.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-useMemo-if-else-both-early-return.expect.md new file mode 100644 index 0000000000..acb3b72e3a --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-useMemo-if-else-both-early-return.expect.md @@ -0,0 +1,118 @@ + +## Input + +```javascript +import {useMemo} from 'react'; +import { + makeObject_Primitives, + mutate, + Stringify, + ValidateMemoization, +} from 'shared-runtime'; + +function Component({cond}) { + const memoized = useMemo(() => { + const value = makeObject_Primitives(); + if (cond) { + return value; + } else { + mutate(value); + return value; + } + }, [cond]); + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{cond: false}], + sequentialRenders: [ + {cond: false}, + {cond: false}, + {cond: true}, + {cond: true}, + {cond: false}, + {cond: true}, + {cond: false}, + {cond: true}, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { useMemo } from "react"; +import { + makeObject_Primitives, + mutate, + Stringify, + ValidateMemoization, +} from "shared-runtime"; + +function Component(t0) { + const $ = _c(7); + const { cond } = t0; + let t1; + if ($[0] !== cond) { + const value = makeObject_Primitives(); + if (cond) { + t1 = value; + } else { + mutate(value); + t1 = value; + } + $[0] = cond; + $[1] = t1; + } else { + t1 = $[1]; + } + const memoized = t1; + let t2; + if ($[2] !== cond) { + t2 = [cond]; + $[2] = cond; + $[3] = t2; + } else { + t2 = $[3]; + } + let t3; + if ($[4] !== memoized || $[5] !== t2) { + t3 = ; + $[4] = memoized; + $[5] = t2; + $[6] = t3; + } else { + t3 = $[6]; + } + return t3; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ cond: false }], + sequentialRenders: [ + { cond: false }, + { cond: false }, + { cond: true }, + { cond: true }, + { cond: false }, + { cond: true }, + { cond: false }, + { cond: true }, + ], +}; + +``` + +### Eval output +(kind: ok)
{"inputs":[false],"output":{"a":0,"b":"value1","c":true,"wat0":"joe"}}
+
{"inputs":[false],"output":{"a":0,"b":"value1","c":true,"wat0":"joe"}}
+
{"inputs":[true],"output":{"a":0,"b":"value1","c":true}}
+
{"inputs":[true],"output":{"a":0,"b":"value1","c":true}}
+
{"inputs":[false],"output":{"a":0,"b":"value1","c":true,"wat0":"joe"}}
+
{"inputs":[true],"output":{"a":0,"b":"value1","c":true}}
+
{"inputs":[false],"output":{"a":0,"b":"value1","c":true,"wat0":"joe"}}
+
{"inputs":[true],"output":{"a":0,"b":"value1","c":true}}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-useMemo-if-else-both-early-return.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-useMemo-if-else-both-early-return.js new file mode 100644 index 0000000000..d381661077 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-useMemo-if-else-both-early-return.js @@ -0,0 +1,35 @@ +import {useMemo} from 'react'; +import { + makeObject_Primitives, + mutate, + Stringify, + ValidateMemoization, +} from 'shared-runtime'; + +function Component({cond}) { + const memoized = useMemo(() => { + const value = makeObject_Primitives(); + if (cond) { + return value; + } else { + mutate(value); + return value; + } + }, [cond]); + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{cond: false}], + sequentialRenders: [ + {cond: false}, + {cond: false}, + {cond: true}, + {cond: true}, + {cond: false}, + {cond: true}, + {cond: false}, + {cond: true}, + ], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-if-else-multiple-return.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-if-else-multiple-return.expect.md index 25edfe4e65..f70ae2069c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-if-else-multiple-return.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-if-else-multiple-return.expect.md @@ -19,30 +19,30 @@ function Component(props) { ```javascript import { c as _c } from "react/compiler-runtime"; function Component(props) { - const $ = _c(4); + const $ = _c(6); let t0; - bb0: { - if (props.cond) { + if ($[0] !== props.a || $[1] !== props.b || $[2] !== props.cond) { + bb0: { + if (props.cond) { + t0 = makeObject(props.a); + break bb0; + } let t1; - if ($[0] !== props.a) { - t1 = makeObject(props.a); - $[0] = props.a; - $[1] = t1; + if ($[4] !== props.b) { + t1 = makeObject(props.b); + $[4] = props.b; + $[5] = t1; } else { - t1 = $[1]; + t1 = $[5]; } t0 = t1; - break bb0; } - let t1; - if ($[2] !== props.b) { - t1 = makeObject(props.b); - $[2] = props.b; - $[3] = t1; - } else { - t1 = $[3]; - } - t0 = t1; + $[0] = props.a; + $[1] = props.b; + $[2] = props.cond; + $[3] = t0; + } else { + t0 = $[3]; } const x = t0; return x; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-inlining-block-return.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-inlining-block-return.expect.md index 45911b4219..3af5281a23 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-inlining-block-return.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-inlining-block-return.expect.md @@ -24,22 +24,21 @@ export const FIXTURE_ENTRYPOINT = { ```javascript import { c as _c } from "react/compiler-runtime"; function component(a, b) { - const $ = _c(2); + const $ = _c(3); let t0; - bb0: { - if (a) { - let t1; - if ($[0] !== b) { - t1 = { b }; - $[0] = b; - $[1] = t1; - } else { - t1 = $[1]; + if ($[0] !== a || $[1] !== b) { + bb0: { + if (a) { + t0 = { b }; + break bb0; } - t0 = t1; - break bb0; + t0 = undefined; } - t0 = undefined; + $[0] = a; + $[1] = b; + $[2] = t0; + } else { + t0 = $[2]; } const x = t0; return x; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-multiple-if-else.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-multiple-if-else.expect.md index 23b05b5482..7f4b8af965 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-multiple-if-else.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-multiple-if-else.expect.md @@ -33,17 +33,16 @@ import { c as _c } from "react/compiler-runtime"; import { useMemo } from "react"; function Component(props) { - const $ = _c(6); + const $ = _c(5); let t0; - bb0: { - let y; - if ( - $[0] !== props.a || - $[1] !== props.b || - $[2] !== props.cond || - $[3] !== props.cond2 - ) { - y = []; + if ( + $[0] !== props.a || + $[1] !== props.b || + $[2] !== props.cond || + $[3] !== props.cond2 + ) { + bb0: { + const y = []; if (props.cond) { y.push(props.a); } @@ -53,17 +52,15 @@ function Component(props) { } y.push(props.b); - $[0] = props.a; - $[1] = props.b; - $[2] = props.cond; - $[3] = props.cond2; - $[4] = y; - $[5] = t0; - } else { - y = $[4]; - t0 = $[5]; + t0 = y; } - t0 = y; + $[0] = props.a; + $[1] = props.b; + $[2] = props.cond; + $[3] = props.cond2; + $[4] = t0; + } else { + t0 = $[4]; } const x = t0; return x;