Repository navigation
Conversation
PruneMaybeThrows nulls the handler of a MaybeThrow block that cannot throw and then cleans up the graph. After SSA the cleanup left stale data behind: - The catch block kept a phi operand for the pruned block. The phi rewrite raised "Expected non-existing phi operand's predecessor to have been mapped to a new terminal" and the function was not compiled. - A phi after a dead try kept the operand of a removed catch block. The merge asserted on it, or the phi rewrite moved it onto another removed block and InferReactivePlaces panicked. - When a whole try body was dead and its catch body had more than one block, the block after the try was not merged into the try body. BuildReactiveFunction ignores the goto to it, so the rest of the function was lost. Remove the operand when the handler edge is removed. Raise the invariant of the phi rewrite before the merge for an operand of a removed block, so that function is skipped. In BuildReactiveFunction, raise an invariant for a try goto whose target nothing scheduled, so that function is skipped too.
|
Status: ready for review. The gate passed: the three CI (build 119075): 179 of 181 jobs passed, and How I reproduced it (main at 26e7a4b and the 1.4.3-canary.1 release binary, linux x64): // skip.jsx: skipped function
export function Skip({ json }) {
let d = [];
try {
const max = 3;
d = parse(json, max);
} catch {}
return <List items={d} />;
}
// abort.jsx: build abort
export function Abort(props) {
let ok = true;
try {
props.store.ready;
} catch {
ok = false;
}
return <div>{ok ? props.a : null}</div>;
}
Tests: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. WalkthroughThe compiler now validates try/catch control-flow cleanup, removes stale handler phi operands, and reports unscheduled try fallthrough blocks. Regression tests cover dead try bodies, nested catches, preserved reachability, cache sizes, and stale phi operands. ChangesTry/catch pruning
Suggested reviewers: Priority: ➖ Normal 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 7:02 PM PT - Sep 20th, 2026
❌ @robobun, your commit 7750323 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 43658That installs a local version of the PR into your bun-43658 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
This branch has not been deployed
1 participant
Problem
trybody makesbun build --react-compilerskip the function, abort, or drop code. Skip:Invariant: Expected non-existing phi operand's predecessor to have been mapped to a new terminalforlet d = []; try { const max = 3; d = parse(json, max); } catch {}(Babel 1.0.0 compiles it).panic: called Option::unwrap() on a None value(infer_reactive_places.rs:410),Found a block with a single predecessor but where a phi has multiple (2) operands. Dropped code:try { const isProd = process.env.NODE_ENV === "production"; if (isProd) track(props); } catch (e) { props.onError?.(e); } return <div />printsfunction Comp(props) { ; }in a development build.src/react_compiler/optimization/prune_maybe_throws.rs) keeps phi operands of edges and blocks it removed. react-compiler: keep the statements after a dead try body with a branching catch #43603 owns the dropped code.Fix
trygoto whose target nothing scheduled: a skip, not lost code. The function compiles once react-compiler: keep the statements after a dead try body with a branching catch #43603 lands.DeadCodeInTryBody,DeadTryBodyKeepsTheCodeAfterIt,DeadTryBodyWithAStalePhiOperandIsSkippedintest/bundler/transpiler/react-compiler.test.ts(compiled variants fail on main). 1807 upstream fixtures and 2372 real-world files print identical output. Alsoreact-compiler-fixtures.test.ts.Background
try, every instruction ends its block with aMaybeThrowterminal that can go to the catch block (the handler). PruneMaybeThrows nulls the handler of a block that cannot throw. If dead code elimination leaves a wholetrybody unable to throw, the catch blocks are removed.tryterminal visits the block after thetry. Without the terminal, only a merge keeps that block.Notes
Provenance. Found by fuzzing
bun build --react-compiler, then reduced. There is no user report and no issue to close. The first report was the skipped function. The review of the first version of this change found that a fix for that alone lets a function with two of these shapes reach the abort or the dropped code, so this change makes each of them a skipped function at least.The three defects, with a small input for each.
let d = []; try { const max = 3; d = parse(json, max); } catch {} return <List items={d} />. Constant propagation folds the read ofmax, and dead code elimination removes its store. The pass nulls the handler of both blocks. The phi ofdin the catch block keeps their operands. The phi rewrite looks a stale predecessor up interminal_mapping, whose keys are continuations of pruned blocks, and returns the invariant. Every instruction that dead code elimination removes from atrybody does this: an unused local, a dead store, a folded constant, a guard that--definefolds (const dev = process.env.NODE_ENV !== "production"; if (dev) validate(json);in a production build). The catch block has a phi only when a local assigned in thetryflows through thecatchto a later read.try(let ok = true; try { props.store.ready; } catch { ok = false; }), the stale operand of the removed catch block reachesassert_eq!inmerge_consecutive_blocks.rs:106. In the nested form (try { try { props.store.ready; } catch { mode = "unsupported"; return null; } props.onLoad(); } catch {}) the removed block is the continuation of a literal block, so the phi rewrite finds a mapping and moves the operand onto another removed block.infer_reactive_places.rs:410then unwraps the control test of a block that does not exist. Upstream iterates the liveMap, visits the operand it moved, and raises the invariant. The port collects the updates first, so it never looked at the moved operand.trybody is dead (the production-only guard above in a development build, ortry { window.localStorage; }), so the catch blocks are removed andremove_unnecessary_try_catchturns thetryterminal into a goto. With more than one catch block (anif,?.,?:in the catch body) a removed block stays in the predecessors of the block after thetry, andmerge_consecutive_blocksdoes not merge it. The goto to that block isGotoVariant::Try, whichbuild_reactive_function.rsignores, so nothing visits the block. Atrynested in atryneeds only a one-statement catch:try { try { props.store.ready; } catch (e) { report(e); } a = props.f(); } catch { a = "caught"; }printedtry { ; } catch { a = "caught"; }.Related open PRs.
remove_unnecessary_try_catch, so the shapes of defect 3 compile. This change does not touch that function. It adds the check in BuildReactiveFunction because a function with defect 1 and defect 3 was skipped on main and would lose code with the first fix alone. With both changes the check never fires for these shapes. I built this branch plus thecfg_utils.rshunk of react-compiler: keep the statements after a dead try body with a branching catch #43603: the two tests of react-compiler: keep the statements after a dead try body with a branching catch #43603 and the tests here pass.DeadTryBodyKeepsTheCodeAfterItdoes not print cache sizes, so it passes before and after react-compiler: keep the statements after a dead try body with a branching catch #43603.assert_eq!of defect 2 into a skipped function. The check here runs before the merge and also covers theunwrap. All three changes are next to each other inprune_maybe_throws.rs, so the later ones to land have a small conflict. The check here makes the other two unreachable from this pass.Upstream.
UPSTREAM_PORTEDis 560db51408. Block ids in the errors match babel-plugin-react-compiler, so the port is faithful for 1 and 3.UnusedLocalInTry,ConstantLocalInTry,UnusedLocalInNestedTry,TwoLocalsInTry_c(5),_c(6),_c(5),_c(7)UnusedLogicalInTry(the fuzzer input)_c(2)CatchAssignsLocalassert_eq!)InnerCatchAssignsLocalunwrap)BranchingCatch,ProductionOnlyGuardfunction BranchingCatch(props) { ; }InnerTryInOuterTrya = props.f()1.0.0 rewrote a pruned terminal to a
goto, and MergeConsecutiveBlocks merged the continuation into the pruned block, which replaced the stale operand. facebook/react PR 35686 keeps themaybe-throwterminal and nulls its handler, so nothing merges.PruneMaybeThrows.tsandcompiler/crates/react_compiler_optimization/src/prune_maybe_throws.rson facebook/react main (59aff3e1) still have this logic.Why the predecessors are not recomputed before the merge. An earlier version called
mark_predecessorsbefore the merge, asconstant_propagation.rsdoes. With exact predecessors everywhere, the merge also takes the block after a deaddo ... whileinto the block of itsbreak, which main does not do in this pass. That merge is wrong inside atry(see "Existing defects" below), and it turneddo { if (p.a) break; try { p.store.ready; return null; } catch (e) { p.g(e); } break; } while (false);inside atryfrom skipped into wrong code. Now no predecessor set changes, and the upstream rewrite loop stays where it is. It finds nothing after the new check.Why a dead
trybody with a stale phi operand is skipped and not compiled.constant_propagation.rsdrops every phi operand whose predecessor is gone. That shape here would compilelet ok = true; try { window.localStorage; } catch { ok = false; }to a function whereokis alwaystrue, also where the load throws. A skip keeps the function as written. #42456 made the same choice, and its testDeadTryBodyWithCatchAssignmentSkipsOnlyThatFunctionpasses on this branch.Behavior changes for functions that compiled before.
let d = props.initial; try { const k = 3; d = 1; foo(); } catch {}: main moved the operand of the emptied store block onto the literal block before it, so the phi keptprops.initialand the element was memoized ond(_c(2)). Now the operand is removed and the output is the_c(1)of 1.0.0. Both are correct:dis 1 wherever thecatchcan run.let v = props.initial; try { try { const unused = 1; } catch { v = 1; return null; } props.c(); } catch {} return <span>{v}</span>compiled on main, with a phi operand keyed by a removed block, because the reactiveprops.initialmade InferReactivePlaces stop before the unwrap. Withlet v = "init"the same function aborted. Now both are skipped, as in Babel. To keep the first one compiling means to compile the whole family above.trythat is the last statement of anifor loop body, where the lost block is only the implicit jump that ends that body, and a deadtryin front of an empty loop. Such a function is skipped now, and compiles again with react-compiler: keep the statements after a dead try body with a branching catch #43603.Reach. 2372
.jsx/.tsxfiles that contain atry, from 14 open-source React apps (PostHog, lobe-chat, supabase, grafana, metabase, documenso, twenty, plane, formbricks, bluesky social-app, dub, mattermost, cal.com, excalidraw), each built alone with--react-compiler: no file hits one of the three defects on main, no file hits the new check, and every output is identical before and after, with--target=browser(1298 files have a memo cache) and with--target=bun(ssr mode). So this merges on severity, not on demand.Differential check. Five generated sets of
"use memo"functions (4200 in total) withtry/catch, nestedtry, deadtrybodies, loops, loops that leave on every path,switch, labels,break/continue, IIFEs, closures, value blocks, unused locals and constant locals. Each function was rendered 14 times with 6 prop sets on one kept cache, plain build against compiled build, on main and on this branch.184 of the 200 are right now: they lost code on main and are skipped now. 16 differ on both. 42 functions went from compiled to skipped for defect 3, and 26 of them were wrong on main. Of the 770 functions that render with a memo cache only on this branch, 12 differ from the plain build. Each of the 12 is an existing defect that main shows for the same shape without the dead code.
Existing defects that a newly compiled function can reach. This change does not cause them, but a function that main skipped for the invariant can now meet them.
catchdecides which one, is not reactive:let ok = false; try { const parsed = JSON.parse(json); const version = parsed.version; ok = true; validate(parsed); } catch (e) { report(e); }renders a staleokon a kept cache. Main does the same without theversionline. react-compiler: treat a throw into a catch handler as reactive control flow #42481 is the fix and should land before or with this change (3 of the 12).letreassigned in a nested memo block is not restored on a cache hit. react_compiler: fix stale values for aletreassigned inside a memo block #42482 is the fix (3 of the 12).do ... whilewhose body leaves on every path, inside atry.try { do { if (p.a) break; return null; } while (p.c); p.f(1); } catch {} return <div />printstry { if (p.a) p.f(1); return null; } catch {}on main, and babel-plugin-react-compiler 1.0.0 prints the same.remove_dead_do_while_statementsreplaces theDoWhileterminal with a goto, MergeConsecutiveBlocks then merges the block after the loop into the block ofbreak, and theGotoVariant::Tryat the end of that chain is ignored in a position that is not the tail of the try block. No PR addresses this yet (6 of the 12, all from the set with such loops).[human-review] gate passed · iteration 0 · 3 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