Improve chevrotain parser error recovery (#1822)

Author: msujew

packages/langium/src/grammar/generated/grammar.ts

@@ -234,7 +234,7 @@ export const LangiumGrammarGrammar = (): Grammar => loadedLangiumGrammarGrammar
                 }
               }
             ],
-            "cardinality": "+"
+            "cardinality": "*"
           }
         ]
       },

packages/langium/src/grammar/langium-grammar.langium

@@ -12,7 +12,7 @@ entry Grammar:
         (definesHiddenTokens?='hidden' '(' (hiddenTokens+=[AbstractRule:ID] (',' hiddenTokens+=[AbstractRule:ID])*)? ')')?
     )?
     imports+=GrammarImport*
-    (rules+=AbstractRule | interfaces+=Interface | types+=Type)+;
+    (rules+=AbstractRule | interfaces+=Interface | types+=Type)*;
 
 Interface:
     'interface' name=ID

packages/langium/src/parser/langium-parser.ts

@@ -5,7 +5,7 @@
  ******************************************************************************/
 
 /* eslint-disable @typescript-eslint/no-explicit-any */
-import type { DSLMethodOpts, ILexingError, IOrAlt, IParserErrorMessageProvider, IRecognitionException, IToken, TokenType, TokenVocabulary } from 'chevrotain';
+import type { DSLMethodOpts, ILexingError, IOrAlt, IParserErrorMessageProvider, IRecognitionException, IRuleConfig, IToken, TokenType, TokenVocabulary } from 'chevrotain';
 import type { AbstractElement, Action, Assignment, ParserRule } from '../languages/generated/ast.js';
 import type { Linker } from '../references/linker.js';
 import type { LangiumCoreServices } from '../services.js';
