From 9c1f8a962c1f15935749e604cac2d095c952af04 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 6 Apr 2023 08:51:31 -0700 Subject: [PATCH] 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 | 31 ++++++++++++++++--- .../src/HIR/ValidateConsistentIdentifiers.ts | 17 +++++----- .../src/HIR/ValidateTerminalSuccessors.ts | 5 +-- .../src/Inference/InferReferenceEffects.ts | 14 ++++----- .../ReactiveScopes/CodegenReactiveFunction.ts | 10 +++--- compiler/forget/src/SSA/EnterSSA.ts | 7 ++--- compiler/forget/src/SSA/LeaveSSA.ts | 26 +++++++++------- ...ror.hoisted-function-declaration.expect.md | 2 +- ...rror.mutate-after-aliased-freeze.expect.md | 2 +- .../error.mutate-after-freeze.expect.md | 2 +- ...r.mutate-captured-arg-separately.expect.md | 2 +- 11 files changed, 74 insertions(+), 44 deletions(-) diff --git a/compiler/forget/src/CompilerError.ts b/compiler/forget/src/CompilerError.ts index 483bb62d99..d184f9157c 100644 --- a/compiler/forget/src/CompilerError.ts +++ b/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, diff --git a/compiler/forget/src/HIR/ValidateConsistentIdentifiers.ts b/compiler/forget/src/HIR/ValidateConsistentIdentifiers.ts index 4413ae2e43..3b73e9e2e6 100644 --- a/compiler/forget/src/HIR/ValidateConsistentIdentifiers.ts +++ b/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}` ); } } diff --git a/compiler/forget/src/HIR/ValidateTerminalSuccessors.ts b/compiler/forget/src/HIR/ValidateTerminalSuccessors.ts index c22d25b63c..56bcd1faa1 100644 --- a/compiler/forget/src/HIR/ValidateTerminalSuccessors.ts +++ b/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; diff --git a/compiler/forget/src/Inference/InferReferenceEffects.ts b/compiler/forget/src/Inference/InferReferenceEffects.ts index 961dcd910f..da168a013c 100644 --- a/compiler/forget/src/Inference/InferReferenceEffects.ts +++ b/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; diff --git a/compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts index ac56da0cb1..cf1c66c4dd 100644 --- a/compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/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"; diff --git a/compiler/forget/src/SSA/EnterSSA.ts b/compiler/forget/src/SSA/EnterSSA.ts index 75a1522134..073b220882 100644 --- a/compiler/forget/src/SSA/EnterSSA.ts +++ b/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` ); } diff --git a/compiler/forget/src/SSA/LeaveSSA.ts b/compiler/forget/src/SSA/LeaveSSA.ts index b6230a2b2c..deb59ce22d 100644 --- a/compiler/forget/src/SSA/LeaveSSA.ts +++ b/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; diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.hoisted-function-declaration.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.hoisted-function-declaration.expect.md index 6f20cf0269..12a57c3c19 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/error.hoisted-function-declaration.expect.md +++ b/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) ``` \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.mutate-after-aliased-freeze.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.mutate-after-aliased-freeze.expect.md index 75663b16c4..965858836c 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/error.mutate-after-aliased-freeze.expect.md +++ b/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 (frozen) (13:13) +[ReactForget] InvalidInput: InferReferenceEffects: inferred mutation of known immutable value. Found mutation of $42:TObject (frozen) (13:13) ``` \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.mutate-after-freeze.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.mutate-after-freeze.expect.md index 24c53d8f85..bd93da23ff 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/error.mutate-after-freeze.expect.md +++ b/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 (frozen) (7:7) +[ReactForget] InvalidInput: InferReferenceEffects: inferred mutation of known immutable value. Found mutation of $25:TObject (frozen) (7:7) ``` \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.mutate-captured-arg-separately.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.mutate-captured-arg-separately.expect.md index 90927cdebe..b23037e49a 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/error.mutate-captured-arg-separately.expect.md +++ b/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) ``` \ No newline at end of file