Skip to content

Commit

Permalink
Report duplicate rule definitions as errors
Browse files Browse the repository at this point in the history
Based on a pull request by Futago-za Ryuu (@futagoza):

  #329

Resolves #318.
  • Loading branch information
dmajda committed Jun 22, 2016
1 parent 149c829 commit eb5875b
Show file tree
Hide file tree
Showing 5 changed files with 55 additions and 6 deletions.
13 changes: 7 additions & 6 deletions lib/compiler/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,17 @@ var compiler = {
*/
passes: {
check: {
reportMissingRules: require("./passes/report-missing-rules"),
reportLeftRecursion: require("./passes/report-left-recursion"),
reportInfiniteLoops: require("./passes/report-infinite-loops")
reportMissingRules: require("./passes/report-missing-rules"),
reportDuplicateRules: require("./passes/report-duplicate-rules"),
reportLeftRecursion: require("./passes/report-left-recursion"),
reportInfiniteLoops: require("./passes/report-infinite-loops")
},
transform: {
removeProxyRules: require("./passes/remove-proxy-rules")
removeProxyRules: require("./passes/remove-proxy-rules")
},
generate: {
generateBytecode: require("./passes/generate-bytecode"),
generateJS: require("./passes/generate-js")
generateBytecode: require("./passes/generate-bytecode"),
generateJS: require("./passes/generate-js")
}
},

Expand Down
26 changes: 26 additions & 0 deletions lib/compiler/passes/report-duplicate-rules.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
"use strict";

var GrammarError = require("../../grammar-error"),
visitor = require("../visitor");

/* Checks that each rule is defined only once. */
function reportDuplicateRules(ast) {
var rules = {};

var check = visitor.build({
rule: function(node) {
if (rules.hasOwnProperty(node.name)) {
throw new GrammarError(
"Rule \"" + node.name + "\" is already defined.",
node.location
);
}

rules[node.name] = true;
}
});

check(ast);
}

module.exports = reportDuplicateRules;
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
"lib/compiler/passes/report-left-recursion.js",
"lib/compiler/passes/report-infinite-loops.js",
"lib/compiler/passes/report-missing-rules.js",
"lib/compiler/passes/report-duplicate-rules.js",
"lib/grammar-error.js",
"lib/parser.js",
"lib/peg.js",
Expand Down
1 change: 1 addition & 0 deletions spec/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
<script src="unit/parser.spec.js"></script>
<script src="unit/compiler/passes/helpers.js"></script>
<script src="unit/compiler/passes/report-missing-rules.spec.js"></script>
<script src="unit/compiler/passes/report-duplicate-rules.spec.js"></script>
<script src="unit/compiler/passes/report-left-recursion.spec.js"></script>
<script src="unit/compiler/passes/report-infinite-loops.spec.js"></script>
<script src="unit/compiler/passes/remove-proxy-rules.spec.js"></script>
Expand Down
20 changes: 20 additions & 0 deletions spec/unit/compiler/passes/report-duplicate-rules.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/* global peg */

"use strict";

describe("compiler pass |reportDuplicateRules|", function() {
var pass = peg.compiler.passes.check.reportDuplicateRules;

it("reports duplicate rules", function() {
expect(pass).toReportError([
'start = "a"',
'start = "b"'
].join('\n'), {
message: 'Rule "start" is already defined.',
location: {
start: { offset: 12, line: 2, column: 1 },
end: { offset: 23, line: 2, column: 12 }
}
});
});
});

7 comments on commit eb5875b

@Mingun
Copy link
Contributor

@Mingun Mingun commented on eb5875b Jun 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, that message will be more informative if it contains location of the first occurrence of the rule.

@dmajda
Copy link
Contributor Author

@dmajda dmajda commented on eb5875b Jun 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mingun Technically, the problem is not the original definition, but the redefinition, so I think it makes sense to point to it rather than the original definition.

Usability-wise, it would probably be best to point to all the duplicate definitions. But this would require either support for multiple locations per error (so one error can point to all the duplicate definitions) or support for producing multiple errors in peg.generate (so each error can point to one duplicate definition). Both solutions add complexity which I don’t want to introduce to PEG.js — at least not yet.

@Mingun
Copy link
Contributor

@Mingun Mingun commented on eb5875b Jun 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not always the second definition of the rule will be erratic. The error can just contain in the first rule. To specify several places there is no need since all the same all rules, since the second, will generate this error

@dmajda
Copy link
Contributor Author

@dmajda dmajda commented on eb5875b Jun 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your response is hard to parse, but I’ll try to reply anyway.

Not always the second definition of the rule will be erratic. The error can just contain in the first rule.

One can’t know which instance of a duplicate rule is the one the user wants to be pointed at. It’s probably the one he/she will want to change in response to the error.

If we hypothesize that users write grammars mostly sequentially, it makes sense to point to the last duplicate rule because that’s the one the user probably added last.

One can also look at things from the technical side. The grammar is processed from the beginning to the end, like pretty much any other source code. The error is triggered by the second instance of the rule (not the first, third, fourth, etc.), so this is where it should be reported.

I sided to the technical view because I find it natural, because I’m convinced it will make sense to most users, and because I don’t think the sequential hypothesis really holds. It was also easy to implement. I don’t see any arguments whatsoever for pointing to the first instance. Nothing you wrote so far has provided one.

To specify several places there is no need since all the same all rules, since the second, will generate this error

This example at the TypeScript playground shows what I consider an ideal report about a duplicate identifier.

@Mingun
Copy link
Contributor

@Mingun Mingun commented on eb5875b Jun 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't say that the error shall arise on the first rule. I say that the good form is include in error message location of the first rule. Many compilers does this

@futagoza
Copy link
Member

@futagoza futagoza commented on eb5875b Jun 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmajda, I think what @Mingun means is that this:

Rule "start" defined at Line 1, Column 1, is redefined at Line 3, Column 1.

is more informative then this:

Rule "start" is already defined.

This does indeed help track down the duplicate rules more easily, but only if you are using PEG.js through the console or like the online editor.

If you have the parser and compiler hooked through a try/catch (like I do ;P) then most likely the thrown messages are ran through a RegExp to find out what type of error it is and then the location field of the thrown error is used, in which case using the approach in this commit is easier.

@dmajda
Copy link
Contributor Author

@dmajda dmajda commented on eb5875b Jun 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mingun I get it now, sorry for misunderstanding. (Re-reading your original comment, it looks like my fault.)

Embedding a location of the original definition into the error message feels a bit hacky, but I agree it is useful, so I implemented it. I filed #430 to track implementation of a more systematic solution at some point in the future.

Please sign in to comment.