[compiler] Cleanup diagnostic messages

Minor sytlistic cleanup

Author: josephsavona

compiler/packages/babel-plugin-react-compiler/src/Babel/BabelPlugin.ts

@@ -84,7 +84,9 @@ export default function BabelPluginReactCompiler(
             }
           } catch (e) {
             if (e instanceof CompilerError) {
-              throw new Error(e.printErrorMessage(pass.file.code));
+              throw new Error(
+                e.printErrorMessage(pass.file.code, {eslint: false}),
+              );
             }
             throw e;
           }

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

@@ -93,6 +93,14 @@ export type CompilerErrorDetailOptions = {
   suggestions?: Array<CompilerSuggestion> | null | undefined;
 };
 
+export type PrintErrorMessageOptions = {
+  /**
+   * ESLint uses 1-indexed columns and prints one error at a time
+   * So it doesn't require the "Found # error(s)" text
+   */
+  eslint: boolean;
+};
+
 export class CompilerDiagnostic {
   options: CompilerDiagnosticOptions;
 
@@ -128,7 +136,7 @@ export class CompilerDiagnostic {
     return this.options.details.filter(d => d.kind === 'error')[0]?.loc ?? null;
   }
 
-  printErrorMessage(source: string): string {
+  printErrorMessage(source: string, options: PrintErrorMessageOptions): string {
     const buffer = [
       printErrorSummary(this.severity, this.category),
       '\n\n',
@@ -162,9 +170,14 @@ export class CompilerDiagnostic {
           } catch (e) {
             codeFrame = detail.message;
           }
-          buffer.push(
-            `\n\n${loc.filename}:${loc.start.line}:${loc.start.column}\n`,
-          );
+          buffer.push('\n\n');
+          if (loc.filename != null) {
+            const line = loc.start.line;
+            const column = options.eslint
+              ? loc.start.column + 1
+              : loc.start.column;
+            buffer.push(`${loc.filename}:${line}:${column}\n`);
+          }
           buffer.push(codeFrame);
           break;
         }
@@ -223,7 +236,7 @@ export class CompilerErrorDetail {
     return this.loc;
   }
 
-  printErrorMessage(source: string): string {
+  printErrorMessage(source: string, options: PrintErrorMessageOptions): string {
     const buffer = [printErrorSummary(this.severity, this.reason)];
     if (this.description != null) {
       buffer.push(`\n\n${this.description}.`);
@@ -251,9 +264,12 @@ export class CompilerErrorDetail {
       } catch (e) {
         codeFrame = '';
       }
-      buffer.push(
-        `\n\n${loc.filename}:${loc.start.line}:${loc.start.column}\n`,
-      );
+      buffer.push(`\n\n`);
+      if (loc.filename != null) {
+        const line = loc.start.line;
+        const column = options.eslint ? loc.start.column + 1 : loc.start.column;
+        buffer.push(`${loc.filename}:${line}:${column}\n`);
+      }
       buffer.push(codeFrame);
       buffer.push('\n\n');
     }
@@ -372,10 +388,15 @@ export class CompilerError extends Error {
     return this.name;
   }
 
-  printErrorMessage(source: string): string {
+  printErrorMessage(source: string, options: PrintErrorMessageOptions): string {
+    if (options.eslint && this.details.length === 1) {
+      return this.details[0].printErrorMessage(source, options);
+    }
     return (
-      `Found ${this.details.length} error${this.details.length === 1 ? '' : 's'}:\n` +
-      this.details.map(detail => detail.printErrorMessage(source)).join('\n')
+      `Found ${this.details.length} error${this.details.length === 1 ? '' : 's'}:\n\n` +
+      this.details
+        .map(detail => detail.printErrorMessage(source, options).trim())
+        .join('\n\n')
     );
   }
 

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

@@ -107,10 +107,9 @@ export function lower(
       if (binding.kind !== 'Identifier') {
         builder.errors.pushDiagnostic(
           CompilerDiagnostic.create({
-            category: 'Could not find binding',
-            description: `[BuildHIR] Could not find binding for param \`${param.node.name}\``,
             severity: ErrorSeverity.Invariant,
-            suggestions: null,
+            category: 'Could not find binding',
+            description: `[BuildHIR] Could not find binding for param \`${param.node.name}\`.`,
           }).withDetail({
             kind: 'error',
             loc: param.node.loc ?? null,
@@ -172,10 +171,9 @@ export function lower(
     } else {
       builder.errors.pushDiagnostic(
         CompilerDiagnostic.create({
-          category: `Handle ${param.node.type} parameters`,
-          description: `[BuildHIR] Add support for ${param.node.type} parameters`,
           severity: ErrorSeverity.Todo,
-          suggestions: null,
+          category: `Handle ${param.node.type} parameters`,
+          description: `[BuildHIR] Add support for ${param.node.type} parameters.`,
         }).withDetail({
           kind: 'error',
           loc: param.node.loc ?? null,
@@ -205,8 +203,7 @@ export function lower(
       CompilerDiagnostic.create({
         severity: ErrorSeverity.InvalidJS,
         category: `Unexpected function body kind`,
-        description: `Expected function body to be an expression or a block statement, got \`${body.type}\``,
-        suggestions: null,
+        description: `Expected function body to be an expression or a block statement, got \`${body.type}\`.`,
       }).withDetail({
         kind: 'error',
         loc: body.node.loc ?? null,

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

@@ -447,23 +447,22 @@ function applySignature(
               reason: value.reason,
               context: new Set(),
             });
-            const message =
+            const variable =
               effect.value.identifier.name !== null &&
               effect.value.identifier.name.kind === 'named'
-                ? `\`${effect.value.identifier.name.value}\` cannot be modified`
-                : 'This value cannot be modified';
+                ? `\`${effect.value.identifier.name.value}\``
+                : 'value';
             effects.push({
               kind: 'MutateFrozen',
               place: effect.value,
               error: CompilerDiagnostic.create({
                 severity: ErrorSeverity.InvalidReact,
                 category: 'This value cannot be modified',
-                description: reason,
-                suggestions: null,
+                description: `${reason}.`,
               }).withDetail({
                 kind: 'error',
                 loc: effect.value.loc,
-                message,
+                message: `${variable} cannot be modified`,
               }),
             });
           }
@@ -1018,30 +1017,30 @@ function applyEffect(
             effect.value.identifier.declarationId,
           )
         ) {
-          const description =
+          const variable =
             effect.value.identifier.name !== null &&
             effect.value.identifier.name.kind === 'named'
-              ? `Variable \`${effect.value.identifier.name.value}\``
-              : 'This variable';
+              ? `\`${effect.value.identifier.name.value}\``
+              : null;
           const hoistedAccess = context.hoistedContextDeclarations.get(
             effect.value.identifier.declarationId,
           );
           const diagnostic = CompilerDiagnostic.create({
             severity: ErrorSeverity.InvalidReact,
             category: 'Cannot access variable before it is declared',
-            description: `${description} is accessed before it is declared, which prevents the earlier access from updating when this value changes over time`,
+            description: `${variable ?? 'This variable'} is accessed before it is declared, which prevents the earlier access from updating when this value changes over time.`,
           });
           if (hoistedAccess != null && hoistedAccess.loc != effect.value.loc) {
             diagnostic.withDetail({
               kind: 'error',
               loc: hoistedAccess.loc,
-              message: 'Variable accessed before it is declared',
+              message: `${variable ?? 'variable'} accessed before it is declared`,
             });
           }
           diagnostic.withDetail({
             kind: 'error',
             loc: effect.value.loc,
-            message: 'The variable is declared here',
+            message: `${variable ?? 'variable'} is declared here`,
           });
 
           applyEffect(
@@ -1061,11 +1060,11 @@ function applyEffect(
             reason: value.reason,
             context: new Set(),
           });
-          const message =
+          const variable =
             effect.value.identifier.name !== null &&
             effect.value.identifier.name.kind === 'named'
-              ? `\`${effect.value.identifier.name.value}\` cannot be modified`
-              : 'This value cannot be modified';
+              ? `\`${effect.value.identifier.name.value}\``
+              : 'value';
           applyEffect(
             context,
             state,
@@ -1078,11 +1077,11 @@ function applyEffect(
               error: CompilerDiagnostic.create({
                 severity: ErrorSeverity.InvalidReact,
                 category: 'This value cannot be modified',
-                description: reason,
+                description: `${reason}.`,
               }).withDetail({
                 kind: 'error',
                 loc: effect.value.loc,
-                message,
+                message: `${variable} cannot be modified`,
               }),
             },
             initialized,
@@ -1987,20 +1986,19 @@ function computeSignatureForInstruction(
       break;
     }
     case 'StoreGlobal': {
+      const variable = `\`${value.name}\``;
       effects.push({
         kind: 'MutateGlobal',
         place: value.value,
         error: CompilerDiagnostic.create({
           severity: ErrorSeverity.InvalidReact,
           category:
             'Cannot reassign variables declared outside of the component/hook',
-          description:
-            'Reassigning a variable declared outside of the component/hook is a form of side effect, which can cause unpredictable behavior depending on when the component happens to re-render. If this variable is used in rendering, use useState instead. Otherwise, consider updating it in an effect (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render)',
-          suggestions: null,
+          description: `Variable ${variable} is declared outside of the component/hook. Reassigning this value during render is a form of side effect, which can cause unpredictable behavior depending on when the component happens to re-render. If this variable is used in rendering, use useState instead. Otherwise, consider updating it in an effect. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render)`,
         }).withDetail({
           kind: 'error',
           loc: instr.loc,
-          message: 'Cannot reassign variable',
+          message: `${variable} cannot be reassigned`,
         }),
       });
       effects.push({kind: 'Assign', from: value.value, into: lvalue});
@@ -2099,7 +2097,6 @@ function computeEffectsForLegacySignature(
             ? `\`${signature.canonicalName}\` is an impure function. `
             : '') +
           'Calling an impure function can produce unstable results that update unpredictably when the component happens to re-render. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent)',
-        suggestions: null,
       }).withDetail({
         kind: 'error',
         loc,

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

@@ -29,15 +29,20 @@ export function validateLocalsNotReassignedAfterRender(fn: HIRFunction): void {
   );
   if (reassignment !== null) {
     const errors = new CompilerError();
+    const variable =
+      reassignment.identifier.name != null &&
+      reassignment.identifier.name.kind === 'named'
+        ? `\`${reassignment.identifier.name.value}\``
+        : 'variable';
     errors.pushDiagnostic(
       CompilerDiagnostic.create({
         severity: ErrorSeverity.InvalidReact,
-        category: 'Cannot reassign a variable after render completes',
-        description: `Reassigning ${reassignment.identifier.name != null && reassignment.identifier.name.kind === 'named' ? `variable \`${reassignment.identifier.name.value}\`` : 'a variable'} after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead`,
+        category: 'Cannot reassign variable after render completes',
+        description: `Reassigning ${variable} after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead.`,
       }).withDetail({
         kind: 'error',
         loc: reassignment.loc,
-        message: 'Cannot reassign variable after render completes',
+        message: `Cannot reassign ${variable} after render completes`,
       }),
     );
     throw errors;
@@ -78,16 +83,25 @@ function getContextReassignment(
           // if the function or its depends reassign, propagate that fact on the lvalue
           if (reassignment !== null) {
             if (isAsync || value.loweredFunc.func.async) {
-              CompilerError.throwInvalidReact({
-                reason:
-                  'Reassigning a variable in an async function can cause inconsistent behavior on subsequent renders. Consider using state instead',
-                description:
-                  reassignment.identifier.name !== null &&
-                  reassignment.identifier.name.kind === 'named'
-                    ? `Variable \`${reassignment.identifier.name.value}\` cannot be reassigned after render`
-                    : '',
-                loc: reassignment.loc,
-              });
+              const errors = new CompilerError();
+              const variable =
+                reassignment.identifier.name !== null &&
+                reassignment.identifier.name.kind === 'named'
+                  ? `\`${reassignment.identifier.name.value}\``
+                  : 'variable';
+              errors.pushDiagnostic(
+                CompilerDiagnostic.create({
+                  severity: ErrorSeverity.InvalidReact,
+                  category: 'Cannot reassign variable in async function',
+                  description:
+                    'Reassigning a variable in an async function can cause inconsistent behavior on subsequent renders. Consider using state instead',
+                }).withDetail({
+                  kind: 'error',
+                  loc: reassignment.loc,
+                  message: `Cannot reassign ${variable}`,
+                }),
+              );
+              throw errors;
             }
             reassigningFunctions.set(lvalue.identifier.id, reassignment);
           }

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

@@ -57,22 +57,28 @@ export function validateNoFreezingKnownMutableFunctions(
     if (operand.effect === Effect.Freeze) {
       const effect = contextMutationEffects.get(operand.identifier.id);
       if (effect != null) {
+        const place = [...effect.places][0];
+        const variable =
+          place != null &&
+          place.identifier.name != null &&
+          place.identifier.name.kind === 'named'
+            ? `\`${place.identifier.name.value}\``
+            : 'a local variable';
         errors.pushDiagnostic(
           CompilerDiagnostic.create({
             severity: ErrorSeverity.InvalidReact,
             category: 'Cannot modify local variables after render completes',
-            description: `This argument is a function which may reassign or mutate local variables after render, which can cause inconsistent behavior on subsequent renders. Consider using state instead`,
+            description: `This argument is a function which may reassign or mutate ${variable} after render, which can cause inconsistent behavior on subsequent renders. Consider using state instead.`,
           })
             .withDetail({
               kind: 'error',
               loc: operand.loc,
-              message:
-                'This function may (indirectly) reassign or modify local variables after render',
+              message: `This function may (indirectly) reassign or modify ${variable} after render`,
             })
             .withDetail({
               kind: 'error',
               loc: effect.loc,
-              message: 'This modifies a local variable',
+              message: `This modifies ${variable}`,
             }),
         );
       }