Make CompilerError.reason a static string

While running the latest Forget build on www I noticed that a lot of the 
bailouts were special-cases where we used interpolation in the error `reason` 
string to provide more context for debugging. This is a pretty cool result, 
because it means that we actually support nearly all the common syntax (at least 
based on a sample of the codebase). But it makes our tools for aggregating 
errors break down a bit. 

This PR adds a new, nullable `description` property to CompilerErrorDetail, and 
manually updates to ensure that we always pass a static `reason` and only use 
interpolation in the `description`. This will allow our aggregation tools to 
group by the reason.

compiler/forget/src/CompilerError.ts

@@ -19,6 +19,7 @@ export enum ErrorSeverity {
 
 export type CompilerErrorOptions = {
   reason: string;
+  description?: string | null | undefined;
   severity: ErrorSeverity;
   nodePath: AnyNodePath | null;
 };
@@ -64,7 +65,8 @@ export function tryPrintCodeFrame(
   try {
     return options.nodePath
       .buildCodeFrameError(
-        options.reason,
+        options.reason +
+          (options.description != null ? `. ${options.description}` : ""),
         mapSeverityToErrorCtor(options.severity)
       )
       .toString();
@@ -79,12 +81,14 @@ export function tryPrintCodeFrame(
  */
 export class CompilerErrorDetail {
   reason: string;
+  description: string | null;
   severity: ErrorSeverity;
   codeframe: string | null;
   loc: BabelSourceLocation | null;
 
   constructor(options: CompilerErrorDetailOptions) {
     this.reason = options.reason;
+    this.description = options.description;
     this.severity = options.severity;
     this.codeframe = options.codeframe;
     this.loc = options.loc;
@@ -95,6 +99,9 @@ export class CompilerErrorDetail {
       return this.codeframe;
     }
     const buffer = [`${this.severity}: ${this.reason}`];
+    if (this.description !== null) {
+      buffer.push(`. ${this.description}`);
+    }
     if (this.loc != null) {
       buffer.push(` (${this.loc.start.line}:${this.loc.end.line})`);
     }
@@ -109,11 +116,16 @@ export class CompilerErrorDetail {
 export class CompilerError extends Error {
   details: CompilerErrorDetail[] = [];
 
-  static invariant(reason: string, loc: SourceLocation): never {
+  static invariant(
+    reason: string,
+    loc: SourceLocation,
+    description: string | null = null
+  ): never {
     const errors = new CompilerError();
     errors.pushErrorDetail(
       new CompilerErrorDetail({
         codeframe: null,
+        description,
         loc: typeof loc === "symbol" ? null : loc,
         reason,
         severity: ErrorSeverity.Invariant,
@@ -122,11 +134,16 @@ export class CompilerError extends Error {
     throw errors;
   }
 
-  static todo(reason: string, loc: SourceLocation): never {
+  static todo(
+    reason: string,
+    loc: SourceLocation,
+    description: string | null = null
+  ): never {
     const errors = new CompilerError();
     errors.pushErrorDetail(
       new CompilerErrorDetail({
         codeframe: null,
+        description,
         loc: typeof loc === "symbol" ? null : loc,
         reason,
         severity: ErrorSeverity.Todo,
@@ -135,11 +152,16 @@ export class CompilerError extends Error {
     throw errors;
   }
 
-  static invalidInput(reason: string, loc: SourceLocation): never {
+  static invalidInput(
+    reason: string,
+    loc: SourceLocation,
+    description: string | null = null
+  ): never {
     const errors = new CompilerError();
     errors.pushErrorDetail(
       new CompilerErrorDetail({
         codeframe: null,
+        description,
         loc: typeof loc === "symbol" ? null : loc,
         reason,
         severity: ErrorSeverity.InvalidInput,
@@ -165,6 +187,7 @@ export class CompilerError extends Error {
   push(options: CompilerErrorOptions): CompilerErrorDetail {
     const detail = new CompilerErrorDetail({
       reason: options.reason,
+      description: options.description ?? null,
       severity: options.severity,
       codeframe: tryPrintCodeFrame(options),
       loc: options.nodePath?.node?.loc ?? null,

compiler/forget/src/HIR/ValidateConsistentIdentifiers.ts

@@ -37,16 +37,16 @@ export function validateConsistentIdentifiers(fn: HIRFunction): void {
     for (const instr of block.instructions) {
       if (instr.lvalue.identifier.name !== null) {
         CompilerError.invariant(
-          `Expected all lvalues to be temporaries, found '${instr.lvalue.identifier.name}'`,
-          instr.lvalue.loc
+          `Expected all lvalues to be temporaries`,
+          instr.lvalue.loc,
+          `Found named lvalue '${instr.lvalue.identifier.name}'`
         );
       }
       if (assignments.has(instr.lvalue.identifier.id)) {
         CompilerError.invariant(
-          `Expected lvalues to be assigned exactly once, found duplicate assignment of '${printPlace(
-            instr.lvalue
-          )}'`,
-          instr.lvalue.loc
+          `Expected lvalues to be assigned exactly once`,
+          instr.lvalue.loc,
+          `Found duplicate assignment of '${printPlace(instr.lvalue)}'`
         );
       }
       assignments.add(instr.lvalue.identifier.id);
@@ -75,8 +75,9 @@ function validate(
     identifiers.set(identifier.id, identifier);
   } else if (identifier !== previous) {
     CompilerError.invariant(
-      `Duplicate identifier for id ${identifier.id}`,
-      loc ?? GeneratedSource
+      `Duplicate identifier object`,
+      loc ?? GeneratedSource,
+      `Found duplicate identifier object for id ${identifier.id}`
     );
   }
 }

compiler/forget/src/HIR/ValidateTerminalSuccessors.ts

@@ -15,10 +15,11 @@ export function validateTerminalSuccessors(fn: HIRFunction): void {
     mapTerminalSuccessors(block.terminal, (successor) => {
       if (!fn.body.blocks.has(successor)) {
         CompilerError.invariant(
+          `Terminal successor references unknown block`,
+          (block.terminal as any).loc ?? GeneratedSource,
           `Block bb${successor} does not exist for terminal '${printTerminal(
             block.terminal
-          )}'`,
-          (block.terminal as any).loc ?? GeneratedSource
+          )}'`
         );
       }
       return successor;

compiler/forget/src/Inference/InferReferenceEffects.ts

@@ -219,10 +219,9 @@ class InferenceState {
     }
     if (mergedKind === null) {
       CompilerError.invariant(
-        `InferReferenceEffects::kind: Expected at least one value at '${printPlace(
-          place
-        )}'`,
-        place.loc
+        `InferReferenceEffects::kind: Expected at least one value`,
+        place.loc,
+        `No value found at '${printPlace(place)}'`
       );
     }
     return mergedKind;
@@ -312,10 +311,11 @@ class InferenceState {
         } else {
           if (shouldError) {
             CompilerError.invalidInput(
-              `InferReferenceEffects: inferred mutation of known immutable value ${printIdentifier(
+              `InferReferenceEffects: inferred mutation of known immutable value`,
+              place.loc,
+              `Found mutation of ${printIdentifier(
                 place.identifier
-              )}${printType(place.identifier.type)} (${valueKind})`,
-              place.loc
+              )}${printType(place.identifier.type)} (${valueKind})`
             );
           }
           effect = Effect.Read;

compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts

@@ -318,8 +318,9 @@ function codegenTerminal(
     case "for-of": {
       if (terminal.init.kind !== "SequenceExpression") {
         CompilerError.invariant(
-          `Expected a sequence expression init for ForOf, got: ${terminal.init.kind}`,
-          terminal.init.loc
+          `Expected a sequence expression init for ForOf`,
+          terminal.init.loc,
+          `Got '${terminal.init.kind}' expression instead`
         );
       }
       if (terminal.init.instructions.length !== 2) {
@@ -342,8 +343,9 @@ function codegenTerminal(
         }
         default:
           CompilerError.invariant(
-            `Expected a StoreLocal or Destructure to be assigned to the collection, got: ${iterableItem.value.kind}`,
-            iterableItem.value.loc
+            `Expected a StoreLocal or Destructure to be assigned to the collection`,
+            iterableItem.value.loc,
+            `Found ${iterableItem.value.kind}`
           );
       }
       let varDeclKind: "const" | "let";

compiler/forget/src/SSA/EnterSSA.ts

@@ -86,10 +86,9 @@ class SSABuilder {
     const oldId = oldPlace.identifier;
     if (this.#unknown.has(oldId)) {
       CompilerError.invariant(
-        `identifier ${printIdentifier(
-          oldId
-        )} should have been defined before use`,
-        oldPlace.loc
+        `EnterSSA: Expected identifier to be defined before being used`,
+        oldPlace.loc,
+        `Identifier ${printIdentifier(oldId)} is undfined`
       );
     }
 

compiler/forget/src/SSA/LeaveSSA.ts

@@ -133,8 +133,9 @@ export function leaveSSA(fn: HIRFunction): void {
         if (name !== null) {
           if (declarations.has(name)) {
             CompilerError.invariant(
-              `Unexpected duplicate declaration of '${name}'`,
-              value.lvalue.place.loc
+              `Unexpected duplicate declaration`,
+              value.lvalue.place.loc,
+              `Found duplicate declaration for '${name}'`
             );
           }
           declarations.set(name, {
@@ -177,10 +178,11 @@ export function leaveSSA(fn: HIRFunction): void {
           if (place.identifier.name == null) {
             if (kind !== null && kind !== InstructionKind.Const) {
               CompilerError.invariant(
-                `Expected consistent kind for destructuring, other places were '${kind}' but '${printPlace(
+                `Expected consistent kind for destructuring`,
+                place.loc,
+                `other places were '${kind}' but '${printPlace(
                   place
-                )}' is const`,
-                place.loc
+                )}' is const`
               );
             }
             kind = InstructionKind.Const;
@@ -202,20 +204,22 @@ export function leaveSSA(fn: HIRFunction): void {
               });
               if (kind !== null && kind !== InstructionKind.Const) {
                 CompilerError.invariant(
-                  `Expected consistent kind for destructuring, other places were '${kind}' but '${printPlace(
+                  `Expected consistent kind for destructuring`,
+                  place.loc,
+                  `Other places were '${kind}' but '${printPlace(
                     place
-                  )}' is const`,
-                  place.loc
+                  )}' is const`
                 );
               }
               kind = InstructionKind.Const;
             } else {
               if (kind !== null && kind !== InstructionKind.Reassign) {
                 CompilerError.invariant(
-                  `Expected consistent kind for destructuring, other places were '${kind}' but '${printPlace(
+                  `Expected consistent kind for destructuring`,
+                  place.loc,
+                  `Other places were '${kind}' but '${printPlace(
                     place
-                  )}' is reassigned`,
-                  place.loc
+                  )}' is reassigned`
                 );
               }
               kind = InstructionKind.Reassign;

compiler/forget/src/__tests__/fixtures/compiler/error.hoisted-function-declaration.expect.md

@@ -17,7 +17,7 @@ function component(a) {
 ## Error
 
 ```
-[ReactForget] Invariant: identifier x$6 should have been defined before use (4:4)
+[ReactForget] Invariant: EnterSSA: Expected identifier to be defined before being used. Identifier x$6 is undfined (4:4)
 ```
           
       

compiler/forget/src/__tests__/fixtures/compiler/error.mutate-after-aliased-freeze.expect.md

@@ -25,7 +25,7 @@ function Component(props) {
 ## Error
 
 ```
-[ReactForget] InvalidInput: InferReferenceEffects: inferred mutation of known immutable value $42:TObject<BuiltInArray> (frozen) (13:13)
+[ReactForget] InvalidInput: InferReferenceEffects: inferred mutation of known immutable value. Found mutation of $42:TObject<BuiltInArray> (frozen) (13:13)
 ```
           
       

compiler/forget/src/__tests__/fixtures/compiler/error.mutate-after-freeze.expect.md

@@ -19,7 +19,7 @@ function Component(props) {
 ## Error
 
 ```
-[ReactForget] InvalidInput: InferReferenceEffects: inferred mutation of known immutable value $25:TObject<BuiltInArray> (frozen) (7:7)
+[ReactForget] InvalidInput: InferReferenceEffects: inferred mutation of known immutable value. Found mutation of $25:TObject<BuiltInArray> (frozen) (7:7)
 ```
           
       

compiler/forget/src/__tests__/fixtures/compiler/error.mutate-captured-arg-separately.expect.md

@@ -19,7 +19,7 @@ function component(a) {
 ## Error
 
 ```
-[ReactForget] Invariant: identifier x$2 should have been defined before use (7:7)
+[ReactForget] Invariant: EnterSSA: Expected identifier to be defined before being used. Identifier x$2 is undfined (7:7)
 ```