@@ -99,10 +99,6 @@ export interface BaseParser {
      * Executes a grammar action that modifies the currently active AST node
      */
     action($type: string, action: Action): void;
-    /**
-     * Finishes construction of the current AST node. Only used by the AST parser.
-     */
-    construct(): unknown;
     /**
      * Whether the parser is currently actually in use or in "recording mode".
      * Recording mode is activated once when the parser is analyzing itself.
@@ -163,7 +159,6 @@ export abstract class AbstractLangiumParser implements BaseParser {
     abstract consume(idx: number, tokenType: TokenType, feature: AbstractElement): void;
     abstract subrule(idx: number, rule: RuleResult, fragment: boolean, feature: AbstractElement, args: Args): void;
     abstract action($type: string, action: Action): void;
-    abstract construct(): unknown;
 
     getRule(name: string): RuleResult | undefined {
         return this.allRules.get(name);
@@ -239,7 +234,7 @@ export class LangiumParser extends AbstractLangiumParser {
         if (!ruleMethod) {
             throw new Error(options.rule ? `No rule found with name '${options.rule}'` : 'No main rule available.');
         }
-        const result = ruleMethod.call(this.wrapper, {});
+        const result = this.doParse(ruleMethod);
         this.nodeBuilder.addHiddenNodes(lexerResult.hidden);
         this.unorderedGroups.clear();
         this.lexerResult = undefined;
@@ -251,6 +246,22 @@ export class LangiumParser extends AbstractLangiumParser {
         };
     }
 
+    private doParse(rule: RuleResult): any {
+        let result = rule.call(this.wrapper, {});
+        if (this.stack.length > 0) {
+            // In case the parser throws on the entry rule, `construct` is not called
+            // We need to call it manually here
+            result = this.construct();
+        }
+        // Perform some sanity checking
+        if (result === undefined) {
+            throw new Error('No result from parser');
+        } else if (this.stack.length > 0) {
+            throw new Error('Parser stack is not empty after parsing');
+        }
+        return result;
+    }
+
     private startImplementation($type: string | symbol | undefined, implementation: RuleImpl): RuleImpl {
         return (args) => {
             // Only create a new AST node in case the calling rule is not a fragment rule
@@ -262,16 +273,12 @@ export class LangiumParser extends AbstractLangiumParser {
                     node.value = '';
                 }
             }
-            let result: unknown;
-            try {
-                result = implementation(args);
-            } catch (err) {
-                result = undefined;
-            }
-            if (result === undefined && createNode) {
-                result = this.construct();
-            }
-            return result;
+            // Execute the actual rule implementation
+            // The `implementation` never returns anything and only manipulates the parser state.
+            implementation(args);
+            // Once the rule implementation is done, we need to construct the AST node
+            // If the implementation throws (likely a recognition error), we relay the construction to the `subrule` method
+            return createNode ? this.construct() : undefined;
         };
     }
 
@@ -293,6 +300,10 @@ export class LangiumParser extends AbstractLangiumParser {
     consume(idx: number, tokenType: TokenType, feature: AbstractElement): void {
         const token = this.wrapper.wrapConsume(idx, tokenType);
         if (!this.isRecording() && this.isValidToken(token)) {
+            // Before inserting the current token into the CST, we want add the hidden tokens (i.e. comments)
+            // These are located directly before the current token, but are not part of the token stream.
+            // Adding the hidden tokens to the CST requires searching through the CST and finding the correct position.
+            // Performing this work here is more efficient than doing it later on.
             const hiddenTokens = this.extractHiddenTokens(token);
             this.nodeBuilder.addHiddenNodes(hiddenTokens);
             const leafNode = this.nodeBuilder.buildLeafNode(token, feature);
@@ -330,9 +341,25 @@ export class LangiumParser extends AbstractLangiumParser {
             // This is intended, as fragment rules only enrich the current AST node
             cstNode = this.nodeBuilder.buildCompositeNode(feature);
         }
-        const subruleResult = this.wrapper.wrapSubrule(idx, rule, args) as any;
-        if (!this.isRecording() && cstNode && cstNode.length > 0) {
-            this.performSubruleAssignment(subruleResult, feature, cstNode);
+        let result: any;
+        try {
+            result = this.wrapper.wrapSubrule(idx, rule, args);
+        } finally {
+            if (!this.isRecording()) {
+                // Calling `subrule` on chevrotain parsers can result in a recognition error
+                // This likely means that we encounter a syntax error in the input.
+                // In this case, the result of the subrule is `undefined` and we need to call `construct` manually.
+                if (result === undefined && !fragment) {
+                    result = this.construct();
+                }
+                // We want to perform the subrule assignment regardless of the recognition error
+                // But only if the subrule call actually consumed any tokens
+                if (result !== undefined && cstNode && cstNode.length > 0) {
+                    this.performSubruleAssignment(result, feature, cstNode);
+                }
+            }
+            // We don't have a catch block in here because we want to propagate the recognition error to the caller
+            // This results in much better error recovery and error messages from chevrotain
         }
     }
 
@@ -375,7 +402,7 @@ export class LangiumParser extends AbstractLangiumParser {
         }
     }
 
-    construct(): unknown {
+    private construct(): unknown {
         if (this.isRecording()) {
             return undefined;
         }
@@ -673,16 +700,16 @@ class ChevrotainWrapper extends EmbeddedActionsParser {
         return this.RECORDING_PHASE;
     }
 
-    DEFINE_RULE(name: string, impl: RuleImpl): RuleResult {
-        return this.RULE(name, impl);
+    DEFINE_RULE(name: string, impl: RuleImpl, config?: IRuleConfig<any>): RuleResult {
+        return this.RULE(name, impl, config);
     }
 
     wrapSelfAnalysis(): void {
         this.performSelfAnalysis();
     }
 
     wrapConsume(idx: number, tokenType: TokenType): IToken {
-        return this.consume(idx, tokenType);
+        return this.consume(idx, tokenType, undefined);
     }
 
     wrapSubrule(idx: number, rule: RuleResult, args: Args): unknown {

packages/langium/test/parser/langium-parser-builder.test.ts

@@ -231,7 +231,7 @@ describe('Handle unordered group', () => {
             description "New description"
             author "foo"
             `);
-        expect(parsed.parserErrors.length).toBe(2);
+        expect(parsed.parserErrors).toHaveLength(1);
     });
 
     test('Should parse multiple instances', () => {
@@ -250,8 +250,7 @@ describe('Handle unordered group', () => {
             description "Cool book2"
             author "foo2"
 
-            `, parser) as { parserErrors?: string | string[], books?: string[] });
-        expect(lib.parserErrors).toBeUndefined();
+            `, parser) as { books?: string[] });
         expect(lib.books).not.toBeUndefined();
         expect(lib.books?.length).toBe(2);
     });
@@ -321,7 +320,7 @@ describe('check the default value converter for data type rules using terminal r
         const result = parser.parse('123 true abc');
         expect(result.lexerErrors.length).toBe(0);
         expect(result.parserErrors.length).toBe(0);
-        const value = result.value as unknown as { propInteger: number, propBoolean: boolean, propString: string };
+        const value = result.value as AstNode & { propInteger: number, propBoolean: boolean, propString: string };
         expect(value.propInteger).not.toBe('123');
         expect(value.propInteger).toBe(123);
         expect(value.propBoolean).toBe(true);
@@ -343,7 +342,7 @@ describe('Boolean value converter', () => {
     });
 
     function expectValue(input: string, value: unknown): void {
-        const main = parser.parse(input).value as unknown as { value: unknown };
+        const main = parser.parse(input).value as AstNode & { value: unknown };
         expect(main.value).toBe(value);
     }
 
@@ -375,7 +374,7 @@ describe('BigInt Parser value converter', () => {
     });
 
     function expectValue(input: string, value: unknown): void {
-        const main = parser.parse(input).value as unknown as { value: unknown };
+        const main = parser.parse(input).value as AstNode & { value: unknown };
         expect(main.value).toBe(value);
     }
 
@@ -412,7 +411,7 @@ describe('Date Parser value converter', () => {
     });
 
     test('Parsed Date is correct Date object', () => {
-        const parseResult = parser.parse('2022-10-12T00:00').value as unknown as { value: unknown };
+        const parseResult = parser.parse('2022-10-12T00:00').value as AstNode & { value: unknown };
         expect(parseResult.value).toEqual(new Date('2022-10-12T00:00'));
     });
 });
@@ -451,12 +450,12 @@ describe('Parser calls value converter', () => {
     });
 
     function expectValue(input: string, value: unknown): void {
-        const main = parser.parse(input).value as unknown as { value: unknown };
+        const main = parser.parse(input).value as AstNode & { value: unknown };
         expect(main.value).toBe(value);
     }
 
     function expectEqual(input: string, value: unknown): void {
-        const main = parser.parse(input).value as unknown as { value: unknown };
+        const main = parser.parse(input).value as AstNode & { value: unknown };
         expect(main.value).toEqual(value);
     }
 
@@ -527,7 +526,7 @@ describe('Parser calls value converter', () => {
         const result = parser.parse('A');
         expect(result.lexerErrors.length).toBe(0);
         expect(result.parserErrors.length).toBe(0);
-        const value = result.value as unknown as { value: string };
+        const value = result.value as AstNode & { value: string };
         expect(value.value).toBeTypeOf('string');
         expect(value.value).toBe('A');
     });
@@ -547,7 +546,7 @@ describe('Parser calls value converter', () => {
         const result = parser.parse('A');
         expect(result.lexerErrors.length).toBe(0);
         expect(result.parserErrors.length).toBe(0);
-        const value = result.value as unknown as { value: string };
+        const value = result.value as AstNode & { value: string };
         expect(value.value).toBeTypeOf('string');
         expect(value.value).toBe('A');
     });
@@ -817,7 +816,7 @@ describe('Parsing default values', () => {
 
     test('Assigns default values for properties', async () => {
         const result = parser.parse('hi');
-        const model = result.value as unknown as {
+        const model = result.value as AstNode & {
             a: string
             b: string
             c: string[]
@@ -829,7 +828,7 @@ describe('Parsing default values', () => {
 
     test('Does not overwrite parsed value for "b"', async () => {
         const result = parser.parse('hi world');
-        const model = result.value as unknown as {
+        const model = result.value as AstNode & {
             a: string
             b: string
             c: string[]
@@ -840,7 +839,7 @@ describe('Parsing default values', () => {
 
     test('Does not overwrite parsed value for "c"', async () => {
         const result = parser.parse('hi world d e');
-        const model = result.value as unknown as {
+        const model = result.value as AstNode & {
             a: string
             b: string
             c: string[]

packages/langium/test/parser/langium-parser.test.ts

@@ -89,6 +89,28 @@ describe('hidden node parsing', () => {
 
 });
 
+describe('parser error recovery', () => {
+
+    for (const [open, close] of [[1, 0], [2, 0], [3, 0], [2, 1], [3, 1], [3, 2]]) {
+        test(`recovers from lexer error with ${open} open and ${close} close parenthesis`, async () => {
+            const text = `
+                grammar Test
+                entry Model: value=Expr ';';
+                Expr: '(' Expr ')' | value=ID;
+                terminal ID: /[_a-zA-Z][\\w_]*/;
+                hidden terminal WS: /\\s+/;
+            `;
+            const services = await createServicesForGrammar({ grammar: text });
+            const opening = '('.repeat(open);
+            const closing = ')'.repeat(close);
+            const result = services.parser.LangiumParser.parse(`${opening}a${closing};`);
+            // Expect only one parser error independent of the number of missing closing parenthesis
+            expect(result.parserErrors).toHaveLength(1);
+        });
+    }
+
+});
+
 interface A extends AstNode {
     name: string
 }