Repository navigation
react-compiler: keep operands in source order around a statement inside their expression - #42859
Merged
Jarred-Sumner merged 3 commits intoSep 16, 2026
Conversation
Collaborator
Author
|
Status How I reproduced it, on main ( mkdir -p node_modules/react && echo '{"name":"react","main":"index.js"}' > node_modules/react/package.json && : > node_modules/react/index.js
echo 'export const jsxDEV = (type, props) => ({ type, props });' > node_modules/react/jsx-dev-runtime.js
echo 'export const c = n => new Array(n).fill(Symbol.for("react.memo_cache_sentinel"));' > node_modules/react/compiler-runtime.js
cat > case.jsx <<'JSX'
class Box { constructor(v) { this.v = v; } }
const list = (...a) => a;
function A(p) { const r = { n: p.n }; let i = 1; const el = <a title={(++r.n && (i += 2))} id={[i, p.n]}>{(p.flag ? r.n : (i += 2))}</a>; return <div>{JSON.stringify([el.props, i, r.n])}</div>; }
function C(p) { const r = { n: p.n }; const a = list(-r.n, new Box(++r.n).v, (--r.n, 2)); return <div>{JSON.stringify([a, r.n])}</div>; }
for (const F of [A, C]) console.log(F.name, F({ n: 2, flag: true }).props.children);
JSX
bun build case.jsx --target=browser --outfile=plain.js && bun plain.js
bun build case.jsx --target=browser --react-compiler --outfile=rc.js && bun rc.jsThe build without the compiler prints The new tests |
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 (4)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughThe compiler now tracks observable effects when promoting temporaries, preserves FBT operands as constants, and models terminal evaluation order. Bundler regressions cover these cases. The mutable ChangesTemporary promotion
IdMap cleanup
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No actionable behavior or operational issue is established, so the change is ready for normal merge checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
Collaborator
Author
|
Updated 9:14 PM PT - Sep 15th, 2026
⏳ @robobun, your commit a7c5477 is still building in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…de their expression Codegen prints an instruction with an unnamed result inside the statement that uses it, and every other instruction as a statement where it stands. A statement in the middle of an expression then runs ahead of the earlier operands that are still inline. PromoteInterposedTemporaries names those operands, with three gaps: - It tracked only loads, calls and stores. An expression over a load (`-r.n`, `[r.n]`) or a ternary, `&&`, `?.` or comma stayed inline. - Only a statement with a side effect raised the flag. A statement that reads (a memo block that compares its dependencies, an operand with a name) did not, so an earlier inline store printed after it. - A store inside a comma operand raised the flag, although it prints in place. That named a later operand ahead of an earlier inline one, or named a temporary inside a value block, which codegen rejects. The pass now gives every instruction a position and every temporary the effect (none, read, write) of the whole expression it prints as. A use promotes the temporary when a statement after its definition conflicts with it. The promoted temporary counts as a statement at its own position, and the operands of one instruction are visited last-defined first, so the operands before it follow. A statement inside a value block counts only for the uses inside that block. A switch case test, a memoization marker and a macro (fbt) operand are never promoted.
…nd name the values inside them The set of macro operands went into `consts`, which also decides whether a load of a local reads state. A `let` captured by a function inside an fbt call then counted as a constant everywhere in the component. The set is now separate and only the promotion check reads it. The set was also every operand of every macro call. It is now the operands that have to print inside the call (the tagged ones of MemoizeFbtAndMacroOperandsInSameScope). The value given to `fbt.param()` is not one of them and keeps its name. When an operand that has to stay inline conflicts with a statement, the operands inside it get the names. A destructuring declaration is a write like every other destructuring: an array pattern runs an iterator.
robobun
force-pushed
the
robobun/8179f182/react-compiler-interposed-operand-order
branch
from
September 16, 2026 03:58
cd9090c to
136b90d
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No new blocking issues. 1 optional suggestion (a nit or a note on pre-existing code) was found and not posted. Nothing in this review needs a push before merging.
One verified lower-impact observation (a convention, logging or cleanup point) was not posted.
Jarred-Sumner
deleted the
robobun/8179f182/react-compiler-interposed-operand-order
branch
September 16, 2026 04:51
This was referenced Sep 16, 2026
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, one expression can run its operands out of source order. The value is silently wrong:list(-r.n, (() => { r.n = 10; return r.n; })())gives[-10,10], not[-2,10]. main and 1.4.3.codegen_instruction,src/react_compiler/codegen.rs:1630). A statement in the middle of an expression then runs ahead of the earlier inline operands.PromoteInterposedTemporaries(src/react_compiler/reactive_scopes/promote_used_temporaries.rs) names those operands. It knows only loads, calls and stores, and only statements with a side effect. It takes a store inside a comma operand for a statement. Upstream has the same gaps.Fix
test/bundler/transpiler/react-compiler.test.ts(3 new tests fail on main),react-compiler-fixtures.test.ts,bundler_jsx.test.ts.Background
&&,?.or comma. It prints as one expression, so it cannot hold a declaration.if ($[0] !== dep) {...} else {...}. Only client mode has them.Notes
The three gaps, with the repro from the report (
p = { n: 2, flag: true },r = { n: p.n }):<a title={++r.n && (i += 2)} id={[i, p.n]}>{p.flag ? r.n : (i += 2)}</a>(client)id: [3,2]id: [1,2]: the memo block of the array readsiahead of the inline&&<a title={[list(r.n), ++r.n]} id={i}>{new Box(`${p.flag ? r.n : r.n}:${m[i++ % 3]}`).v}</a>(client)children: "3:6""2:6": the template has a name (a memo block depends on it) and readsr.nahead of the inline storelist(-r.n, new Box(++r.n).v, (--r.n, 2))(both modes)[-2,3,2][-3,3,2]: the store in the comma operand namesnew Box(++r.n).v, which moves ahead of-r.nThe third one now prints fully inline. The first two print
const t1 = (r.n = r.n + 1) && (i = 3);andconst t1 = [list(r.n), r.n = r.n + 1];ahead of the statement.Why positions and not the upstream flag. Upstream keeps one needs-promotion flag per tracked temporary and sets all of them at a statement with a side effect. A use by another temporary (
-r.nuses the load ofr.n) happens before the flag, so the load stays inline, and the unary was never tracked. With every kind tracked, the temporary that is still unused when the statement prints is the one that gets the name, together with the expression inside it. The position compare also drops the loop over all tracked temporaries at each statement, which was quadratic in the size of the function.Effects. Write: calls,
new, tagged templates,await, stores, deletes, updates, every destructuring (an array pattern runs an iterator), iterator steps. Read: a load of a local that is reassigned, a property load that is not on a global, a spread, and an operator (+, unary minus, a template literal,in) over an operand whose inferred type is not primitive, because the conversion reads the object. A declaration writes a variable that nothing earlier can read. A temporary inherits the effects of the unnamed operands inside it. The model has no aliasing: any write conflicts with any read. That is what upstream does for the kinds it tracks.Memo block entry. It counts as a read only when a dependency has a property path or is a local that is reassigned.
$[0] !== lastnameon a constant cannot change, so a pending call is not named for it.Value blocks. A store with no result inside
(--r.n, 2)prints in place inside that expression. It is a statement for the uses inside the same value block, where a name is a declaration that codegen rejects ("Cannot declare variables in a value block", the function stays uncompiled, as before). It is no longer a statement for the operands around the block. In 2,000 generated components with control flow, 94 that main leaves uncompiled for that reason now compile, for examplefor (let k = 0; k < 2; k += (--r.n, 1)). No generated component that compiles on main bails on this branch.Not named on purpose.
case bump(r):ahead of the discriminant. The test has this form as a control.StartMemoizeandFinishMemoize. Codegen prints nothing for them, so they do not evaluate their operand. TreatingFinishMemoizeas a use named everyuseMemoresult.initandtestof afor..ofprint the collection once, so they count as one statement.fbt.param(...)call (upstream fixturefbt/repro-fbt-param-nested-fbt-jsx). Upstream never names one there, because a JSX statement does not raise its flag. Here the firstfbt.param()call is pending when the memo block of the second element readsprops.lastname, and without an exemption the output isconst t2 = fbt.param("firstname", t1); ... fbt(["Name: ", t2, ...]).MemoizeFbtAndMacroOperandsInSameScopenow returns, next tomacroValues, the operands it tags: the ones that have to print inside the macro call. Only the promotion check reads that set. A value given tofbt.param()is not in it and gets a name as before. When a tagged operand conflicts with a statement, the operands inside it get the names:fbt(["a", fbt.param("n", r.n), fbt.param("m", (() => { r.n = r.n * 10; return r.n; })())], "d")printsconst t1 = r.n; r.n = r.n * 10; fbt(["a", fbt.param("n", t1), ...]). main hoists the wholefbt.param("n", r.n)call there. The pass tags nothing in ssr mode (it needs reactive scopes), so ssr output is as on main.Differential runs. A seeded generator of components whose expressions mix reads and writes of the same state (a
let, a capturedlet, a member, an array): unary, binary, template, array, object,new, JSX, spread,in, ternary,&&,||,??,?., comma, calls, prefix and postfix updates, assignments, destructuring assignment,delete, inside a declaration, JSX attributes,if, loops,switch,try, a labeled block, an early return, an inlined IIFE and auseMemocallback. Each component is built without the compiler and for both targets, and rendered four times with a fresh memo cache and four times with a kept one. The counts are results that differ from the build without the compiler.r.n++The 1 is a
letthat is reassigned inside a memo block and is stale on a cache hit. main has it too, and #42482 is open for it.Upstream fixtures. I built every fixture that builds (1,585 in client mode, 1,622 in ssr mode), with and without
minify.syntax, on main and on this branch, and compared the files. One differs, in ssr mode:jsx-tag-evaluation-order-non-globalprintsjsxDEV(Tag, { children: [(Tag = props.alternateComponent, maybeMutate(maybeMutable)), jsxDEV(Tag, {})] })where main printsconst T0 = Tag;first. The store in the comma operand named the tag on main. The tag is the first argument, so it still reads the oldTag. No slot count changes.Not fixed here.
{ ...r, a: (delete r.k) }when the later operand has a name. HIR reads a spread when the object is created, after all operands. That needs a change in lowering.x.y++andx += 1in place in a nested function #42630).++on a string is lowered tox + 1(upstream desugaring).IdMap::values_muthad no other caller, and the crate denies dead code.