Merge pull request #115 from firebase/koss-parser-errors

Improve parsing errors to be more specific.

docs/changelog.txt

@@ -0,0 +1 @@
+fixed - Parse errors are more granular - reporting the source of the error line rather than just failing a whole type or path statement.

src/logger.ts

@@ -45,7 +45,12 @@ export function setContext(fn: () => ErrorContext) {
 }
 
 export function error(s: string) {
-  lastMessage = errorString(s);
+  let err = errorString(s);
+  // De-dup identical messages
+  if (err  === lastMessage) {
+    return;
+  }
+  lastMessage = err;
   lastError = lastMessage;
   if (!silenceOutput) {
     console.error(lastError);
@@ -54,7 +59,12 @@ export function error(s: string) {
 }
 
 export function warn(s: string) {
-  lastMessage = errorString(s);
+  let err = errorString(s);
+  // De-dup identical messages
+  if (err === lastMessage) {
+    return;
+  }
+  lastMessage = err;
   if (!silenceOutput) {
     console.warn(lastMessage);
   }

src/rules-parser.pegjs

@@ -86,19 +86,56 @@ Statements = rules:(Statement _)*
 
 Statement = f:Function / p:Path / s:Schema
 
-Function "function definition" = ("function" __)? name:Identifier params:ParameterList _ body:FunctionBody {
-  symbols.registerFunction(ensureLowerCase(name, "Function names"), params, body);
+Function "function definition" = func:FunctionStart body:FunctionBody? {
+  if (func.name === null) {
+    error("Missing function name.");
+    return;
+  }
+  if (func.params === null) {
+    error("Function " + func.name + " missing parameters.");
+    return;
+  }
+  if (body === null) {
+    error("Function " + func.name + " missing or invalid function body.");
+    return;
+  }
+  symbols.registerFunction(ensureLowerCase(func.name, "Function names"), func.params, body);
 }
 
-Path "path statement" = ("path" __)? path:(path:PathExpression { currentPath.push(path); return path; })
-  isType:(__ "is" __ id:TypeExpression { return id; })? _
-  methods:("{" _ all:PathsAndMethods "}" { return all; } / ";" { return {}; } ) _ {
+// For better error handling.
+FunctionStart = "function" __ name:Identifier? _ params:ParameterList? _ { return {name: name, params: params}; }
+  / name:Identifier _ params:ParameterList _ {return {name: name, params: params}; }
+
+Path "path statement" = path:PathStart isType:(__ "is" __ id:TypeExpression { return id; })? _
+  methods:("{" _ all:PathsAndMethods "}" { return all; } / ";" { return {}; } )? _ {
+    if (path === null) {
+      return;
+    }
+    if (methods === null) {
+      error("Missing body of path statement.");
+      return;
+    }
     symbols.registerPath(currentPath, isType, methods);
     currentPath.pop(path);
   }
 
+// Break out for better error handling.
+PathStart = "path" __ path:PathTemplate?
+  {
+    if (path === null) {
+      error("Missing Path Template in path statement.");
+      return path;
+    }
+    currentPath.push(path);
+    return path;
+  }
+  / path:PathTemplate
+  {
+    currentPath.push(path); return path;
+  }
+
 // Parse trailing slash and empty parts but emit error message.
-PathExpression "path" =  parts:("/" part:PathKey? { return part; })+ {
+PathTemplate "path template" =  parts:("/" part:PathKey? { return part; })+ {
   var hasError = false;
   if (parts.length === 1 && parts[0] === null) {
     parts = [];
@@ -113,7 +150,7 @@ PathExpression "path" =  parts:("/" part:PathKey? { return part; })+ {
   if (hasError) {
     error((parts[parts.length - 1] === ''
            ? "Paths may not end in a slash (/) character"
-           : "Paths may not contain an empty part") + ": /" + parts.join('/'));
+           : "Paths may not contain an empty part") + ": /" + parts.map(function(part) { return part.label; }).join('/'));
   }
   return new ast.PathTemplate(parts);
 }
@@ -133,14 +170,18 @@ LiteralPathKey = chars: [^ /;]+ {
   return new ast.PathPart(result);
 }
 
-PathsAndMethods = all:(Path / Method)* _ {
+PathsAndMethods = all:(Path / Method / AnyBlock)* _ {
   var result = {};
   for (var i = 0; i < all.length; i++) {
     var method = all[i];
     // Skip embedded path statements.
     if (method === undefined) {
       continue;
     }
+    if (typeof method == 'string') {
+      error("Invalid path or method: '" + method + "'.");
+      continue;
+    }
     if (method.name in result) {
       error("Duplicate method name: " + method.name);
     }
@@ -150,26 +191,38 @@ PathsAndMethods = all:(Path / Method)* _ {
 }
 
 Schema "type statement" =
-  "type" __ type:Identifier
+  "type" __ type:Identifier?
   params:("<" list:IdentifierList ">" { return ensureUpperCase(list, "Type names"); })?
   ext:(__ "extends" __ type:TypeExpression  _ { return type; })?
   body:(_ "{" _ all:PropertiesAndMethods "}" { return all; }
-        / _ ";" { return {properties: {}, methods: {}}; } ) {
+        / _ ";" { return {properties: {}, methods: {}}; } )? {
     if (params === null) {
       params = [];
     }
+    if (type === null) {
+      error("Missing type name.");
+      return;
+    }
+    if (body === null) {
+      error("Missing or invalid type statement body.");
+      return;
+    }
     symbols.registerSchema(ensureUpperCase(type, "Type names"),
                            ext, body.properties, body.methods, params);
 }
 
-PropertiesAndMethods = all:(Property / Method)* _ {
+PropertiesAndMethods = all:(Property / Method / AnyBlock)* _ {
   var result = {
      properties: {},
      methods: {}
   };
 
   function addPart(part) {
     // TODO: Make sure methods and properties don't shadow each other.
+    if (typeof part === 'string') {
+      error("Invalid property or method: '" + part + "'.");
+      return;
+    }
     if ('type' in part) {
       if (result.properties[part.name]) {
         error("Duplicate property name: " + part.name);
@@ -197,18 +250,21 @@ Property = name:(Identifier / String) _ ":" _ type:TypeExpression _ PropSep {
   };
 }
 
-PropSep = ("," / ";")? _
+PropSep = sep:("," / ";")? _ { return sep; }
 
-Method "method" = name:Identifier params:ParameterList _ body:FunctionBody {
+Method "method" = name:Identifier params:ParameterList _ body:FunctionBody sep:PropSep {
+  if (sep !== null) {
+    warn("Extra separator (" + sep + ") not needed.");
+  }
   return {
     name:  ensureLowerCase(name, "Method names"),
     params: params,
     body:  body
   };
 }
 
-FunctionBody = "{" _ ("return" _)? exp:Expression _ ";"? _ "}" _ { return exp; }
-  / "=" _ exp:Expression _ ";" _ {
+FunctionBody = "{" _ ("return" __)? exp:Expression _ ";"? _ "}" _ { return exp; }
+  / "=" _ exp:Expression ";"? _ {
     warn("Use of fn(x) = exp; format is deprecated; use fn(x) { exp }, instead.")
     return exp;
   }
@@ -368,6 +424,7 @@ EqualityExpression
     }
 
 EqualityOperator = ("===" / "==") { return "=="; }
+                 / "=" { error("Equality operator should be written as ==, not =.");  return "=="; }
                  / ("!==" / "!=") { return "!="; }
 
 LogicalANDExpression
@@ -548,5 +605,6 @@ MultiLineComment
 SingleLineComment
   = "//" (!NewLine .)*;
 
+AnyBlock = chars:(![;,}] char_:. { return char_; })+ [;,}] _ { return chars.join(''); }
 
 NewLine = [\n\r]

src/test/parser-test.ts

@@ -28,7 +28,6 @@ import bolt = require('../bolt');
 import helper = require('./test-helper');
 
 // TODO: Test duplicated function, and schema definitions.
-// TODO: Test other parser errors - appropriate messages (exceptions).
 
 suite("Rules Parser Tests", function() {
   test("Empty input", function() {
@@ -456,6 +455,24 @@ suite("Rules Parser Tests", function() {
         expect: /./ },
       { data: "path /x { validate() { return this.test(/a/g); } }",
         expect: /unsupported regexp modifier/i },
+      { data: "path {}",
+        expect: /missing path template/i },
+      { data: "path / }",
+        expect: /missing body of path/i },
+      { data: "function foo { 7 }",
+        expect: /missing parameters/i },
+      { data: "foo { 7 }",
+        expect: /expected.*function/i },
+      { data: "foo(x)",
+        expect: /missing.*body/i },
+      { data: "path /x { foo(x); }",
+        expect: /invalid path or method/i },
+      { data: "foo(x) { x = 'a' }",
+        expect: /equality/i },
+      { data: "type X { bad-prop: String; }",
+        expect: /invalid property or method/i },
+      { data: "type { foo: String;}",
+        expect: /missing type name/i },
     ];
 
     helper.dataDrivenTest(tests, function(data, expect) {
@@ -469,12 +486,26 @@ suite("Rules Parser Tests", function() {
     });
   });
 
+  suite("Syntax warnings.", function() {
+    var tests = [
+      { data: "path /x { read() { true }; }",
+        expect: /extra separator/i },
+    ];
+
+    helper.dataDrivenTest(tests, function(data, expect) {
+      parse(data);
+      assert.match(logger.getLastMessage(), expect);
+    });
+  });
+
   suite("Deprecation warnings.", function() {
     var tests = [
       { data: "path /x/$y is String;",
         expect: /path segment is deprecated/ },
       { data: "f(x) = x + 1;",
         expect: /fn\(x\) = exp; format is deprecated/ },
+      { data: "f(x) = x + 1",
+        expect: /fn\(x\) = exp; format is deprecated/ },
     ];
 
     helper.dataDrivenTest(tests, function(data, expect) {