From 9d1067a5ca07bb09fbc3375b8a2fd17b21ed5841 Mon Sep 17 00:00:00 2001 From: Mike Koss Date: Wed, 13 Jan 2016 09:07:40 -0800 Subject: [PATCH 1/4] Better path statement error messages. --- src/rules-parser.pegjs | 31 ++++++++++++++++++++++++++----- src/test/parser-test.ts | 5 ++++- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/src/rules-parser.pegjs b/src/rules-parser.pegjs index 3d21de2..92e4fe7 100644 --- a/src/rules-parser.pegjs +++ b/src/rules-parser.pegjs @@ -90,15 +90,36 @@ Function "function definition" = ("function" __)? name:Identifier params:Paramet symbols.registerFunction(ensureLowerCase(name, "Function names"), 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 {}; } ) _ { +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 +134,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); } diff --git a/src/test/parser-test.ts b/src/test/parser-test.ts index 40ae1ae..ce582e8 100644 --- a/src/test/parser-test.ts +++ b/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,10 @@ 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 }, ]; helper.dataDrivenTest(tests, function(data, expect) { From bb15c4beb5db648900ca72f1c529a11308546e41 Mon Sep 17 00:00:00 2001 From: Mike Koss Date: Wed, 13 Jan 2016 09:42:18 -0800 Subject: [PATCH 2/4] Better function parsing error handling. --- src/rules-parser.pegjs | 24 ++++++++++++++++++++---- src/test/parser-test.ts | 10 ++++++++++ 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/src/rules-parser.pegjs b/src/rules-parser.pegjs index 92e4fe7..10b7956 100644 --- a/src/rules-parser.pegjs +++ b/src/rules-parser.pegjs @@ -86,10 +86,26 @@ 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 function body."); + return; + } + symbols.registerFunction(ensureLowerCase(func.name, "Function names"), func.params, body); } +// 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) { @@ -228,8 +244,8 @@ Method "method" = name:Identifier params:ParameterList _ body:FunctionBody { }; } -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; } diff --git a/src/test/parser-test.ts b/src/test/parser-test.ts index ce582e8..46b17da 100644 --- a/src/test/parser-test.ts +++ b/src/test/parser-test.ts @@ -459,6 +459,14 @@ suite("Rules Parser Tests", function() { 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: /missing.*body/i }, ]; helper.dataDrivenTest(tests, function(data, expect) { @@ -478,6 +486,8 @@ suite("Rules Parser Tests", function() { 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) { From 6e1e7759cf59fa1210f86d62b77ff98634cb46d8 Mon Sep 17 00:00:00 2001 From: Mike Koss Date: Wed, 13 Jan 2016 12:16:56 -0800 Subject: [PATCH 3/4] Add AnyBlock fallback to generate specific errors within type and path statements. --- src/logger.ts | 14 ++++++++++++-- src/rules-parser.pegjs | 37 +++++++++++++++++++++++++++++-------- src/test/parser-test.ts | 22 ++++++++++++++++++++-- 3 files changed, 61 insertions(+), 12 deletions(-) diff --git a/src/logger.ts b/src/logger.ts index c2c02a3..6403223 100644 --- a/src/logger.ts +++ b/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); } diff --git a/src/rules-parser.pegjs b/src/rules-parser.pegjs index 10b7956..aa806bb 100644 --- a/src/rules-parser.pegjs +++ b/src/rules-parser.pegjs @@ -96,7 +96,7 @@ Function "function definition" = func:FunctionStart body:FunctionBody? { return; } if (body === null) { - error("Function " + func.name + " missing function body."); + error("Function " + func.name + " missing or invalid function body."); return; } symbols.registerFunction(ensureLowerCase(func.name, "Function names"), func.params, body); @@ -170,7 +170,7 @@ 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]; @@ -178,6 +178,10 @@ PathsAndMethods = all:(Path / Method)* _ { 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); } @@ -187,19 +191,27 @@ 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: {} @@ -207,6 +219,10 @@ PropertiesAndMethods = all:(Property / Method)* _ { 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); @@ -234,9 +250,12 @@ 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, @@ -245,7 +264,7 @@ Method "method" = name:Identifier params:ParameterList _ body:FunctionBody { } FunctionBody = "{" _ ("return" __)? exp:Expression _ ";"? _ "}" _ { return exp; } - / "=" _ exp:Expression _ ";"? _ { + / "=" _ exp:Expression ";"? _ { warn("Use of fn(x) = exp; format is deprecated; use fn(x) { exp }, instead.") return exp; } @@ -405,6 +424,7 @@ EqualityExpression } EqualityOperator = ("===" / "==") { return "=="; } + / "=" { error("Equality operator should be written as ==, not =."); return "=="; } / ("!==" / "!=") { return "!="; } LogicalANDExpression @@ -585,5 +605,6 @@ MultiLineComment SingleLineComment = "//" (!NewLine .)*; +AnyBlock = chars:(![;,}] char_:. { return char_; })+ [;,}] _ { return chars.join(''); } NewLine = [\n\r] diff --git a/src/test/parser-test.ts b/src/test/parser-test.ts index 46b17da..0e4a399 100644 --- a/src/test/parser-test.ts +++ b/src/test/parser-test.ts @@ -465,8 +465,14 @@ suite("Rules Parser Tests", function() { expect: /expected.*function/i }, { data: "foo(x)", expect: /missing.*body/i }, - { data: "path /x { 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) { @@ -480,6 +486,18 @@ 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;", From 642d6257d349bcc6ef4789a942e9ea3d3fb05472 Mon Sep 17 00:00:00 2001 From: Mike Koss Date: Thu, 14 Jan 2016 09:38:01 -0800 Subject: [PATCH 4/4] Update changelog for parse errors. --- docs/changelog.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/changelog.txt b/docs/changelog.txt index e69de29..9166d53 100644 --- a/docs/changelog.txt +++ b/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.