[compiler] ValidateExhaustiveDeps disallows unnecessary non-reactive deps (facebook#34472)

Just to be consistent, we disallow unnecessary deps even if they're
known to be non-reactive.

Author: josephsavona

compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts

@@ -600,7 +600,8 @@ function printErrorSummary(category: ErrorCategory, message: string): string {
     case ErrorCategory.Suppression:
     case ErrorCategory.Syntax:
     case ErrorCategory.UseMemo:
-    case ErrorCategory.VoidUseMemo: {
+    case ErrorCategory.VoidUseMemo:
+    case ErrorCategory.MemoDependencies: {
       heading = 'Error';
       break;
     }
@@ -658,6 +659,10 @@ export enum ErrorCategory {
    * Checks that manual memoization is preserved
    */
   PreserveManualMemo = 'PreserveManualMemo',
+  /**
+   * Checks for exhaustive useMemo/useCallback dependencies without extraneous values
+   */
+  MemoDependencies = 'MemoDependencies',
   /**
    * Checks for known incompatible libraries
    */
@@ -1055,6 +1060,16 @@ function getRuleForCategoryImpl(category: ErrorCategory): LintRule {
         preset: LintRulePreset.RecommendedLatest,
       };
     }
+    case ErrorCategory.MemoDependencies: {
+      return {
+        category,
+        severity: ErrorSeverity.Error,
+        name: 'memo-dependencies',
+        description:
+          'Validates that useMemo() and useCallback() specify comprehensive dependencies without extraneous values. See [`useMemo()` docs](https://react.dev/reference/react/useMemo) for more information.',
+        preset: LintRulePreset.RecommendedLatest,
+      };
+    }
     case ErrorCategory.IncompatibleLibrary: {
       return {
         category,

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

@@ -22,7 +22,9 @@ import {
   Identifier,
   IdentifierId,
   InstructionKind,
+  isStableType,
   isSubPath,
+  isUseRefType,
   LoadGlobal,
   ManualMemoDependency,
   Place,
@@ -46,9 +48,10 @@ const DEBUG = false;
  * or less times.
  *
  * TODOs:
- * - Better handling of cases where we infer multiple dependencies related to a single
- *   variable. Eg if the user has dep `x` and we inferred `x.y, x.z`, the user's dep
- *   is sufficient.
+ * - 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
@@ -108,7 +111,7 @@ export function validateExhaustiveDependencies(
     );
     visitCandidateDependency(value.decl, temporaries, dependencies, locals);
     const inferred: Array<InferredDependency> = Array.from(dependencies);
-    // Sort dependencies by name, and path, with shorter/non-optional paths first
+    // Sort dependencies by name and path, with shorter/non-optional paths first
     inferred.sort((a, b) => {
       if (a.kind === 'Global' && b.kind == 'Global') {
         return a.binding.name.localeCompare(b.binding.name);
@@ -205,6 +208,31 @@ 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);
       let hasMatchingManualDependency = false;
       for (const manualDependency of manualDependencies) {
         if (
@@ -216,19 +244,18 @@ export function validateExhaustiveDependencies(
         ) {
           hasMatchingManualDependency = true;
           matched.add(manualDependency);
+          if (!isRequiredDependency) {
+            extra.push(manualDependency);
+          }
         }
       }
-      if (!hasMatchingManualDependency) {
+      if (isRequiredDependency && !hasMatchingManualDependency) {
         missing.push(inferredDependency);
       }
     }
 
     for (const dep of startMemo.deps ?? []) {
-      if (
-        matched.has(dep) ||
-        (dep.root.kind === 'NamedLocal' &&
-          !reactive.has(dep.root.value.identifier.id))
-      ) {
+      if (matched.has(dep)) {
         continue;
       }
       extra.push(dep);
@@ -247,36 +274,39 @@ export function validateExhaustiveDependencies(
         ];
       }
       if (missing.length !== 0) {
-        // Error
         const diagnostic = CompilerDiagnostic.create({
-          category: ErrorCategory.PreserveManualMemo,
+          category: ErrorCategory.MemoDependencies,
           reason: 'Found non-exhaustive dependencies',
           description:
             'Missing dependencies can cause a value not to update when those inputs change, ' +
-            'resulting in stale UI. This memoization cannot be safely rewritten by the compiler.',
+            'resulting in stale UI',
           suggestions,
         });
         for (const dep of missing) {
+          let reactiveStableValueHint = '';
+          if (isStableType(dep.identifier)) {
+            reactiveStableValueHint =
+              '. Refs, setState functions, and other "stable" values generally do not need to be added as dependencies, but this variable may change over time to point to different values';
+          }
           diagnostic.withDetails({
             kind: 'error',
-            message: `Missing dependency \`${printInferredDependency(dep)}\``,
+            message: `Missing dependency \`${printInferredDependency(dep)}\`${reactiveStableValueHint}`,
             loc: dep.loc,
           });
         }
         error.pushDiagnostic(diagnostic);
       } else if (extra.length !== 0) {
         const diagnostic = CompilerDiagnostic.create({
-          category: ErrorCategory.PreserveManualMemo,
+          category: ErrorCategory.MemoDependencies,
           reason: 'Found unnecessary memoization dependencies',
           description:
             'Unnecessary dependencies can cause a value to update more often than necessary, ' +
-            'which can cause effects to run more than expected. This memoization cannot be safely ' +
-            'rewritten by the compiler',
+            'which can cause effects to run more than expected',
         });
         diagnostic.withDetails({
           kind: 'error',
           message: `Unnecessary dependencies ${extra.map(dep => `\`${printManualMemoDependency(dep)}\``).join(', ')}`,
-          loc: value.loc,
+          loc: startMemo.depsLoc ?? value.loc,
         });
         error.pushDiagnostic(diagnostic);
       }
@@ -287,10 +317,15 @@ export function validateExhaustiveDependencies(
     startMemo = null;
   }
 
-  collectDependencies(fn, temporaries, {
-    onStartMemoize,
-    onFinishMemoize,
-  });
+  collectDependencies(
+    fn,
+    temporaries,
+    {
+      onStartMemoize,
+      onFinishMemoize,
+    },
+    false, // isFunctionExpression
+  );
   return error.asResult();
 }
 
@@ -383,12 +418,20 @@ function collectDependencies(
       locals: Set<IdentifierId>,
     ) => void;
   } | null,
+  isFunctionExpression: boolean,
 ): Extract<Temporary, {kind: 'Function'}> {
   const optionals = findOptionalPlaces(fn);
   if (DEBUG) {
     console.log(prettyFormat(optionals));
   }
   const locals: Set<IdentifierId> = new Set();
+  if (isFunctionExpression) {
+    for (const param of fn.params) {
+      const place = param.kind === 'Identifier' ? param : param.place;
+      locals.add(place.identifier.id);
+    }
+  }
+
   const dependencies: Set<InferredDependency> = new Set();
   function visit(place: Place): void {
     visitCandidateDependency(place, temporaries, dependencies, locals);
@@ -523,7 +566,11 @@ function collectDependencies(
           break;
         }
         case 'PropertyLoad': {
-          if (typeof value.property === 'number') {
+          if (
+            typeof value.property === 'number' ||
+            (isUseRefType(value.object.identifier) &&
+              value.property === 'current')
+          ) {
             visit(value.object);
             break;
           }
@@ -553,6 +600,7 @@ function collectDependencies(
             value.loweredFunc.func,
             temporaries,
             null,
+            true, // isFunctionExpression
           );
           temporaries.set(lvalue.identifier.id, functionDeps);
           addDependency(functionDeps, dependencies, locals);

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

@@ -26,8 +26,16 @@ function Component({x, y, z}) {
   }, [x]);
   const f = useMemo(() => {
     return [];
-  }, [x, y.z, z?.y?.a]);
-  return <Stringify results={[a, b, c, d, e, f]} />;
+  }, [x, y.z, z?.y?.a, UNUSED_GLOBAL]);
+  const ref1 = useRef(null);
+  const ref2 = useRef(null);
+  const ref = z ? ref1 : ref2;
+  const cb = useMemo(() => {
+    return () => {
+      return ref.current;
+    };
+  }, []);
+  return <Stringify results={[a, b, c, d, e, f, cb]} />;
 }
 
 ```
@@ -36,11 +44,11 @@ function Component({x, y, z}) {
 ## Error
 
 ```
-Found 4 errors:
+Found 5 errors:
 
-Compilation Skipped: Found non-exhaustive dependencies
+Error: Found non-exhaustive dependencies
 
-Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI. This memoization cannot be safely rewritten by the compiler..
+Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI.
 
 error.invalid-exhaustive-deps.ts:7:11
    5 | function Component({x, y, z}) {
@@ -51,9 +59,9 @@ error.invalid-exhaustive-deps.ts:7:11
    9 |   const b = useMemo(() => {
   10 |     return x.y.z?.a;
 
-Compilation Skipped: Found non-exhaustive dependencies
+Error: Found non-exhaustive dependencies
 
-Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI. This memoization cannot be safely rewritten by the compiler..
+Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI.
 
 error.invalid-exhaustive-deps.ts:10:11
    8 |   }, [x?.y.z?.a.b]);
@@ -64,9 +72,9 @@ error.invalid-exhaustive-deps.ts:10:11
   12 |   const c = useMemo(() => {
   13 |     return x?.y.z.a?.b;
 
-Compilation Skipped: Found non-exhaustive dependencies
+Error: Found non-exhaustive dependencies
 
-Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI. This memoization cannot be safely rewritten by the compiler..
+Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI.
 
 error.invalid-exhaustive-deps.ts:13:11
   11 |   }, [x.y.z.a]);
@@ -77,22 +85,31 @@ error.invalid-exhaustive-deps.ts:13:11
   15 |   const d = useMemo(() => {
   16 |     return x?.y?.[(console.log(y), z?.b)];
 
-Compilation Skipped: Found unnecessary memoization dependencies
-
-Unnecessary dependencies can cause a value to update more often than necessary, which can cause effects to run more than expected. This memoization cannot be safely rewritten by the compiler.
-
-error.invalid-exhaustive-deps.ts:23:20
-  21 |     return e;
-  22 |   }, [x]);
-> 23 |   const f = useMemo(() => {
-     |                     ^^^^^^^
-> 24 |     return [];
-     | ^^^^^^^^^^^^^^
-> 25 |   }, [x, y.z, z?.y?.a]);
-     | ^^^^ Unnecessary dependencies `x`, `y.z`, `z?.y?.a`
-  26 |   return <Stringify results={[a, b, c, d, e, f]} />;
-  27 | }
-  28 |
+Error: Found unnecessary memoization dependencies
+
+Unnecessary dependencies can cause a value to update more often than necessary, which can cause effects to run more than expected.
+
+error.invalid-exhaustive-deps.ts:25:5
+  23 |   const f = useMemo(() => {
+  24 |     return [];
+> 25 |   }, [x, y.z, z?.y?.a, UNUSED_GLOBAL]);
+     |      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Unnecessary dependencies `x`, `y.z`, `z?.y?.a`, `UNUSED_GLOBAL`
+  26 |   const ref1 = useRef(null);
+  27 |   const ref2 = useRef(null);
+  28 |   const ref = z ? ref1 : ref2;
+
+Error: Found non-exhaustive dependencies
+
+Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI.
+
+error.invalid-exhaustive-deps.ts:31:13
+  29 |   const cb = useMemo(() => {
+  30 |     return () => {
+> 31 |       return ref.current;
+     |              ^^^ Missing dependency `ref`. Refs, setState functions, and other "stable" values generally do not need to be added as dependencies, but this variable may change over time to point to different values
+  32 |     };
+  33 |   }, []);
+  34 |   return <Stringify results={[a, b, c, d, e, f, cb]} />;
 ```
           
       

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

@@ -22,6 +22,14 @@ function Component({x, y, z}) {
   }, [x]);
   const f = useMemo(() => {
     return [];
-  }, [x, y.z, z?.y?.a]);
-  return <Stringify results={[a, b, c, d, e, f]} />;
+  }, [x, y.z, z?.y?.a, UNUSED_GLOBAL]);
+  const ref1 = useRef(null);
+  const ref2 = useRef(null);
+  const ref = z ? ref1 : ref2;
+  const cb = useMemo(() => {
+    return () => {
+      return ref.current;
+    };
+  }, []);
+  return <Stringify results={[a, b, c, d, e, f, cb]} />;
 }