react-compiler: hoist the declaration of a local that a later scope assigns

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)))
         }

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| {

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 <Stub r={r} v={(x = 5)} />;
+        }
+        // The one read of \`x\` folds to 5.
+        export function FoldedRead(p) {
+          const r = {};
+          let x = 0;
+          r.a = 1;
+          return <Stub r={r} v={[(x = 5), x]} />;
+        }
+        export function Argument(p) {
+          const r = {};
+          let x;
+          r.a = 1;
+          p.call((x = 5));
+          return <Stub r={r} />;
+        }
+        export function Handler(p) {
+          const r = {};
+          let x;
+          r.a = 1;
+          try {
+            JSON.parse(p.bad);
+          } catch {
+            x = 1;
+          }
+          return <Stub r={r} />;
+        }
+        // \`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 <Stub r={r} q={q} v={(x = 5)} />;
+        }
+        export function PropFromProps(p) {
+          const r = {};
+          let x;
+          r.a = 1;
+          return <Stub r={r} v={(x = p.n)} />;
+        }
+        export function ArrayFromProps(p) {
+          const r = {};
+          let x;
+          r.a = 1;
+          return <Stub r={r} v={[(x = p.n)]} />;
+        }
+        export function NestedFromProps(p) {
+          const r = {};
+          const q = {};
+          let x;
+          q.b = r;
+          r.a = 1;
+          return <Stub r={r} q={q} v={(x = p.n)} />;
+        }
+        // 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 <Stub r={r} v={[(x = p.n), x++]} />;
+        }
+      `,
+      ...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