[pull] main from facebook:main

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

@@ -221,7 +221,7 @@ export const EnvironmentConfigSchema = z.object({
   /**
    * Validate that dependencies supplied to manual memoization calls are exhaustive.
    */
-  validateExhaustiveMemoizationDependencies: z.boolean().default(false),
+  validateExhaustiveMemoizationDependencies: z.boolean().default(true),
 
   /**
    * When this is true, rather than pruning existing manual memoization but ensuring or validating

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

@@ -803,9 +803,11 @@ export type ManualMemoDependency = {
     | {
         kind: 'NamedLocal';
         value: Place;
+        constant: boolean;
       }
     | {kind: 'Global'; identifierName: string};
   path: DependencyPath;
+  loc: SourceLocation;
 };
 
 export type StartMemoize = {

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

@@ -65,6 +65,7 @@ export function collectMaybeMemoDependencies(
           identifierName: value.binding.name,
         },
         path: [],
+        loc: value.loc,
       };
     }
     case 'PropertyLoad': {
@@ -74,6 +75,7 @@ export function collectMaybeMemoDependencies(
           root: object.root,
           // TODO: determine if the access is optional
           path: [...object.path, {property: value.property, optional}],
+          loc: value.loc,
         };
       }
       break;
@@ -92,8 +94,10 @@ export function collectMaybeMemoDependencies(
           root: {
             kind: 'NamedLocal',
             value: {...value.place},
+            constant: false,
           },
           path: [],
+          loc: value.place.loc,
         };
       }
       break;

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,
@@ -53,20 +54,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 +225,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,81 +239,140 @@ export function validateExhaustiveDependencies(
         ) {
           hasMatchingManualDependency = true;
           matched.add(manualDependency);
-          if (!isRequiredDependency) {
-            extra.push(manualDependency);
-          }
         }
       }
-      if (isRequiredDependency && !hasMatchingManualDependency) {
-        missing.push(inferredDependency);
+      if (
+        hasMatchingManualDependency ||
+        isOptionalDependency(inferredDependency, reactive)
+      ) {
+        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;
+      let suggestion: CompilerSuggestion | null = null;
       if (startMemo.depsLoc != null && typeof startMemo.depsLoc !== 'symbol') {
-        suggestions = [
-          {
-            description: 'Update dependencies',
-            range: [startMemo.depsLoc.start.index, startMemo.depsLoc.end.index],
-            op: CompilerSuggestionOperation.Replace,
-            text: `[${inferred.map(printInferredDependency).join(', ')}]`,
-          },
-        ];
+        suggestion = {
+          description: 'Update dependencies',
+          range: [startMemo.depsLoc.start.index, startMemo.depsLoc.end.index],
+          op: CompilerSuggestionOperation.Replace,
+          text: `[${inferred
+            .filter(
+              dep =>
+                dep.kind === 'Local' && !isOptionalDependency(dep, reactive),
+            )
+            .map(printInferredDependency)
+            .join(', ')}]`,
+        };
       }
-      if (missing.length !== 0) {
-        const diagnostic = CompilerDiagnostic.create({
-          category: ErrorCategory.MemoDependencies,
-          reason: 'Found missing memoization dependencies',
-          description:
-            'Missing dependencies can cause a value not to update when those inputs change, ' +
-            'resulting in stale UI',
-          suggestions,
+      const diagnostic = CompilerDiagnostic.create({
+        category: ErrorCategory.MemoDependencies,
+        reason: 'Found missing/extra memoization dependencies',
+        description: [
+          missing.length !== 0
+            ? 'Missing dependencies can cause a value to update less often than it should, ' +
+              'resulting in stale UI'
+            : null,
+          extra.length !== 0
+            ? 'Extra dependencies can cause a value to update more often than it should, ' +
+              'resulting in performance problems such as excessive renders or effects firing too often'
+            : null,
+        ]
+          .filter(Boolean)
+          .join('. '),
+        suggestions: suggestion != null ? [suggestion] : null,
+      });
+      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)}\`${reactiveStableValueHint}`,
+          loc: dep.loc,
         });
-        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';
-          }
+      }
+      for (const dep of extra) {
+        if (dep.root.kind === 'Global') {
           diagnostic.withDetails({
             kind: 'error',
-            message: `Missing dependency \`${printInferredDependency(dep)}\`${reactiveStableValueHint}`,
-            loc: dep.loc,
+            message:
+              `Unnecessary dependency \`${printManualMemoDependency(dep)}\`. ` +
+              'Values declared outside of a component/hook should not be listed as ' +
+              'dependencies as the component will not re-render if they change',
+            loc: dep.loc ?? startMemo.depsLoc ?? value.loc,
           });
+          error.pushDiagnostic(diagnostic);
+        } else {
+          const root = dep.root.value;
+          const matchingInferred = inferred.find(
+            (
+              inferredDep,
+            ): inferredDep is Extract<InferredDependency, {kind: 'Local'}> => {
+              return (
+                inferredDep.kind === 'Local' &&
+                inferredDep.identifier.id === root.identifier.id &&
+                isSubPathIgnoringOptionals(inferredDep.path, dep.path)
+              );
+            },
+          );
+          if (
+            matchingInferred != null &&
+            !isOptionalDependency(matchingInferred, reactive)
+          ) {
+            diagnostic.withDetails({
+              kind: 'error',
+              message:
+                `Overly precise dependency \`${printManualMemoDependency(dep)}\`, ` +
+                `use \`${printInferredDependency(matchingInferred)}\` instead`,
+              loc: dep.loc ?? startMemo.depsLoc ?? value.loc,
+            });
+          } else {
+            /**
+             * Else this dependency doesn't correspond to anything referenced in the memo function,
+             * or is an optional dependency so we don't want to suggest adding it
+             */
+            diagnostic.withDetails({
+              kind: 'error',
+              message: `Unnecessary dependency \`${printManualMemoDependency(dep)}\``,
+              loc: dep.loc ?? startMemo.depsLoc ?? value.loc,
+            });
+          }
         }
