Improve programmer productivity with diagnostics (#301)

1. Make throwDiagnostic take the target directly and figure out the location rather than having the programmer call getSourceLocation. 2. If getting location or formatting message fails, include as much of the diagnostic as possible in the internal compiler error.
2021-02-16 10:41:51 -08:00 · 2021-02-16 10:41:51 -08:00 · 02141ec9ba
--- a/packages/adl/compiler/binder.ts
+++ b/packages/adl/compiler/binder.ts
@ -1,13 +1,9 @@
-import { Suite } from 'mocha';
-import { format } from '../lib/decorators.js';
-import { DiagnosticError, formatDiagnostic, getSourceLocationOfNode, throwDiagnostic } from './diagnostics.js';
+import { DiagnosticError, formatDiagnostic } from './diagnostics.js';
 import { visitChildren } from './parser.js';
 import { ADLSourceFile, Program } from './program.js';
 import { createSourceFile } from './scanner.js';
-import { NamespaceStatementNode, ModelStatementNode, Node, SyntaxKind, TemplateParameterDeclarationNode, SourceLocation } from './types.js';
+import { NamespaceStatementNode, ModelStatementNode, Node, SyntaxKind, TemplateParameterDeclarationNode, SourceLocation, Sym } from './types.js';

-// trying to avoid masking built-in Symbol
-export type Sym = DecoratorSymbol | TypeSymbol;

 export class SymbolTable extends Map<string, Sym> {
  duplicates = new Set<Sym>();
@ -156,28 +152,13 @@ export function createBinder(): Binder {
    function report(symbol: Sym) {
      if (!reported.has(symbol)) {
        reported.add(symbol);
-        const message = formatDiagnostic('Duplicate name: ' + symbol.name, getSourceLocationOfSymbol(symbol));
+        const message = formatDiagnostic('Duplicate name: ' + symbol.name, symbol);
        messages.push(message);
      }
    }
  }
 }

-function getSourceLocationOfSymbol(symbol: Sym): SourceLocation {
-  switch (symbol.kind) {
-    case 'decorator':
-      return {
-        // We currently report all decorators at line 1 of defining .js path.
-        file: createSourceFile("", symbol.path),
-        pos: 0,
-        end: 0
-      };
-    case 'type':
-      return getSourceLocationOfNode(symbol.node);
-  }
-}
-
-
 function hasScope(node: Node) {
  switch (node.kind) {
    case SyntaxKind.ModelStatement:
--- a/packages/adl/compiler/checker.ts
+++ b/packages/adl/compiler/checker.ts
@ -1,4 +1,4 @@
-import { getSourceLocationOfNode, throwDiagnostic } from './diagnostics.js';
+import { throwDiagnostic } from './diagnostics.js';
 import { ADLSourceFile, Program } from './program.js';
 import {
  ArrayExpressionNode,
@ -121,7 +121,7 @@ export function createChecker(program: Program) {
        return checkTemplateParameterDeclaration(node);
    }

-    throwDiagnostic('Cannot evaluate ' + SyntaxKind[node.kind], getSourceLocationOfNode(node));
+    throwDiagnostic('Cannot evaluate ' + SyntaxKind[node.kind], node);
  }

  function getTypeName(type: Type): string {
@ -190,11 +190,11 @@ export function createChecker(program: Program) {
   */
  function instantiateTemplate(templateNode: ModelStatementNode, args: Array<Type>): ModelType {
    if (templateNode.templateParameters!.length < args.length) {
-      throwDiagnostic('Too few template arguments provided.', getSourceLocationOfNode(templateNode));
+      throwDiagnostic('Too few template arguments provided.', templateNode);
    }

    if (templateNode.templateParameters!.length > args.length) {
-      throwDiagnostic('Too many template arguments provided.', getSourceLocationOfNode(templateNode));
+      throwDiagnostic('Too many template arguments provided.', templateNode);
    }

    const oldTis = templateInstantiation;
@ -229,7 +229,7 @@ export function createChecker(program: Program) {
  function checkIntersectionExpression(node: IntersectionExpressionNode) {
    const optionTypes = node.options.map(getTypeForNode);
    if (!allModelTypes(optionTypes)) {
-      throwDiagnostic('Cannot intersect non-model types (including union types).', getSourceLocationOfNode(node));
+      throwDiagnostic('Cannot intersect non-model types (including union types).', node);
    }

    const properties = new Map<string, ModelTypeProperty>();
@ -237,7 +237,7 @@ export function createChecker(program: Program) {
      const allProps = walkPropertiesInherited(option);
      for (const prop of allProps) {
        if (properties.has(prop.name)) {
-          throwDiagnostic(`Intersection contains duplicate property definitions for ${prop.name}`, getSourceLocationOfNode(node));
+          throwDiagnostic(`Intersection contains duplicate property definitions for ${prop.name}`, node);
        }

        const newPropType = createType({
@ -330,7 +330,7 @@ export function createChecker(program: Program) {
    }

    if (!binding) {
-      throwDiagnostic('Unknown identifier ' + node.sv, getSourceLocationOfNode(node));
+      throwDiagnostic('Unknown identifier ' + node.sv, node);
    }

    return binding;
@ -377,7 +377,7 @@ export function createChecker(program: Program) {

          for (const newProp of newProperties) {
            if (properties.has(newProp.name)) {
-              throwDiagnostic(`Model already has a property named ${newProp.name}`, getSourceLocationOfNode(node));
+              throwDiagnostic(`Model already has a property named ${newProp.name}`, node);
            }

            properties.set(newProp.name, newProp);
@ -420,7 +420,7 @@ export function createChecker(program: Program) {
      const heritageType = getTypeForNode(heritageRef);

      if (heritageType.kind !== "Model") {
-        throwDiagnostic("Models must extend other models.", getSourceLocationOfNode(heritageRef))
+        throwDiagnostic("Models must extend other models.", heritageRef);
      }

      return heritageType;
@ -434,7 +434,7 @@ export function createChecker(program: Program) {

    if (targetType.kind != 'TemplateParameter') {
      if (targetType.kind !== 'Model') {
-        throwDiagnostic('Cannot spread properties of non-model type.', getSourceLocationOfNode(targetNode));
+        throwDiagnostic('Cannot spread properties of non-model type.', targetNode);
      }

      // copy each property
--- a/packages/adl/compiler/cli.ts
+++ b/packages/adl/compiler/cli.ts
@ -8,7 +8,7 @@ import { readFileSync } from "fs";
 import { compile } from "../compiler/program.js";
 import { spawnSync } from "child_process";
 import { CompilerOptions } from "../compiler/options.js";
-import { DiagnosticError } from "./diagnostics.js";
+import { DiagnosticError, dumpError } from "./diagnostics.js";

 const adlVersion = getVersion();

@ -162,7 +162,7 @@ main()
    // considered a bug and therefore we should not suppress the stack trace as
    // that risks losing it in the case of a bug that does not repro easily.
    console.error("Internal compiler error!");
-    console.error("File issue at https://github.com/azure/adl\n");
-    console.error(err.stack); // includes message.
+    console.error("File issue at https://github.com/azure/adl");
+    dumpError(err, console.error);
    process.exit(1);
  });
--- a/packages/adl/compiler/diagnostics.ts
+++ b/packages/adl/compiler/diagnostics.ts
@ -1,6 +1,7 @@
-import { Message, Node, SourceLocation, SyntaxKind, SourceFile, Type } from "./types.js";
+import { createSourceFile } from "./scanner.js";
+import { Message, Node, SourceLocation, SyntaxKind, Type, Sym } from "./types.js";

-/** 
+/**
 * Represents an error in the code input that is fatal and bails the compilation.
 * 
 * This isn't meant to be kept long term, but we currently do this on all errors.
@ -11,48 +12,125 @@ export class DiagnosticError extends Error {
  }
 }

-export type ErrorHandler = (message: Message | string, location: SourceLocation, ...args: Array<string | number>) => void;
+/**
+ * Represents a failure with multiple errors.
+ */
+export class ChainedError extends Error {
+  readonly innerErrors: readonly Error[];
+
+  constructor(message: string, ...innerErrors: (Error | undefined)[]) {
+    super(message);
+    this.innerErrors = innerErrors.filter(isNotUndefined);
+  }
+}
+
+export type DiagnosticTarget = Node | Type | Sym | SourceLocation;
+
+export type ErrorHandler = (message: Message | string, target: DiagnosticTarget, ...args: Array<string | number>) => void;

 export const throwOnError: ErrorHandler = throwDiagnostic;

-export function throwDiagnostic(message: Message | string, location: SourceLocation, ...args: Array<string | number>): never {
-  throw new DiagnosticError(formatDiagnostic(message, location, ...args));
+export function throwDiagnostic(message: Message | string,  target: DiagnosticTarget, ...args: Array<string | number>): never {
+  throw new DiagnosticError(formatDiagnostic(message, target, ...args));
 }

-export function formatDiagnostic(message: Message | string, location: SourceLocation, ...args: Array<string | number>) { 
+/** 
+ * Format a diagnostic into <file>:<line> - ADL<code> <category>: <text>.
+ * Take extra care to preserve all info in thrown Error if this fails.
+ */
+export function formatDiagnostic(message: Message | string, target: DiagnosticTarget, ...args: Array<string | number>) { 
+  let location: SourceLocation;
+  let locationError: Error | undefined;
+
+  try {
+    location = getSourceLocation(target);
+  } catch (err) {
+    locationError = err;
+    location = { 
+      file: createSourceFile("", "<unknown location>"),
+      pos: 0,
+      end: 0
+    }
+  }
+
  if (typeof message === 'string') {
    // Temporarily allow ad-hoc strings as error messages.
    message = { code: -1, text: message, category: 'error' }
  }

+  const [msg, formatError] = format(message.text, ...args);
  const code = message.code < 0 ? "" : ` ADL${message.code}`;
  const pos = location.file.getLineAndCharacterOfPosition(location.pos);
-  const msg = format(message.text, ...args);
-  return `${location.file.path}:${pos.line + 1}:${pos.character + 1} - ${message.category}${code}: ${msg}`;
+  const diagnostic = `${location.file.path}:${pos.line + 1}:${pos.character + 1} - ${message.category}${code}: ${msg}`;
+
+  if (locationError || formatError) {
+    throw new ChainedError(diagnostic, locationError, formatError);
+  }
+
+  return diagnostic;
 }

-export function getSourceFileOfNode(node: Node): SourceFile {
-  while (node.parent !== undefined) {
-    node = node.parent;
+export function getSourceLocation(target: DiagnosticTarget): SourceLocation {
+  if ("file" in target) {
+    return target;
  }
-  if (node.kind !== SyntaxKind.ADLScript) {
+
+  if (target.kind === "decorator") {
+    return {
+      // We currently report all decorators at line 1 of defining .js path.
+      file: createSourceFile("", target.path),
+      pos: 0,
+      end: 0,
+    };
+  }
+
+  return getSourceLocationOfNode("node" in target ? target.node : target);
+}
+
+function getSourceLocationOfNode(node: Node): SourceLocation {
+  let root = node;
+
+  while (root.parent !== undefined) {
+    root = root.parent;
+  }
+
+  if (root.kind !== SyntaxKind.ADLScript) {
    throw new Error("Cannot obtain source file of unbound node.");
  }
-  return node.file;
-}

-export function getSourceLocationOfNode(node: Node): SourceLocation {
  return {
-    file: getSourceFileOfNode(node),
+    file: root.file,
    pos: node.pos,
-    end: node.end,
+    end: node.end
  }
 }

-export function getSourceLocationOfType(type: Type): SourceLocation {
-  return getSourceLocationOfNode(type.node);
+export function dumpError(error: Error, writeLine: (s?: string) => void) {
+  writeLine("");
+  writeLine(error.stack);
+
+  if (error instanceof ChainedError) {
+    for (const inner of error.innerErrors) {
+      dumpError(inner, writeLine);
+    }
+  }
 }

-function format(text: string, ...args: Array<string | number>): string {
-  return text.replace(/{(\d+)}/g, (_match, index: string) => '' + args[+index] || '<ARGMISSING>');
+function format(text: string, ...args: Array<string | number>): [string, Error?] {
+  let error: Error | undefined;
+  const message = text.replace(/{(\d+)}/g, (_match, indexString: string) => {
+    const index = Number(indexString);
+    if (index >= args.length) {
+      error = new Error("Missing format argument.");
+      return "<missing argument>";
+    }
+
+    return args[index].toString();
+  });
+
+  return [message, error];
+}
+
+function isNotUndefined<T>(value: T | undefined): value is T {
+  return value !== undefined;
 }
--- a/packages/adl/compiler/program.ts
+++ b/packages/adl/compiler/program.ts
@ -2,7 +2,7 @@ import url from "url";
 import path from "path";
 import { readdir, readFile } from 'fs/promises';
 import { join } from 'path';
-import { createBinder, DecoratorSymbol, SymbolTable } from './binder.js';
+import { createBinder, SymbolTable } from './binder.js';
 import { createChecker, MultiKeyMap } from './checker.js';
 import { CompilerOptions } from './options.js';
 import { parse } from './parser.js';
@ -17,10 +17,11 @@ import {
  SyntaxKind,
  Type,
  SourceFile,
+  DecoratorSymbol,

 } from './types.js';
 import { createSourceFile } from "./scanner.js";
-import { getSourceLocationOfNode, getSourceLocationOfType, throwDiagnostic } from "./diagnostics.js";
+import { throwDiagnostic } from "./diagnostics.js";

 export interface Program {
  compilerOptions: CompilerOptions;
@ -118,7 +119,7 @@ export async function compile(rootDir: string, options?: CompilerOptions) {

  function executeDecorator(dec: DecoratorExpressionNode, program: Program, type: Type) {
    if (dec.target.kind !== SyntaxKind.Identifier) {
-      throwDiagnostic('Decorator must be identifier', getSourceLocationOfNode(dec));
+      throwDiagnostic('Decorator must be identifier', dec);
    }

    const decName = dec.target.sv;
@ -127,7 +128,7 @@ export async function compile(rootDir: string, options?: CompilerOptions) {
    );
    const decBinding = <DecoratorSymbol>program.globalSymbols.get(decName);
    if (!decBinding) {
-      throwDiagnostic(`Can't find decorator ${decName}`, getSourceLocationOfNode(dec));
+      throwDiagnostic(`Can't find decorator ${decName}`, dec);
    }
    const decFn = decBinding.value;
    decFn(program, type, ...args);
@ -227,7 +228,6 @@ export async function compile(rootDir: string, options?: CompilerOptions) {
    const sourceFile = {
      ...unparsedFile,
      ast,
-      path: filePath,
      interfaces: [],
      models: [],
      symbols: new SymbolTable(),
--- a/packages/adl/compiler/types.ts
+++ b/packages/adl/compiler/types.ts
@ -100,6 +100,21 @@ export interface TemplateParameterType extends BaseType {
  kind: 'TemplateParameter';
 }

+// trying to avoid masking built-in Symbol
+export type Sym = DecoratorSymbol | TypeSymbol;
+
+export interface DecoratorSymbol {
+  kind: 'decorator';
+  path: string;
+  name: string;
+  value: (...args: Array<any>) => any;
+}
+
+export interface TypeSymbol {
+  kind: 'type';
+  node: Node;
+  name: string;
+}

 /**
 * AST types