[compiler] Allow extraneous non-reactive locals

The existing exhaustive-deps rule allows omitting non-reactive dependencies, even if they're not memoized. Conceptually, if a value is non-reactive then it cannot semantically change. Even if the value is a new object, that object represents the exact same value and doesn't necessitate redoing downstream computation. Thus its fine to exclude nonreactive dependencies, whether they're a stable type or not.

Author: josephsavona

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

@@ -43,20 +43,37 @@ import {retainWhere} from '../Utils/utils';
 const DEBUG = false;
 
 /**
- * Validates that existing manual memoization had exhaustive dependencies.
- * Memoization with missing or extra reactive dependencies is invalid React
- * and compilation can change behavior, causing a value to be computed more
- * or less times.
+ * Validates that existing manual memoization is exhaustive and does not
+ * have extraneous dependencies. The goal of the validation is to ensure
+ * that auto-memoization will not substantially change the behavior of
+ * the program:
+ * - If the manual dependencies were non-exhaustive (missing important deps)
+ *   then auto-memoization will include those dependencies, and cause the
+ *   value to update *more* frequently.
+ * - If the manual dependencies had extraneous deps, then auto memoization
+ *   will remove them and cause the value to update *less* frequently.
  *
- * TODOs:
- * - Handle cases of mixed optional and non-optional versions of the same path,
- *   eg referecing both x.y.z and x.y?.z in the same memo block. we should collapse
- *   this into a single canonical dep that we look for in the manual deps. see the
- *   existing exhaustive deps rule for implementation.
- * - Handle cases where the user deps were not simple identifiers + property chains.
- *   We try to detect this in ValidateUseMemo but we miss some cases. The problem
- *   is that invalid forms can be value blocks or function calls that don't get
- *   removed by DCE, leaving a structure like:
+ * We consider a value V as missing if ALL of the following conditions are met:
+ * - V is reactive
+ * - There is no manual dependency path P such that whenever V would change,
+ *   P would also change. If V is `x.y.z`, this means there must be some
+ *   path P that is either `x.y.z`, `x.y`, or `x`. Note that we assume no
+ *   interior mutability, such that a shorter path "covers" changes to longer
+ *   more precise paths.
+ *
+ * We consider a value V extraneous if either of the folowing are true:
+ * - V is a reactive local that is unreferenced
+ * - V is a global that is unreferenced
+ *
+ * In other words, we allow extraneous non-reactive values since we know they cannot
+ * impact how often the memoization would run.
+ *
+ * ## TODO: Invalid, Complex Deps
+ *
+ * Handle cases where the user deps were not simple identifiers + property chains.
+ * We try to detect this in ValidateUseMemo but we miss some cases. The problem
+ * is that invalid forms can be value blocks or function calls that don't get
+ * removed by DCE, leaving a structure like:
  *
  * StartMemoize
  * t0 = <value to memoize>
@@ -209,31 +226,9 @@ export function validateExhaustiveDependencies(
         reason: 'Unexpected function dependency',
         loc: value.loc,
       });
-      /**
-       * Dependencies technically only need to include reactive values. However,
-       * reactivity inference for general values is subtle since it involves all
-       * of our complex control and data flow analysis. To keep results more
-       * stable and predictable to developers, we intentionally stay closer to
-       * the rules of the classic exhaustive-deps rule. Values should be included
-       * as dependencies if either of the following is true:
-       * - They're reactive
-       * - They're non-reactive and not a known-stable value type.
-       *
-       * Thus `const ref: Ref = cond ? ref1 : ref2` has to be a dependency
-       * (assuming `cond` is reactive) since it's reactive despite being a ref.
-       *
-       * Similarly, `const x = [1,2,3]` has to be a dependency since even
-       * though it's non reactive, it's not a known stable type.
-       *
-       * TODO: consider reimplementing a simpler form of reactivity inference.
-       * Ideally we'd consider `const ref: Ref = cond ? ref1 : ref2` as a required
-       * dependency even if our data/control flow tells us that `cond` is non-reactive.
-       * It's simpler for developers to reason about based on a more structural/AST
-       * driven approach.
-       */
-      const isRequiredDependency =
-        reactive.has(inferredDependency.identifier.id) ||
-        !isStableType(inferredDependency.identifier);
+      const isRequiredDependency = reactive.has(
+        inferredDependency.identifier.id,
+      );
       let hasMatchingManualDependency = false;
       for (const manualDependency of manualDependencies) {
         if (
@@ -265,18 +260,13 @@ export function validateExhaustiveDependencies(
       extra.push(dep);
     }
 
-    /*
-     * For compatiblity with the existing exhaustive-deps rule, we allow
-     * known-stable values as dependencies even if the value is not reactive.
-     * This allows code that takes a dep on a non-reactive setState function
-     * to pass, for example.
+    /**
+     * Per docblock, we only consider dependencies as extraneous if
+     * they are unused globals or reactive locals. Notably, this allows
+     * non-reactive locals.
      */
     retainWhere(extra, dep => {
-      const isNonReactiveStableValue =
-        dep.root.kind === 'NamedLocal' &&
-        !dep.root.value.reactive &&
-        isStableType(dep.root.value.identifier);
-      return !isNonReactiveStableValue;
+      return dep.root.kind === 'Global' || dep.root.value.reactive;
     });
 
     if (missing.length !== 0 || extra.length !== 0) {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps.expect.md

@@ -3,7 +3,7 @@
 
 ```javascript
 // @validateExhaustiveMemoizationDependencies
-import {useMemo} from 'react';
+import {useCallback, useMemo} from 'react';
 import {makeObject_Primitives, Stringify} from 'shared-runtime';
 
 function useHook1(x) {
@@ -47,14 +47,25 @@ function useHook6(x) {
   }, [x]);
 }
 
+function useHook7(x) {
+  const [state, setState] = useState(true);
+  const f = () => {
+    setState(x => !x);
+  };
+  return useCallback(() => {
+    f();
+  }, [f]);
+}
+
 function Component({x, y, z}) {
   const a = useHook1(x);
   const b = useHook2(x);
   const c = useHook3(x);
   const d = useHook4(x, y, z);
   const e = useHook5(x);
   const f = useHook6(x);
-  return <Stringify results={[a, b, c, d, e, f]} />;
+  const g = useHook7(x);
+  return <Stringify results={[a, b, c, d, e, f, g]} />;
 }
 
 ```
@@ -63,7 +74,7 @@ function Component({x, y, z}) {
 
 ```javascript
 import { c as _c } from "react/compiler-runtime"; // @validateExhaustiveMemoizationDependencies
-import { useMemo } from "react";
+import { useCallback, useMemo } from "react";
 import { makeObject_Primitives, Stringify } from "shared-runtime";
 
 function useHook1(x) {
@@ -121,34 +132,65 @@ function useHook6(x) {
   return f;
 }
 
+function useHook7(x) {
+  const $ = _c(2);
+  const [, setState] = useState(true);
+  let t0;
+  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
+    t0 = () => {
+      setState(_temp);
+    };
+    $[0] = t0;
+  } else {
+    t0 = $[0];
+  }
+  const f = t0;
+  let t1;
+  if ($[1] === Symbol.for("react.memo_cache_sentinel")) {
+    t1 = () => {
+      f();
+    };
+    $[1] = t1;
+  } else {
+    t1 = $[1];
+  }
+  return t1;
+}
+function _temp(x_0) {
+  return !x_0;
+}
+
 function Component(t0) {
-  const $ = _c(7);
+  const $ = _c(8);
   const { x, y, z } = t0;
   const a = useHook1(x);
   const b = useHook2(x);
   const c = useHook3(x);
   const d = useHook4(x, y, z);
   const e = useHook5(x);
   const f = useHook6(x);
+  const g = useHook7(x);
   let t1;
   if (
     $[0] !== a ||
     $[1] !== b ||
     $[2] !== c ||
     $[3] !== d ||
     $[4] !== e ||
-    $[5] !== f
+    $[5] !== f ||
+    $[6] !== g
   ) {
-    t1 = <Stringify results={[a, b, c, d, e, f]} />;
+    t1 = <Stringify results={[a, b, c, d, e, f, g]} />;
     $[0] = a;
     $[1] = b;
     $[2] = c;
     $[3] = d;
     $[4] = e;
     $[5] = f;
-    $[6] = t1;
+    $[6] = g;
+    $[7] = t1;
   } else {
-    t1 = $[6];
+    t1 = $[7];
   }
   return t1;
 }

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps.js

@@ -1,5 +1,5 @@
 // @validateExhaustiveMemoizationDependencies
-import {useMemo} from 'react';
+import {useCallback, useMemo} from 'react';
 import {makeObject_Primitives, Stringify} from 'shared-runtime';
 
 function useHook1(x) {
@@ -43,12 +43,23 @@ function useHook6(x) {
   }, [x]);
 }
 
+function useHook7(x) {
+  const [state, setState] = useState(true);
+  const f = () => {
+    setState(x => !x);
+  };
+  return useCallback(() => {
+    f();
+  }, [f]);
+}
+
 function Component({x, y, z}) {
   const a = useHook1(x);
   const b = useHook2(x);
   const c = useHook3(x);
   const d = useHook4(x, y, z);
   const e = useHook5(x);
   const f = useHook6(x);
-  return <Stringify results={[a, b, c, d, e, f]} />;
+  const g = useHook7(x);
+  return <Stringify results={[a, b, c, d, e, f, g]} />;
 }