[compiler] Remove transitive memo check in validatePreserveMemo

[ghstack-poisoned]

compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts

@@ -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,
       )) {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-dep-scope-pruned.expect.md

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

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-extended-contextvar-scope.expect.md

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