[compiler] Distingush optional/extraneous deps

In ValidateExhaustiveDependencies, I previously changed to allow extraneous dependencies as long as they were non-reactive. Here we make that more precise, and distinguish between values that are definitely referenced in the memo function but optional as dependencies vs values that are not even referenced in the memo function. The latter now error as extraneous even if they're non-reactive. This also turned up a case where constant-folded primitives could show up as false positives of the latter category, so now we track manual deps which quality for constant folding and don't error on them.

Author: josephsavona

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

@@ -803,6 +803,7 @@ export type ManualMemoDependency = {
     | {
         kind: 'NamedLocal';
         value: Place;
+        constant: boolean;
       }
     | {kind: 'Global'; identifierName: string};
   path: DependencyPath;

compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts

@@ -92,6 +92,7 @@ export function collectMaybeMemoDependencies(
           root: {
             kind: 'NamedLocal',
             value: {...value.place},
+            constant: false,
           },
           path: [],
         };

compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts

@@ -609,6 +609,19 @@ function evaluateInstruction(
       constantPropagationImpl(value.loweredFunc.func, constants);
       return null;
     }
+    case 'StartMemoize': {
+      if (value.deps != null) {
+        for (const dep of value.deps) {
+          if (dep.root.kind === 'NamedLocal') {
+            const placeValue = read(constants, dep.root.value);
+            if (placeValue != null && placeValue.kind === 'Primitive') {
+              dep.root.constant = true;
+            }
+          }
+        }
+      }
+      return null;
+    }
     default: {
       // TODO: handle more cases
       return null;

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

@@ -22,6 +22,7 @@ import {
   Identifier,
   IdentifierId,
   InstructionKind,
+  isPrimitiveType,
   isStableType,
   isSubPath,
   isSubPathIgnoringOptionals,
@@ -39,6 +40,7 @@ import {
 } from '../HIR/visitors';
 import {Result} from '../Utils/Result';
 import {retainWhere} from '../Utils/utils';
+import {printType} from '../HIR/PrintHIR';
 
 const DEBUG = false;
 
@@ -53,20 +55,18 @@ const DEBUG = false;
  * - If the manual dependencies had extraneous deps, then auto memoization
  *   will remove them and cause the value to update *less* frequently.
  *
- * 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.
+ * The implementation compares the manual dependencies against the values
+ * actually used within the memoization function
+ * - For each value V referenced in the memo function, either:
+ *   - If the value is non-reactive *and* a known stable type, then the
+ *     value may optionally be specified as an exact dependency.
+ *   - Otherwise, report an error unless there is a manual dependency that will
+ *     invalidate whenever V invalidates. If `x.y.z` is referenced, there must
+ *     be a manual dependency for `x.y.z`, `x.y`, or `x`. Note that we assume
+ *     no interior mutability, ie we assume that any changes to inner paths must
+ *     always cause the other path to change as well.
+ * - Any dependencies that do not correspond to a value referenced in the memo
+ *   function are considered extraneous and throw an error
  *
  * ## TODO: Invalid, Complex Deps
  *
@@ -226,9 +226,6 @@ export function validateExhaustiveDependencies(
         reason: 'Unexpected function dependency',
         loc: value.loc,
       });
-      const isRequiredDependency = reactive.has(
-        inferredDependency.identifier.id,
-      );
       let hasMatchingManualDependency = false;
       for (const manualDependency of manualDependencies) {
         if (
@@ -243,32 +240,40 @@ export function validateExhaustiveDependencies(
         ) {
           hasMatchingManualDependency = true;
           matched.add(manualDependency);
-          if (!isRequiredDependency) {
-            extra.push(manualDependency);
-          }
         }
       }
-      if (isRequiredDependency && !hasMatchingManualDependency) {
-        missing.push(inferredDependency);
+      const isOptionalDependency =
+        !reactive.has(inferredDependency.identifier.id) &&
+        (isStableType(inferredDependency.identifier) ||
+          isPrimitiveType(inferredDependency.identifier));
+      if (hasMatchingManualDependency || isOptionalDependency) {
+        continue;
       }
+      missing.push(inferredDependency);
     }
 
     for (const dep of startMemo.deps ?? []) {
       if (matched.has(dep)) {
         continue;
       }
+      if (dep.root.kind === 'NamedLocal' && dep.root.constant) {
+        CompilerError.simpleInvariant(
+          !dep.root.value.reactive &&
+            isPrimitiveType(dep.root.value.identifier),
+          {
+            reason: 'Expected constant-folded dependency to be non-reactive',
+            loc: dep.root.value.loc,
+          },
+        );
+        /*
+         * Constant primitives can get constant-folded, which means we won't
+         * see a LoadLocal for the value within the memo function.
+         */
+        continue;
+      }
       extra.push(dep);
     }
 
-    /**
-     * 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 => {
-      return dep.root.kind === 'Global' || dep.root.value.reactive;
-    });
-
     if (missing.length !== 0 || extra.length !== 0) {
       let suggestions: Array<CompilerSuggestion> | null = null;
       if (startMemo.depsLoc != null && typeof startMemo.depsLoc !== 'symbol') {
@@ -313,7 +318,7 @@ export function validateExhaustiveDependencies(
         });
         diagnostic.withDetails({
           kind: 'error',
-          message: `Unnecessary dependencies ${extra.map(dep => `\`${printManualMemoDependency(dep)}\``).join(', ')}`,
+          message: `Unnecessary dependencies ${extra.map(dep => `\`${printManualMemoDependency(dep)}\` (${dep.root.kind === 'NamedLocal' ? printType(dep.root.value.identifier.type) : ''})`).join(', ')}`,
           loc: startMemo.depsLoc ?? value.loc,
         });
         error.pushDiagnostic(diagnostic);

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

@@ -267,6 +267,7 @@ function validateInferredDep(
           effect: Effect.Read,
           reactive: false,
         },
+        constant: false,
       },
       path: [...dep.path],
     };
@@ -379,6 +380,7 @@ class Visitor extends ReactiveFunctionVisitor<VisitorState> {
                 root: {
                   kind: 'NamedLocal',
                   value: storeTarget,
+                  constant: false,
                 },
                 path: [],
               });
@@ -408,6 +410,7 @@ class Visitor extends ReactiveFunctionVisitor<VisitorState> {
         root: {
           kind: 'NamedLocal',
           value: {...lvalue},
+          constant: false,
         },
         path: [],
       });

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/context-variable-as-jsx-element-tag.expect.md

@@ -11,7 +11,7 @@ function Component(props) {
 
   Component = useMemo(() => {
     return Component;
-  });
+  }, [Component]);
 
   return <Component {...props} />;
 }
@@ -36,6 +36,7 @@ function Component(props) {
   if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
     Component = Stringify;
 
+    Component;
     Component = Component;
     $[0] = Component;
   } else {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/context-variable-as-jsx-element-tag.js

@@ -7,7 +7,7 @@ function Component(props) {
 
   Component = useMemo(() => {
     return Component;
-  });
+  }, [Component]);
 
   return <Component {...props} />;
 }

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

@@ -87,7 +87,7 @@ error.invalid-exhaustive-deps.ts:31:5
   29 |     return [];
   30 |     // error: unnecessary
 > 31 |   }, [x, y.z, z?.y?.a, UNUSED_GLOBAL]);
-     |      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Unnecessary dependencies `x`, `y.z`, `z?.y?.a`, `UNUSED_GLOBAL`
+     |      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Unnecessary dependencies `x` (), `y.z` (), `z?.y?.a` (), `UNUSED_GLOBAL` ()
   32 |   const ref1 = useRef(null);
   33 |   const ref2 = useRef(null);
   34 |   const ref = z ? ref1 : ref2;

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/todo-ensure-constant-prop-decls-get-removed.expect.md

@@ -2,7 +2,7 @@
 ## Input
 
 ```javascript
-// @validatePreserveExistingMemoizationGuarantees
+// @validatePreserveExistingMemoizationGuarantees @validateExhaustiveMemoizationDependencies:false
 
 import {useMemo} from 'react';
 
@@ -27,7 +27,7 @@ export const FIXTURE_ENTRYPOINT = {
 ## Code
 
 ```javascript
-import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees
+import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees @validateExhaustiveMemoizationDependencies:false
 
 import { useMemo } from "react";
 

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/todo-ensure-constant-prop-decls-get-removed.ts

@@ -1,4 +1,4 @@
-// @validatePreserveExistingMemoizationGuarantees
+// @validatePreserveExistingMemoizationGuarantees @validateExhaustiveMemoizationDependencies:false
 
 import {useMemo} from 'react';