From 7629beee94e231f617488cf210fc44a4d71b6b56 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 15 Sep 2026 22:57:02 +0000 Subject: [PATCH 1/2] react_compiler: fix stale values for a `let` reassigned inside a memo block A `let` that is declared before a reactive scope and reassigned inside it is an output of the scope. The scope stores it after the computation and restores it when the cache hits. Several shapes gave a stale value. Upstream (babel-plugin-react-compiler 1.0.0) emits the same code for each, see facebook/react#37224. - A path through the scope that does not reassign the variable leaves it with the value it had on entry. Only a phi reads that value, so it was not a dependency, and a cache hit restored the value of an older render. A phi in a scope now makes an operand from before the scope a dependency. - A read of a phi that sits inside the scope was a dependency, dated by the first declaration of the variable. Codegen reads a dependency by name before the scope, so it read the value on entry, not the value after the join, and could throw on a property of null. A phi is now recorded in `reassignments`, so such a read is no longer a dependency. - A dependency on a variable that the scope reassigns was stored after the computation, so the slot held the new value and the next comparison tested the value on entry against it. Codegen now copies such a dependency to a const before the scope, and compares and stores the copy. - `x++` and `--x` did not register `x` as a reassignment of the scope. - A reassignment in a nested or pruned scope was recorded on that scope only. The enclosing scope did not restore the variable when its own cache hit. No pass and no per-function table is added. The phi handling is the body of the loop over `block.phis` that `handle_function_deps` already had. --- .claude/skills/sync-react-compiler.md | 8 +- scripts/sync-react-compiler.sh | 6 + src/react_compiler/DESIGN.md | 69 ++ src/react_compiler/codegen.rs | 52 +- .../inference/infer_reactive_places.rs | 4 + .../propagate_scope_dependencies_hir.rs | 92 ++- .../prune_non_reactive_dependencies.rs | 3 +- .../bundler/transpiler/react-compiler.test.ts | 602 ++++++++++++++++++ 8 files changed, 826 insertions(+), 10 deletions(-) diff --git a/.claude/skills/sync-react-compiler.md b/.claude/skills/sync-react-compiler.md index 75d6e2c1653f..7cb1275c6ebc 100644 --- a/.claude/skills/sync-react-compiler.md +++ b/.claude/skills/sync-react-compiler.md @@ -44,7 +44,10 @@ the porting reference for the AST-boundary files. 2. **Apply whole-crate diffs mechanically.** For each hunk under the whole-crate section, apply it to the corresponding `src/react_compiler//` file. The only systematic edit is import paths (`react_compiler_hir::` → - `crate::hir::`, etc.); everything else lands verbatim. + `crate::hir::`, etc.); everything else lands verbatim. A few sites fix + miscompiles that upstream still has. They carry a "Not in upstream" comment, + and the script lists them at the end of its output. DESIGN.md says why each + exists, under "Behaviour that differs from upstream". Keep them. 3. **Re-port AST-boundary diffs by hand.** For each hunk under the AST-boundary section, re-port it into the named Bun file using the @@ -54,7 +57,8 @@ the porting reference for the AST-boundary files. `react_compiler_ast::statements::Statement::Foo { … }`, the Bun port calls `Stmt::alloc(S::Foo { … }, loc)`. Keep control flow, pass ordering, variable names, and comments 1:1 with upstream — only the AST reads/writes - change. + change. The "Not in upstream" rule from step 2 applies here too: `codegen.rs` + has such a site (`codegen_reactive_scope`). Keep it. For large diffs, fan out one agent per file with the upstream diff + the Bun port + DESIGN.md as context, then adversarially review each port. diff --git a/scripts/sync-react-compiler.sh b/scripts/sync-react-compiler.sh index cb0e4e9a6125..7eb03768f7f5 100755 --- a/scripts/sync-react-compiler.sh +++ b/scripts/sync-react-compiler.sh @@ -129,4 +129,10 @@ git -C "$tmp" grep -l '\breact_compiler_ast\b' -- 'compiler/crates/*/src/*.rs' \ || echo "(none)" echo +# See "Behaviour that differs from upstream" in src/react_compiler/DESIGN.md. +echo "### sites that fix what upstream still gets wrong (keep them) ###" +git -C "$repo_root" grep -n 'Not in upstream' -- src/react_compiler ':!src/react_compiler/DESIGN.md' \ + || echo "(none)" +echo + echo "When porting is done: echo $new > $ported_file" diff --git a/src/react_compiler/DESIGN.md b/src/react_compiler/DESIGN.md index 285c66f33d32..1beda0c0e52b 100644 --- a/src/react_compiler/DESIGN.md +++ b/src/react_compiler/DESIGN.md @@ -66,6 +66,75 @@ Keep the diff between upstream and the port as small as the type substitution allows — the `/sync-react-compiler` skill re-ports upstream changes hunk by hunk, so gratuitous restructuring makes that harder. +### Behaviour that differs from upstream + +A few sites fix miscompiles that upstream still has. Each one carries a +one-line comment that starts with "Not in upstream": + +```sh +git grep -n "Not in upstream" src/react_compiler +``` + +Keep them on a sync. `react-compiler-fixtures.test.ts` cannot tell whether they +survived: it compiles the upstream fixtures in `infer` mode, which skips most +of their functions, and none of these sites changes what Bun emits for one. The +tests in `test/bundler/transpiler/react-compiler.test.ts` that run compiled +components can. When upstream fixes the same case, run Bun's test for it +against upstream's fix before you drop Bun's. + +The code comments are short on purpose. The reasons for a group of sites go +here. + +#### A `let` declared before a reactive scope and reassigned inside it + +facebook/react#37224. The scope stores the variable after the computation and +restores it when the cache hits, so the variable is an output of the scope. +`react-compiler/ReassignedLocalDeclaredBeforeMemoBlock` runs a component for +each point below. + +- `visit_phi` (`propagate_scope_dependencies_hir.rs`). A path that does not + reassign the variable leaves it with the value it had on entry. Only a phi + reads that value, and upstream visits instruction operands only. So the value + is not a dependency, and a cache hit restores the value of an older render. + `visit_phi` runs in the block loop that already walks the phis. It makes an + operand a dependency when `reassignments` dates it before the current scope. + An operand that `reassignments` does not hold yet comes from a loop back + edge, so its definition is later in the scope. +- `visit_phi` also records the phi itself in `reassignments`, dated just before + its block, with an empty scope stack (nothing reads the scope stack of a + `reassignments` entry). Upstream never records a phi, so + `check_valid_dependency` dates a read of one by the first declaration of its + variable, and a read of a phi inside the scope becomes a dependency. Codegen + reads a dependency by name before the scope, where the variable still holds + the value on entry. That is the wrong value, and it throws when the read is + `u.profile.name` and the value on entry is null. +- `apply_reactive_flags_replay` (`infer_reactive_places.rs`) flags every phi + operand whose identifier is reactive. Upstream stops at the first reactive + operand of a phi, so the flag of a later operand can be unset. `visit_phi` + copies that flag into the dependency. +- `prune_non_reactive_dependencies.rs`, `|| dep.reactive`. The value on entry + can be a parameter, or a phi that only another phi reads. No place in the + reactive function carries that identifier, so the set the pass builds never + holds it. +- `exit_scope`. Upstream records a reassignment on the innermost scope. When the + cache of an enclosing scope hits, the inner scope does not run, so the + enclosing scope has to restore the variable too. +- `handle_instruction`, `PrefixUpdate` and `PostfixUpdate`. Upstream visits only + `value`, so `x++` is not a reassignment and the scope does not restore `x`. +- `codegen_reactive_scope` (`codegen.rs`). The cache stores run after the + computation, so a dependency on a variable the scope reassigns stored the new + value, and the next comparison tested the value on entry against it. The + dependency is copied to a `const` before the scope, and the comparison and + the store use the copy. `rename_variables` has run by then, so + `fresh_temporary_name` picks the first unused `t`. + +The open upstream fix, facebook/react#37273, adds the same dependency. It +differs in three ways. It tests `operand.reactive` without the change to +`apply_reactive_flags_replay`. It stores the dependency before the computation, +which breaks the rule that the cache never holds inputs without outputs (React +shares the cache between render attempts). It does not pass a reassignment to +the enclosing scope. + ### Type mapping (input: lowering) | upstream `react_compiler_ast` | `bun_ast` | diff --git a/src/react_compiler/codegen.rs b/src/react_compiler/codegen.rs index 473b14c9d2ed..66cdac7a8668 100644 --- a/src/react_compiler/codegen.rs +++ b/src/react_compiler/codegen.rs @@ -452,6 +452,22 @@ impl<'a, 'h> Context<'a, 'h> { validated } + /// The first `t` that no identifier uses, for a temporary that only codegen creates. + fn fresh_temporary_name(&mut self) -> String { + let mut itoa = bun_core::fmt::ItoaBuf::new(); + let mut name = String::new(); + for index in 0u32.. { + name.clear(); + name.push('t'); + name.push_str(itoa.format(index)); + if !self.unique_identifiers.contains(&name) { + break; + } + } + self.unique_identifiers.insert(name.clone()); + name + } + fn record_error(&mut self, detail: CompilerErrorDetail) -> Result<(), CompilerError> { self.env.record_error(detail) } @@ -680,17 +696,49 @@ fn codegen_reactive_scope( for dep in &deps { let index = cx.alloc_cache_index(); + let dep_decl = cx.env.identifiers[dep.identifier.0 as usize].declaration_id; + // Not in upstream: the stores run last, so a dependency the scope reassigns is copied first. + let value_on_entry = if scope_reassignments + .iter() + .any(|id| cx.env.identifiers[id.0 as usize].declaration_id == dep_decl) + { + let name = store_str(cx.fresh_temporary_name().as_bytes()); + let copy_ref = cx.cg.ref_for_name(name); + statements.push(Stmt::alloc( + S::Local { + kind: S::Kind::KConst, + decls: decl_list([G::Decl { + binding: Binding::alloc( + cx.cg.arena, + b::Identifier { r#ref: copy_ref }, + loc, + ), + value: Some(codegen_dependency(cx, dep)?), + }]), + ..Default::default() + }, + loc, + )); + Some(name) + } else { + None + }; + let dep_expr = |cx: &mut Context| match value_on_entry { + Some(name) => Ok(cx.cg.ident_expr(name, loc)), + None => codegen_dependency(cx, dep), + }; + let comparison = Expr::init( E::Binary { op: OpCode::BinStrictNe, left: cache_slot(index), - right: codegen_dependency(cx, dep)?, + right: dep_expr(cx)?, }, loc, ); change_exprs.push(comparison); - let dep_value = codegen_dependency(cx, dep)?; + let dep_value = dep_expr(cx)?; cache_store_exprs.push(Expr::init( E::Binary { op: OpCode::BinAssign, diff --git a/src/react_compiler/inference/infer_reactive_places.rs b/src/react_compiler/inference/infer_reactive_places.rs index 3ea70884de56..b4c2c3f77d10 100644 --- a/src/react_compiler/inference/infer_reactive_places.rs +++ b/src/react_compiler/inference/infer_reactive_places.rs @@ -581,6 +581,10 @@ fn apply_reactive_flags_replay( operand.reactive = true; } } + // Not in upstream: the fixpoint stops at the first reactive operand. Flag the rest. + if reactive_ids[operand.identifier.0 as usize] { + operand.reactive = true; + } } } diff --git a/src/react_compiler/inference/propagate_scope_dependencies_hir.rs b/src/react_compiler/inference/propagate_scope_dependencies_hir.rs index 08116abb4acd..699361710809 100644 --- a/src/react_compiler/inference/propagate_scope_dependencies_hir.rs +++ b/src/react_compiler/inference/propagate_scope_dependencies_hir.rs @@ -1781,11 +1781,72 @@ impl<'a> DependencyCollectionContext<'a> { } } + // Not in upstream: the enclosing scope has to restore what this scope reassigns. + if let Some(parent) = self.current_scope() { + let parent_start = env.scopes[parent.0 as usize].range.start; + for index in 0..env.scopes[scope_id.0 as usize].reassignments.len() { + let id = env.scopes[scope_id.0 as usize].reassignments[index]; + let decl_id = env.identifiers[id.0 as usize].declaration_id; + let declared_before_parent = self + .declarations + .get(decl_id) + .is_some_and(|decl| decl.id < parent_start); + let already = env.scopes[parent.0 as usize] + .reassignments + .iter() + .any(|other| env.identifiers[other.0 as usize].declaration_id == decl_id); + if declared_before_parent && !already { + env.scopes[parent.0 as usize].reassignments.push(id); + } + } + } + if !pruned { self.deps.insert(scope_id, scoped_deps); } } + /// Not in upstream: a value from before the scope that reaches a phi in it is a dependency. + fn visit_phi( + &mut self, + phi: &crate::hir::Phi, + before_block: EvaluationOrder, + env: &mut Environment, + ) { + for (_pred_id, operand) in &phi.operands { + if let Some(maybe_optional_chain) = self.temporaries.get(operand.identifier) { + self.visit_dependency(maybe_optional_chain.clone(), env); + continue; + } + // An operand that is not declared yet comes from a loop back edge, later in the scope. + let (Some(scope), Some(decl)) = ( + self.current_scope(), + self.reassignments.get(operand.identifier), + ) else { + continue; + }; + if decl.id < env.scopes[scope.0 as usize].range.start { + self.visit_dependency( + ReactiveScopeDependency { + identifier: operand.identifier, + reactive: operand.reactive, + path: hir_vec![], + loc: operand.loc, + }, + env, + ); + } + } + // A read of this phi inside the scope it sits in is then not a dependency of that scope. + self.reassignments.insert( + phi.place.identifier, + Decl { + id: before_block, + scope_stack: Vec::new(), + }, + ); + } + fn current_scope(&self) -> Option { self.scope_stack.last().copied() } @@ -2134,6 +2195,25 @@ fn handle_instruction( ctx.visit_operand(&lvalue.place, env); ctx.visit_operand(val, env); } + // Not in upstream: `x++` reassigns `x`, so the scope has to restore it on a cache hit. + InstructionValue::PrefixUpdate { + lvalue, value: val, .. + } + | InstructionValue::PostfixUpdate { + lvalue, value: val, .. + } => { + ctx.visit_operand(val, env); + ctx.visit_reassignment(lvalue, env); + let scope_stack_copy = ctx.scope_stack.clone(); + ctx.declare( + lvalue.identifier, + Decl { + id, + scope_stack: scope_stack_copy, + }, + env, + ); + } _ => { // Visit all value operands let operands = visitors::each_instruction_value_operand(&instr.value, env); @@ -2207,11 +2287,13 @@ fn handle_function_deps( } // Record phi operands - for phi in &block.phis { - for (_pred_id, operand) in &phi.operands { - if let Some(maybe_optional_chain) = ctx.temporaries.get(operand.identifier) { - ctx.visit_dependency(maybe_optional_chain.clone(), env); - } + if !block.phis.is_empty() { + let first = match block.instructions.first() { + Some(instr_id) => func.instructions[instr_id.0 as usize].id, + None => block.terminal.evaluation_order(), + }; + for phi in &block.phis { + ctx.visit_phi(phi, EvaluationOrder(first.0.saturating_sub(1)), env); } } diff --git a/src/react_compiler/reactive_scopes/prune_non_reactive_dependencies.rs b/src/react_compiler/reactive_scopes/prune_non_reactive_dependencies.rs index 3f3678f1588b..aff3c41da6a1 100644 --- a/src/react_compiler/reactive_scopes/prune_non_reactive_dependencies.rs +++ b/src/react_compiler/reactive_scopes/prune_non_reactive_dependencies.rs @@ -214,9 +214,10 @@ impl<'a> ReactiveFunctionTransform for PruneVisitor<'a> { let scope_data = &mut self.env.scopes[scope_id.0 as usize]; // Remove non-reactive dependencies + // Not in upstream: `|| dep.reactive`, for a parameter or a phi that only a phi reads. scope_data .dependencies - .retain(|dep| state.contains(&dep.identifier)); + .retain(|dep| state.contains(&dep.identifier) || dep.reactive); // If any deps remain, mark all declarations and reassignments as reactive if !scope_data.dependencies.is_empty() { diff --git a/test/bundler/transpiler/react-compiler.test.ts b/test/bundler/transpiler/react-compiler.test.ts index a53221bd3769..27a8c300cceb 100644 --- a/test/bundler/transpiler/react-compiler.test.ts +++ b/test/bundler/transpiler/react-compiler.test.ts @@ -2790,6 +2790,608 @@ describe("bundler", () => { }, }); } + + // A `let` that is declared before a memo block and reassigned inside it is an + // output of the block: the block stores it after the computation and restores + // it when the cache hits. Every component below renders several times against + // one memo cache, and has to render what the uncompiled component renders. + // `*` marks a render that got the same `list` array as the render before it, + // so each line also shows that the block still hits its cache. + const reassignedLocalOutput = ` + ConditionalReassign [1,["x"]] [2,["x"]] [2,["x"]]* ["flagged",[]] + ReadThenReassign [2,["x"]] [3,["x"]] [3,["x"]]* + UpdateExpression [2,["x"]] [3,["x"]] [3,["x"]]* + CounterInLoop [1,[3]] [1,[3]]* [0,[1,2]] + NestedScope ["flagged",[[1],"x"]] ["flagged",[[1],"x"]]* ["none",[[1],"x"]] + PrunedNestedScope [true,[{"v":3,"w":1}]] [true,[{"v":3,"w":1}]] [true,[{"v":3,"w":1}]]* ["k",[{"v":1,"w":1},{"v":2,"w":1}]] + LoopReassign ["i",[1,2]] ["j",[1,2]] ["j",[1,2]]* [3,[3]] + JoinBeforeScope ["a1",["x"]] ["a2",["x"]] ["base",["x"]] ["base",["x"]]* + ChainedScopes ["1two",[[0],[]]] ["2two",[[0],[]]] ["2two",[[0],[]]]* ["onetwo",[[],[]]] + DependencyPath ["b",["a","b"]] ["b",["b","b"]] ["b",["b","b"]]* + DestructuringSwap ["2:1",[0]] ["1:2",[0]] ["1:2",[0]]* + ForOfOnly ["5,1,2",[5,1,2]] ["6,1,2",[6,1,2]] ["6,1,2",[6,1,2]]* ["none",null] + ForWithContinue ["5,1,2",[5,1,2]] ["6,1,2",[6,1,2]] ["6,1,2",[6,1,2]]* + useFormErrors [false,[[],[]]] [true,[["That name is already taken"],[]]] [true,[["That name is already taken"],[]]]* [true,[[],["Pick an X variable"]]] [true,[["That name is already taken"],["Pick an X variable"]]] + useBareParam [1,["b"]] [2,["b"]] [2,["b"]]* ["X",[]] + NullableEntryThenProp ["anon",["anon"]] ["ann",["ann"]] ["ann",["ann"]]* + BothArmsThenProp ["b",["b"]] ["b",["b"]]* ["c",["c"]] + TwoArrays ["v1",[3]] ["v1",[3]]* ["v2",[3]] + ThreeArrays ["v1",[3]] ["v1",[3]]* ["v2",[3]] + SameDependencies ["v1",[1]] ["v1",[1]]* ["v2",[2]] + DestructuringInNestedScope ["1:2",[3]] ["1:2",[3]]* ["2:2",[3]] + PrefixUpdateInNestedScope [-1,[3]] [-1,[3]]* [0,[3]] + NestedScopeWithEntryFromProp ["a",[3]] ["b",[3]] ["b",[3]]* ["off",[3]] ["off",[3]]* + DoWhileInsideScopeOfSibling ["v1",{}] ["v1",{}]* ["v2",{}] + SpreadInsideLoopsInsideScopeOfSibling [{"q":1},{}] [{"q":1},{}]* ["init",{}] + LoopInsideScopeOfSibling ["a",{}] ["b",{}] ["b",{}]* ["off",{}] ["off",{}]* + ClosureReassigns [1,["x"]] [2,["x"]] [2,["x"]]* ["flagged",[]] ["flagged",[]] ["flagged",[]]* + CompoundInTernaryTest [2,[1,2,2]] [4,[2,4,4]] [4,[2,4,4]]* [2,[3,2,2]] [4,[2,4,4]] + CompoundInArray [5,[6,3,3,5,5]] [4,[5,2,2,4,4]] [4,[5,2,2,4,4]]* + CompoundInIfTest [3,["big",3]] [2,["small",2]] [2,["small",2]]* + CompoundInJsxAttribute [2,{"title":8,"id":2}] [0,{"title":2,"id":0}] [0,{"title":2,"id":0}]* + CompoundInMapEntries [2,[[8,2],["after",2]]] [0,[[2,0],["after",0]]] [0,[[2,0],["after",0]]]* + PlainAssignmentInArray [2,[4,2,2]] [1,[2,1,1]] [1,[2,1,1]]* + CompoundStatement [2,[2]] [4,[4]] [4,[4]]* + UpdateInTernaryTest [2,[[1]]] [3,[[1]]] [3,[[1]]] + `; + for (const reactCompiler of [false, true]) { + itBundled(`react-compiler/ReassignedLocalDeclaredBeforeMemoBlock-${reactCompiler ? "compiled" : "plain"}`, { + files: { + "/entry.jsx": /* jsx */ ` + import { render } from "react"; + + // The else path leaves label with the value it had on entry, so that + // value is an input of the block although no instruction in it reads it. + function ConditionalReassign(p) { + let label = p.count; + const list = []; + if (p.flag) { + label = "flagged"; + } else { + list.push(p.b); + } + return
; + } + + // The block reads label, so it is a dependency. Its cache slot has to + // hold the value on entry, not the value after the reassignment. + function ReadThenReassign(p) { + let label = p.count; + const list = []; + if (p.flag) label = label + 1; + list.push(p.b); + return
; + } + + function UpdateExpression(p) { + let n = p.count; + const list = []; + if (p.flag) n++; + list.push(p.b); + return
; + } + + function CounterInLoop(p) { + let count = 0; + const list = []; + for (const item of p.items) { + if (item.ok) count++; + list.push(item.v); + } + return
; + } + + // inner has its own memo block inside the block of outer. When the + // outer cache hits, the inner block does not run. + function NestedScope(p) { + let label = "none"; + const outer = []; + const inner = []; + if (p.flag) label = "flagged"; + inner.push(p.a); + outer.push(inner); + outer.push(p.b); + return
; + } + + // The block of copy is in a loop, so the compiler prunes it. + function PrunedNestedScope(p) { + let hasOk = p.initial; + const list = []; + for (const item of p.items) { + const copy = { v: item.v }; + if (item.ok) hasOk = true; + copy.w = p.w; + list.push(copy); + } + return
; + } + + function LoopReassign(p) { + let found = p.initial; + const list = []; + for (const item of p.items) { + if (item.ok) found = item.v; + list.push(item.v); + } + return
; + } + + // The value on entry is itself the join of two paths, and nothing reads + // it between that join and the block. + function JoinBeforeScope(p) { + let label = "base"; + if (p.primary) label = p.a; + const list = []; + if (p.flag) label = "flagged"; + else list.push(p.b); + return
; + } + + function ChainedScopes(p) { + let label = p.count; + const first = []; + if (p.f1) label = "one"; + else first.push(p.x); + const second = []; + if (p.f2) label = label + "two"; + else second.push(p.y); + return
; + } + + function DependencyPath(p) { + let object = p.o; + const list = []; + list.push(object.x); + if (p.flag) object = p.o2; + list.push(object.x); + return
; + } + + function DestructuringSwap(p) { + let a = p.a; + let b = p.b; + const list = []; + if (p.flag) [a, b] = [b, a]; + list.push(p.x); + return
; + } + + // The memo block belongs to the loop, not to another value. + function ForOfOnly(p) { + let derived = [p.value, ...p.items]; + for (const item of p.items) { + if (item < 0) derived = null; + } + return
; + } + + const check = () => false; + function ForWithContinue(p) { + let derived = [p.value, ...p.items]; + for (let i = 0; i < 2; i++) { + if (i === 0) continue; + if (check()) derived = null; + } + return
; + } + + // facebook/react#37224. Each check has a memo block of its own, and both + // blocks restore hasErrors. "use memo" because the compiler only infers a + // hook from a call to another hook. + function useFormErrors(name, existingNames, settings) { + "use memo"; + let hasErrors = false; + + const nameErrors = []; + if (existingNames.includes(name)) { + hasErrors = true; + nameErrors.push("That name is already taken"); + } + + const settingsErrors = []; + if (settings.chartView === "histogram" && !settings.xVariableId) { + hasErrors = true; + settingsErrors.push("Pick an X variable"); + } + + return { label: hasErrors, list: [nameErrors, settingsErrors] }; + } + + // The value on entry is a parameter that nothing but the join reads. + function useBareParam(p, label) { + "use memo"; + const list = []; + if (p.flag) label = p.x; + else list.push(p.b); + return { label, list }; + } + + // user.profile.name is read from the value after the join. The value on + // entry can be null, so nothing may read that path before the block. + function NullableEntryThenProp({ user, fallbackUser }) { + let u = user; + const list = []; + if (!u) u = fallbackUser; + list.push(u.profile.name); + return
; + } + + // Both arms assign, so the value on entry does not reach the read at all. + function BothArmsThenProp(p) { + let object = p.a; + const list = []; + if (p.kind) object = p.b; + else object = p.c; + list.push(object.v.w); + return
; + } + + // The components from here on have a memo block inside another one, + // and the assignment is in the inner block. When the cache of the + // outer block hits, the inner block does not run, so the outer block + // has to restore the variable too. In the first two, every block + // survives to the output. + function TwoArrays(p) { + let v = "init"; + const outer = []; + const inner = []; + v = "v" + p.a; + inner.push(p.b); + outer.push(p.c); + return
; + } + + function ThreeArrays(p) { + let v = "init"; + const outer = []; + const middle = []; + const inner = []; + v = "v" + p.a; + inner.push(p.a); + middle.push(p.b); + outer.push(p.c); + return
; + } + + // Both blocks depend on p.a only, so the compiler merges the inner one + // into the outer one. + function SameDependencies(p) { + let v = "init"; + const outer = []; + const inner = []; + v = "v" + p.a; + inner.push(p.a); + outer.push(p.a); + return
; + } + + function DestructuringInNestedScope(p) { + let v = "init", u = "init"; + const outer = []; + const inner = []; + [v, u] = [p.a, p.b]; + inner.push(p.b); + outer.push(p.c); + return
; + } + + function PrefixUpdateInNestedScope(p) { + let v = 0; + const outer = []; + const inner = []; + if (p.a === 1) --v; + inner.push(p.b); + outer.push(p.c); + return
; + } + + // The value on entry is a prop, and the outer block is the one that + // has to depend on it. + function NestedScopeWithEntryFromProp(p) { + let v = p.v; + const outer = []; + const inner = []; + if (p.off) v = "off"; + inner.push(p.b); + outer.push(p.c); + return
; + } + + // The block of w runs from its declaration to the end of the for-of. + // The do-while counter j has a block of its own inside it. The + // compiler prunes that block later, because nothing it computes needs + // memoization. + function DoWhileInsideScopeOfSibling(p) { + let v = "init"; + let w = "w"; + let j = 0; + do { j++; v = "v" + p.a; } while (j < p.n); + for (const it of p.items) { w = {}; } + return
; + } + + // The block of the object spread is inside loops, so the compiler + // flattens it. + function SpreadInsideLoopsInsideScopeOfSibling(p) { + let v = "init"; + let w = "w"; + for (const k in p.obj) { + for (let i = 0; i < p.n; i++) { + switch (p.a) { + case 1: v = { ...p.obj }; break; + default: break; + } + } + } + for (const it of p.items) { w = {}; } + return
; + } + + function LoopInsideScopeOfSibling(p) { + let v = p.v; + let w = "w"; + for (const it of p.items) { if (p.off) v = "off"; } + for (const it of p.items) { w = {}; } + return
; + } + + // Right on 1.4.3 too. A local that a closure assigns is not in SSA form. + // Its declaration is part of the same memo block as the call. + function ClosureReassigns(p) { + let label = p.count; + const set = () => { + label = "flagged"; + }; + const list = []; + if (p.flag) set(); + else list.push(p.b); + return
; + } + + // From here on the block reads i and then assigns it, in one expression. + // Each run renders a value of n that equals the i the render before + // ended with, which is what the cache slot of i held. The compiler + // moves the assignments of CompoundInArray, CompoundInJsxAttribute, + // CompoundInMapEntries and PlainAssignmentInArray out of the block, so + // those four were right before. They guard the copy of i. + function CompoundInTernaryTest(p) { + let i = p.n; + const list = (i *= 1) > 2 ? [i, (i -= 1), i] : [i, (i *= 2), i]; + return
; + } + + function CompoundInArray(p) { + let i = p.n; + const list = [i, (i >>= 1), i, (i += 2), i]; + return
; + } + + function CompoundInIfTest(p) { + let i = p.n; + const list = []; + if ((i -= 1) > 2) list.push("big", i); + else list.push("small", i); + return
; + } + + function CompoundInJsxAttribute(p) { + let i = p.n; + const list = >= 2)} />; + return
; + } + + function CompoundInMapEntries(p) { + let i = p.n; + const list = [...new Map([[i, (i >>= 2)], ["after", i]])]; + return
; + } + + function PlainAssignmentInArray(p) { + let i = p.n; + const list = [i, (i = i >> 1), i]; + return
; + } + + function CompoundStatement(p) { + let i = p.n; + const list = []; + i *= 2; + list.push(i); + return
; + } + + // The block that assigns n has all of its instructions in the test of + // the conditional. The compiler leaves this function as written, so no + // render hits a cache. + const yes = () => true; + function UpdateInTernaryTest(p) { + let n = p.count; + const outer = []; + const inner = []; + const r = (n++, p.f()) ? 1 : 0; + inner.push(r); + outer.push(inner); + return
; + } + + function run(Component, ...renders) { + let previous; + const results = renders.map(args => { + try { + const { label, list } = render(Component, ...(Array.isArray(args) ? args : [args])); + const hit = list === previous ? "*" : ""; + previous = list; + return JSON.stringify([label, list]) + hit; + } catch (error) { + return "THROW(" + error.message + ")"; + } + }); + console.log(Component.name + " " + results.join(" ")); + } + + const none = [{ ok: false, v: 1 }, { ok: false, v: 2 }]; + const some = [{ ok: true, v: 3 }]; + const o1 = { x: "a" }, o2 = { x: "b" }, o3 = { x: "b" }; + + run( + ConditionalReassign, + { count: 1, flag: false, b: "x" }, + { count: 2, flag: false, b: "x" }, + { count: 2, flag: false, b: "x" }, + { count: 3, flag: true, b: "x" }, + ); + run( + ReadThenReassign, + { count: 1, flag: true, b: "x" }, + { count: 2, flag: true, b: "x" }, + { count: 2, flag: true, b: "x" }, + ); + run( + UpdateExpression, + { count: 1, flag: true, b: "x" }, + { count: 2, flag: true, b: "x" }, + { count: 2, flag: true, b: "x" }, + ); + run(CounterInLoop, { items: some }, { items: some }, { items: none }); + run( + NestedScope, + { flag: true, a: 1, b: "x" }, + { flag: true, a: 1, b: "x" }, + { flag: false, a: 1, b: "x" }, + ); + run( + PrunedNestedScope, + { initial: "i", items: some, w: 1 }, + { initial: "j", items: some, w: 1 }, + { initial: "j", items: some, w: 1 }, + { initial: "k", items: none, w: 1 }, + ); + run( + LoopReassign, + { initial: "i", items: none }, + { initial: "j", items: none }, + { initial: "j", items: none }, + { initial: "k", items: some }, + ); + run( + JoinBeforeScope, + { primary: true, a: "a1", flag: false, b: "x" }, + { primary: true, a: "a2", flag: false, b: "x" }, + { primary: false, a: "a2", flag: false, b: "x" }, + { primary: false, a: "a3", flag: false, b: "x" }, + ); + run( + ChainedScopes, + { count: 1, f1: false, f2: true, x: 0, y: 0 }, + { count: 2, f1: false, f2: true, x: 0, y: 0 }, + { count: 2, f1: false, f2: true, x: 0, y: 0 }, + { count: 3, f1: true, f2: true, x: 0, y: 0 }, + ); + run(DependencyPath, { o: o1, o2, flag: true }, { o: o3, o2, flag: true }, { o: o3, o2, flag: true }); + run( + DestructuringSwap, + { a: 1, b: 2, flag: true, x: 0 }, + { a: 2, b: 1, flag: true, x: 0 }, + { a: 2, b: 1, flag: true, x: 0 }, + ); + const positive = [1, 2]; + run( + ForOfOnly, + { value: 5, items: positive }, + { value: 6, items: positive }, + { value: 6, items: positive }, + { value: 7, items: [1, -2] }, + ); + run( + ForWithContinue, + { value: 5, items: positive }, + { value: 6, items: positive }, + { value: 6, items: positive }, + ); + const names = ["taken"]; + const bar = { chartView: "bar" }; + const histogram = { chartView: "histogram" }; + run( + useFormErrors, + ["free", names, bar], + ["taken", names, bar], + ["taken", names, bar], + ["free", names, histogram], + ["taken", names, histogram], + ); + const flagOff = { flag: false, x: "X", b: "b" }; + run(useBareParam, [flagOff, 1], [flagOff, 2], [flagOff, 2], [{ flag: true, x: "X", b: "b" }, 3]); + const ann = { profile: { name: "ann" } }; + run( + NullableEntryThenProp, + { user: null, fallbackUser: { profile: { name: "anon" } } }, + { user: ann, fallbackUser: null }, + { user: ann, fallbackUser: null }, + ); + const w = value => ({ v: { w: value } }); + const b1 = w("b"), c1 = w("c"); + run( + BothArmsThenProp, + { a: null, kind: 1, b: b1, c: null }, + { a: null, kind: 1, b: b1, c: null }, + { a: null, kind: 0, b: null, c: c1 }, + ); + const nested1 = { a: 1, b: 2, c: 3, n: 1, items: [1], obj: { q: 1 } }; + const nested2 = { ...nested1, a: 2 }; + run(TwoArrays, nested1, nested1, nested2); + run(ThreeArrays, nested1, nested1, nested2); + run(SameDependencies, nested1, nested1, nested2); + run(DestructuringInNestedScope, nested1, nested1, nested2); + run(PrefixUpdateInNestedScope, nested1, nested1, nested2); + const entryA = { v: "a", off: false, b: 2, c: 3, items: nested1.items }; + const entryB = { ...entryA, v: "b" }; + const entryOff = { ...entryB, off: true }; + run(NestedScopeWithEntryFromProp, entryA, entryB, entryB, entryOff, entryOff); + run(DoWhileInsideScopeOfSibling, nested1, nested1, nested2); + run(SpreadInsideLoopsInsideScopeOfSibling, nested1, nested1, nested2); + run(LoopInsideScopeOfSibling, entryA, entryB, entryB, entryOff, entryOff); + run( + ClosureReassigns, + { count: 1, flag: false, b: "x" }, + { count: 2, flag: false, b: "x" }, + { count: 2, flag: false, b: "x" }, + { count: 3, flag: true, b: "x" }, + { count: 4, flag: true, b: "x" }, + { count: 4, flag: true, b: "x" }, + ); + run(CompoundInTernaryTest, { n: 1 }, { n: 2 }, { n: 2 }, { n: 3 }, { n: 2 }); + run(CompoundInArray, { n: 6 }, { n: 5 }, { n: 5 }); + run(CompoundInIfTest, { n: 4 }, { n: 3 }, { n: 3 }); + run(CompoundInJsxAttribute, { n: 8 }, { n: 2 }, { n: 2 }); + run(CompoundInMapEntries, { n: 8 }, { n: 2 }, { n: 2 }); + run(PlainAssignmentInArray, { n: 4 }, { n: 2 }, { n: 2 }); + run(CompoundStatement, { n: 1 }, { n: 2 }, { n: 2 }); + run(UpdateInTernaryTest, { count: 1, f: yes }, { count: 2, f: yes }, { count: 2, f: yes }); + `, + // One memo cache per component, kept between renders, as a fiber keeps it. + "/node_modules/react/index.js": /* js */ ` + const fibers = new Map(); + let current; + exports.render = (Component, ...args) => { + current = fibers.get(Component); + if (!current) fibers.set(Component, (current = { cache: null })); + return Component(...args); + }; + exports.memoCache = size => + (current.cache ??= new Array(size).fill(Symbol.for("react.memo_cache_sentinel"))); + `, + "/node_modules/react/compiler-runtime.js": `exports.c = size => require("./index.js").memoCache(size);`, + "/node_modules/react/jsx-runtime.js": `exports.jsx = exports.jsxs = (type, props) => props;`, + "/node_modules/react/jsx-dev-runtime.js": `exports.jsxDEV = (type, props) => props;`, + "/node_modules/react/package.json": `{"name":"react","main":"./index.js"}`, + }, + reactCompiler, + target: "browser", + backend: "cli", + run: { stdout: reactCompiler ? reassignedLocalOutput : reassignedLocalOutput.replaceAll("*", "") }, + }); + } }); // Three passes kept one copy of their work per basic block or per nesting From 9a29b3f4a780ffc900d1ea9c6b66e8cbbd4060fb Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 15 Sep 2026 23:37:08 +0000 Subject: [PATCH 2/2] react_compiler: declare the copy of a reassigned dependency as a local Since #42463 a symbol that the compiled function declares goes through `ref_for_local`, so the renamer names it like any other local. The copy went through `ref_for_name`, which registers a module-level symbol. A copy named `t1` then shadowed an import that the bundler prints as `t1`, and the function read the copy. --- src/react_compiler/codegen.rs | 4 ++-- test/bundler/transpiler/react-compiler.test.ts | 14 ++++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/react_compiler/codegen.rs b/src/react_compiler/codegen.rs index 66cdac7a8668..7ed1f1de8a56 100644 --- a/src/react_compiler/codegen.rs +++ b/src/react_compiler/codegen.rs @@ -703,7 +703,7 @@ fn codegen_reactive_scope( .any(|id| cx.env.identifiers[id.0 as usize].declaration_id == dep_decl) { let name = store_str(cx.fresh_temporary_name().as_bytes()); - let copy_ref = cx.cg.ref_for_name(name); + let copy_ref = cx.cg.ref_for_local(name); statements.push(Stmt::alloc( S::Local { kind: S::Kind::KConst, @@ -724,7 +724,7 @@ fn codegen_reactive_scope( None }; let dep_expr = |cx: &mut Context| match value_on_entry { - Some(name) => Ok(cx.cg.ident_expr(name, loc)), + Some(name) => Ok(Expr::init_identifier(cx.cg.ref_for_local(name), loc)), None => codegen_dependency(cx, dep), }; diff --git a/test/bundler/transpiler/react-compiler.test.ts b/test/bundler/transpiler/react-compiler.test.ts index 27a8c300cceb..96e49b895b0c 100644 --- a/test/bundler/transpiler/react-compiler.test.ts +++ b/test/bundler/transpiler/react-compiler.test.ts @@ -2833,12 +2833,14 @@ describe("bundler", () => { PlainAssignmentInArray [2,[4,2,2]] [1,[2,1,1]] [1,[2,1,1]]* CompoundStatement [2,[2]] [4,[4]] [4,[4]]* UpdateInTernaryTest [2,[[1]]] [3,[[1]]] [3,[[1]]] + CopyNamedLikeAnExport [1,["imported"]] [2,["imported"]] [2,["imported"]]* `; for (const reactCompiler of [false, true]) { itBundled(`react-compiler/ReassignedLocalDeclaredBeforeMemoBlock-${reactCompiler ? "compiled" : "plain"}`, { files: { "/entry.jsx": /* jsx */ ` import { render } from "react"; + import { t1 as imported } from "./names"; // The else path leaves label with the value it had on entry, so that // value is an input of the block although no instruction in it reads it. @@ -3215,6 +3217,16 @@ describe("bundler", () => { return
; } + // The copy of label is the first free t, which is t1 here. The bundler + // prints the import under the name of its export, which is t1 too. + function CopyNamedLikeAnExport(p) { + let label = p.count; + const list = []; + if (p.flag) label = "flagged"; + else list.push(imported); + return
; + } + function run(Component, ...renders) { let previous; const results = renders.map(args => { @@ -3368,7 +3380,9 @@ describe("bundler", () => { run(PlainAssignmentInArray, { n: 4 }, { n: 2 }, { n: 2 }); run(CompoundStatement, { n: 1 }, { n: 2 }, { n: 2 }); run(UpdateInTernaryTest, { count: 1, f: yes }, { count: 2, f: yes }, { count: 2, f: yes }); + run(CopyNamedLikeAnExport, { count: 1, flag: false }, { count: 2, flag: false }, { count: 2, flag: false }); `, + "/names.js": `export const t1 = "imported";`, // One memo cache per component, kept between renders, as a fiber keeps it. "/node_modules/react/index.js": /* js */ ` const fibers = new Map();