refactor(parser): improve/test error reporting

aurelia · Apr 29, 2018 · 2ffeb84 · 2ffeb84
1 parent bb185e3
commit 2ffeb84
Show file tree

Hide file tree

Showing 2 changed files with 112 additions and 25 deletions.
diff --git a/src/parser.js b/src/parser.js
@@ -31,6 +31,7 @@ export class ParserImplementation {
   constructor(input) {
     this.index = 0;
     this.startIndex = 0;
+    this.lastIndex = 0;
     this.input = input;
     this.length = input.length;
     this.currentToken = T_EOF;
@@ -41,12 +42,11 @@ export class ParserImplementation {
   parseChain() {
     this.nextToken();
 
-    let isChain = false;
     let expressions = [];
 
     while (this.currentToken !== T_EOF) {
-      while (this.optional(T_Semicolon)) {
-        isChain = true;
+      if (this.optional(T_Semicolon)) {
+        this.error('Multiple expressions are not allowed.');
       }
 
       if ((this.currentToken & T_ClosingToken) === T_ClosingToken) {
@@ -56,11 +56,7 @@ export class ParserImplementation {
       const expr = this.parseBindingBehavior();
       expressions.push(expr);
 
-      while (this.optional(T_Semicolon)) {
-        isChain = true;
-      }
-
-      if (isChain) {
+      if (this.optional(T_Semicolon)) {
         this.error('Multiple expressions are not allowed.');
       }
     }
@@ -108,13 +104,11 @@ export class ParserImplementation {
   }
 
   parseExpression() {
-    let start = this.index;
     let result = this.parseConditional();
 
     while (this.currentToken === T_Eq) {
       if (!result.isAssignable) {
-        let end = (this.index < this.length) ? this.index : this.length;
-        let expression = this.input.slice(start, end);
+        let expression = this.input.slice(this.lastIndex, this.startIndex);
 
         this.error(`Expression ${expression} is not assignable`);
       }
@@ -349,12 +343,14 @@ export class ParserImplementation {
 
   scanToken() {
     while (this.hasNext) {
-      this.startIndex = this.index;
       // skip whitespace.
       if (this.currentChar <= $SPACE) {
         this.nextChar();
         continue;
       }
+
+      this.lastIndex = this.startIndex;
+      this.startIndex = this.index;
 
       // handle identifiers and numbers.
       if (isIdentifierStart(this.currentChar)) {
@@ -640,7 +636,7 @@ export class ParserImplementation {
   }
 
   error(message) {
-    throw new Error(`Lexer Error: ${message} at column ${this.index} in expression [${this.input}]`);
+    throw new Error(`Parser Error: ${message} at column ${this.startIndex} in expression [${this.input}]`);
   }
 
   optional(type) {
@@ -652,12 +648,11 @@ export class ParserImplementation {
     return false;
   }
 
-  expect(type) {
-    if (this.currentToken === type) {
+  expect(token) {
+    if (this.currentToken === token) {
       this.nextToken();
     } else {
-      // todo(fkleuver): translate to string value for readable error messages
-      this.error(`Missing expected token type ${type}`);
+      this.error(`Missing expected token ${TokenValues[token & T_TokenMask]}`);
     }
   }
 }

diff --git a/test/parser.spec.js b/test/parser.spec.js
@@ -20,6 +20,14 @@ import {
   PrefixNot
 } from '../src/ast';
 
+const operators = [
+  '&&', '||',
+  '==', '!=', '===', '!==',
+  '<', '>', '<=', '>=', 
+  '+', '-', 
+  '*', '%', '/'
+];
+
 describe('Parser', () => {
   let parser;
 
@@ -102,14 +110,6 @@ describe('Parser', () => {
   });
 
   describe('parses binary', () => {
-    const operators = [
-      '&&', '||',
-      '==', '!=', '===', '!==',
-      '<', '>', '<=', '>=', 
-      '+', '-', 
-      '*', '%', '/'
-    ];
-
     for (let op of operators) {
       it(`\"${op}\"`, () => {
         let expression = parser.parse(`foo ${op} bar`);
@@ -592,6 +592,98 @@ describe('Parser', () => {
     } catch (e) { pass = false; }
     expect(pass).toBe(false);
   });
+
+  describe('does not parse multiple expressions', () => {
+    const expressions = [
+      ';',
+      'foo;',
+      ';foo',
+      'foo&bar;baz|qux'
+    ];
+
+    for (const expr of expressions) {
+      it(expr, () => {
+        try {
+          parser.parse(expr);
+        } catch(e) {
+          expect(e.message).toContain('Multiple expressions are not allowed');
+        }
+      });
+    }
+  });
+
+  describe('throw on extra closing token', () => {
+    const expressions = [
+      ')',
+      ']',
+      '}',
+      'foo())',
+      'foo[x]]',
+      '{foo}}'
+    ];
+
+    for (const expr of expressions) {
+      it(expr, () => {
+        try {
+          parser.parse(expr);
+        } catch(e) {
+          expect(e.message).toContain('Unconsumed token');
+        }
+      });
+    }
+  });
+
+  describe('throw on assigning unassignable', () => {
+    const expressions = [
+      'foo ? bar : baz = qux',
+      '$this = foo',
+      'foo() = bar',
+      'foo.bar() = baz',
+      '!foo = bar',
+      '-foo = bar',
+      '-foo = bar',
+      '\'foo\' = bar',
+      '42 = foo',
+      '[] = foo',
+      '{} = foo'
+    ].concat(operators.map(op => `foo ${op} bar`));
+
+    for (const expr of expressions) {
+      it(expr, () => {
+        try {
+          parser.parse(expr);
+        } catch(e) {
+          expect(e.message).toContain('is not assignable');
+        }
+      });
+    }
+  });
+
+  it('throw on incomplete conditional', () => {
+    try {
+      parser.parse('foo ? bar');
+    } catch(e) {
+      expect(e.message).toContain('requires all 3 expressions');
+    }
+  });
+
+  describe('throw on invalid exponent', () => {
+    const expressions = [
+      '1e',
+      '1ee',
+      '1e.'
+    ];
+
+    for (const expr of expressions) {
+      it(expr, () => {
+        try {
+          parser.parse(expr);
+        } catch(e) {
+          expect(e.message).toContain('Invalid exponent');
+        }
+      });
+    }
+  });
 });
 
 function verifyEqual(actual, expected) {