From 4ec6a6f71475a6f2fee39a0e604ddbbd2f124164 Mon Sep 17 00:00:00 2001
From: Joseph Savona <6425824+josephsavona@users.noreply.github.com>
Date: Tue, 28 May 2024 10:06:05 -0700
Subject: [PATCH] Repro function expr hoisting (#29615)

Modified version of @mofeiZ's #29232 with CI passing (had to run
prettier)

---------

Co-authored-by: Mofei Zhang <feifei0@meta.com>
---
 ...ug-invalid-hoisting-functionexpr.expect.md |  93 ++++++++++++++
 .../bug-invalid-hoisting-functionexpr.tsx     |  30 +++++
 ...invalid-pruned-scope-leaks-value.expect.md | 119 ++++++++++++++++++
 .../bug-invalid-pruned-scope-leaks-value.ts   |  43 +++++++
 .../packages/snap/src/SproutTodoFilter.ts     |   2 +
 .../snap/src/sprout/shared-runtime.ts         |   4 +
 6 files changed, 291 insertions(+)
 create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-hoisting-functionexpr.expect.md
 create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-hoisting-functionexpr.tsx
 create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-pruned-scope-leaks-value.expect.md
 create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-pruned-scope-leaks-value.ts

diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-hoisting-functionexpr.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-hoisting-functionexpr.expect.md
new file mode 100644
index 0000000000..37cd740908
--- /dev/null
+++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-hoisting-functionexpr.expect.md
@@ -0,0 +1,93 @@
+
+## Input
+
+```javascript
+import { Stringify } from "shared-runtime";
+
+/**
+ * We currently hoist the accessed properties of function expressions,
+ * regardless of control flow. This is simply because we wrote support for
+ * function expressions before doing a lot of work in PropagateScopeDeps
+ * to handle conditionally accessed dependencies.
+ *
+ * Current evaluator error:
+ *  Found differences in evaluator results
+ *  Non-forget (expected):
+ *  (kind: ok) <div>{"shouldInvokeFns":true,"callback":{"kind":"Function","result":null}}</div>
+ *  Forget:
+ *  (kind: exception) Cannot read properties of null (reading 'prop')
+ */
+function Component({ obj, isObjNull }) {
+  const callback = () => {
+    if (!isObjNull) {
+      return obj.prop;
+    } else {
+      return null;
+    }
+  };
+  return <Stringify shouldInvokeFns={true} callback={callback} />;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{ obj: null, isObjNull: true }],
+};
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime";
+import { Stringify } from "shared-runtime";
+
+/**
+ * We currently hoist the accessed properties of function expressions,
+ * regardless of control flow. This is simply because we wrote support for
+ * function expressions before doing a lot of work in PropagateScopeDeps
+ * to handle conditionally accessed dependencies.
+ *
+ * Current evaluator error:
+ *  Found differences in evaluator results
+ *  Non-forget (expected):
+ *  (kind: ok) <div>{"shouldInvokeFns":true,"callback":{"kind":"Function","result":null}}</div>
+ *  Forget:
+ *  (kind: exception) Cannot read properties of null (reading 'prop')
+ */
+function Component(t0) {
+  const $ = _c(5);
+  const { obj, isObjNull } = t0;
+  let t1;
+  if ($[0] !== isObjNull || $[1] !== obj.prop) {
+    t1 = () => {
+      if (!isObjNull) {
+        return obj.prop;
+      } else {
+        return null;
+      }
+    };
+    $[0] = isObjNull;
+    $[1] = obj.prop;
+    $[2] = t1;
+  } else {
+    t1 = $[2];
+  }
+  const callback = t1;
+  let t2;
+  if ($[3] !== callback) {
+    t2 = <Stringify shouldInvokeFns={true} callback={callback} />;
+    $[3] = callback;
+    $[4] = t2;
+  } else {
+    t2 = $[4];
+  }
+  return t2;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{ obj: null, isObjNull: true }],
+};
+
+```
+      
\ No newline at end of file
diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-hoisting-functionexpr.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-hoisting-functionexpr.tsx
new file mode 100644
index 0000000000..047d178868
--- /dev/null
+++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-hoisting-functionexpr.tsx
@@ -0,0 +1,30 @@
+import { Stringify } from "shared-runtime";
+
+/**
+ * We currently hoist the accessed properties of function expressions,
+ * regardless of control flow. This is simply because we wrote support for
+ * function expressions before doing a lot of work in PropagateScopeDeps
+ * to handle conditionally accessed dependencies.
+ *
+ * Current evaluator error:
+ *  Found differences in evaluator results
+ *  Non-forget (expected):
+ *  (kind: ok) <div>{"shouldInvokeFns":true,"callback":{"kind":"Function","result":null}}</div>
+ *  Forget:
+ *  (kind: exception) Cannot read properties of null (reading 'prop')
+ */
+function Component({ obj, isObjNull }) {
+  const callback = () => {
+    if (!isObjNull) {
+      return obj.prop;
+    } else {
+      return null;
+    }
+  };
+  return <Stringify shouldInvokeFns={true} callback={callback} />;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{ obj: null, isObjNull: true }],
+};
diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-pruned-scope-leaks-value.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-pruned-scope-leaks-value.expect.md
new file mode 100644
index 0000000000..d490c6e8f3
--- /dev/null
+++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-pruned-scope-leaks-value.expect.md
@@ -0,0 +1,119 @@
+
+## Input
+
+```javascript
+import invariant from "invariant";
+import {
+  makeObject_Primitives,
+  mutate,
+  sum,
+  useIdentity,
+} from "shared-runtime";
+
+/**
+ * Exposes fundamental issue with pruning 'non-reactive' dependencies + flattening
+ * those scopes. Here, `z`'s original memo block is removed due to the inner hook call.
+ * However, we also infer that `z` is non-reactive and does not need to be a memo
+ * dependency.
+ *
+ * Current evaluator error:
+ *  Found differences in evaluator results
+ *  Non-forget (expected):
+ *  (kind: ok) [4,{"a":0,"b":"value1","c":true,"wat0":"joe"}]
+ *  [4,{"a":0,"b":"value1","c":true,"wat0":"joe"}]
+ *  [5,{"a":0,"b":"value1","c":true,"wat0":"joe"}]
+ *  Forget:
+ *  (kind: ok) [4,{"a":0,"b":"value1","c":true,"wat0":"joe"}]
+ *  [[ (exception in render) Invariant Violation: oh no! ]]
+ *  [5,{"a":0,"b":"value1","c":true,"wat0":"joe"}]
+ */
+
+function MyApp({ count }) {
+  const z = makeObject_Primitives();
+  const x = useIdentity(2);
+  const y = sum(x, count);
+  mutate(z);
+  const thing = [y, z];
+  if (thing[1] !== z) {
+    invariant(false, "oh no!");
+  }
+  return thing;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: MyApp,
+  params: [{ count: 2 }],
+  sequentialRenders: [{ count: 2 }, { count: 2 }, { count: 3 }],
+};
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime";
+import invariant from "invariant";
+import {
+  makeObject_Primitives,
+  mutate,
+  sum,
+  useIdentity,
+} from "shared-runtime";
+
+/**
+ * Exposes fundamental issue with pruning 'non-reactive' dependencies + flattening
+ * those scopes. Here, `z`'s original memo block is removed due to the inner hook call.
+ * However, we also infer that `z` is non-reactive and does not need to be a memo
+ * dependency.
+ *
+ * Current evaluator error:
+ *  Found differences in evaluator results
+ *  Non-forget (expected):
+ *  (kind: ok) [4,{"a":0,"b":"value1","c":true,"wat0":"joe"}]
+ *  [4,{"a":0,"b":"value1","c":true,"wat0":"joe"}]
+ *  [5,{"a":0,"b":"value1","c":true,"wat0":"joe"}]
+ *  Forget:
+ *  (kind: ok) [4,{"a":0,"b":"value1","c":true,"wat0":"joe"}]
+ *  [[ (exception in render) Invariant Violation: oh no! ]]
+ *  [5,{"a":0,"b":"value1","c":true,"wat0":"joe"}]
+ */
+
+function MyApp(t0) {
+  const $ = _c(5);
+  const { count } = t0;
+  const z = makeObject_Primitives();
+  const x = useIdentity(2);
+  let t1;
+  if ($[0] !== x || $[1] !== count) {
+    t1 = sum(x, count);
+    $[0] = x;
+    $[1] = count;
+    $[2] = t1;
+  } else {
+    t1 = $[2];
+  }
+  const y = t1;
+  mutate(z);
+  let t2;
+  if ($[3] !== y) {
+    t2 = [y, z];
+    $[3] = y;
+    $[4] = t2;
+  } else {
+    t2 = $[4];
+  }
+  const thing = t2;
+  if (thing[1] !== z) {
+    invariant(false, "oh no!");
+  }
+  return thing;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: MyApp,
+  params: [{ count: 2 }],
+  sequentialRenders: [{ count: 2 }, { count: 2 }, { count: 3 }],
+};
+
+```
+      
\ No newline at end of file
diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-pruned-scope-leaks-value.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-pruned-scope-leaks-value.ts
new file mode 100644
index 0000000000..5b9c2527ae
--- /dev/null
+++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-pruned-scope-leaks-value.ts
@@ -0,0 +1,43 @@
+import invariant from "invariant";
+import {
+  makeObject_Primitives,
+  mutate,
+  sum,
+  useIdentity,
+} from "shared-runtime";
+
+/**
+ * Exposes fundamental issue with pruning 'non-reactive' dependencies + flattening
+ * those scopes. Here, `z`'s original memo block is removed due to the inner hook call.
+ * However, we also infer that `z` is non-reactive and does not need to be a memo
+ * dependency.
+ *
+ * Current evaluator error:
+ *  Found differences in evaluator results
+ *  Non-forget (expected):
+ *  (kind: ok) [4,{"a":0,"b":"value1","c":true,"wat0":"joe"}]
+ *  [4,{"a":0,"b":"value1","c":true,"wat0":"joe"}]
+ *  [5,{"a":0,"b":"value1","c":true,"wat0":"joe"}]
+ *  Forget:
+ *  (kind: ok) [4,{"a":0,"b":"value1","c":true,"wat0":"joe"}]
+ *  [[ (exception in render) Invariant Violation: oh no! ]]
+ *  [5,{"a":0,"b":"value1","c":true,"wat0":"joe"}]
+ */
+
+function MyApp({ count }) {
+  const z = makeObject_Primitives();
+  const x = useIdentity(2);
+  const y = sum(x, count);
+  mutate(z);
+  const thing = [y, z];
+  if (thing[1] !== z) {
+    invariant(false, "oh no!");
+  }
+  return thing;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: MyApp,
+  params: [{ count: 2 }],
+  sequentialRenders: [{ count: 2 }, { count: 2 }, { count: 3 }],
+};
diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts
index 25ac01cdd5..a8fc53606d 100644
--- a/compiler/packages/snap/src/SproutTodoFilter.ts
+++ b/compiler/packages/snap/src/SproutTodoFilter.ts
@@ -486,6 +486,8 @@ const skipFilter = new Set([
 
   // bugs
   "bug-invalid-reactivity-value-block",
+  "bug-invalid-pruned-scope-leaks-value",
+  "bug-invalid-hoisting-functionexpr",
   "original-reactive-scopes-fork/bug-nonmutating-capture-in-unsplittable-memo-block",
   "original-reactive-scopes-fork/bug-hoisted-declaration-with-scope",
 
diff --git a/compiler/packages/snap/src/sprout/shared-runtime.ts b/compiler/packages/snap/src/sprout/shared-runtime.ts
index 0f1dedd152..94fba22c04 100644
--- a/compiler/packages/snap/src/sprout/shared-runtime.ts
+++ b/compiler/packages/snap/src/sprout/shared-runtime.ts
@@ -176,6 +176,10 @@ export function useNoAlias(...args: Array<any>): object {
   return noAliasObject;
 }
 
+export function useIdentity<T>(arg: T): T {
+  return arg;
+}
+
 export function invoke<T extends Array<any>, ReturnType>(
   fn: (...input: T) => ReturnType,
   ...params: T