diff --git a/src/react_compiler/codegen.rs b/src/react_compiler/codegen.rs index 71cbd8098eb0..a7c5e39ca580 100644 --- a/src/react_compiler/codegen.rs +++ b/src/react_compiler/codegen.rs @@ -1598,22 +1598,13 @@ fn emit_store( }, stmt_loc, ); - if let Some(ref lvalue_place) = instr.lvalue { - let is_store_context = matches!( - &instr.value, - ReactiveValue::Instruction(InstructionValue::StoreContext { .. }) - ); - if !is_store_context { - let ident = &cx.env.identifiers[lvalue_place.identifier.0 as usize]; - cx.temp.insert(ident.declaration_id, Some(expr)); + if instr.lvalue.is_some() { + // Not in upstream: a promoted result is read by name from a later memo block. + let stmt = codegen_instruction(cx, instr, expr)?; + if matches!(stmt.data, StmtData::SEmpty(_)) { return Ok(None); - } else { - let stmt = codegen_instruction(cx, instr, expr)?; - if matches!(stmt.data, StmtData::SEmpty(_)) { - return Ok(None); - } - return Ok(Some(stmt)); } + return Ok(Some(stmt)); } Ok(Some(expr_stmt(expr, stmt_loc))) } diff --git a/src/react_compiler/inference/propagate_scope_dependencies_hir.rs b/src/react_compiler/inference/propagate_scope_dependencies_hir.rs index 8c6b9652625c..08116abb4acd 100644 --- a/src/react_compiler/inference/propagate_scope_dependencies_hir.rs +++ b/src/react_compiler/inference/propagate_scope_dependencies_hir.rs @@ -1858,35 +1858,38 @@ impl<'a> DependencyCollectionContext<'a> { self.visit_dependency(dep, env); } - fn visit_dependency(&mut self, dep: ReactiveScopeDependency, env: &mut Environment) { - let ident = &env.identifiers[dep.identifier.0 as usize]; - let decl_id = ident.declaration_id; - - // Record scope declarations for values used outside their declaring scope - if let Some(original_decl) = self.declarations.get(decl_id) { - if !original_decl.scope_stack.is_empty() { - let orig_scope_stack = original_decl.scope_stack.clone(); - for &scope_id in &orig_scope_stack { - if !self.scope_stack.contains(&scope_id) { - // Check if already declared in this scope - let scope = &env.scopes[scope_id.0 as usize]; - let already_declared = scope.declarations.iter().any(|(_, d)| { - env.identifiers[d.identifier.0 as usize].declaration_id == decl_id - }); - if !already_declared { - let orig_scope_id = *orig_scope_stack.last().unwrap(); - let new_decl = crate::hir::ReactiveScopeDeclaration { - identifier: dep.identifier, - scope: orig_scope_id, - }; - env.scopes[scope_id.0 as usize] - .declarations - .push((dep.identifier, new_decl)); - } - } - } + /// A value used after its declaring scope ends is an output of that scope. + fn declare_outside_original_scope(&self, identifier: IdentifierId, env: &mut Environment) { + let decl_id = env.identifiers[identifier.0 as usize].declaration_id; + let Some(original_decl) = self.declarations.get(decl_id) else { + return; + }; + let Some(&orig_scope_id) = original_decl.scope_stack.last() else { + return; + }; + for &scope_id in &original_decl.scope_stack { + if self.scope_stack.contains(&scope_id) { + continue; + } + let scope = &env.scopes[scope_id.0 as usize]; + let already_declared = scope + .declarations + .iter() + .any(|(_, d)| env.identifiers[d.identifier.0 as usize].declaration_id == decl_id); + if !already_declared { + let new_decl = crate::hir::ReactiveScopeDeclaration { + identifier, + scope: orig_scope_id, + }; + env.scopes[scope_id.0 as usize] + .declarations + .push((identifier, new_decl)); } } + } + + fn visit_dependency(&mut self, dep: ReactiveScopeDependency, env: &mut Environment) { + self.declare_outside_original_scope(dep.identifier, env); // Handle ref.current access let dep = if crate::hir::is_use_ref_type( @@ -1915,6 +1918,8 @@ impl<'a> DependencyCollectionContext<'a> { } fn visit_reassignment(&mut self, place: &Place, env: &mut Environment) { + // Not in upstream: a store that dead_code_elimination.rs retains can be in a later scope. + self.declare_outside_original_scope(place.identifier, env); if let Some(current_scope) = self.current_scope() { let scope = &env.scopes[current_scope.0 as usize]; let already = scope.reassignments.iter().any(|id| { diff --git a/test/bundler/transpiler/react-compiler.test.ts b/test/bundler/transpiler/react-compiler.test.ts index 900d706ab54f..52dc54101cd1 100644 --- a/test/bundler/transpiler/react-compiler.test.ts +++ b/test/bundler/transpiler/react-compiler.test.ts @@ -1631,6 +1631,133 @@ describe("bundler", () => { }, }); + // With `let x` kept, it can sit inside the memo block of another scope: here + // between `const r = {}` and the last mutation of `r`. Only a read of `x` + // from a later scope hoisted the `let` out of that block. A store alone left + // it inside, and the store then printed outside the block: + // `ReferenceError: x is not defined`. + // + // With a reactive value (`x = p.n`) the store is a dependency of the memo + // block that reads it, under a promoted name. Codegen inlined the store into + // that block all the same: `if ($[2] !== t0)` with no `t0` declared. + itBundled("react-compiler/DeadStoreAfterTheScopeOfItsDeclaration", { + files: { + "/entry.js": /* js */ ` + import * as forms from "./forms"; + const calls = []; + const props = { bad: "{bad", n: 7, call: v => calls.push(v) }; + const lines = []; + for (const [name, form] of Object.entries(forms)) { + try { + calls.length = 0; + const out = JSON.stringify(form(props).p); + lines.push(name + "=" + out + (calls.length ? " calls=" + JSON.stringify(calls) : "")); + } catch (e) { + lines.push(name + " threw " + e); + } + } + console.log(lines.join("\\n")); + `, + "/forms.jsx": /* jsx */ ` + const Stub = () => null; + + export function Prop(p) { + const r = {}; + let x; + r.a = 1; + return ; + } + // The one read of \`x\` folds to 5. + export function FoldedRead(p) { + const r = {}; + let x = 0; + r.a = 1; + return ; + } + export function Argument(p) { + const r = {}; + let x; + r.a = 1; + p.call((x = 5)); + return ; + } + export function Handler(p) { + const r = {}; + let x; + r.a = 1; + try { + JSON.parse(p.bad); + } catch { + x = 1; + } + return ; + } + // \`q\` holds \`r\`, so the declaration sits in two nested scopes. + export function Nested(p) { + const r = {}; + const q = {}; + let x; + q.b = r; + r.a = 1; + return ; + } + export function PropFromProps(p) { + const r = {}; + let x; + r.a = 1; + return ; + } + export function ArrayFromProps(p) { + const r = {}; + let x; + r.a = 1; + return ; + } + export function NestedFromProps(p) { + const r = {}; + const q = {}; + let x; + q.b = r; + r.a = 1; + return ; + } + // The declaration is above the scope of \`r\`. Only the promoted store + // was wrong here. + export function DeclaredAbove(p) { + let x = 0; + const r = {}; + r.a = 1; + return ; + } + `, + ...stubReact, + }, + reactCompiler: true, + backend: "cli", + target: "browser", + run: { + stdout: ` + Argument={"r":{"a":1}} calls=[5] + ArrayFromProps={"r":{"a":1},"v":[7]} + DeclaredAbove={"r":{"a":1},"v":[7,7]} + FoldedRead={"r":{"a":1},"v":[5,5]} + Handler={"r":{"a":1}} + Nested={"r":{"a":1},"q":{"b":{"a":1}},"v":5} + NestedFromProps={"r":{"a":1},"q":{"b":{"a":1}},"v":7} + Prop={"r":{"a":1},"v":5} + PropFromProps={"r":{"a":1},"v":7} + `, + }, + onAfterBundle(api) { + // Every form compiled. Each `let x` declared inside the scope of `r` + // sits before its memo block, and each promoted store is a statement. + const out = api.readFile("/out.js"); + expect(out.match(/let (q, )?r, x;/g)).toHaveLength(8); + expect(out).not.toMatch(/let x;\s*r\.a = 1;/); + expect(out.match(/const t\d = x = p\.n;/g)).toHaveLength(4); + }, + }); + // Outside the compiler, the bundler binds a local that holds a `require()` / // `import()` export to the export itself: `const { a } = require("./m")` // declares nothing, and `ns.a` off `const ns = require("./m")` is an import