-        error.pushDiagnostic(diagnostic);
-      } else if (extra.length !== 0) {
-        const diagnostic = CompilerDiagnostic.create({
-          category: ErrorCategory.MemoDependencies,
-          reason: 'Found unnecessary memoization dependencies',
-          description:
-            'Unnecessary dependencies can cause a value to update more often than necessary, ' +
-            'causing performance regressions and effects to fire more often than expected',
-        });
+      }
+      if (suggestion != null) {
         diagnostic.withDetails({
-          kind: 'error',
-          message: `Unnecessary dependencies ${extra.map(dep => `\`${printManualMemoDependency(dep)}\``).join(', ')}`,
-          loc: startMemo.depsLoc ?? value.loc,
+          kind: 'hint',
+          message: `Inferred dependencies: \`${suggestion.text}\``,
         });
-        error.pushDiagnostic(diagnostic);
       }
+      error.pushDiagnostic(diagnostic);
     }
 
     dependencies.clear();
@@ -822,3 +877,14 @@ export function findOptionalPlaces(
   }
   return optionals;
 }
+
+function isOptionalDependency(
+  inferredDependency: Extract<InferredDependency, {kind: 'Local'}>,
+  reactive: Set<IdentifierId>,
+): boolean {
+  return (
+    !reactive.has(inferredDependency.identifier.id) &&
+    (isStableType(inferredDependency.identifier) ||
+      isPrimitiveType(inferredDependency.identifier))
+  );
+}

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

@@ -242,6 +242,7 @@ function validateInferredDep(
     normalizedDep = {
       root: maybeNormalizedRoot.root,
       path: [...maybeNormalizedRoot.path, ...dep.path],
+      loc: maybeNormalizedRoot.loc,
     };
   } else {
     CompilerError.invariant(dep.identifier.name?.kind === 'named', {
@@ -267,8 +268,10 @@ function validateInferredDep(
           effect: Effect.Read,
           reactive: false,
         },
+        constant: false,
       },
       path: [...dep.path],
+      loc: GeneratedSource,
     };
   }
   for (const decl of declsWithinMemoBlock) {
@@ -379,8 +382,10 @@ class Visitor extends ReactiveFunctionVisitor<VisitorState> {
                 root: {
                   kind: 'NamedLocal',
                   value: storeTarget,
+                  constant: false,
                 },
                 path: [],
+                loc: storeTarget.loc,
               });
             }
           }
@@ -408,8 +413,10 @@ class Visitor extends ReactiveFunctionVisitor<VisitorState> {
         root: {
           kind: 'NamedLocal',
           value: {...lvalue},
+          constant: false,
         },
         path: [],
+        loc: lvalue.loc,
       });
     }
   }

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-modify-global-in-callback-jsx.expect.md

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

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-modify-global-in-callback-jsx.js

@@ -1,4 +1,4 @@
-// @enablePreserveExistingMemoizationGuarantees:false
+// @enablePreserveExistingMemoizationGuarantees:false @validateExhaustiveMemoizationDependencies:false
 import {useMemo} from 'react';
 
 const someGlobal = {value: 0};