Multi-errors: Capture all errors in each compilation stage

(review with "ignore whitespace changes" on)

Author: Mingun

lib/compiler/index.js

@@ -98,6 +98,9 @@ const compiler = {
 
         pass(ast, options, session);
       });
+
+      // Collect all errors by stage
+      session.checkErrors();
     });
 
     switch (options.output) {

lib/compiler/session.js

@@ -11,8 +11,8 @@ class Defaults {
     if (typeof options.info === "function") { this.info = options.info; }
   }
 
-  error(stage, ...args) {
-    throw new GrammarError(...args);
+  error() {
+    // Intentionally empty placeholder
   }
 
   warning() {
@@ -27,13 +27,36 @@ class Defaults {
 class Session {
   constructor(options) {
     this._callbacks = new Defaults(options);
+    this._firstError = null;
     this.errors = 0;
+    /** @type {import("../peg").Problem[]} */
     this.problems = [];
+    /** @type {import("../peg").Stage} */
     this.stage = null;
   }
 
   error(...args) {
     ++this.errors;
+    // In order to preserve backward compatibility we cannot change `GrammarError`
+    // constructor, nor throw another class of error:
+    // - if we change `GrammarError` constructor, this will break plugins that
+    //   throws `GrammarError`
+    // - if we throw another Error class, this will break parser clients that
+    //   catches GrammarError
+    //
+    // So we use a compromise: we throw an `GrammarError` with all found problems
+    // in the `problems` property, but the thrown error itself is the first
+    // registered error.
+    //
+    // Thus when the old client catches the error it can find all properties on
+    // the Grammar error that it want. On the other hand the new client can
+    // inspect the `problems` property to get all problems.
+    if (this._firstError === null) {
+      this._firstError = new GrammarError(...args);
+      this._firstError.stage = this.stage;
+      this._firstError.problems = this.problems;
+    }
+
     this.problems.push(["error", ...args]);
     this._callbacks.error(this.stage, ...args);
   }
@@ -47,6 +70,12 @@ class Session {
     this.problems.push(["info", ...args]);
     this._callbacks.info(this.stage, ...args);
   }
+
+  checkErrors() {
+    if (this.errors !== 0) {
+      throw this._firstError;
+    }
+  }
 }
 
 module.exports = Session;

lib/grammar-error.js

@@ -17,6 +17,7 @@ const setProtoOf = Object.setPrototypeOf
   };
 
 // Thrown when the grammar contains an error.
+/** @type {import("./peg").GrammarError} */
 class GrammarError extends Error {
   constructor(message, location, diagnostics) {
     super(message);
@@ -27,6 +28,10 @@ class GrammarError extends Error {
       diagnostics = [];
     }
     this.diagnostics = diagnostics;
+    // All problems if this error is thrown by the plugin and not at stage
+    // checking phase
+    this.stage = null;
+    this.problems = [["error", message, location, diagnostics]];
   }
 
   toString() {
@@ -51,9 +56,6 @@ class GrammarError extends Error {
     return str;
   }
 
-  /**
-   * @typedef SourceText {source: any, text: string}
-   */
   /**
    * Format the error with associated sources.  The `location.source` should have
    * a `toString()` representation in order the result to look nice. If source
@@ -73,7 +75,7 @@ class GrammarError extends Error {
    *    |     ^^^^
    * ```
    *
-   * @param {SourceText[]} sources mapping from location source to source text
+   * @param {import("./peg").SourceText[]} sources mapping from location source to source text
    *
    * @returns {string} the formatted error
    */
@@ -83,6 +85,14 @@ class GrammarError extends Error {
       text: text.split(/\r\n|\n|\r/g),
     }));
 
+    /**
+     * Returns a highlighted piece of source to which the `location` points
+     *
+     * @param {import("./peg").LocationRange} location
+     * @param {number} indent How much width in symbols line number strip should have
+     * @param {string} message Additional message that will be shown after location
+     * @returns {string}
+     */
     function entry(location, indent, message = "") {
       let str = "";
       const src = srcLines.find(({ source }) => source === location.source);
@@ -109,30 +119,43 @@ ${"".padEnd(indent)} | ${"".padEnd(s.column - 1)}${"".padEnd(last - s.column, "^
       return str;
     }
 
-    // Calculate maximum width of all lines
-    let maxLine;
-    if (this.location) {
-      maxLine = this.diagnostics.reduce(
-        (t, { location }) => Math.max(t, location.start.line),
-        this.location.start.line
-      );
-    } else {
-      maxLine = Math.max.apply(
-        null,
-        this.diagnostics.map(d => d.location.start.line)
-      );
-    }
-    maxLine = maxLine.toString().length;
+    /**
+     * Returns a formatted representation of the one problem in the error.
+     *
+     * @param {import("./peg").Severity} severity Importance of the message
+     * @param {string} message Test message of the problem
+     * @param {import("./peg").LocationRange?} location Location of the problem in the source
+     * @param {import("./peg").DiagnosticNote[]} diagnostics Additional notes about the problem
+     * @returns {string}
+     */
+    function formatProblem(severity, message, location, diagnostics) {
+      // Calculate maximum width of all lines
+      let maxLine;
+      if (location) {
+        maxLine = diagnostics.reduce(
+          (t, { location }) => Math.max(t, location.start.line),
+          location.start.line
+        );
+      } else {
+        maxLine = Math.max.apply(
+          null,
+          diagnostics.map(d => d.location.start.line)
+        );
+      }
+      maxLine = maxLine.toString().length;
 
-    let str = `Error: ${this.message}`;
-    if (this.location) {
-      str += entry(this.location, maxLine);
-    }
-    for (const diag of this.diagnostics) {
-      str += entry(diag.location, maxLine, diag.message);
+      let str = `${severity}: ${message}`;
+      if (location) {
+        str += entry(location, maxLine);
+      }
+      for (const diag of diagnostics) {
+        str += entry(diag.location, maxLine, diag.message);
+      }
+
+      return str;
     }
 
-    return str;
+    return this.problems.map(p => formatProblem(p[0], p[1], p[2], p[3])).join("\n\n");
   }
 }
 

lib/peg.d.ts

@@ -271,6 +271,22 @@ export interface DiagnosticNote {
   location: LocationRange;
 }
 
+/** Possible compilation stage name. */
+type Stage = keyof Stages;
+/** Severity level of problems that can be registered in compilation session. */
+type Severity = "error" | "warning" | "info";
+
+type Problem = [
+  /** Problem severity. */
+  Severity,
+  /** Diagnostic message. */
+  string,
+  /** Location where message is generated, if applicable. */
+  LocationRange?,
+  /** List of additional messages with their locations, if applicable. */
+  DiagnosticNote[]?,
+];
+
 /** Thrown if the grammar contains a semantic error. */
 export class GrammarError extends Error {
   /** Location of the error in the source. */
@@ -279,6 +295,15 @@ export class GrammarError extends Error {
   /** Additional messages with context information. */
   diagnostics: DiagnosticNote[];
 
+  /** Compilation stage during which error was generated. */
+  stage: Stage | null;
+
+  /**
+   * List of diagnostics containing all errors, warnings and information
+   * messages generated during compilation stage `stage`.
+   */
+  problems: Problem[];
+
   constructor(
     message: string,
     location?: LocationRange,
@@ -951,6 +976,51 @@ export interface Session {
   ): void;
 }
 
+export interface DiagnosticCallback {
+  /**
+   * Called when compiler reports an error.
+   *
+   * @param stage Stage in which this diagnostic was originated
+   * @param message Main message, which should describe error objectives
+   * @param location If defined, this is location described in the `message`
+   * @param notes Additional messages with context information
+   */
+  error?(
+    stage: Stage,
+    message: string,
+    location?: LocationRange,
+    notes?: DiagnosticNote[]
+  ): void;
+  /**
+   * Called when compiler reports a warning.
+   *
+   * @param stage Stage in which this diagnostic was originated
+   * @param message Main message, which should describe warning objectives
+   * @param location If defined, this is location described in the `message`
+   * @param notes Additional messages with context information
+   */
+  warning?(
+    stage: Stage,
+    message: string,
+    location?: LocationRange,
+    notes?: DiagnosticNote[]
+  ): void;
+  /**
+   * Called when compiler reports an informational message.
+   *
+   * @param stage Stage in which this diagnostic was originated
+   * @param message Main message, which gives information about an event
+   * @param location If defined, this is location described in the `message`
+   * @param notes Additional messages with context information
+   */
+  info?(
+    stage: Stage,
+    message: string,
+    location?: LocationRange,
+    notes?: DiagnosticNote[]
+  ): void;
+}
+
 /**
  * Parser dependencies, is an object which maps variables used to access the
  * dependencies in the parser to module IDs used to load them
@@ -988,6 +1058,13 @@ export interface BuildOptionsBase {
 
   /** Makes the parser trace its progress (default: `false`) */
   trace?: boolean;
+
+  /** Called when a semantic error during build was detected. */
+  error?: DiagnosticCallback;
+  /** Called when a warning during build was registered. */
+  warning?: DiagnosticCallback;
+  /** Called when an informational message during build was registered. */
+  info?: DiagnosticCallback;
 }
 
 export interface ParserBuildOptions extends BuildOptionsBase {

test/behavior/generated-parser-behavior.spec.js

@@ -1780,5 +1780,36 @@ Error: Expected "{", code block, comment, end of line, identifier, or whitespace
         }
       }).to.throw(peg.parser.SyntaxError);
     });
+
+    it("reports multiple errors in each compilation stage", () => {
+      try {
+        peg.generate(`
+          start = leftRecursion
+          leftRecursion = duplicatedLabel:duplicatedRule duplicatedLabel:missingRule
+          duplicatedRule = missingRule
+          duplicatedRule = start
+        `);
+      } catch (e) {
+        expect(e).with.property("stage", "check");
+        expect(e).with.property("problems").to.be.an("array");
+
+        // Check that each problem is an array with at least two members and the first is a severity
+        e.problems.forEach(problem => {
+          expect(problem).to.be.an("array").lengthOf.gte(2);
+          expect(problem[0]).to.be.oneOf(["error", "warning", "info"]);
+        });
+
+        // Get second elements of errors (error messages)
+        const messages = e.problems.filter(p => p[0] === "error").map(p => p[1]);
+
+        // Check that all messages present in the list
+        expect(messages).to.include.members([
+          "Rule \"missingRule\" is not defined",
+          "Rule \"duplicatedRule\" is already defined",
+          "Label \"duplicatedLabel\" is already defined",
+          "Possible infinite loop when parsing (left recursion: duplicatedRule -> start -> leftRecursion -> duplicatedRule)",
+        ]);
+      }
+    });
   });
 });

test/unit/compiler/passes/helpers.js

@@ -1,5 +1,6 @@
 "use strict";
 
+const { GrammarError } = require("../../../../lib/peg");
 const parser = require("../../../../lib/parser");
 const Session = require("../../../../lib/compiler/session");
 
@@ -47,7 +48,9 @@ module.exports = function(chai, utils) {
 
     const ast = parser.parse(grammar);
 
-    utils.flag(this, "object")(ast, options, new Session());
+    utils.flag(this, "object")(ast, options, new Session({
+      error(stage, ...args) { throw new GrammarError(...args); },
+    }));
 
     this.assert(
       matchProps(ast, props),
@@ -64,7 +67,9 @@ module.exports = function(chai, utils) {
     let passed, result;
 
     try {
-      utils.flag(this, "object")(ast, {}, new Session());
+      utils.flag(this, "object")(ast, {}, new Session({
+        error(stage, ...args) { throw new GrammarError(...args); },
+      }));
       passed = true;
     } catch (e) {
       result = e;