Repository navigation
Conversation
…king merge_consecutive_blocks returns the 'Found a block with a single predecessor but where a phi has multiple operands' invariant as an error. Its callers (prune_maybe_throws, constant_propagation, inline_iifes and the pipeline) propagate it. validate_no_set_state_in_render returns 'Unexpected nested StartMemoize instructions' and 'Expected FinishMemoize to align with previous StartMemoize instruction' as invariant errors. validate_exhaustive_dependencies does the same for 'Found FinishMemoize without corresponding StartMemoize'. program.rs leaves a function that fails with an error uncompiled, so one such function no longer aborts the build.
|
Updated 3:55 AM PT - Sep 12th, 2026
✅ @robobun, your commit a002e9c9d45ace98b3192ff6ae16241019d06a8c passed in 🧪 To try this PR locally: bunx bun-pr 42456That installs a local version of the PR into your bun-42456 --bun |
|
Status: open, waiting for CI. Head a002e9c on main b993710. Reproduction: on 1.4.3 canary (6a92015), each of these aborts with exit 134. Main b993710 still has both asserts. // a.jsx
export function Banner() {
let supported = true;
try {
window.localStorage;
} catch {
supported = false;
}
return <span>{supported ? "on" : "off"}</span>;
}// c.jsx
import { useMemo } from "react";
export function Sum({ a, b }) {
const x = useMemo(() => a + b, [a, useMemo(() => b * 2, [b])]);
return <div>{x}</div>;
}With this branch both commands exit 0 and print the function as written, for |
|
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 (8)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. WalkthroughThe compiler optimization and validation passes now return and propagate ChangesCompiler diagnostic propagation
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to Invariant failures now bail out only the affected function while preserving compilation of unaffected functions. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical assert→Result conversion that matches the existing cold_invariant / mem::replace+? pattern already used elsewhere in this crate.
What was reviewed:
- The new
?betweenmem::replace(&mut env.functions[...], placeholder_function())and the restore inprocess_inner_functionandmerge_consecutive_blocks—envis created fresh percompile_fn(pipeline.rs:132) and dropped onErr, and the identical pattern already exists at enter_ssa.rs:449, infer_types.rs:473, and align_object_method_scopes.rs:106. - The
evaluate_instructionOption<Constant>→Result<Option<Constant>, _>rewrite — every earlyreturn Some/Noneis nowreturn Ok(Some/None)and the trailing match value is wrapped once; no branch changed semantics. - The
(Some(operand), None)iterator match inmerge_consecutive_blocks— correctly errors on both 0 and >1 operands, preserving the originalassert_eq!(len, 1)contract with the same message text as upstream babel. - Tests:
itBundledwith local stubreact/compiler-runtimecountingc()calls; each test has control functions asserting compilation still happens (call count 1) alongside the skipped ones (call count 0), so a regression that stops compiling everything would fail.
Extended reasoning...
Overview
This PR converts four user-reachable assert!/assert_eq! panics in the React Compiler HIR passes into recoverable CompilerDiagnostic errors, so that inputs which trip these upstream invariants (a dead try body leaving a stale phi operand, or nested useMemo/useCallback producing nested StartMemoize/FinishMemoize markers) skip only the affected function instead of aborting the process. merge_consecutive_blocks now returns Result<(), CompilerDiagnostic>; its four callers (constant_propagation, inline_iifes, prune_maybe_throws, and the standalone pipeline pass) thread the ? through, and evaluate_instruction is mechanically rewritten from Option<Constant> to Result<Option<Constant>, _>. Two itBundled tests exercise both bug classes with multiple variants plus control functions that must still compile.
Security risks
None. This is compiler-internal error propagation; no parsing of untrusted headers, no auth, no crypto, no filesystem or network. The change strictly reduces attack surface by turning a user-input-reachable panic = "abort" (a DoS vector per REVIEW.md) into a per-function skip that already existed for every other CompilerDiagnostic.
Level of scrutiny
Moderate, focused on the REVIEW.md rule "new early returns → re-audit acquisitions above them." The only acquisition above a new ? is the mem::replace(..., placeholder_function()) swap in process_inner_function and the recursive merge_consecutive_blocks loop. I confirmed Environment is constructed per compile_fn call (pipeline.rs:132) and discarded on Err (program.rs:1385-1391), so a placeholder left in env.functions on the error path is never observed. This is not a new pattern: enter_ssa.rs:449, infer_types.rs:473, and align_object_method_scopes.rs:106 already ? between the same replace/restore pair. The #[cold] #[inline(never)] diagnostic constructor mirrors the existing cold_invariant/cold_todo helpers in diagnostics/mod.rs.
Other factors
The tests are well-designed: they use the itBundled harness, stub react/react/compiler-runtime locally (no network), and assert both the rendered output and the c() call count for each function — so they distinguish "left uncompiled" from "compiled" from "crashed," and the control functions (CallInTryBody, Plain, SequentialMemos) would catch a regression that over-skips. The PR description documents that both tests SIGABRT on the base commit, satisfying the fails-on-USE_SYSTEM_BUN=1 requirement. No CODEOWNERS entry covers src/react_compiler/ or test/bundler/. The bug hunt ran to a dry streak with no findings and no candidates ruled out. The PR timeline has no outstanding reviewer objections.
|
I had the same abort from a hand-off and found this PR when my change was ready. I will not open a second PR for it. This PR covers the crash. For reference, the branch Three facts from that work, in case someone takes it up after this PR:
|
This branch has not been deployed
2 participants
Problem
bun build --react-compileraborts ontry { window.localStorage; } catch { ok = false; }whenokis read later:panic: assertion `left == right` failed: Found a block with a single predecessor but where a phi has multiple (2) operands(src/react_compiler/optimization/merge_consecutive_blocks.rs:106). Dead code elimination empties the try body,prune_maybe_throwsdrops the edge to the catch block, and the phi forokkeeps the operand of the removed block.useMemo(() => a + b, [a, useMemo(() => b * 2, [b])])aborts withpanic: Unexpected nested StartMemoize instructions(src/react_compiler/validation/validate_no_set_state_in_render.rs:96).drop_manual_memoizationnests the marker pairs of the two calls.assert!, and Bun builds withpanic = "abort".Fix
merge_consecutive_blocksreturns the invariant as an error. Its four callers propagate it with?.validate_no_set_state_in_render,validate_exhaustive_dependencies) becomecold_invarianterrors.program.rsleaves a function that returns an error uncompiled, as babel-plugin-react-compiler 1.0.0 does for the same inputs. Compiled output does not change.test/bundler/transpiler/react-compiler.test.ts(two new tests, SIGABRT on 1.4.3 canary). Alsoreact-compiler-fixtures.test.ts.Background
prune_maybe_throwsremoves the edge from a try body to its catch block when nothing left in the body can throw.useMemo/useCallbackcall for later validations.compile_fnreturnsResult.program.rsskips that one function for eachErr.Notes
Provenance. Found by fuzzing. There is no user report and no issue to close. Same family as #42375 (merged) and #42378 (open): an upstream
CompilerError.invariantthat the port has as a panic.Repro (any directory, 1.4.3 canary 6a92015; main b993710 still has both asserts):
Every
--targetaborts, andBun.build({ reactCompiler: true })aborts the calling process. With this change both builds exit 0,BUN_DEBUG_react_compiler=1on a debug build logscompile_fn err: ... Invariant ...for that function, and the function is printed as written. ABun.buildwitha.jsx,c.jsxand a third entry point returnssuccess: truewith the third file memoized.First site. Frames:
merge_consecutive_blocks(merge_consecutive_blocks.rs:106),prune_maybe_throws(the second run, after SSA and DeadCodeElimination),run_hir_passes,compile_fn(pipeline.rs:172). The try body must be fully dead after dead code elimination (property, global and index loads, destructuring,typeof, literals, unused declarations; a call or a value that is read later keeps it alive), and the catch block must assign a local that is read after the try.const unused = 1in the try body is enough (#42432 lists that form as not fixed there). The same body in the callback of auseMemoaborts too, because the callback is inlined. A variant builds in development and aborts only with--productionor--define process.env.NODE_ENV='"production"':try { if (process.env.NODE_ENV !== "production") { validate(props); } const label = props.label; } catch { ok = false; }.The cause is upstream's.
PruneMaybeThrows.tscallsmergeConsecutiveBlocksbefore it rewrites phi operands, and it has no step that drops the operand of a predecessor that became unreachable.ConstantPropagation.tshas that step. facebook/react main (019019be) still has this order in TS and incompiler/crates/react_compiler_optimization, and both still have the invariant (assert_eq!in the Rust crate).Alternative not taken for the first site. Drop the phi operands of removed predecessors in
prune_maybe_throwsbefore the merge, asconstant_propagationdoes. The function then compiles, which upstream does not do today. The compiled output also has no try/catch any more, because dead code elimination removed the load.try { window.localStorage } catch { supported = false }would always renderon, also where the load throws.pipeline.rssays the pass sequence stays identical with upstream, so this PR only changes how the invariant is reported.Why
constant_propagation.rsandinline_iifes.rschange. They callmerge_consecutive_blocks. Neither can reach the invariant today:inline_iifesand the standalone pipeline pass run before SSA, so no block has a phi, andconstant_propagationremoves the operands of unreachable predecessors right before it merges. They returnResultso that the helper has no caller that has to panic or drop the error.evaluate_instructionrecurses into nested functions, so its return type changes fromOption<Constant>toResult<Option<Constant>, _>. The edits there are mechanical (return Some(x)toreturn Ok(Some(x))).Second site. Frames:
validate_impl(validate_no_set_state_in_render.rs:96),validate_no_set_state_in_render(:26),run_hir_passes(pipeline.rs:467).drop_manual_memoizationputs StartMemoize after the load ofuseMemo/useCallbackand FinishMemoize after the call, so a manual memo call anywhere in the arguments of another one nests the pairs. For a call in the dependency list, the port also recordsExpected the dependency list to be an array of simple expressions, as upstream main does (env.recordError), and continues. This validation runs before the pipeline checks the recorded errors.useMemo(() => a, [a], useMemo(() => b, [b]))nests the pairs with no recorded error. AuseMemoin the callback of anotheruseMemodoes not nest them, because the pass does not visit nested functions.Comparison with babel-plugin-react-compiler 1.0.0. I ran it on the 11 functions of the two new tests. It leaves the same 8 as written and compiles the same 3. Its errors:
Found a block with a single predecessor but where a phi has multiple (2) operandsfor the four try forms,Unexpected nested StartMemoize instructionsfor the third-argument form, andExpected the dependency list to be an array of simple expressionsfor the three dependency-list forms (1.0.0 throws from DropManualMemoization, one pass earlier than main).The two FinishMemoize checks. No known input reaches
Expected FinishMemoize to align with previous StartMemoize instruction(validate_no_set_state_in_render.rs:103) orFound FinishMemoize without corresponding StartMemoize(validate_exhaustive_dependencies.rs:787). A nested input always fails the StartMemoize check first, andvalidate_exhaustive_dependenciestolerates nesting. They are the same pairing invariant, so they change with it. To exercise the new paths I forced each comparison to fail in a local build (selected by an environment variable, not committed). Each timebun buildexited 0, the debug log showed the invariant for the function with theuseMemocalls, and the other function in the file still compiled.validate_preserved_manual_memoization.rsalready returns early on both conditions.Not changed here. The other
assert!/.expect()/unreachable!sites insrc/react_compilerstay as they are. No known input reaches them, and most needResultplumbing through a pass that returns(). Invalidation/these arevalidate_exhaustive_dependencies.rs:187and:326andvalidate_locals_not_reassigned_after_render.rs:262. #42378 lists the ones inreactive_scopes/andinference/. A stack overflow or an allocation failure still ends the process.What the tests check. Each test bundles one file with
--target=browserthrough the CLI and runs the output against a fakereact/compiler-runtimewhoseccounts its calls. A compiled function callsconce per render, so the count is 1 for a compiled function and 0 for a skipped one. The try test also definesprocess.env.NODE_ENVas"production"for the production-only form.windowis not defined when the output runs, so a function that is left as written takes its catch path (off,ReferenceError). Each of the 8 skipped functions aborts 1.4.3 canary on its own. The controls (a call in the try body, a plain component, two sequentialuseMemocalls) make 1 call each, so the skip is per function and not per file.Sweep. Built all 1806 source files under
test/bundler/transpiler/react-compiler-fixtures/with--target=browserand--target=bunon 1.4.3 canary. None aborts with either message, so no upstream fixture has these shapes.Suites run with the debug build:
test/bundler/transpiler/react-compiler.test.ts(60 pass),test/bundler/transpiler/react-compiler-fixtures.test.ts(3293 pass, 320 skip).cargo clippy -p bun_react_compilerandcargo fmt --checkare clean.cargo check -p bun_react_compiler --releasepasses.Self-reviewed: the review raised no code concern. It asked for the provenance line and the "Not changed here" paragraph above.
[human-review] gate passed · iteration 0 · 8 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