[Compiler] Fix incorrect closure hoisting causing runtime errors (#34901

compiler/packages/babel-plugin-react-compiler/src/Optimization/OutlineFunctions.ts

@@ -7,6 +7,45 @@
 
 import {HIRFunction, IdentifierId} from '../HIR';
 
+/**
+ * Checks if a function accesses context variables that are not explicitly
+ * tracked in its context array. Context variables are those that are both
+ * accessed within a function expression and reassigned somewhere in the
+ * outer scope.
+ */
+function accessesOuterContextVariables(fn: HIRFunction): boolean {
+  const contextIdentifiers = new Set(
+    fn.context.map(place => place.identifier.id),
+  );
+
+  for (const [, block] of fn.body.blocks) {
+    for (const instr of block.instructions) {
+      if (
+        instr.value.kind === 'LoadContext' ||
+        instr.value.kind === 'StoreContext'
+      ) {
+        const place =
+          instr.value.kind === 'LoadContext'
+            ? instr.value.place
+            : instr.value.lvalue.place;
+        if (!contextIdentifiers.has(place.identifier.id)) {
+          return true;
+        }
+      }
+
+      if (
+        instr.value.kind === 'FunctionExpression' ||
+        instr.value.kind === 'ObjectMethod'
+      ) {
+        if (accessesOuterContextVariables(instr.value.loweredFunc.func)) {
+          return true;
+        }
+      }
+    }
+  }
+  return false;
+}
+
 export function outlineFunctions(
   fn: HIRFunction,
   fbtOperands: Set<IdentifierId>,
@@ -27,7 +66,8 @@ export function outlineFunctions(
         value.loweredFunc.func.context.length === 0 &&
         // TODO: handle outlining named functions
         value.loweredFunc.func.id === null &&
-        !fbtOperands.has(lvalue.identifier.id)
+        !fbtOperands.has(lvalue.identifier.id) &&
+        !accessesOuterContextVariables(value.loweredFunc.func)
       ) {
         const loweredFunc = value.loweredFunc.func;
 

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/closure-hoisting-bug.expect.md

@@ -0,0 +1,86 @@
+
+## Input
+
+```javascript
+// @compilationMode(infer)
+// Regression test for https://github.com/facebook/react/issues/34901
+// The compiler should NOT outline functions that capture variables from their closure.
+// In this case, `() => store` captures `store` from the outer scope and should not
+// be hoisted to a top-level function because `store` would be undefined.
+
+class SomeStore {
+  test = 'hello';
+}
+
+function useLocalObservable(fn) {
+  return fn();
+}
+
+export function createSomething() {
+  const store = new SomeStore();
+
+  const Cmp = () => {
+    const observedStore = useLocalObservable(() => store);
+    return <div>{observedStore.test}</div>;
+  };
+
+  return Cmp;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: createSomething,
+  params: [],
+};
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime";
+class SomeStore {
+  test = "hello";
+}
+
+function useLocalObservable(fn) {
+  return fn();
+}
+
+export function createSomething() {
+  const $ = _c(2);
+  let t0;
+  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
+    const store = new SomeStore();
+
+    const Cmp = () => {
+      const $ = _c(2);
+      const observedStore = useLocalObservable(() => store);
+      let t0;
+      if ($[0] !== observedStore.test) {
+        t0 = <div>{observedStore.test}</div>;
+        $[0] = observedStore.test;
+        $[1] = t0;
+      } else {
+        t0 = $[1];
+      }
+      return t0;
+    };
+
+    t0 = Cmp;
+    $[0] = t0;
+    $[1] = store;
+  } else {
+    t0 = $[0];
+  }
+  return t0;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: createSomething,
+  params: [],
+};
+
+```
+
+### Eval output
+(kind: ok) <div>hello</div>

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/closure-hoisting-bug.js

@@ -0,0 +1,29 @@
+// @compilationMode(infer)
+// Regression test for https://github.com/facebook/react/issues/34901
+// The compiler should NOT outline functions that capture variables from their closure.
+// In this case, `() => store` captures `store` from the outer scope and should not
+// be hoisted to a top-level function because `store` would be undefined.
+
+class SomeStore {
+  test = 'hello';
+}
+
+function useLocalObservable(fn) {
+  return fn();
+}
+
+export function createSomething() {
+  const store = new SomeStore();
+
+  const Cmp = () => {
+    const observedStore = useLocalObservable(() => store);
+    return <div>{observedStore.test}</div>;
+  };
+
+  return Cmp;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: createSomething,
+  params: [],
+};

packages/shared/ReactVersion.js

@@ -1,15 +1 @@
-/**
- * Copyright (c) Meta Platforms, Inc. and affiliates.
- *
- * This source code is licensed under the MIT license found in the
- * LICENSE file in the root directory of this source tree.
- */
-
-// TODO: this is special because it gets imported during build.
-//
-// It exists as a placeholder so that DevTools can support work tag changes between releases.
-// When we next publish a release, update the matching TODO in backend/renderer.js
-// TODO: This module is used both by the release scripts and to expose a version
-// at runtime. We should instead inject the version number as part of the build
-// process, and use the ReactVersions.js module as the single source of truth.
 export default '19.3.0';