[validation] Reduce false positives in validatePreserveExistingMemo

--- Previously (in #2663), we check that inferred dependencies exactly match source dependencies. As @validatePreserveExistingMemoizationGuarantees checks that Forget output does not invalidate *any more* than source, we now allow more specific inferred dependencies when neither the inferred dep or matching source dependency reads into a ref (based on `.current` access). See added comments for more details
2025-11-01 09:12:30 +00:00 · 2024-03-20 16:05:23 -04:00
parent b35779c6a4
commit 63bb781781
9 changed files with 198 additions and 96 deletions
@@ -101,24 +101,59 @@ function prettyPrintScopeDependency(val: ReactiveScopeDependency): string {
  }
  return `${rootStr}${val.path.length > 0 ? "." : ""}${val.path.join(".")}`;
 }
-function depsEqual(
-  dep1: ManualMemoDependency,
-  dep2: ManualMemoDependency
+
+function compareDeps(
+  inferred: ManualMemoDependency,
+  source: ManualMemoDependency
 ): boolean {
  const rootsEqual =
-    (dep1.root.kind === "Global" &&
-      dep2.root.kind === "Global" &&
-      dep1.root.identifierName === dep2.root.identifierName) ||
-    (dep1.root.kind === "NamedLocal" &&
-      dep2.root.kind === "NamedLocal" &&
-      dep1.root.value.identifier.id === dep2.root.value.identifier.id);
-  return (
-    rootsEqual &&
-    dep1.path.length === dep2.path.length &&
-    dep1.path.every((val, idx) => val === dep2.path[idx])
-  );
+    (inferred.root.kind === "Global" &&
+      source.root.kind === "Global" &&
+      inferred.root.identifierName === source.root.identifierName) ||
+    (inferred.root.kind === "NamedLocal" &&
+      source.root.kind === "NamedLocal" &&
+      inferred.root.value.identifier.id === source.root.value.identifier.id);
+  if (!rootsEqual) {
+    return false;
+  }
+
+  let isSubpath = true;
+  for (let i = 0; i < Math.min(inferred.path.length, source.path.length); i++) {
+    if (inferred.path[i] !== source.path[i]) {
+      isSubpath = false;
+      break;
+    }
+  }
+
+  if (
+    isSubpath &&
+    (source.path.length === inferred.path.length ||
+      (inferred.path.length >= source.path.length &&
+        !inferred.path.includes("current")))
+  ) {
+    return true;
+  } else {
+    return false;
+  }
 }

+/**
+ * Validate that an inferred dependency either matches a source dependency
+ * or is produced by earlier instructions in the same manual memoization
+ * call.
+ * Inferred dependency `rootA.[pathA]` matches a source dependency `rootB.[pathB]`
+ * when:
+ *   - rootA and rootB are loads from the same named identifier. Note that this
+ *     identifier must be also named in source, as DropManualMemoization, which
+ *     runs before any renaming passes, only records loads from named variables.
+ *   - and one of the following holds:
+ *       - pathA and pathB are identifical
+ *       - pathB is a subpath of pathA and neither read into a `ref` type*
+ *
+ * We do not allow for partial matches on ref types because they are not immutable
+ * values, e.g.
+ * ref_prev === ref_new does not imply ref_prev.current === ref_new.current
+ */
 function validateInferredDep(
  dep: ReactiveScopeDependency,
  temporaries: Map<IdentifierId, ManualMemoDependency>,
@@ -154,22 +189,19 @@ function validateInferredDep(
      path: [...dep.path],
    };
  }
-  for (const originalDep of validDepsInMemoBlock) {
-    if (depsEqual(originalDep, normalizedDep)) {
-      return;
-    }
-  }
  for (const decl of declsWithinMemoBlock) {
-    const normalizedDecl = temporaries.get(decl);
-    if (normalizedDecl != null && depsEqual(normalizedDecl, normalizedDep)) {
-      return;
-    } else if (
+    if (
      normalizedDep.root.kind === "NamedLocal" &&
      decl === normalizedDep.root.value.identifier.id
    ) {
      return;
    }
  }
+  for (const originalDep of validDepsInMemoBlock) {
+    if (compareDeps(normalizedDep, originalDep)) {
+      return;
+    }
+  }
  errorState.push({
    severity: ErrorSeverity.Todo,
    reason:
@@ -1,28 +0,0 @@
-
-## Input
-
-```javascript
-// @validatePreserveExistingMemoizationGuarantees
-
-import { useCallback } from "react";
-
-// False positive as more specific memoization always results
-// in fewer memo block executions.
-// Precisely:
-//  x_new != x_prev does not imply x.y.z_new != x.y.z_prev
-//  x.y.z_new != x.y.z_prev does imply x_new != x_prev
-// One fix would be to depend on optional chains
-function useHook(x) {
-  return useCallback(() => [x.y.z], [x]);
-}
-
-```
-
-
-## Error
-
-```
-[ReactForget] Todo: Could not preserve manual memoization because an inferred dependency does not match the dependency list in source. The inferred dependency was `x.y.z`, but the source dependencies were [x]
-```
-          
-      
@@ -1,27 +0,0 @@
-
-## Input
-
-```javascript
-// @validatePreserveExistingMemoizationGuarantees
-
-import { useMemo } from "react";
-
-// False positive as more specific memoization always results
-// in fewer memo block executions.
-// Precisely:
-//  x_new != x_prev does not imply x.y.z_new != x.y.z_prev
-//  x.y.z_new != x.y.z_prev does imply x_new != x_prev
-function useHook(x) {
-  return useMemo(() => [x.y.z], [x]);
-}
-
-```
-
-
-## Error
-
-```
-[ReactForget] Todo: Could not preserve manual memoization because an inferred dependency does not match the dependency list in source. The inferred dependency was `x.y.z`, but the source dependencies were [x]
-```
-          
-      
@@ -1,12 +0,0 @@
-// @validatePreserveExistingMemoizationGuarantees
-
-import { useMemo } from "react";
-
-// False positive as more specific memoization always results
-// in fewer memo block executions.
-// Precisely:
-//  x_new != x_prev does not imply x.y.z_new != x.y.z_prev
-//  x.y.z_new != x.y.z_prev does imply x_new != x_prev
-function useHook(x) {
-  return useMemo(() => [x.y.z], [x]);
-}
@@ -20,8 +20,6 @@ function useHook(x) {

 ```
 [ReactForget] Todo: Could not preserve manual memoization because an inferred dependency does not match the dependency list in source. The inferred dependency was `aliasedX`, but the source dependencies were [x, aliasedProp]
-
-[ReactForget] Todo: Could not preserve manual memoization because an inferred dependency does not match the dependency list in source. The inferred dependency was `x.y.z`, but the source dependencies were [x, aliasedProp]
 ```
          
      
@@ -0,0 +1,58 @@
+
+## Input
+
+```javascript
+// @validatePreserveExistingMemoizationGuarantees
+
+import { useCallback } from "react";
+
+// More specific memoization always results in fewer memo block
+// executions.
+// Precisely:
+//  x_new != x_prev does NOT imply x.y.z_new != x.y.z_prev
+//  x.y.z_new != x.y.z_prev does imply x_new != x_prev
+function useHook(x) {
+  return useCallback(() => [x.y.z], [x]);
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useHook,
+  params: [{ y: { z: 2 } }],
+};
+
+```
+
+## Code
+
+```javascript
+// @validatePreserveExistingMemoizationGuarantees
+
+import { useCallback, unstable_useMemoCache as useMemoCache } from "react";
+
+// More specific memoization always results in fewer memo block
+// executions.
+// Precisely:
+//  x_new != x_prev does NOT imply x.y.z_new != x.y.z_prev
+//  x.y.z_new != x.y.z_prev does imply x_new != x_prev
+function useHook(x) {
+  const $ = useMemoCache(2);
+  let t0;
+  if ($[0] !== x.y.z) {
+    t0 = () => [x.y.z];
+    $[0] = x.y.z;
+    $[1] = t0;
+  } else {
+    t0 = $[1];
+  }
+  return t0;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useHook,
+  params: [{ y: { z: 2 } }],
+};
+
+```
+      
+### Eval output
+(kind: ok) "[[ function params=0 ]]"
@@ -2,12 +2,16 @@

 import { useCallback } from "react";

-// False positive as more specific memoization always results
-// in fewer memo block executions.
+// More specific memoization always results in fewer memo block
+// executions.
 // Precisely:
-//  x_new != x_prev does not imply x.y.z_new != x.y.z_prev
+//  x_new != x_prev does NOT imply x.y.z_new != x.y.z_prev
 //  x.y.z_new != x.y.z_prev does imply x_new != x_prev
-// One fix would be to depend on optional chains
 function useHook(x) {
  return useCallback(() => [x.y.z], [x]);
 }
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useHook,
+  params: [{ y: { z: 2 } }],
+};
@@ -0,0 +1,60 @@
+
+## Input
+
+```javascript
+// @validatePreserveExistingMemoizationGuarantees
+
+import { useMemo } from "react";
+
+// More specific memoization always results in fewer memo block
+// executions.
+// Precisely:
+//  x_new != x_prev does NOT imply x.y.z_new != x.y.z_prev
+//  x.y.z_new != x.y.z_prev does imply x_new != x_prev
+function useHook(x) {
+  return useMemo(() => [x.y.z], [x]);
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useHook,
+  params: [{ y: { z: 2 } }],
+};
+
+```
+
+## Code
+
+```javascript
+// @validatePreserveExistingMemoizationGuarantees
+
+import { useMemo, unstable_useMemoCache as useMemoCache } from "react";
+
+// More specific memoization always results in fewer memo block
+// executions.
+// Precisely:
+//  x_new != x_prev does NOT imply x.y.z_new != x.y.z_prev
+//  x.y.z_new != x.y.z_prev does imply x_new != x_prev
+function useHook(x) {
+  const $ = useMemoCache(2);
+  let t0;
+  let t1;
+  if ($[0] !== x.y.z) {
+    t1 = [x.y.z];
+    $[0] = x.y.z;
+    $[1] = t1;
+  } else {
+    t1 = $[1];
+  }
+  t0 = t1;
+  return t0;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useHook,
+  params: [{ y: { z: 2 } }],
+};
+
+```
+      
+### Eval output
+(kind: ok) [2]
@@ -0,0 +1,17 @@
+// @validatePreserveExistingMemoizationGuarantees
+
+import { useMemo } from "react";
+
+// More specific memoization always results in fewer memo block
+// executions.
+// Precisely:
+//  x_new != x_prev does NOT imply x.y.z_new != x.y.z_prev
+//  x.y.z_new != x.y.z_prev does imply x_new != x_prev
+function useHook(x) {
+  return useMemo(() => [x.y.z], [x]);
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useHook,
+  params: [{ y: { z: 2 } }],
+};