Repository navigation
react_compiler: hoist a context variable that a closure reads before its declaration - #42390
Merged
Jarred-Sumner merged 1 commit intoSep 12, 2026
Merged
Conversation
…its declaration The block hoisting pass skipped every binding that was already a context identifier. FindContextIdentifiers marks a local that is captured and reassigned, so a `let` with a closure above it and a reassignment below it got no `DeclareContext HoistedLet`, and the closure captured a variable that no instruction had declared yet. Upstream skips a binding only if it was already hoisted (`isHoistedIdentifier`). Do the same.
Collaborator
Author
|
Status: fix pushed. CI build 114478: 180 of 181 jobs pass. The one red test is Reproduction: on 1.4.2 and on main 471b586, this aborts with exit 134: // g.jsx
export function List(p) {
const renderRow = () => <Row items={items} />;
let items = p.items;
if (p.onlyActive) {
items = items.filter((i) => i.active);
}
return <ul>{renderRow()}</ul>;
}With this branch the same command exits 0 and |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. WalkthroughThe compiler now checks the hoisted-identifier registry before emitting hoisting instructions. A bundler regression test covers closures that read reassigned or later-declared ChangesClosure hoisting
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The hoisting fix and regression coverage do not leave an actionable merge risk. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
LGTM — one-predicate fix that aligns the hoist guard with upstream BuildHIR.ts, with a test asserting exact babel-1.0.0 memo-cache sizes.
What was reviewed:
- The guard swap:
is_context_identifier→is_hoisted_identifiermatches theadd_hoisted_identifiercall at line 184, so the skip now dedups on actual prior hoisting rather than on the pre-lowering context mark. is_context_identifierstill has 19 other call sites, so no dead code was created.- Test covers four variants (fn decl, arrow, self-ref, ternary), asserts both rendered output and the
_c(n)sizes, and is hermetic via a stubbedreact/compiler-runtime.
Extended reasoning...
Overview
The PR changes a single predicate in src/react_compiler/lowering/build_hir/stmt.rs inside lower_block_statement_inner's hoist-emission loop. Previously the loop skipped emitting DeclareContext HoistedLet when is_context_identifier(target) was true; now it skips only when is_hoisted_identifier(target.inner_index()) is true. Since the same block calls both add_context_identifier and add_hoisted_identifier after emitting the declaration (lines 181–184), the new guard is the correct dedup key — it only suppresses a second hoist for the same binding, not a first hoist for a binding that FindContextIdentifiers already marked as a context variable. The PR description ties this directly to upstream BuildHIR.ts and the upstream Rust crate, satisfying REVIEW.md's "reference implementation is the spec" rule.
The test addition is an itBundled case in the existing react-compiler.test.ts covering four component shapes that each independently aborted on main. It stubs react/compiler-runtime.c to record every _c(n) size and asserts both the runtime-visible closure semantics (the closure sees the reassigned value) and the exact cache sizes babel-plugin-react-compiler 1.0.0 emits — the strongest invariant available, per REVIEW.md. The comment cites the upstream version for the magic constants.
Security risks
None. This is a compiler-internal HIR construction change with no I/O, no untrusted input parsing beyond what the bundler already does, and no auth/crypto/permissions surface.
Level of scrutiny
Low-to-medium. The Rust change is a single, well-localized predicate swap whose correctness is legible from the surrounding ten lines (the add_hoisted_identifier call makes the intended dedup key obvious) and is backed by upstream reference behavior. The PR reports the full 3293-case fixture suite still passes and a 119-variant probe matrix now matches upstream exactly. I checked that is_context_identifier remains used elsewhere (19 call sites), so no dead code was introduced.
Other factors
No CODEOWNERS entries cover these paths. No outstanding reviewer objections in the timeline. The test follows repo conventions: added to the existing file, uses itBundled with backend: "cli", is hermetic (in-tree fake react package, no network), has no sleeps or ports, and per the PR would SIGABRT under USE_SYSTEM_BUN=1. The bug-hunting pass ran to dry_streak with no findings.
Jarred-Sumner
deleted the
robobun/5a0e3abc/react-compiler-hoist-context-let
branch
September 12, 2026 03:55
Jarred-Sumner
pushed a commit
that referenced
this pull request
Sep 15, 2026
) ### Problem - `bun build --react-compiler --target=browser` aborts on `const r = (m = p.f()) ? 1 : 0; return <div data-v={[m, r]} />`: `panic: Expected a node for all scopes`. `Bun.build` aborts the calling process the same way. - `CollectDependenciesVisitor` skips the test of a `?:`, so the reactive scope that reassigns `m` gets no scope node. `visit_scope` still attaches that scope to `m`. When `m` is force-memoized, `force_memoize_scope_dependencies` (`src/react_compiler/reactive_scopes/prune_non_escaping_scopes.rs:1170`) calls `.expect()` on the missing node. ### Fix - That invariant is now a `cold_invariant` error. `program.rs` leaves that one function uncompiled, like upstream's `CompilerError.invariant`. - The three `Expected identifier to be initialized` invariants in the same pass get the same treatment. Their visitor callbacks return `()`, so they record the first error in `CollectState`, as `assert_scope_instructions_within_scopes` does. - Correct because babel-plugin-react-compiler 1.0.0 raises the same invariant on the same inputs and skips the same functions. Compiled output does not change. - Verified: `test/bundler/transpiler/react-compiler.test.ts` (new `AssignmentInConditionalTestSkipsOnlyThatFunction`, SIGABRT on 1.4.3 canary). Also `react-compiler-fixtures.test.ts`. ### Background - The React Compiler groups values that change together into reactive scopes and emits one memo block per scope. PruneNonEscapingScopes removes the scopes whose values never leave the function. - The pass builds a graph of identifier nodes and scope nodes, then walks it from the returned values. - Upstream's `CompilerError.invariant` is a caught per-function error in TS. Bun builds with `panic = "abort"`, so a ported `.expect()` ends the process. <details><summary>Notes</summary> Repro (any directory, 1.4.2, 1.4.3 canary 6a92015 and main 4b5862f): ```jsx // a.jsx export default function App(p) { let m; const r = (m = p.f()) ? 1 : 0; return <div data-v={[m, r]} />; } ``` ``` $ bun build --react-compiler --target=browser --external react a.jsx panic: Expected a node for all scopes Crashed while visiting a.jsx ``` Frames: `force_memoize_scope_dependencies` (`prune_non_escaping_scopes.rs:1170`), `compute_memoized_identifiers::visit` (`:1156`), `prune_non_escaping_scopes` (`:70`), `pipeline::run_hir_passes` (`pipeline.rs:622`), `compile_fn` (`:172`). The smallest shape: an assignment whose value allocates (a call, an array or an object literal) in the test of a `?:`, and the assigned local as an operand of a memoized value (`[m]`, `{ m }`). The result of the conditional does not need to reach that value. `m = p.a` compiles. `&&`, `??`, `?.` or an `if` statement in place of `?:` compile. `<div data-m={m} />` compiles because nothing force-memoizes `m`. `--target=bun` and `--target=node` (ssr mode) create no reactive scopes and compile. A second shape, with no assignment expression (the input of react/react#37228): `return (() => { try { return check(raw) ? raw : null; } catch { return null; } })();`. 1.4.2 aborts with the same panic and this branch skips the function with the same invariant. babel-plugin-react-compiler 1.0.0 skips it one step earlier, with `Todo: Support value blocks (conditional, logical, optional chaining, etc) within a try/catch statement`. Two loop shapes with no `?:` at all, where `identity` is an imported function: `let r; do { r = identity(value); } while (cond()); return r;` and `let r; while (cond()) { r = identity(value); break; } return r;`. 1.4.2 aborts on both with the same panic, this branch skips the function, and babel-plugin-react-compiler 1.0.0 raises the same invariant and leaves the function as written. With `String(value)` in place of `identity(value)` nothing aborts. I ran both by hand on this branch. They are not in the test. Why the node is missing. `compute_memoization_inputs` returns the rvalues of the consequent and the alternate of a `ConditionalExpression` only (upstream: "Conditionals do not alias their test value"), and `visit_instruction` does not traverse nested values on its own. So nothing calls `visit_operand` for `t = p.f()` or `m = t`, and `visit_operand` is the only place that creates a scope node. The scope is aligned to the whole `const r = ...` statement and lists `m` in `reassignments`, so `visit_scope` adds it to the node of `m`. The array `[m]` is `Memoized`, its scope depends on `m`, `m` is `Unmemoized` (from `let m;`) and forced, and the walk reaches the scope with no node. `(m = p.f()) ? p.a() : p.b()` compiles because the calls in the branches are in the same scope and create the node. Upstream. `PruneNonEscapingScopes.ts` on facebook/react main still has all four invariants, and `react_compiler_reactive_scopes/src/prune_non_escaping_scopes.rs` there has the same four `.expect()` calls. I ran babel-plugin-react-compiler 1.0.0 on 20 variants of the input. It logs `CompileError: Expected a node for all scopes` and leaves the function as written for exactly the 10 variants that abort 1.4.3 canary. The two agree on the other 10 as well: 8 compile with one memo cache, 1 compiles with none, and 1 fails in both with `Unexpected StoreLocal in codegenInstructionValue`, which Bun already returns as an error. With this change Bun skips the same 10 functions. Alternative not taken: create the scope node on demand in `force_memoize_scope_dependencies` from `env.scopes[id].dependencies`, which is all `visit_operand` stores. That compiles these functions, but upstream does not compile them, and nothing in that test value has been analysed by this pass. The three `Expected identifier to be initialized` sites. Upstream never reaches them for the inputs below, but two other bugs of the port do. `visit_operand` (`:195`): a closure above a `let` that it reads and that is reassigned later, for example `const row = () => <Row items={items} />; let items = p.items; if (p.c) items = [];`. The lowering did not declare `items` before the closure there. #42390 fixed that (merged), and upstream compiles and memoizes those functions. `visit_scope` (`:1060`): dead code elimination dropped `let v` but kept a store to it, fixed in #42385 (merged). Those inputs were not parity cases, and they compile on main now. The conversion here is for the next input that reaches one of these sites. To exercise the three paths on their own I also forced each lookup to miss in a local build (one site at a time, selected by an environment variable, not committed). Each time `bun build` exited 0, the debug log showed `compile_fn err: ... "Expected identifier to be initialized"` for the affected functions, and the other functions in the file still compiled. Not changed here: the other upstream invariants in this crate that are still `.expect()` or `assert!` (`build_reactive_scope_terminals_hir.rs:137/145/173`, `propagate_scope_dependencies_hir.rs:90/1772`, `align_reactive_scopes_to_block_scopes_hir.rs:262`, `eliminate_redundant_phi.rs:75`, `build_reactive_function.rs:216/1388`). No known input reaches them and each needs `Result` plumbing through a different pass. `align_object_method_scopes.rs` is #42375. What the test checks. It bundles one file with `--target=browser` through the CLI and runs the output against a fake `react/compiler-runtime` whose `c` counts its calls. Four functions have the shape (call, array and object in the test, a `let m = null` initializer, a hook that returns the array, the conditional nested in a binary expression). A fifth is the react/react#37228 input, a `?:` in a `try` in an IIFE. Each returns the right value and makes 0 calls to `c`. Two controls (the same `?:` with `m` not memoized, and a plain component) make 1 call each, so the skip is per function and not per file. Each of the five aborts 1.4.2 on its own. Sweep: built all 1807 source files under `test/bundler/transpiler/react-compiler-fixtures/` with `bun build --react-compiler --target=browser --external '*'` on 1.4.3 canary. None aborts, so no upstream fixture has this shape. Suites run with the debug build, after the rebase on 7a7cc96: `test/bundler/transpiler/react-compiler.test.ts` (58 pass), `test/bundler/transpiler/react-compiler-fixtures.test.ts` (3293 pass, 320 skip). `cargo clippy -p bun_react_compiler` and `cargo fmt --check` are clean. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 1 · 2 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 2 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/bundler/transpiler/react-compiler.test.ts bun test v1.4.3 (6a92015) test/bundler/transpiler/react-compiler.test.ts: (pass) bundler > react-compiler/SimpleComponent [1077.04ms] (pass) bundler > react-compiler/ComponentWithHooks [1018.66ms] (pass) bundler > react-compiler/ObjectPatternRestInProps [466.17ms] (pass) bundler > react-compiler/UnderscoreAndDollarComponentTags [677.15ms] (pass) bundler > react-compiler/OutputModeDefaultsByTarget-Browser [586.00ms] (pass) bundler > react-compiler/OutputModeDefaultsByTarget-Bun [413.21ms] (pass) bundler > react-compiler/FullstackHtmlImportCompilesClientGraphInClientMode [963.12ms] (pass) bundler > react-compiler/OutputModeExplicitSsrOverridesTarget [227.16ms] (pass) bundler > react-compiler/SsrObjectMethodShorthand [1037.88ms] (pass) bundler > react-compiler/OutputModeIgnoredWhenCompilerDisabled-Client [325.23ms] (pass) bundler > react-compiler/OutputModeIgnoredWhenCompilerDisabled-Ssr [307.01ms] (pass) bundler > react-compiler/BundledReactPreservesImportRefs [538.68ms] (pass) bundler > r ... (truncated) release without fix: 12 failed, 1 skipped bun test v1.4.3-canary.1 (3c62dfb) test/bundler/transpiler/react-compiler.test.ts: (pass) bundler > react-compiler/SimpleComponent [63.16ms] (pass) bundler > react-compiler/ComponentWithHooks [20.36ms] (pass) bundler > react-compiler/ObjectPatternRestInProps [11.72ms] (pass) bundler > react-compiler/UnderscoreAndDollarComponentTags [14.43ms] (pass) bundler > react-compiler/OutputModeDefaultsByTarget-Browser [30.53ms] (pass) bundler > react-compiler/OutputModeDefaultsByTarget-Bun [7.03ms] (pass) bundler > react-compiler/FullstackHtmlImportCompilesClientGraphInClientMode [24.77ms] (pass) bundler > react-compiler/OutputModeExplicitSsrOverridesTarget [11.01ms] 1031 | ]); 1032 | const stdout = Buffer.from(stdoutBytes); 1033 | const stderr = Buffer.from(stderrBytes); 1034 | const success = exitCode === 0; 1035 | if (buildProc.signalCode) { 1036 | throw new Error( ^ error: [react-compiler/SsrObjectMethodShorthand] 'bun build' subprocess killed by SIGABRT cmd: /workspace/bun/build/release/bun build /tmp/bun-build-tests/bun-eKoWEm/react-compiler/SsrObjectMethodShorthand/entry.jsx --outfile=/tmp/bun-build-tests ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/bundler/transpiler/react-compiler.test.ts bun test v1.4.3 (6a92015) test/bundler/transpiler/react-compiler.test.ts: (pass) bundler > react-compiler/SimpleComponent [984.04ms] (pass) bundler > react-compiler/ComponentWithHooks [341.71ms] (pass) bundler > react-compiler/ObjectPatternRestInProps [417.08ms] (pass) bundler > react-compiler/UnderscoreAndDollarComponentTags [333.24ms] (pass) bundler > react-compiler/OutputModeDefaultsByTarget-Browser [192.84ms] (pass) bundler > react-compiler/OutputModeDefaultsByTarget-Bun [179.22ms] (pass) bundler > react-compiler/FullstackHtmlImportCompilesClientGraphInClientMode [421.84ms] (pass) bundler > react-compiler/OutputModeExplicitSsrOverridesTarget [149.25ms] (pass) bundler > react-compiler/SsrObjectMethodShorthand [850.95ms] (pass) bundler > react-compiler/OutputModeIgnoredWhenCompilerDisabled-Client [149.08ms] (pass) bundler > react-compiler/OutputModeIgnoredWhenCompilerDisabled-Ssr [164.78ms] (pass) bundler > react-compiler/BundledReactPreservesImportRefs [344.39ms] (pass) bundler > reac ... (truncated) release with fix: 1 skipped $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 814ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [0/4] cargo bun_runtime → libbun_runtime.a ^[[1m^[[92m Compiling^[[0m bun_react_compiler v0.0.0 (/workspace/bun/src/react_compiler) ^[[1m^[[92m Compiling^[[0m bun_js_parser v0.0.0 (/workspace/bun/src/js_parser) ^[[1m^[[92m Compiling^[[0m bun_resolver v0.0.0 (/workspace/bun/src/resolver) ^[[1m^[[92m Compiling^[[0m bun_ini v0.0.0 (/workspace/bun/src/ini) ^[[1m^[[92m Compiling^[[0m bun_bundler v0.0.0 (/workspace/bun/src/bundler) ^[[1m^[[92m Compiling^[[0m bun_router v0.0.0 (/workspace/bun/src/router) ^[[1m^[[92m Compiling^[[0m bun_standalone_graph v0.0.0 (/workspace/bun/src/standalone_graph) ^[[1m^[[92m Compiling^[[0m bun_transpiler v0.0.0 (/workspace/bun/src/transpiler) ^[[1m^[[92m Compiling^[[0m bun_bunfig v0.0.0 (/workspace/bun/src/bunfig) ^[[1m^[[92m Compiling^[[0m bun_install v0.0.0 (/workspace/bun/src/install) ^[[1m^[[92m Compiling^[[0m bun_jsc v0.0.0 (/workspace/bun/src/jsc) ^[[1m^[[92m Compiling^[[0m bun_js_parser_jsc v0.0.0 (/workspace/bun/src/js_parser_jsc) ^[[1m^[[92m Compiling^[[0m bun_http_jsc v0.0.0 (/work ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` .../reactive_scopes/prune_non_escaping_scopes.rs | 103 +++++++++++++-------- test/bundler/transpiler/react-compiler.test.ts | 103 +++++++++++++++++++++ 2 files changed, 166 insertions(+), 40 deletions(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 1 <details><summary>evidence per changed file</summary> ``` file reads edits tests …t_compiler/reactive_scopes/prune_non_escaping_scopes.rs 6 12 29 test/bundler/transpiler/react-compiler.test.ts 2 4 29 ``` </details> <!-- robobun:evidence:end -->
usrbinkat
pushed a commit
to usrbinkat/bun
that referenced
this pull request
Sep 15, 2026
…n-sh#42378) ### Problem - `bun build --react-compiler --target=browser` aborts on `const r = (m = p.f()) ? 1 : 0; return <div data-v={[m, r]} />`: `panic: Expected a node for all scopes`. `Bun.build` aborts the calling process the same way. - `CollectDependenciesVisitor` skips the test of a `?:`, so the reactive scope that reassigns `m` gets no scope node. `visit_scope` still attaches that scope to `m`. When `m` is force-memoized, `force_memoize_scope_dependencies` (`src/react_compiler/reactive_scopes/prune_non_escaping_scopes.rs:1170`) calls `.expect()` on the missing node. ### Fix - That invariant is now a `cold_invariant` error. `program.rs` leaves that one function uncompiled, like upstream's `CompilerError.invariant`. - The three `Expected identifier to be initialized` invariants in the same pass get the same treatment. Their visitor callbacks return `()`, so they record the first error in `CollectState`, as `assert_scope_instructions_within_scopes` does. - Correct because babel-plugin-react-compiler 1.0.0 raises the same invariant on the same inputs and skips the same functions. Compiled output does not change. - Verified: `test/bundler/transpiler/react-compiler.test.ts` (new `AssignmentInConditionalTestSkipsOnlyThatFunction`, SIGABRT on 1.4.3 canary). Also `react-compiler-fixtures.test.ts`. ### Background - The React Compiler groups values that change together into reactive scopes and emits one memo block per scope. PruneNonEscapingScopes removes the scopes whose values never leave the function. - The pass builds a graph of identifier nodes and scope nodes, then walks it from the returned values. - Upstream's `CompilerError.invariant` is a caught per-function error in TS. Bun builds with `panic = "abort"`, so a ported `.expect()` ends the process. <details><summary>Notes</summary> Repro (any directory, 1.4.2, 1.4.3 canary 6a92015 and main 4b5862f): ```jsx // a.jsx export default function App(p) { let m; const r = (m = p.f()) ? 1 : 0; return <div data-v={[m, r]} />; } ``` ``` $ bun build --react-compiler --target=browser --external react a.jsx panic: Expected a node for all scopes Crashed while visiting a.jsx ``` Frames: `force_memoize_scope_dependencies` (`prune_non_escaping_scopes.rs:1170`), `compute_memoized_identifiers::visit` (`:1156`), `prune_non_escaping_scopes` (`:70`), `pipeline::run_hir_passes` (`pipeline.rs:622`), `compile_fn` (`:172`). The smallest shape: an assignment whose value allocates (a call, an array or an object literal) in the test of a `?:`, and the assigned local as an operand of a memoized value (`[m]`, `{ m }`). The result of the conditional does not need to reach that value. `m = p.a` compiles. `&&`, `??`, `?.` or an `if` statement in place of `?:` compile. `<div data-m={m} />` compiles because nothing force-memoizes `m`. `--target=bun` and `--target=node` (ssr mode) create no reactive scopes and compile. A second shape, with no assignment expression (the input of react/react#37228): `return (() => { try { return check(raw) ? raw : null; } catch { return null; } })();`. 1.4.2 aborts with the same panic and this branch skips the function with the same invariant. babel-plugin-react-compiler 1.0.0 skips it one step earlier, with `Todo: Support value blocks (conditional, logical, optional chaining, etc) within a try/catch statement`. Two loop shapes with no `?:` at all, where `identity` is an imported function: `let r; do { r = identity(value); } while (cond()); return r;` and `let r; while (cond()) { r = identity(value); break; } return r;`. 1.4.2 aborts on both with the same panic, this branch skips the function, and babel-plugin-react-compiler 1.0.0 raises the same invariant and leaves the function as written. With `String(value)` in place of `identity(value)` nothing aborts. I ran both by hand on this branch. They are not in the test. Why the node is missing. `compute_memoization_inputs` returns the rvalues of the consequent and the alternate of a `ConditionalExpression` only (upstream: "Conditionals do not alias their test value"), and `visit_instruction` does not traverse nested values on its own. So nothing calls `visit_operand` for `t = p.f()` or `m = t`, and `visit_operand` is the only place that creates a scope node. The scope is aligned to the whole `const r = ...` statement and lists `m` in `reassignments`, so `visit_scope` adds it to the node of `m`. The array `[m]` is `Memoized`, its scope depends on `m`, `m` is `Unmemoized` (from `let m;`) and forced, and the walk reaches the scope with no node. `(m = p.f()) ? p.a() : p.b()` compiles because the calls in the branches are in the same scope and create the node. Upstream. `PruneNonEscapingScopes.ts` on facebook/react main still has all four invariants, and `react_compiler_reactive_scopes/src/prune_non_escaping_scopes.rs` there has the same four `.expect()` calls. I ran babel-plugin-react-compiler 1.0.0 on 20 variants of the input. It logs `CompileError: Expected a node for all scopes` and leaves the function as written for exactly the 10 variants that abort 1.4.3 canary. The two agree on the other 10 as well: 8 compile with one memo cache, 1 compiles with none, and 1 fails in both with `Unexpected StoreLocal in codegenInstructionValue`, which Bun already returns as an error. With this change Bun skips the same 10 functions. Alternative not taken: create the scope node on demand in `force_memoize_scope_dependencies` from `env.scopes[id].dependencies`, which is all `visit_operand` stores. That compiles these functions, but upstream does not compile them, and nothing in that test value has been analysed by this pass. The three `Expected identifier to be initialized` sites. Upstream never reaches them for the inputs below, but two other bugs of the port do. `visit_operand` (`:195`): a closure above a `let` that it reads and that is reassigned later, for example `const row = () => <Row items={items} />; let items = p.items; if (p.c) items = [];`. The lowering did not declare `items` before the closure there. oven-sh#42390 fixed that (merged), and upstream compiles and memoizes those functions. `visit_scope` (`:1060`): dead code elimination dropped `let v` but kept a store to it, fixed in oven-sh#42385 (merged). Those inputs were not parity cases, and they compile on main now. The conversion here is for the next input that reaches one of these sites. To exercise the three paths on their own I also forced each lookup to miss in a local build (one site at a time, selected by an environment variable, not committed). Each time `bun build` exited 0, the debug log showed `compile_fn err: ... "Expected identifier to be initialized"` for the affected functions, and the other functions in the file still compiled. Not changed here: the other upstream invariants in this crate that are still `.expect()` or `assert!` (`build_reactive_scope_terminals_hir.rs:137/145/173`, `propagate_scope_dependencies_hir.rs:90/1772`, `align_reactive_scopes_to_block_scopes_hir.rs:262`, `eliminate_redundant_phi.rs:75`, `build_reactive_function.rs:216/1388`). No known input reaches them and each needs `Result` plumbing through a different pass. `align_object_method_scopes.rs` is oven-sh#42375. What the test checks. It bundles one file with `--target=browser` through the CLI and runs the output against a fake `react/compiler-runtime` whose `c` counts its calls. Four functions have the shape (call, array and object in the test, a `let m = null` initializer, a hook that returns the array, the conditional nested in a binary expression). A fifth is the react/react#37228 input, a `?:` in a `try` in an IIFE. Each returns the right value and makes 0 calls to `c`. Two controls (the same `?:` with `m` not memoized, and a plain component) make 1 call each, so the skip is per function and not per file. Each of the five aborts 1.4.2 on its own. Sweep: built all 1807 source files under `test/bundler/transpiler/react-compiler-fixtures/` with `bun build --react-compiler --target=browser --external '*'` on 1.4.3 canary. None aborts, so no upstream fixture has this shape. Suites run with the debug build, after the rebase on 7a7cc96: `test/bundler/transpiler/react-compiler.test.ts` (58 pass), `test/bundler/transpiler/react-compiler-fixtures.test.ts` (3293 pass, 320 skip). `cargo clippy -p bun_react_compiler` and `cargo fmt --check` are clean. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 1 · 2 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 2 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/bundler/transpiler/react-compiler.test.ts bun test v1.4.3 (6a92015) test/bundler/transpiler/react-compiler.test.ts: (pass) bundler > react-compiler/SimpleComponent [1077.04ms] (pass) bundler > react-compiler/ComponentWithHooks [1018.66ms] (pass) bundler > react-compiler/ObjectPatternRestInProps [466.17ms] (pass) bundler > react-compiler/UnderscoreAndDollarComponentTags [677.15ms] (pass) bundler > react-compiler/OutputModeDefaultsByTarget-Browser [586.00ms] (pass) bundler > react-compiler/OutputModeDefaultsByTarget-Bun [413.21ms] (pass) bundler > react-compiler/FullstackHtmlImportCompilesClientGraphInClientMode [963.12ms] (pass) bundler > react-compiler/OutputModeExplicitSsrOverridesTarget [227.16ms] (pass) bundler > react-compiler/SsrObjectMethodShorthand [1037.88ms] (pass) bundler > react-compiler/OutputModeIgnoredWhenCompilerDisabled-Client [325.23ms] (pass) bundler > react-compiler/OutputModeIgnoredWhenCompilerDisabled-Ssr [307.01ms] (pass) bundler > react-compiler/BundledReactPreservesImportRefs [538.68ms] (pass) bundler > r ... (truncated) release without fix: 12 failed, 1 skipped bun test v1.4.3-canary.1 (3c62dfb) test/bundler/transpiler/react-compiler.test.ts: (pass) bundler > react-compiler/SimpleComponent [63.16ms] (pass) bundler > react-compiler/ComponentWithHooks [20.36ms] (pass) bundler > react-compiler/ObjectPatternRestInProps [11.72ms] (pass) bundler > react-compiler/UnderscoreAndDollarComponentTags [14.43ms] (pass) bundler > react-compiler/OutputModeDefaultsByTarget-Browser [30.53ms] (pass) bundler > react-compiler/OutputModeDefaultsByTarget-Bun [7.03ms] (pass) bundler > react-compiler/FullstackHtmlImportCompilesClientGraphInClientMode [24.77ms] (pass) bundler > react-compiler/OutputModeExplicitSsrOverridesTarget [11.01ms] 1031 | ]); 1032 | const stdout = Buffer.from(stdoutBytes); 1033 | const stderr = Buffer.from(stderrBytes); 1034 | const success = exitCode === 0; 1035 | if (buildProc.signalCode) { 1036 | throw new Error( ^ error: [react-compiler/SsrObjectMethodShorthand] 'bun build' subprocess killed by SIGABRT cmd: /workspace/bun/build/release/bun build /tmp/bun-build-tests/bun-eKoWEm/react-compiler/SsrObjectMethodShorthand/entry.jsx --outfile=/tmp/bun-build-tests ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/bundler/transpiler/react-compiler.test.ts bun test v1.4.3 (6a92015) test/bundler/transpiler/react-compiler.test.ts: (pass) bundler > react-compiler/SimpleComponent [984.04ms] (pass) bundler > react-compiler/ComponentWithHooks [341.71ms] (pass) bundler > react-compiler/ObjectPatternRestInProps [417.08ms] (pass) bundler > react-compiler/UnderscoreAndDollarComponentTags [333.24ms] (pass) bundler > react-compiler/OutputModeDefaultsByTarget-Browser [192.84ms] (pass) bundler > react-compiler/OutputModeDefaultsByTarget-Bun [179.22ms] (pass) bundler > react-compiler/FullstackHtmlImportCompilesClientGraphInClientMode [421.84ms] (pass) bundler > react-compiler/OutputModeExplicitSsrOverridesTarget [149.25ms] (pass) bundler > react-compiler/SsrObjectMethodShorthand [850.95ms] (pass) bundler > react-compiler/OutputModeIgnoredWhenCompilerDisabled-Client [149.08ms] (pass) bundler > react-compiler/OutputModeIgnoredWhenCompilerDisabled-Ssr [164.78ms] (pass) bundler > react-compiler/BundledReactPreservesImportRefs [344.39ms] (pass) bundler > reac ... (truncated) release with fix: 1 skipped $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 814ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [0/4] cargo bun_runtime → libbun_runtime.a ^[[1m^[[92m Compiling^[[0m bun_react_compiler v0.0.0 (/workspace/bun/src/react_compiler) ^[[1m^[[92m Compiling^[[0m bun_js_parser v0.0.0 (/workspace/bun/src/js_parser) ^[[1m^[[92m Compiling^[[0m bun_resolver v0.0.0 (/workspace/bun/src/resolver) ^[[1m^[[92m Compiling^[[0m bun_ini v0.0.0 (/workspace/bun/src/ini) ^[[1m^[[92m Compiling^[[0m bun_bundler v0.0.0 (/workspace/bun/src/bundler) ^[[1m^[[92m Compiling^[[0m bun_router v0.0.0 (/workspace/bun/src/router) ^[[1m^[[92m Compiling^[[0m bun_standalone_graph v0.0.0 (/workspace/bun/src/standalone_graph) ^[[1m^[[92m Compiling^[[0m bun_transpiler v0.0.0 (/workspace/bun/src/transpiler) ^[[1m^[[92m Compiling^[[0m bun_bunfig v0.0.0 (/workspace/bun/src/bunfig) ^[[1m^[[92m Compiling^[[0m bun_install v0.0.0 (/workspace/bun/src/install) ^[[1m^[[92m Compiling^[[0m bun_jsc v0.0.0 (/workspace/bun/src/jsc) ^[[1m^[[92m Compiling^[[0m bun_js_parser_jsc v0.0.0 (/workspace/bun/src/js_parser_jsc) ^[[1m^[[92m Compiling^[[0m bun_http_jsc v0.0.0 (/work ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` .../reactive_scopes/prune_non_escaping_scopes.rs | 103 +++++++++++++-------- test/bundler/transpiler/react-compiler.test.ts | 103 +++++++++++++++++++++ 2 files changed, 166 insertions(+), 40 deletions(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 1 <details><summary>evidence per changed file</summary> ``` file reads edits tests …t_compiler/reactive_scopes/prune_non_escaping_scopes.rs 6 12 29 test/bundler/transpiler/react-compiler.test.ts 2 4 29 ``` </details> <!-- robobun:evidence:end -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
bun build --react-compiler --target=browseraborts on a component with a helper above aletthat the helper reads and that is reassigned later:panic: Expected identifier to be initialized(prune_non_escaping_scopes.rs:195). With other helper bodies the compiler skips the function instead (Invariant: [InferMutationAliasingEffects] Expected value kind to be initialized), so it is not memoized.src/react_compiler/lowering/build_hir/stmt.rs:158) skipped every binding thatis_context_identifier.FindContextIdentifiersmarks a captured and reassigned local before lowering starts, so that local got noDeclareContext HoistedLet. The closure then captured a variable that no instruction had declared.Fix
is_hoisted_identifier), likeBuildHIR.tsand the upstream Rust crate.DeclareContext HoistedLetthat upstream prints in front of the closure. On 119 probe variants the outcome and every_c(n)size equal babel-plugin-react-compiler 1.0.0. Before: 41 abort, 50 skipped, 28 equal.test/bundler/transpiler/react-compiler.test.ts(newClosureAboveReassignedLet, SIGABRT on 1.4.2 and main). Alsoreact-compiler-fixtures.test.ts(3293 pass).Background
LoadContextandStoreContextand declares it withDeclareContext.letthat is declared below it. Lowering emitsDeclareContext HoistedLet xin front of the first statement that has such a closure, so every pass sees a declaration before the capture.PruneHoistedContextsremoves it before codegen.FindContextIdentifiersis the pass before lowering that finds those locals.Notes
Repro (any directory, 1.4.2 and main 471b586):
Frames:
CollectState::visit_operand(prune_non_escaping_scopes.rs:195),visit_value_for_memoization(:896),prune_non_escaping_scopes(:66),pipeline::run_hir_passes(pipeline.rs:622). The operand is the context operanditemsof theFunctionExpression. It is in a reactive scope that is active at that instruction, and no earlier instruction has it as an lvalue, so it has no identifier node.The same cause with a self reference:
let render = () => <Row again={render} />; render = wrap(render);.Upstream HIR for
g.jsx(babel-plugin-react-compiler 1.0.0,debugLogIRs), first instructions:itemsis a context identifier fromFindContextIdentifiersthere too (captured and reassigned), and upstream still hoists it.BuildHIR.tshasif (builder.environment.isHoistedIdentifier(binding.identifier)) continue; // Already hoisted, andreact_compiler_lowering/src/build_hir.rson facebook/react main hasis_hoisted_identifier(info.binding_id.0). Theis_context_identifiercheck is in the port since #32504.Why only some helper bodies abort. Without the declaration,
InferMutationAliasingEffectsfails when the closure captures or aliases the variable (items.length,[items],foo(items), a bareitems): the value kind ofitemsis not initialized, and that invariant is already a per-function error. When the closure only reads or freezes it (a JSX attribute or child, a template literal,items + 1,items ? 1 : 2), inference passes and PruneNonEscapingScopes is the first pass that needs the declaration. That one is an.expect().Probes. Matrix 1 is 104 files: helper as arrow or function declaration, 13 helper bodies, the
letreassigned in anif, in a loop, in a plain statement, or never. Matrix 2 is 15 files: function declaration reassigned after a closure calls it, destructuredlet,letin a nested block, two closures, a closure that reassigns, a closure in a closure,useEffect, an object method,for,switch,try, the self reference. For each file I compared the compile event and the list of_c(n)sizes with babel-plugin-react-compiler 1.0.0.One of the 119 is a function declaration that names itself in JSX and is reassigned. Upstream skips it with
Todo: [PruneHoistedContexts] Rewrite hoisted function references. This branch reports the same error. Main aborts.The fixture
hoisting-reassigned-let-declaration.jshas this shape with a bare() => x. It passes before and after: the port reached the same output without the declaration.Relation to #42378. That PR turns the
.expect()atprune_non_escaping_scopes.rs:195into a per-function error. The two are independent. With only #42378 these functions are skipped and not memoized. With only this PR they compile.What the test checks. It bundles four functions with
--target=browserthrough the CLI and runs the output against a fakereact/compiler-runtimewhosecrecords every cache size. Each function returns the right value (the closure sees the reassigned value) and allocates the cache size that upstream emits: 4, 3, 4, 6. Each of the four aborts 1.4.2 on its own. On a build that has only #42378 the test also fails, with[]in place of each size.ssr mode (
--target=bun) compiled these before and still does.Suites run with the debug build:
test/bundler/transpiler/react-compiler.test.ts(46 pass),test/bundler/transpiler/react-compiler-fixtures.test.ts(3293 pass, 320 skip).cargo clippy -p bun_react_compilerandcargo fmt --checkare clean.[human-review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file