Stop passing NodePaths to CompilerError

This was causing issues in various places where errors would be stringified. 
Because the inner detail objects would contain a NodePath with circular 
structures this would cause a JSON.stringify error in code outside of our 
control. This change makes it so we always print the codeframe from the NodePath 
and then passing the string.

compiler/forget/packages/playground/lib/forgetMonacoDiagnostics.ts

@@ -23,18 +23,18 @@ function mapForgetDiagnosticToMonacoMarker(
   detail: CompilerErrorDetail,
   monaco: Monaco
 ): editor.IMarkerData | null {
-  if (detail.nodePath == null || detail.nodePath.node?.loc == null) {
+  if (detail.loc == null) {
     return null;
   }
   const severity = mapForgetSeverityToMonaco(detail.severity, monaco);
-  let message = detail.errorMessage;
+  let message = detail.printErrorMessage();
   return {
     severity,
     message,
-    startLineNumber: detail.nodePath.node.loc.start.line,
-    startColumn: detail.nodePath.node.loc.start.column + 1,
-    endLineNumber: detail.nodePath.node.loc.end.line,
-    endColumn: detail.nodePath.node.loc.end.column + 1,
+    startLineNumber: detail.loc.start.line,
+    startColumn: detail.loc.start.column + 1,
+    endLineNumber: detail.loc.end.line,
+    endColumn: detail.loc.end.column + 1,
   };
 }
 

compiler/forget/src/CompilerError.ts

@@ -1,4 +1,6 @@
-import { Node, NodePath } from "@babel/core";
+import { Node, NodePath } from "@babel/traverse";
+import { SourceLocation } from "@babel/types";
+import { ExtractClassProperties } from "./Utils/types";
 import { assertExhaustive } from "./Utils/utils";
 
 export enum ErrorSeverity {
@@ -9,10 +11,11 @@ export enum ErrorSeverity {
 export type CompilerErrorOptions = {
   reason: string;
   severity: ErrorSeverity;
-  nodePath: AnyNodePath | null;
+  nodePath: AnyNodePath;
 };
 type AnyNodePath = NodePath<Node | null | undefined>;
 type CompilerErrorKind = typeof InvalidInputError | typeof TodoError;
+type CompilerErrorDetailOptions = ExtractClassProperties<CompilerErrorDetail>;
 
 function mapSeverityToErrorCtor(severity: ErrorSeverity): CompilerErrorKind {
   switch (severity) {
@@ -37,6 +40,22 @@ class TodoError extends Error {
   }
 }
 
+export function tryPrintCodeFrame(
+  options: CompilerErrorOptions
+): string | null {
+  if (options.nodePath == null) return null;
+  try {
+    return options.nodePath
+      .buildCodeFrameError(
+        options.reason,
+        mapSeverityToErrorCtor(options.severity)
+      )
+      .toString();
+  } catch {
+    return null;
+  }
+}
+
 /**
  * Each bailout or invariant in HIR lowering creates an {@link CompilerErrorDetail}, which is then
  * aggregated into a single {@link CompilerError} later.
@@ -44,43 +63,29 @@ class TodoError extends Error {
 export class CompilerErrorDetail {
   reason: string;
   severity: ErrorSeverity;
-  /**
-   * If a NodePath is provided, we will prefer Babel's built in codeframe error generation which
-   * will print error markers in the correct location.
-   */
-  nodePath: AnyNodePath | null;
+  codeframe: string | null;
+  loc: SourceLocation | null;
 
-  constructor(options: CompilerErrorOptions) {
+  constructor(options: CompilerErrorDetailOptions) {
     this.reason = options.reason;
     this.severity = options.severity;
-    this.nodePath = options.nodePath;
+    this.codeframe = options.codeframe;
+    this.loc = options.loc;
   }
 
-  get errorMessage(): string {
+  printErrorMessage(): string {
+    if (this.codeframe != null) {
+      return this.codeframe;
+    }
     const buffer = [`${this.severity}: ${this.reason}`];
-    if (this.nodePath != null && this.nodePath.node?.loc != null) {
-      buffer.push(
-        ` (${this.nodePath.node.loc.start.line}:${this.nodePath.node.loc.end.line})`
-      );
+    if (this.loc != null) {
+      buffer.push(` (${this.loc.start.line}:${this.loc.end.line})`);
     }
     return buffer.join("");
   }
 
-  get codeFrame() {
-    if (this.nodePath == null) {
-      return this.errorMessage;
-    }
-    try {
-      return this.nodePath
-        .buildCodeFrameError(this.reason, mapSeverityToErrorCtor(this.severity))
-        .toString();
-    } catch {
-      return this.errorMessage;
-    }
-  }
-
   toString(): string {
-    return `[ReactForget] ${this.errorMessage}`;
+    return `[ReactForget] ${this.printErrorMessage()}`;
   }
 }
 

compiler/forget/src/HIR/BuildHIR.ts

@@ -73,7 +73,7 @@ export function lower(
       };
       params.push(place);
     } else {
-      builder.pushError({
+      builder.recordError({
         reason: `(BuildHIR::lower) Handle ${param.node.type} params`,
         severity: ErrorSeverity.Todo,
         nodePath: param,
@@ -94,7 +94,7 @@ export function lower(
   } else if (body.isBlockStatement()) {
     lowerStatement(builder, body);
   } else {
-    builder.pushError({
+    builder.recordError({
       reason: `(BuildHIR::lower) Unexpected function body kind: ${body.type}}`,
       severity: ErrorSeverity.InvalidInput,
       nodePath: body,
@@ -341,7 +341,7 @@ function lowerStatement(
       const initBlock = builder.enter("value", (blockId) => {
         const init = stmt.get("init");
         if (!init.isVariableDeclaration()) {
-          builder.pushError({
+          builder.recordError({
             reason:
               "(BuildHIR::lowerStatement) Handle non-variable initialization in ForStatement",
             severity: ErrorSeverity.Todo,
@@ -361,7 +361,7 @@ function lowerStatement(
       const updateBlock = builder.enter("value", (blockId) => {
         const update = stmt.get("update");
         if (update.node == null) {
-          builder.pushError({
+          builder.recordError({
             reason: `(BuildHIR::lowerStatement) Handle empty update in ForStatement`,
             severity: ErrorSeverity.Todo,
             nodePath: stmt,
@@ -404,7 +404,7 @@ function lowerStatement(
 
       const test = stmt.get("test");
       if (test.node == null) {
-        builder.pushError({
+        builder.recordError({
           reason: `(BuildHIR::lowerStatement) Handle empty test in ForStatement`,
           severity: ErrorSeverity.Todo,
           nodePath: stmt,
@@ -573,7 +573,7 @@ function lowerStatement(
         const test = case_.get("test");
         if (test.node == null) {
           if (hasDefault) {
-            builder.pushError({
+            builder.recordError({
               reason:
                 "(BuildHIR::lowerStatement) Expected at most one `default` branch in SwitchStatement, this code should have failed to parse",
               severity: ErrorSeverity.InvalidInput,
@@ -659,7 +659,7 @@ function lowerStatement(
       const stmt = stmtPath as NodePath<t.VariableDeclaration>;
       const nodeKind: string = stmt.node.kind;
       if (nodeKind === "var") {
-        builder.pushError({
+        builder.recordError({
           reason: `(BuildHIR::lowerStatement) Handle ${nodeKind} kinds in VariableDeclaration`,
           severity: ErrorSeverity.Todo,
           nodePath: stmt,
@@ -742,7 +742,7 @@ function lowerStatement(
     case "TSNamespaceExportDeclaration":
     case "TSTypeAliasDeclaration":
     case "WithStatement": {
-      builder.pushError({
+      builder.recordError({
         reason: `(BuildHIR::lowerStatement) Handle ${stmtPath.type} statements`,
         severity: ErrorSeverity.Todo,
         nodePath: stmtPath,
@@ -814,7 +814,7 @@ function lowerExpression(
       let hasError = false;
       for (const propertyPath of propertyPaths) {
         if (!propertyPath.isObjectProperty()) {
-          builder.pushError({
+          builder.recordError({
             reason: `(BuildHIR::lowerExpression) Handle ${propertyPath.type} properties in ObjectExpression`,
             severity: ErrorSeverity.Todo,
             nodePath: propertyPath,
@@ -824,7 +824,7 @@ function lowerExpression(
         }
         const key = propertyPath.node.key;
         if (key.type !== "Identifier") {
-          builder.pushError({
+          builder.recordError({
             reason: `(BuildHIR::lowerExpression) Expected Identifier, got ${key.type} key in ObjectExpression`,
             severity: ErrorSeverity.InvalidInput,
             nodePath: propertyPath,
@@ -834,7 +834,7 @@ function lowerExpression(
         }
         const valuePath = propertyPath.get("value");
         if (!valuePath.isExpression()) {
-          builder.pushError({
+          builder.recordError({
             reason: `(BuildHIR::lowerExpression) Handle ${valuePath.type} values in ObjectExpression`,
             severity: ErrorSeverity.Todo,
             nodePath: valuePath,
@@ -859,7 +859,7 @@ function lowerExpression(
       let elements: Place[] = [];
       for (const element of expr.get("elements")) {
         if (element.node == null || !element.isExpression()) {
-          builder.pushError({
+          builder.recordError({
             reason: `(BuildHIR::lowerExpression) Handle ${element.type} elements in ArrayExpression`,
             severity: ErrorSeverity.Todo,
             nodePath: element,
@@ -883,7 +883,7 @@ function lowerExpression(
       const expr = exprPath as NodePath<t.NewExpression>;
       const calleePath = expr.get("callee");
       if (!calleePath.isExpression()) {
-        builder.pushError({
+        builder.recordError({
           reason: `(BuildHIR::lowerExpression) Expected Expression, got ${calleePath.type} in NewExpression (v8 intrinsics not supported): ${calleePath.type}`,
           severity: ErrorSeverity.InvalidInput,
           nodePath: calleePath,
@@ -895,7 +895,7 @@ function lowerExpression(
       let hasError = false;
       for (const argPath of expr.get("arguments")) {
         if (!argPath.isExpression()) {
-          builder.pushError({
+          builder.recordError({
             reason: `(BuildHIR::lowerExpression) Handle ${argPath.type} arguments in NewExpression`,
             severity: ErrorSeverity.Todo,
             nodePath: argPath,
@@ -920,7 +920,7 @@ function lowerExpression(
       const calleePath = expr.get("callee");
       let hasError = false;
       if (!calleePath.isExpression()) {
-        builder.pushError({
+        builder.recordError({
           reason: `(BuildHIR::lowerExpression) Expected Expression, got ${calleePath.type} in CallExpression (v8 intrinsics not supported)`,
           severity: ErrorSeverity.InvalidInput,
           nodePath: calleePath,
@@ -935,7 +935,7 @@ function lowerExpression(
         let args: Place[] = [];
         for (const argPath of expr.get("arguments")) {
           if (!argPath.isExpression()) {
-            builder.pushError({
+            builder.recordError({
               reason: `(BuildHIR::lowerExpression) Handle ${argPath.type} arguments in CallExpression`,
               severity: ErrorSeverity.Todo,
               nodePath: argPath,
@@ -967,7 +967,7 @@ function lowerExpression(
         let args: Place[] = [];
         for (const argPath of expr.get("arguments")) {
           if (!argPath.isExpression()) {
-            builder.pushError({
+            builder.recordError({
               reason: `(BuildHIR::lowerExpression) Handle ${argPath.type} arguments in CallExpression`,
               severity: ErrorSeverity.Todo,
               nodePath: argPath,
@@ -991,7 +991,7 @@ function lowerExpression(
       const expr = exprPath as NodePath<t.BinaryExpression>;
       const leftPath = expr.get("left");
       if (!leftPath.isExpression()) {
-        builder.pushError({
+        builder.recordError({
           reason: `(BuildHIR::lowerExpression) Expected Expression, got ${leftPath.type} lval in BinaryExpression`,
           severity: ErrorSeverity.InvalidInput,
           nodePath: leftPath,
@@ -1115,7 +1115,7 @@ function lowerExpression(
       };
       const binaryOperator = operators[operator];
       if (binaryOperator == null) {
-        builder.pushError({
+        builder.recordError({
           reason: `(BuildHIR::lowerExpression) Handle ${operator} operaators in AssignmentExpression`,
           severity: ErrorSeverity.Todo,
           nodePath: expr.get("operator"),
@@ -1154,7 +1154,7 @@ function lowerExpression(
           // Extract the final property to be read from and re-assigned, eg 'c'
           const property = leftExpr.get("property");
           if (!property.isIdentifier()) {
-            builder.pushError({
+            builder.recordError({
               reason: `(BuildHIR::lowerExpression) Handle ${property.type} properties in MemberExpression`,
               severity: ErrorSeverity.Todo,
               nodePath: property,
@@ -1213,7 +1213,7 @@ function lowerExpression(
           };
         }
         default: {
-          builder.pushError({
+          builder.recordError({
             reason: `(BuildHIR::lowerExpression) Expected Identifier or MemberExpression, got ${expr.type} lval in AssignmentExpression`,
             severity: ErrorSeverity.InvalidInput,
             nodePath: expr,
@@ -1248,7 +1248,7 @@ function lowerExpression(
       let hasError = false;
       for (const attribute of opening.get("attributes")) {
         if (!attribute.isJSXAttribute()) {
-          builder.pushError({
+          builder.recordError({
             reason: `(BuildHIR::lowerExpression) Handle ${attribute.type} attributes in JSXElement`,
             severity: ErrorSeverity.Todo,
             nodePath: attribute,
@@ -1258,7 +1258,7 @@ function lowerExpression(
         }
         const name = attribute.get("name");
         if (!name.isJSXIdentifier()) {
-          builder.pushError({
+          builder.recordError({
             reason: `(BuildHIR::lowerExpression) Handle ${name.type} attribute names in JSXElement`,
             severity: ErrorSeverity.Todo,
             nodePath: name,
@@ -1272,7 +1272,7 @@ function lowerExpression(
           value = lowerExpressionToPlace(builder, valueExpr);
         } else {
           if (!valueExpr.isJSXExpressionContainer()) {
-            builder.pushError({
+            builder.recordError({
               reason: `(BuildHIR::lowerExpression) Handle ${valueExpr.type} attribute values in JSXElement`,
               severity: ErrorSeverity.Todo,
               nodePath: valueExpr,
@@ -1282,7 +1282,7 @@ function lowerExpression(
           }
           const expression = valueExpr.get("expression");
           if (!expression.isExpression()) {
-            builder.pushError({
+            builder.recordError({
               reason: `(BuildHIR::lowerExpression) Handle ${expression.type} expressions in JSXExpressionContainer within JSXElement`,
               severity: ErrorSeverity.Todo,
               nodePath: valueExpr,
@@ -1334,7 +1334,7 @@ function lowerExpression(
       const lowering = lower(expr);
       let loweredFunc: HIRFunction;
       if (lowering.isErr()) {
-        lowering.unwrapErr().forEach((e) => builder.pushError(e));
+        lowering.unwrapErr().forEach((e) => builder.pushErrorDetail(e));
         return {
           kind: "UnsupportedNode",
           node: exprNode,
@@ -1347,7 +1347,7 @@ function lowerExpression(
       const params: Array<string> = [];
       for (const p of expr.get("params")) {
         if (!p.isIdentifier()) {
-          builder.pushError({
+          builder.recordError({
             reason: `(BuildHIR::lowerExpression) Handle ${p.type} params in FunctionExpression`,
             severity: ErrorSeverity.Todo,
             nodePath: p,
@@ -1373,7 +1373,7 @@ function lowerExpression(
     case "TaggedTemplateExpression": {
       const expr = exprPath as NodePath<t.TaggedTemplateExpression>;
       if (expr.get("quasi").get("expressions").length !== 0) {
-        builder.pushError({
+        builder.recordError({
           reason: "Unhandled tagged template with interpolations",
           severity: ErrorSeverity.Todo,
           nodePath: exprPath,
@@ -1387,7 +1387,7 @@ function lowerExpression(
 
       const value = expr.get("quasi").get("quasis").at(0)!.node.value;
       if (value.raw !== value.cooked) {
-        builder.pushError({
+        builder.recordError({
           reason:
             "Unhandled tagged template where cooked value is different from raw value",
           severity: ErrorSeverity.Todo,
@@ -1413,7 +1413,7 @@ function lowerExpression(
       };
     }
     default: {
-      builder.pushError({
+      builder.recordError({
         reason: `(BuildHIR::lowerExpression) Handle ${exprPath.type} expressions`,
         severity: ErrorSeverity.Todo,
         nodePath: exprPath,
@@ -1433,7 +1433,7 @@ function lowerMemberExpression(
   const property = expr.get("property");
   if (!expr.node.computed) {
     if (!property.isIdentifier()) {
-      builder.pushError({
+      builder.recordError({
         reason: `(BuildHIR::lowerExpression) Handle ${property.type} property`,
         severity: ErrorSeverity.Todo,
         nodePath: property,
@@ -1454,7 +1454,7 @@ function lowerMemberExpression(
     return { object, property: property.node.name, value };
   } else {
     if (!property.isExpression()) {
-      builder.pushError({
+      builder.recordError({
         reason: `(BuildHIR::lowerMemberExpression) Expected Expression, got ${property.type} property`,
         severity: ErrorSeverity.InvalidInput,
         nodePath: property,
@@ -1543,7 +1543,7 @@ function lowerJsxElementName(
   const exprNode = exprPath.node;
   const exprLoc = exprNode.loc ?? GeneratedSource;
   if (!exprPath.isJSXIdentifier()) {
-    builder.pushError({
+    builder.recordError({
       reason: `(BuildHIR::lowerJsxElementName) Handle ${exprPath.type} tags`,
       severity: ErrorSeverity.Todo,
       nodePath: exprPath,
@@ -1604,7 +1604,7 @@ function lowerJsxElement(
   } else if (exprPath.isJSXExpressionContainer()) {
     const expression = exprPath.get("expression");
     if (!expression.isExpression()) {
-      builder.pushError({
+      builder.recordError({
         reason: `(BuildHIR::lowerJsxElement) Handle ${expression.type} expressions`,
         severity: ErrorSeverity.Todo,
         nodePath: expression,
@@ -1638,7 +1638,7 @@ function lowerJsxElement(
     return place;
   } else {
     if (!(t.isJSXFragment(exprNode) || t.isJSXSpreadChild(exprNode))) {
-      builder.pushError({
+      builder.recordError({
         reason: `(BuildHIR::lowerJsxElement) Expected refinement to work, got: ${exprPath.type}`,
         severity: ErrorSeverity.InvalidInput,
         nodePath: exprPath,
@@ -1765,7 +1765,7 @@ function lowerAssignment(
       }
       if (!lvalue.node.computed) {
         if (!property.isIdentifier()) {
-          builder.pushError({
+          builder.recordError({
             reason: `(BuildHIR::lowerAssignment) Handle ${property.type} properties in MemberExpression`,
             severity: ErrorSeverity.Todo,
             nodePath: property,
@@ -1781,7 +1781,7 @@ function lowerAssignment(
         };
       } else {
         if (!property.isExpression()) {
-          builder.pushError({
+          builder.recordError({
             reason:
               "Expected private name to appear as a non-computed property",
             severity: ErrorSeverity.InvalidInput,
@@ -1816,7 +1816,7 @@ function lowerAssignment(
           continue;
         }
         if (element.node.type === "RestElement") {
-          builder.pushError({
+          builder.recordError({
             reason: `(BuildHIR::lowerAssignment) Handle ${element.type} in ArrayPattern`,
             severity: ErrorSeverity.Todo,
             nodePath: element,
@@ -1864,7 +1864,7 @@ function lowerAssignment(
       for (let i = 0; i < properties.length; i++) {
         const property = properties[i];
         if (!property.isObjectProperty()) {
-          builder.pushError({
+          builder.recordError({
             reason: `(BuildHIR::lowerAssignment) Handle ${property.type} properties in ObjectPattern`,
             severity: ErrorSeverity.Todo,
             nodePath: property,
@@ -1874,7 +1874,7 @@ function lowerAssignment(
         }
         const key = property.get("key");
         if (!key.isIdentifier()) {
-          builder.pushError({
+          builder.recordError({
             reason: `(BuildHIR::lowerAssignment) Handle ${key.type} keys in ObjectPattern`,
             severity: ErrorSeverity.Todo,
             nodePath: key,
@@ -1884,7 +1884,7 @@ function lowerAssignment(
         }
         const element = property.get("value");
         if (!element.isLVal()) {
-          builder.pushError({
+          builder.recordError({
             reason: `(BuildHIR::lowerAssignment) Expected object property value to be an LVal, got: ${element.type}`,
             severity: ErrorSeverity.InvalidInput,
             nodePath: element,
@@ -1906,7 +1906,7 @@ function lowerAssignment(
         : objectPlace;
     }
     default: {
-      builder.pushError({
+      builder.recordError({
         reason: `(BuildHIR::lowerAssignment) Handle ${lvaluePath.type} assignments`,
         severity: ErrorSeverity.Todo,
         nodePath: lvaluePath,

compiler/forget/src/HIR/HIRBuilder.ts

@@ -8,7 +8,11 @@
 import { NodePath } from "@babel/traverse";
 import * as t from "@babel/types";
 import invariant from "invariant";
-import { CompilerErrorDetail, CompilerErrorOptions } from "../CompilerError";
+import {
+  CompilerErrorDetail,
+  CompilerErrorOptions,
+  tryPrintCodeFrame,
+} from "../CompilerError";
 import { logHIR } from "../Utils/logger";
 import { assertExhaustive } from "../Utils/utils";
 import { getOrAddGlobal } from "./Globals";
@@ -415,8 +419,17 @@ export default class HIRBuilder {
     invariant(false, "Expected a loop to be in scope");
   }
 
-  pushError(options: CompilerErrorOptions): CompilerErrorDetail {
-    const detail = new CompilerErrorDetail(options);
+  recordError(options: CompilerErrorOptions): CompilerErrorDetail {
+    const detail = new CompilerErrorDetail({
+      reason: options.reason,
+      severity: options.severity,
+      codeframe: tryPrintCodeFrame(options),
+      loc: options.nodePath?.node?.loc ?? null,
+    });
+    return this.pushErrorDetail(detail);
+  }
+
+  pushErrorDetail(detail: CompilerErrorDetail): CompilerErrorDetail {
     this.errors.push(detail);
     return detail;
   }

compiler/forget/src/__tests__/hir-test.ts

@@ -10,7 +10,6 @@
 import { wasmFolder } from "@hpcc-js/wasm";
 import path from "path";
 import runReactForgetBabelPlugin from "../Babel/RunReactForgetBabelPlugin";
-import { CompilerError } from "../CompilerError";
 import { toggleLogging } from "../Utils/logger";
 import generateTestsFromFixtures from "./test-utils/generateTestsFromFixtures";
 
@@ -99,13 +98,7 @@ ${outputs.join("\n")}
 });
 
 function formatErrorOutput(error: Error): string {
-  if (error instanceof CompilerError) {
-    error.message = error.details
-      .map((detail) => `[ReactForget] ${detail.codeFrame}`)
-      .join("\n\n");
-  } else {
-    error.message = error.message.replace(/^\/.*?:\s/, "");
-  }
+  error.message = error.message.replace(/^\/.*?:\s/, "");
   return `
 ## Error