[compiler] Auto-fix for non-exhaustive deps

Records more information in DropManualMemoization so that we know the full span of the manual dependencies array (if present). This allows ValidateExhaustiveDeps to include a suggestion with the correct deps.

Author: josephsavona

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

@@ -817,6 +817,11 @@ export type StartMemoize = {
    * (e.g. useMemo without a second arg)
    */
   deps: Array<ManualMemoDependency> | null;
+  /**
+   * The source location of the dependencies argument. Used for
+   * emitting diagnostics with a suggested replacement
+   */
+  depsLoc: SourceLocation | null;
   loc: SourceLocation;
 };
 export type FinishMemoize = {

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

@@ -42,7 +42,7 @@ type IdentifierSidemap = {
   functions: Map<IdentifierId, TInstruction<FunctionExpression>>;
   manualMemos: Map<IdentifierId, ManualMemoCallee>;
   react: Set<IdentifierId>;
-  maybeDepsLists: Map<IdentifierId, Array<Place>>;
+  maybeDepsLists: Map<IdentifierId, {loc: SourceLocation; deps: Array<Place>}>;
   maybeDeps: Map<IdentifierId, ManualMemoDependency>;
   optionals: Set<IdentifierId>;
 };
@@ -159,10 +159,10 @@ function collectTemporaries(
     }
     case 'ArrayExpression': {
       if (value.elements.every(e => e.kind === 'Identifier')) {
-        sidemap.maybeDepsLists.set(
-          instr.lvalue.identifier.id,
-          value.elements as Array<Place>,
-        );
+        sidemap.maybeDepsLists.set(instr.lvalue.identifier.id, {
+          loc: value.loc,
+          deps: value.elements as Array<Place>,
+        });
       }
       break;
     }
@@ -182,6 +182,7 @@ function makeManualMemoizationMarkers(
   fnExpr: Place,
   env: Environment,
   depsList: Array<ManualMemoDependency> | null,
+  depsLoc: SourceLocation | null,
   memoDecl: Place,
   manualMemoId: number,
 ): [TInstruction<StartMemoize>, TInstruction<FinishMemoize>] {
@@ -197,6 +198,7 @@ function makeManualMemoizationMarkers(
          * as dependencies
          */
         deps: depsList,
+        depsLoc,
         loc: fnExpr.loc,
       },
       effects: null,
@@ -287,86 +289,85 @@ function extractManualMemoizationArgs(
   sidemap: IdentifierSidemap,
   errors: CompilerError,
 ): {
-  fnPlace: Place | null;
+  fnPlace: Place;
   depsList: Array<ManualMemoDependency> | null;
-} {
+  depsLoc: SourceLocation | null;
+} | null {
   const [fnPlace, depsListPlace] = instr.value.args as Array<
     Place | SpreadPattern | undefined
   >;
-  if (fnPlace == null) {
+  if (fnPlace == null || fnPlace.kind !== 'Identifier') {
     errors.pushDiagnostic(
       CompilerDiagnostic.create({
         category: ErrorCategory.UseMemo,
         reason: `Expected a callback function to be passed to ${kind}`,
-        description: `Expected a callback function to be passed to ${kind}`,
+        description:
+          kind === 'useCallback'
+            ? 'The first argument to useCallback() must be a function to cache'
+            : 'The first argument to useMemo() must be a function that calculates a result to cache',
         suggestions: null,
       }).withDetails({
         kind: 'error',
         loc: instr.value.loc,
-        message: `Expected a callback function to be passed to ${kind}`,
+        message:
+          kind === 'useCallback'
+            ? `Expected a callback function`
+            : `Expected a memoization function`,
       }),
     );
-    return {fnPlace: null, depsList: null};
+    return null;
   }
-  if (fnPlace.kind === 'Spread' || depsListPlace?.kind === 'Spread') {
+  if (depsListPlace == null) {
+    return {
+      fnPlace,
+      depsList: null,
+      depsLoc: null,
+    };
+  }
+  const maybeDepsList =
+    depsListPlace.kind === 'Identifier'
+      ? sidemap.maybeDepsLists.get(depsListPlace.identifier.id)
+      : null;
+  if (maybeDepsList == null) {
     errors.pushDiagnostic(
       CompilerDiagnostic.create({
         category: ErrorCategory.UseMemo,
-        reason: `Unexpected spread argument to ${kind}`,
-        description: `Unexpected spread argument to ${kind}`,
+        reason: `Expected the dependency list for ${kind} to be an array literal`,
+        description: `Expected the dependency list for ${kind} to be an array literal`,
         suggestions: null,
       }).withDetails({
         kind: 'error',
-        loc: instr.value.loc,
-        message: `Unexpected spread argument to ${kind}`,
+        loc:
+          depsListPlace?.kind === 'Identifier' ? depsListPlace.loc : instr.loc,
+        message: `Expected the dependency list for ${kind} to be an array literal`,
       }),
     );
-    return {fnPlace: null, depsList: null};
+    return null;
   }
-  let depsList: Array<ManualMemoDependency> | null = null;
-  if (depsListPlace != null) {
-    const maybeDepsList = sidemap.maybeDepsLists.get(
-      depsListPlace.identifier.id,
-    );
-    if (maybeDepsList == null) {
+  const depsList: Array<ManualMemoDependency> = [];
+  for (const dep of maybeDepsList.deps) {
+    const maybeDep = sidemap.maybeDeps.get(dep.identifier.id);
+    if (maybeDep == null) {
       errors.pushDiagnostic(
         CompilerDiagnostic.create({
           category: ErrorCategory.UseMemo,
-          reason: `Expected the dependency list for ${kind} to be an array literal`,
-          description: `Expected the dependency list for ${kind} to be an array literal`,
+          reason: `Expected the dependency list to be an array of simple expressions (e.g. \`x\`, \`x.y.z\`, \`x?.y?.z\`)`,
+          description: `Expected the dependency list to be an array of simple expressions (e.g. \`x\`, \`x.y.z\`, \`x?.y?.z\`)`,
           suggestions: null,
         }).withDetails({
           kind: 'error',
-          loc: depsListPlace.loc,
-          message: `Expected the dependency list for ${kind} to be an array literal`,
+          loc: dep.loc,
+          message: `Expected the dependency list to be an array of simple expressions (e.g. \`x\`, \`x.y.z\`, \`x?.y?.z\`)`,
         }),
       );
-      return {fnPlace, depsList: null};
-    }
-    depsList = [];
-    for (const dep of maybeDepsList) {
-      const maybeDep = sidemap.maybeDeps.get(dep.identifier.id);
-      if (maybeDep == null) {
-        errors.pushDiagnostic(
-          CompilerDiagnostic.create({
-            category: ErrorCategory.UseMemo,
-            reason: `Expected the dependency list to be an array of simple expressions (e.g. \`x\`, \`x.y.z\`, \`x?.y?.z\`)`,
-            description: `Expected the dependency list to be an array of simple expressions (e.g. \`x\`, \`x.y.z\`, \`x?.y?.z\`)`,
-            suggestions: null,
-          }).withDetails({
-            kind: 'error',
-            loc: dep.loc,
-            message: `Expected the dependency list to be an array of simple expressions (e.g. \`x\`, \`x.y.z\`, \`x?.y?.z\`)`,
-          }),
-        );
-      } else {
-        depsList.push(maybeDep);
-      }
+    } else {
+      depsList.push(maybeDep);
     }
   }
   return {
     fnPlace,
     depsList,
+    depsLoc: maybeDepsList.loc,
   };
 }
 
@@ -427,16 +428,17 @@ export function dropManualMemoization(
 
         const manualMemo = sidemap.manualMemos.get(id);
         if (manualMemo != null) {
-          const {fnPlace, depsList} = extractManualMemoizationArgs(
+          const memoDetails = extractManualMemoizationArgs(
             instr as TInstruction<CallExpression> | TInstruction<MethodCall>,
             manualMemo.kind,
             sidemap,
             errors,
           );
 
-          if (fnPlace == null) {
+          if (memoDetails == null) {
             continue;
           }
+          const {fnPlace, depsList, depsLoc} = memoDetails;
 
           /**
            * Bailout on void return useMemos. This is an anti-pattern where code might be using
@@ -521,6 +523,7 @@ export function dropManualMemoization(
               fnPlace,
               func.env,
               depsList,
+              depsLoc,
               memoDecl,
               nextManualMemoId++,
             );

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

@@ -6,8 +6,13 @@
  */
 
 import prettyFormat from 'pretty-format';
-import {CompilerDiagnostic, CompilerError, SourceLocation} from '..';
-import {ErrorCategory} from '../CompilerError';
+import {
+  CompilerDiagnostic,
+  CompilerError,
+  CompilerSuggestionOperation,
+  SourceLocation,
+} from '..';
+import {CompilerSuggestion, ErrorCategory} from '../CompilerError';
 import {
   areEqualPaths,
   BlockId,
@@ -229,38 +234,52 @@ export function validateExhaustiveDependencies(
       extra.push(dep);
     }
 
-    if (missing.length !== 0) {
-      // Error
-      const diagnostic = CompilerDiagnostic.create({
-        category: ErrorCategory.PreserveManualMemo,
-        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.',
-      });
-      for (const dep of missing) {
+    if (missing.length !== 0 || extra.length !== 0) {
+      let suggestions: Array<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(', ')}]`,
+          },
+        ];
+      }
+      if (missing.length !== 0) {
+        // Error
+        const diagnostic = CompilerDiagnostic.create({
+          category: ErrorCategory.PreserveManualMemo,
+          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.',
+          suggestions,
+        });
+        for (const dep of missing) {
+          diagnostic.withDetails({
+            kind: 'error',
+            message: `Missing dependency \`${printInferredDependency(dep)}\``,
+            loc: dep.loc,
+          });
+        }
+        error.pushDiagnostic(diagnostic);
+      } else if (extra.length !== 0) {
+        const diagnostic = CompilerDiagnostic.create({
+          category: ErrorCategory.PreserveManualMemo,
+          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',
+        });
         diagnostic.withDetails({
           kind: 'error',
-          message: `Missing dependency \`${printInferredDependency(dep)}\``,
-          loc: dep.loc,
+          message: `Unnecessary dependencies ${extra.map(dep => `\`${printManualMemoDependency(dep)}\``).join(', ')}`,
+          loc: value.loc,
         });
+        error.pushDiagnostic(diagnostic);
       }
-      error.pushDiagnostic(diagnostic);
-    } else if (extra.length !== 0) {
-      const diagnostic = CompilerDiagnostic.create({
-        category: ErrorCategory.PreserveManualMemo,
-        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',
-      });
-      diagnostic.withDetails({
-        kind: 'error',
-        message: `Unnecessary dependencies ${extra.map(dep => `\`${printManualMemoDependency(dep)}\``).join(', ')}`,
-        loc: value.loc,
-      });
-      error.pushDiagnostic(diagnostic);
     }
 
     dependencies.clear();