Repository: facebook/react
Pull request: [compiler] Remove transitive memo check in validatePreserveMemo

Stack from [ghstack](https://github.com/ezyang/ghstack) (oldest at bottom):
* #30603
* __->__ #30630
* #30629
* #30602
* #30601

---
The original version of validatePreserveMemo tried to check that all manual memo declarations are *transitively* memoized. This may have been intended to stabilize `useEffect` by ensuring that optimized components trigger effects minimally.

This transitive memoization check only rarely affects compilation output. (Syncing this PR resulted in 12 changes out of tens of thousands of files). This is likely because many `StartMemoize` and `FinishMemoize` deps / decls are identifiers with no attached scopes (see #30603 for more complete explanation).

Meta internal reviewers: see [changes](https://fburl.com/phabricator/yzlich3a) from this PR

Changed files:
filename: compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts
patch: @@ -276,9 +276,16 @@ function validateInferredDep(
 }
 
 class Visitor extends ReactiveFunctionVisitor<VisitorState> {
+  /**
+   * Records all completed scopes (regardless of transitive memoization
+   * of scope dependencies)
+   *
+   * Both @scopes and @prunedScopes are live sets. We rely on iterating
+   * the reactive-ir in evaluation order, as they are used to determine
+   * whether scope dependencies / declarations have completed mutation.
+   */
   scopes: Set<ScopeId> = new Set();
   prunedScopes: Set<ScopeId> = new Set();
-  scopeMapping = new Map();
   temporaries: Map<IdentifierId, ManualMemoDependency> = new Map();
 
   /**
@@ -394,25 +401,9 @@ class Visitor extends ReactiveFunctionVisitor<VisitorState> {
       }
     }
 
-    /*
-     * Record scopes that exist in the AST so we can later check to see if
-     * effect dependencies which should be memoized (have a scope assigned)
-     * actually are memoized (that scope exists).
-     * However, we only record scopes if *their* dependencies are also
-     * memoized, allowing a transitive memoization check.
-     */
-    let areDependenciesMemoized = true;
-    for (const dep of scopeBlock.scope.dependencies) {
-      if (isUnmemoized(dep.identifier, this.scopes)) {
-        areDependenciesMemoized = false;
-        break;
-      }
-    }
-    if (areDependenciesMemoized) {
-      this.scopes.add(scopeBlock.scope.id);
-      for (const id of scopeBlock.scope.merged) {
-        this.scopes.add(id);
-      }
+    this.scopes.add(scopeBlock.scope.id);
+    for (const id of scopeBlock.scope.merged) {
+      this.scopes.add(id);
     }
   }
 
@@ -453,6 +444,23 @@ class Visitor extends ReactiveFunctionVisitor<VisitorState> {
         manualMemoId: value.manualMemoId,
       };
 
+      /**
+       * We check that each scope dependency is either:
+       * (1) Not scoped
+       *     Checking `identifier.scope == null` is a proxy for whether the dep
+       *     is a primitive, global, or other guaranteed non-allocating value.
+       *     Non-allocating values do not need memoization.
+       *     Note that this is a conservative estimate as some primitive-typed
+       *     variables do receive scopes.
+       * (2) Scoped (a maybe newly-allocated value with a mutable range)
+       *     Here, we check that the dependency's scope has completed before
+       *     the manual useMemo as a proxy for mutable-range checking. This
+       *     validates that there are no potential rule-of-react violations
+       *     in source.
+       *     Note that scope range is an overly conservative proxy as we merge
+       *     overlapping ranges.
+       *     See fixture `error.false-positive-useMemo-overlap-scopes`
+       */
       for (const {identifier, loc} of eachInstructionValueOperand(
         value as InstructionValue,
       )) {

filename: compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-extended-contextvar-scope.expect.md
patch: @@ -1,57 +0,0 @@
-
-## Input
-
-```javascript
-// @validatePreserveExistingMemoizationGuarantees:true
-
-import {useCallback} from 'react';
-import {Stringify, useIdentity} from 'shared-runtime';
-
-/**
- * Here, the *inferred* dependencies of cb are `a` and `t1 = LoadContext capture x_@1`.
- * - t1 does not have a scope as it captures `x` after x's mutable range
- * - `x` is a context variable, which means its mutable range extends to all
- *    references / aliases.
- * - `a`, `b`, and `x` get the same mutable range due to potential aliasing.
- *
- * We currently bail out because `a` has a scope and is not transitively memoized
- * (as its scope is pruned due to a hook call)
- */
-function useBar({a, b}, cond) {
-  let x = useIdentity({val: 3});
-  if (cond) {
-    x = b;
-  }
-
-  const cb = useCallback(() => {
-    return [a, x];
-  }, [a, x]);
-
-  return <Stringify cb={cb} shouldInvoke={true} />;
-}
-
-export const FIXTURE_ENTRYPOINT = {
-  fn: useBar,
-  params: [{a: 1, b: 2}, true],
-};
-
-```
-
-
-## Error
-
-```
-  20 |   }
-  21 |
-> 22 |   const cb = useCallback(() => {
-     |                          ^^^^^^^
-> 23 |     return [a, x];
-     | ^^^^^^^^^^^^^^^^^^
-> 24 |   }, [a, x]);
-     | ^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This value was memoized in source but not in compilation output. (22:24)
-  25 |
-  26 |   return <Stringify cb={cb} shouldInvoke={true} />;
-  27 | }
-```
-          
-      
\ No newline at end of file

filename: compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useCallback-dep-scope-pruned.expect.md
patch: @@ -1,42 +0,0 @@
-
-## Input
-
-```javascript
-// @validatePreserveExistingMemoizationGuarantees
-import {useCallback} from 'react';
-import {identity, useIdentity} from 'shared-runtime';
-
-/**
- * Repro showing a manual memo whose declaration (useCallback's 1st argument)
- * is memoized, but not its dependency (x). In this case, `x`'s scope is pruned
- * due to hook-call flattening.
- */
-function useFoo(a) {
-  const x = identity(a);
-  useIdentity(2);
-  mutate(x);
-
-  return useCallback(() => [x, []], [x]);
-}
-
-export const FIXTURE_ENTRYPOINT = {
-  fn: useFoo,
-  params: [3],
-};
-
-```
-
-
-## Error
-
-```
-  13 |   mutate(x);
-  14 |
-> 15 |   return useCallback(() => [x, []], [x]);
-     |                      ^^^^^^^^^^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This value was memoized in source but not in compilation output. (15:15)
-  16 | }
-  17 |
-  18 | export const FIXTURE_ENTRYPOINT = {
-```
-          
-      
\ No newline at end of file

filename: compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-dep-scope-pruned.expect.md
patch: @@ -0,0 +1,65 @@
+
+## Input
+
+```javascript
+// @validatePreserveExistingMemoizationGuarantees
+import {useCallback} from 'react';
+import {identity, useIdentity} from 'shared-runtime';
+
+/**
+ * Repro showing a manual memo whose declaration (useCallback's 1st argument)
+ * is memoized, but not its dependency (x). In this case, `x`'s scope is pruned
+ * due to hook-call flattening.
+ */
+function useFoo(a) {
+  const x = identity(a);
+  useIdentity(2);
+  mutate(x);
+
+  return useCallback(() => [x, []], [x]);
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useFoo,
+  params: [3],
+};
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees
+import { useCallback } from "react";
+import { identity, useIdentity } from "shared-runtime";
+
+/**
+ * Repro showing a manual memo whose declaration (useCallback's 1st argument)
+ * is memoized, but not its dependency (x). In this case, `x`'s scope is pruned
+ * due to hook-call flattening.
+ */
+function useFoo(a) {
+  const $ = _c(2);
+  const x = identity(a);
+  useIdentity(2);
+  mutate(x);
+  let t0;
+  if ($[0] !== x) {
+    t0 = () => [x, []];
+    $[0] = x;
+    $[1] = t0;
+  } else {
+    t0 = $[1];
+  }
+  return t0;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useFoo,
+  params: [3],
+};
+
+```
+      
+### Eval output
+(kind: exception) mutate is not defined
\ No newline at end of file

filename: compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-extended-contextvar-scope.expect.md
patch: @@ -0,0 +1,105 @@
+
+## Input
+
+```javascript
+// @validatePreserveExistingMemoizationGuarantees:true
+
+import {useCallback} from 'react';
+import {Stringify, useIdentity} from 'shared-runtime';
+
+/**
+ * Here, the *inferred* dependencies of cb are `a` and `t1 = LoadContext capture x_@1`.
+ * - t1 does not have a scope as it captures `x` after x's mutable range
+ * - `x` is a context variable, which means its mutable range extends to all
+ *    references / aliases.
+ * - `a`, `b`, and `x` get the same mutable range due to potential aliasing.
+ *
+ * We currently bail out because `a` has a scope and is not transitively memoized
+ * (as its scope is pruned due to a hook call)
+ */
+function useBar({a, b}, cond) {
+  let x = useIdentity({val: 3});
+  if (cond) {
+    x = b;
+  }
+
+  const cb = useCallback(() => {
+    return [a, x];
+  }, [a, x]);
+
+  return <Stringify cb={cb} shouldInvoke={true} />;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useBar,
+  params: [{a: 1, b: 2}, true],
+};
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees:true
+
+import { useCallback } from "react";
+import { Stringify, useIdentity } from "shared-runtime";
+
+/**
+ * Here, the *inferred* dependencies of cb are `a` and `t1 = LoadContext capture x_@1`.
+ * - t1 does not have a scope as it captures `x` after x's mutable range
+ * - `x` is a context variable, which means its mutable range extends to all
+ *    references / aliases.
+ * - `a`, `b`, and `x` get the same mutable range due to potential aliasing.
+ *
+ * We currently bail out because `a` has a scope and is not transitively memoized
+ * (as its scope is pruned due to a hook call)
+ */
+function useBar(t0, cond) {
+  const $ = _c(6);
+  const { a, b } = t0;
+  let t1;
+  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
+    t1 = { val: 3 };
+    $[0] = t1;
+  } else {
+    t1 = $[0];
+  }
+  let x;
+  x = useIdentity(t1);
+  if (cond) {
+    x = b;
+  }
+
+  const t2 = x;
+  let t3;
+  if ($[1] !== a || $[2] !== t2) {
+    t3 = () => [a, x];
+    $[1] = a;
+    $[2] = t2;
+    $[3] = t3;
+  } else {
+    t3 = $[3];
+  }
+  x;
+  const cb = t3;
+  let t4;
+  if ($[4] !== cb) {
+    t4 = <Stringify cb={cb} shouldInvoke={true} />;
+    $[4] = cb;
+    $[5] = t4;
+  } else {
+    t4 = $[5];
+  }
+  return t4;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useBar,
+  params: [{ a: 1, b: 2 }, true],
+};
+
+```
+      
+### Eval output
+(kind: ok) <div>{"cb":"[[ function params=0 ]]","shouldInvoke":true}</div>
\ No newline at end of file