Extend effect dep validation to handle pruned memoization

Extends the validation that effect deps are memoized to handle an additional case that @gsathya pointed out: when a dependency has a reactive scope but that scope ends up being pruned. We track reactive scopes which actually exist in the ReactiveFunction, and reject useEffect deps that have an associated reactive scope but where that scope does not exist (bc it got pruned).
2025-11-01 09:12:30 +00:00 · 2023-11-27 12:31:27 -08:00
parent d7db416167
commit 445baf9ae4
8 changed files with 193 additions and 7 deletions
@@ -1016,6 +1016,14 @@ export type ReactiveScope = {
  dependencies: ReactiveScopeDependencies;
  declarations: Map<IdentifierId, ReactiveScopeDeclaration>;
  reassignments: Set<Identifier>;
+
+  /*
+   * Some passes may merge scopes together. The merged set contains the
+   * ids of scopes that were merged into this one, for passes that need
+   * to track which scopes are still present (in some form) vs scopes that
+   * no longer exist due to being pruned.
+   */
+  merged: Set<ScopeId>;
 };

 export type ReactiveScopeDependencies = Set<ReactiveScopeDependency>;
@@ -199,6 +199,7 @@ export function inferReactiveScopeVariables(fn: HIRFunction): void {
        dependencies: new Set(),
        declarations: new Map(),
        reassignments: new Set(),
+        merged: new Set(),
      };
      scopes.set(groupIdentifier, scope);
    } else {
@@ -314,6 +314,7 @@ class Transform extends ReactiveFunctionTransform<ReactiveScopeDependencies | nu
        const instr = block[index++]!;
        if (instr.kind === "scope") {
          mergedScope.instructions.push(...instr.instructions);
+          mergedScope.scope.merged.add(instr.scope.id);
        } else {
          mergedScope.instructions.push(instr);
        }
@@ -11,6 +11,8 @@ import {
  Instruction,
  ReactiveFunction,
  ReactiveInstruction,
+  ReactiveScopeBlock,
+  ScopeId,
  isUseEffectHookType,
  isUseInsertionEffectHookType,
  isUseLayoutEffectHookType,
@@ -22,12 +24,16 @@ import {
 } from "../ReactiveScopes/visitors";

 /**
- * Validates that all known effect dependencies are memoized. The algorithm does not directly check
- * for memoization but instead uses an inverted test: it reports any effects whose dependency arrays
- * are mutable for a range that encompasses the effect call. This corresponds to any values which
- * Forget knows may be mutable and may be mutated after the effect. Note that it's possible Forget
- * may miss not memoize a value for some other reason, but in general this is a bug. The only reason
- * Forget would _choose_ to skip memoization of an effect dependency is because it's mutated later.
+ * Validates that all known effect dependencies are memoized. The algorithm checks two things:
+ * - Disallow effect dependencies that should be memoized (have a reactive scope assigned) but
+ *   where that reactive scope does not exist. This checks for cases where a reactive scope was
+ *   pruned for some reason, such as spanning a hook.
+ * - Disallow effect dependencies whose a mutable range that encompasses the effect call.
+ *
+ * This latter check corresponds to any values which Forget knows may be mutable and may be mutated
+ * after the effect. Note that it's possible Forget may miss not memoize a value for some other reason,
+ * but in general this is a bug. The only reason Forget would _choose_ to skip memoization of an
+ * effect dependency is because it's mutated later.
 *
 * Example:
 *
@@ -50,6 +56,36 @@ export function validateMemoizedEffectDependencies(fn: ReactiveFunction): void {
 }

 class Visitor extends ReactiveFunctionVisitor<CompilerError> {
+  scopes: Set<ScopeId> = new Set();
+
+  override visitScope(
+    scopeBlock: ReactiveScopeBlock,
+    state: CompilerError
+  ): void {
+    this.traverseScope(scopeBlock, state);
+
+    /*
+     * 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);
+      }
+    }
+  }
+
  override visitInstruction(
    instruction: ReactiveInstruction,
    state: CompilerError
@@ -63,7 +99,8 @@ class Visitor extends ReactiveFunctionVisitor<CompilerError> {
      const deps = instruction.value.args[1]!;
      if (
        deps.kind === "Identifier" &&
-        isMutable(instruction as Instruction, deps)
+        (isMutable(instruction as Instruction, deps) ||
+          isUnmemoized(deps.identifier, this.scopes))
      ) {
        state.push({
          reason:
@@ -78,6 +115,10 @@ class Visitor extends ReactiveFunctionVisitor<CompilerError> {
  }
 }

+function isUnmemoized(operand: Identifier, scopes: Set<ScopeId>): boolean {
+  return operand.scope != null && !scopes.has(operand.scope.id);
+}
+
 function isEffectHook(identifier: Identifier): boolean {
  return (
    isUseEffectHookType(identifier) ||
@@ -0,0 +1,29 @@
+
+## Input
+
+```javascript
+// @validateMemoizedEffectDependencies
+function Component(props) {
+  // Items cannot be memoized bc its mutation spans a hook call
+  const items = [props.value];
+  const [state, _setState] = useState(null);
+  mutate(items);
+
+  // Items is no longer mutable here, but it hasn't been memoized
+  useEffect(() => {
+    console.log(items);
+  }, [items]);
+
+  return [items, state];
+}
+
+```
+
+
+## Error
+
+```
+[ReactForget] InvalidReact: This effect may trigger an infinite loop: one or more of its dependencies could not be memoized due to a later mutation (9:11)
+```
+          
+      
@@ -0,0 +1,14 @@
+// @validateMemoizedEffectDependencies
+function Component(props) {
+  // Items cannot be memoized bc its mutation spans a hook call
+  const items = [props.value];
+  const [state, _setState] = useState(null);
+  mutate(items);
+
+  // Items is no longer mutable here, but it hasn't been memoized
+  useEffect(() => {
+    console.log(items);
+  }, [items]);
+
+  return [items, state];
+}
@@ -0,0 +1,73 @@
+
+## Input
+
+```javascript
+// @validateMemoizedEffectDependencies
+
+import { useEffect } from "react";
+
+function Component(props) {
+  const y = [[props.value]]; // merged w scope for inner array
+
+  useEffect(() => {
+    console.log(y);
+  }, [y]); // should still be a valid dependency here
+
+  return y;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{ value: 42 }],
+  isComponent: false,
+};
+
+```
+
+## Code
+
+```javascript
+// @validateMemoizedEffectDependencies
+
+import { useEffect, unstable_useMemoCache as useMemoCache } from "react";
+
+function Component(props) {
+  const $ = useMemoCache(5);
+  let t0;
+  if ($[0] !== props.value) {
+    t0 = [[props.value]];
+    $[0] = props.value;
+    $[1] = t0;
+  } else {
+    t0 = $[1];
+  }
+  const y = t0;
+  let t1;
+  let t2;
+  if ($[2] !== y) {
+    t1 = () => {
+      console.log(y);
+    };
+    t2 = [y];
+    $[2] = y;
+    $[3] = t1;
+    $[4] = t2;
+  } else {
+    t1 = $[3];
+    t2 = $[4];
+  }
+  useEffect(t1, t2);
+  return y;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{ value: 42 }],
+  isComponent: false,
+};
+
+```
+      
+### Eval output
+(kind: ok) [[42]]
+logs: [[ [ 42 ] ]]
@@ -0,0 +1,19 @@
+// @validateMemoizedEffectDependencies
+
+import { useEffect } from "react";
+
+function Component(props) {
+  const y = [[props.value]]; // merged w scope for inner array
+
+  useEffect(() => {
+    console.log(y);
+  }, [y]); // should still be a valid dependency here
+
+  return y;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{ value: 42 }],
+  isComponent: false,
+};