Repository: facebook/react
Pull request: [compiler] Aggregate all errors reported from DropManualMemoization


Noticed this from my previous PR that this pass was throwing on the first error. This PR is a small refactor to aggregate every violation and report them all at once.

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34002).
* #34022
* __->__ #34002

Changed files:
filename: compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts
patch: @@ -289,50 +289,84 @@ function extractManualMemoizationArgs(
   instr: TInstruction<CallExpression> | TInstruction<MethodCall>,
   kind: 'useCallback' | 'useMemo',
   sidemap: IdentifierSidemap,
+  errors: CompilerError,
 ): {
-  fnPlace: Place;
+  fnPlace: Place | null;
   depsList: Array<ManualMemoDependency> | null;
 } {
   const [fnPlace, depsListPlace] = instr.value.args as Array<
     Place | SpreadPattern | undefined
   >;
   if (fnPlace == null) {
-    CompilerError.throwInvalidReact({
-      reason: `Expected a callback function to be passed to ${kind}`,
-      loc: instr.value.loc,
-      suggestions: null,
-    });
+    errors.pushDiagnostic(
+      CompilerDiagnostic.create({
+        severity: ErrorSeverity.InvalidReact,
+        category: `Expected a callback function to be passed to ${kind}`,
+        description: `Expected a callback function to be passed to ${kind}`,
+        suggestions: null,
+      }).withDetail({
+        kind: 'error',
+        loc: instr.value.loc,
+        message: `Expected a callback function to be passed to ${kind}`,
+      }),
+    );
+    return {fnPlace: null, depsList: null};
   }
   if (fnPlace.kind === 'Spread' || depsListPlace?.kind === 'Spread') {
-    CompilerError.throwInvalidReact({
-      reason: `Unexpected spread argument to ${kind}`,
-      loc: instr.value.loc,
-      suggestions: null,
-    });
+    errors.pushDiagnostic(
+      CompilerDiagnostic.create({
+        severity: ErrorSeverity.InvalidReact,
+        category: `Unexpected spread argument to ${kind}`,
+        description: `Unexpected spread argument to ${kind}`,
+        suggestions: null,
+      }).withDetail({
+        kind: 'error',
+        loc: instr.value.loc,
+        message: `Unexpected spread argument to ${kind}`,
+      }),
+    );
+    return {fnPlace: null, depsList: null};
   }
   let depsList: Array<ManualMemoDependency> | null = null;
   if (depsListPlace != null) {
     const maybeDepsList = sidemap.maybeDepsLists.get(
       depsListPlace.identifier.id,
     );
     if (maybeDepsList == null) {
-      CompilerError.throwInvalidReact({
-        reason: `Expected the dependency list for ${kind} to be an array literal`,
-        suggestions: null,
-        loc: depsListPlace.loc,
-      });
+      errors.pushDiagnostic(
+        CompilerDiagnostic.create({
+          severity: ErrorSeverity.InvalidReact,
+          category: `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,
+        }).withDetail({
+          kind: 'error',
+          loc: depsListPlace.loc,
+          message: `Expected the dependency list for ${kind} to be an array literal`,
+        }),
+      );
+      return {fnPlace, depsList: null};
     }
-    depsList = maybeDepsList.map(dep => {
+    depsList = [];
+    for (const dep of maybeDepsList) {
       const maybeDep = sidemap.maybeDeps.get(dep.identifier.id);
       if (maybeDep == null) {
-        CompilerError.throwInvalidReact({
-          reason: `Expected the dependency list to be an array of simple expressions (e.g. \`x\`, \`x.y.z\`, \`x?.y?.z\`)`,
-          suggestions: null,
-          loc: dep.loc,
-        });
+        errors.pushDiagnostic(
+          CompilerDiagnostic.create({
+            severity: ErrorSeverity.InvalidReact,
+            category: `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,
+          }).withDetail({
+            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);
       }
-      return maybeDep;
-    });
+    }
   }
   return {
     fnPlace,
@@ -401,8 +435,13 @@ export function dropManualMemoization(
             instr as TInstruction<CallExpression> | TInstruction<MethodCall>,
             manualMemo.kind,
             sidemap,
+            errors,
           );
 
+          if (fnPlace == null) {
+            continue;
+          }
+
           /**
            * Bailout on void return useMemos. This is an anti-pattern where code might be using
            * useMemo like useEffect: running arbirtary side-effects synced to changes in specific
@@ -457,11 +496,19 @@ export function dropManualMemoization(
              * is rare and likely sketchy.
              */
             if (!sidemap.functions.has(fnPlace.identifier.id)) {
-              CompilerError.throwInvalidReact({
-                reason: `Expected the first argument to be an inline function expression`,
-                suggestions: [],
-                loc: fnPlace.loc,
-              });
+              errors.pushDiagnostic(
+                CompilerDiagnostic.create({
+                  severity: ErrorSeverity.InvalidReact,
+                  category: `Expected the first argument to be an inline function expression`,
+                  description: `Expected the first argument to be an inline function expression`,
+                  suggestions: [],
+                }).withDetail({
+                  kind: 'error',
+                  loc: fnPlace.loc,
+                  message: `Expected the first argument to be an inline function expression`,
+                }),
+              );
+              continue;
             }
             const memoDecl: Place =
               manualMemo.kind === 'useMemo'

filename: compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.useMemo-non-literal-depslist.expect.md
patch: @@ -32,6 +32,8 @@ Found 1 error:
 
 Error: Expected the dependency list for useMemo to be an array literal
 
+Expected the dependency list for useMemo to be an array literal
+
 error.useMemo-non-literal-depslist.ts:10:4
    8 |       return text.toUpperCase();
    9 |     },

filename: compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.validate-useMemo-named-function.expect.md
patch: @@ -24,6 +24,8 @@ Found 1 error:
 
 Error: Expected the first argument to be an inline function expression
 
+Expected the first argument to be an inline function expression
+
 error.validate-useMemo-named-function.ts:9:20
    7 | // for now.
    8 | function Component(props) {