From 805430177c024ffb39b99174d79c2e30dcb1e978 Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Thu, 4 Apr 2013 13:06:22 -0500 Subject: [PATCH 1/4] More thorough testing of tool error reporting --- tool/test/org/antlr/v4/test/BaseTest.java | 72 +++++++------------ tool/test/org/antlr/v4/test/ErrorQueue.java | 34 +++++++-- .../antlr/v4/test/TestCompositeGrammars.java | 6 +- .../antlr/v4/test/TestToolSyntaxErrors.java | 6 +- 4 files changed, 59 insertions(+), 59 deletions(-) diff --git a/tool/test/org/antlr/v4/test/BaseTest.java b/tool/test/org/antlr/v4/test/BaseTest.java index f87db29dd..ae8913edd 100644 --- a/tool/test/org/antlr/v4/test/BaseTest.java +++ b/tool/test/org/antlr/v4/test/BaseTest.java @@ -405,15 +405,11 @@ public abstract class BaseTest { */ } - - /** Return true if all is ok, no errors */ - protected boolean antlr(String fileName, String grammarFileName, String grammarStr, boolean defaultListener, String... extraOptions) { - boolean allIsWell = true; + protected ErrorQueue antlr(String fileName, String grammarFileName, String grammarStr, boolean defaultListener, String... extraOptions) { System.out.println("dir "+tmpdir); mkdir(tmpdir); writeFile(tmpdir, fileName, grammarStr); - ErrorQueue equeue = new ErrorQueue(); final List options = new ArrayList(); Collections.addAll(options, extraOptions); options.add("-o"); @@ -421,23 +417,17 @@ public abstract class BaseTest { options.add("-lib"); options.add(tmpdir); options.add(new File(tmpdir,grammarFileName).toString()); - try { - final String[] optionsA = new String[options.size()]; - options.toArray(optionsA); - Tool antlr = newTool(optionsA); - antlr.addListener(equeue); - if (defaultListener) { - antlr.addListener(new DefaultToolListener(antlr)); - } - antlr.processGrammarsOnCommandLine(); - } - catch (Exception e) { - allIsWell = false; - System.err.println("problems building grammar: "+e); - e.printStackTrace(System.err); - } - allIsWell = equeue.errors.isEmpty(); + final String[] optionsA = new String[options.size()]; + options.toArray(optionsA); + Tool antlr = newTool(optionsA); + ErrorQueue equeue = new ErrorQueue(antlr); + antlr.addListener(equeue); + if (defaultListener) { + antlr.addListener(new DefaultToolListener(antlr)); + } + antlr.processGrammarsOnCommandLine(); + if ( !defaultListener && !equeue.errors.isEmpty() ) { System.err.println("antlr reports errors from "+options); for (int i = 0; i < equeue.errors.size(); i++) { @@ -456,7 +446,7 @@ public abstract class BaseTest { } } - return allIsWell; + return equeue; } protected String execLexer(String grammarFileName, @@ -526,9 +516,9 @@ public abstract class BaseTest { boolean defaultListener, String... extraOptions) { - boolean allIsWell = + ErrorQueue equeue = antlr(grammarFileName, grammarFileName, grammarStr, defaultListener, extraOptions); - if (!allIsWell) { + if (!equeue.errors.isEmpty()) { return false; } @@ -546,7 +536,7 @@ public abstract class BaseTest { files.add(grammarFileName.substring(0, grammarFileName.lastIndexOf('.'))+"BaseVisitor.java"); } } - allIsWell = compile(files.toArray(new String[files.size()])); + boolean allIsWell = compile(files.toArray(new String[files.size()])); return allIsWell; } @@ -670,23 +660,13 @@ public abstract class BaseTest { for (int i = 0; i < pairs.length; i+=2) { String input = pairs[i]; String expect = pairs[i+1]; - ErrorQueue equeue = new ErrorQueue(); - Grammar g = null; - try { - String[] lines = input.split("\n"); - String fileName = getFilenameFromFirstLineOfGrammar(lines[0]); - if (input.startsWith("lexer ")) { - g = new LexerGrammar(fileName, input, equeue); - } else { - g = new Grammar(fileName, input, equeue); - } - } - catch (UnsupportedOperationException ex) { - } - catch (org.antlr.runtime.RecognitionException re) { - re.printStackTrace(System.err); - } - String actual = equeue.toString(g != null ? g.tool : new Tool()); + + String[] lines = input.split("\n"); + String fileName = getFilenameFromFirstLineOfGrammar(lines[0]); + ErrorQueue equeue = antlr(fileName, fileName, input, false); + + String actual = equeue.toString(true); + actual = actual.replace(tmpdir + File.separator, ""); System.err.println(actual); String msg = input; msg = msg.replace("\n","\\n"); @@ -698,14 +678,14 @@ public abstract class BaseTest { } public String getFilenameFromFirstLineOfGrammar(String line) { - String fileName = ""; + String fileName = "A" + Tool.GRAMMAR_EXTENSION; int grIndex = line.lastIndexOf("grammar"); int semi = line.lastIndexOf(';'); if ( grIndex>=0 && semi>=0 ) { int space = line.indexOf(' ', grIndex); fileName = line.substring(space+1, semi)+Tool.GRAMMAR_EXTENSION; } - if ( fileName.length()==Tool.GRAMMAR_EXTENSION.length() ) fileName = ""; + if ( fileName.length()==Tool.GRAMMAR_EXTENSION.length() ) fileName = "A" + Tool.GRAMMAR_EXTENSION; return fileName; } @@ -776,7 +756,7 @@ public abstract class BaseTest { st.add(actionName, action); String grammar = st.render(); ErrorQueue equeue = new ErrorQueue(); - Grammar g = new Grammar(grammar); + Grammar g = new Grammar(grammar, equeue); if ( g.ast!=null && !g.ast.hasErrors ) { SemanticPipeline sem = new SemanticPipeline(g); sem.process(); @@ -797,7 +777,7 @@ public abstract class BaseTest { assertEquals(expected, snippet); } if ( equeue.size()>0 ) { - System.err.println(equeue.toString(g.tool)); + System.err.println(equeue.toString()); } } diff --git a/tool/test/org/antlr/v4/test/ErrorQueue.java b/tool/test/org/antlr/v4/test/ErrorQueue.java index 939a43b24..91295bace 100644 --- a/tool/test/org/antlr/v4/test/ErrorQueue.java +++ b/tool/test/org/antlr/v4/test/ErrorQueue.java @@ -40,10 +40,19 @@ import java.util.ArrayList; import java.util.List; public class ErrorQueue implements ANTLRToolListener { - public List infos = new ArrayList(); - public List errors = new ArrayList(); - public List warnings = new ArrayList(); - public List all = new ArrayList(); + public final Tool tool; + public final List infos = new ArrayList(); + public final List errors = new ArrayList(); + public final List warnings = new ArrayList(); + public final List all = new ArrayList(); + + public ErrorQueue() { + this(null); + } + + public ErrorQueue(Tool tool) { + this.tool = tool; + } @Override public void info(String msg) { @@ -64,7 +73,7 @@ public class ErrorQueue implements ANTLRToolListener { public void error(ToolMessage msg) { errors.add(msg); - all.add(msg); + all.add(msg); } public int size() { @@ -72,15 +81,26 @@ public class ErrorQueue implements ANTLRToolListener { } @Override - public String toString() { return Utils.join(all.iterator(), "\n"); } + public String toString() { + return toString(false); + } + + public String toString(boolean rendered) { + if (!rendered) { + return Utils.join(all.iterator(), "\n"); + } + + if (tool == null) { + throw new IllegalStateException(String.format("No %s instance is available.", Tool.class.getName())); + } - public String toString(Tool tool) { StringBuilder buf = new StringBuilder(); for (ANTLRMessage m : all) { ST st = tool.errMgr.getMessageTemplate(m); buf.append(st.render()); buf.append("\n"); } + return buf.toString(); } diff --git a/tool/test/org/antlr/v4/test/TestCompositeGrammars.java b/tool/test/org/antlr/v4/test/TestCompositeGrammars.java index e91440cfc..3c89db9ef 100644 --- a/tool/test/org/antlr/v4/test/TestCompositeGrammars.java +++ b/tool/test/org/antlr/v4/test/TestCompositeGrammars.java @@ -641,9 +641,9 @@ public class TestCompositeGrammars extends BaseTest { "s : a ;\n" + "B : 'b' ;" + // defines B from inherited token space "WS : (' '|'\\n') -> skip ;\n" ; - boolean ok = antlr("M.g4", "M.g4", master, false); - boolean expecting = true; // should be ok - assertEquals(expecting, ok); + ErrorQueue equeue = antlr("M.g4", "M.g4", master, false); + int expecting = 0; // should be ok + assertEquals(expecting, equeue.errors.size()); } @Test public void testImportedRuleWithAction() throws Exception { diff --git a/tool/test/org/antlr/v4/test/TestToolSyntaxErrors.java b/tool/test/org/antlr/v4/test/TestToolSyntaxErrors.java index 905262642..cc900af5d 100644 --- a/tool/test/org/antlr/v4/test/TestToolSyntaxErrors.java +++ b/tool/test/org/antlr/v4/test/TestToolSyntaxErrors.java @@ -42,14 +42,14 @@ public class TestToolSyntaxErrors extends BaseTest { "error(" + ErrorType.NO_RULES.code + "): A.g4::: grammar 'A' has no rules\n", "A;", - "error(" + ErrorType.SYNTAX_ERROR.code + "): :1:0: syntax error: 'A' came as a complete surprise to me\n", + "error(" + ErrorType.SYNTAX_ERROR.code + "): A.g4:1:0: syntax error: 'A' came as a complete surprise to me\n", "grammar ;", - "error(" + ErrorType.SYNTAX_ERROR.code + "): :1:8: syntax error: ';' came as a complete surprise to me while looking for an identifier\n", + "error(" + ErrorType.SYNTAX_ERROR.code + "): A.g4:1:8: syntax error: ';' came as a complete surprise to me while looking for an identifier\n", "grammar A\n" + "a : ID ;\n", - "error(" + ErrorType.SYNTAX_ERROR.code + "): :2:0: syntax error: missing SEMI at 'a'\n", + "error(" + ErrorType.SYNTAX_ERROR.code + "): A.g4:2:0: syntax error: missing SEMI at 'a'\n", "grammar A;\n" + "a : ID ;;\n"+ From 7235d71cc0e6cd21792f059cf95952206b531461 Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Thu, 4 Apr 2013 13:07:43 -0500 Subject: [PATCH 2/4] Fix NPE revealed by updated testing method --- tool/src/org/antlr/v4/Tool.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tool/src/org/antlr/v4/Tool.java b/tool/src/org/antlr/v4/Tool.java index abd714e7e..de01f5eeb 100644 --- a/tool/src/org/antlr/v4/Tool.java +++ b/tool/src/org/antlr/v4/Tool.java @@ -527,7 +527,7 @@ public class Tool { /** Manually get option node from tree; return null if no defined. */ public static GrammarAST findOptionValueAST(GrammarRootAST root, String option) { GrammarAST options = (GrammarAST)root.getFirstChildWithType(ANTLRParser.OPTIONS); - if ( options!=null ) { + if ( options!=null && options.getChildCount() > 0 ) { for (Object o : options.getChildren()) { GrammarAST c = (GrammarAST)o; if ( c.getType() == ANTLRParser.ASSIGN && From 4433a57baa994288b74736ce26463fa66f253b2e Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Thu, 4 Apr 2013 13:09:31 -0500 Subject: [PATCH 3/4] Strict handling of redefined rules (do not attempt to generate code), fixes #210 --- tool/src/org/antlr/v4/Tool.java | 15 +++++++++------ .../src/org/antlr/v4/parse/GrammarTreeVisitor.g | 17 +---------------- tool/src/org/antlr/v4/tool/ErrorType.java | 2 +- tool/src/org/antlr/v4/tool/ast/RuleAST.java | 3 --- .../org/antlr/v4/test/TestToolSyntaxErrors.java | 15 +++++++++++++++ 5 files changed, 26 insertions(+), 26 deletions(-) diff --git a/tool/src/org/antlr/v4/Tool.java b/tool/src/org/antlr/v4/Tool.java index de01f5eeb..247927595 100644 --- a/tool/src/org/antlr/v4/Tool.java +++ b/tool/src/org/antlr/v4/Tool.java @@ -423,10 +423,11 @@ public class Tool { } } - /** Important enough to avoid multiple defs that we do very early, - * right after AST construction. Turn redef'd rule's AST RULE node dead - * field to true. Also check for undefined rules in parser/lexer to - * avoid exceptions later. Return true if we find an undefined rule. + /** + * Important enough to avoid multiple definitions that we do very early, + * right after AST construction. Also check for undefined rules in + * parser/lexer to avoid exceptions later. Return true if we find multiple + * definitions of the same rule or a reference to an undefined rule. */ public boolean checkForRuleIssues(final Grammar g) { // check for redefined rules @@ -436,6 +437,7 @@ public class Tool { rules.addAll(mode.getAllChildrenWithType(ANTLRParser.RULE)); } + boolean redefinition = false; final Map ruleToAST = new HashMap(); for (GrammarAST r : rules) { RuleAST ruleAST = (RuleAST)r; @@ -449,7 +451,7 @@ public class Tool { ID.getToken(), ruleName, prevChild.getToken().getLine()); - ruleAST.dead = true; + redefinition = true; continue; } ruleToAST.put(ruleName, ruleAST); @@ -478,10 +480,11 @@ public class Tool { } } } + UndefChecker chk = new UndefChecker(); chk.visitGrammar(g.ast); - return chk.undefined; // no problem + return redefinition || chk.undefined; } public List sortGrammarByTokenVocab(List fileNames) { diff --git a/tool/src/org/antlr/v4/parse/GrammarTreeVisitor.g b/tool/src/org/antlr/v4/parse/GrammarTreeVisitor.g index a7f7f5172..9d42213bd 100644 --- a/tool/src/org/antlr/v4/parse/GrammarTreeVisitor.g +++ b/tool/src/org/antlr/v4/parse/GrammarTreeVisitor.g @@ -509,8 +509,7 @@ rule @after { exitRule($start); } - : {!((RuleAST)$start).dead}? - ^( RULE RULE_REF {currentRuleName=$RULE_REF.text; currentRuleAST=$RULE;} + : ^( RULE RULE_REF {currentRuleName=$RULE_REF.text; currentRuleAST=$RULE;} DOC_COMMENT? (^(RULEMODIFIERS (m=ruleModifier{mods.add($m.start);})+))? ARG_ACTION? ret=ruleReturns? @@ -527,20 +526,6 @@ rule ruleBlock exceptionGroup {finishRule((RuleAST)$RULE, $RULE_REF, $ruleBlock.start); currentRuleName=null; currentRuleAST=null;} ) - | // ugly repeated alt w/o actions but needed to force ANTLR to use - // sem pred to avoid actions when rule dead - {((RuleAST)$start).dead}? - ^( RULE RULE_REF - DOC_COMMENT? (^(RULEMODIFIERS (m=ruleModifier)+))? - ARG_ACTION? - ret=ruleReturns? - thr=throwsSpec? - loc=locals? - ( opts=optionsSpec - | a=ruleAction - )* - ruleBlock exceptionGroup - ) ; exceptionGroup diff --git a/tool/src/org/antlr/v4/tool/ErrorType.java b/tool/src/org/antlr/v4/tool/ErrorType.java index 6a11d7aa2..605b83c31 100644 --- a/tool/src/org/antlr/v4/tool/ErrorType.java +++ b/tool/src/org/antlr/v4/tool/ErrorType.java @@ -69,7 +69,7 @@ public enum ErrorType { // Grammar errors SYNTAX_ERROR(50, "syntax error: ", ErrorSeverity.ERROR), - RULE_REDEFINITION(51, "rule '' redefinition (ignoring); previous at line ", ErrorSeverity.ERROR), + RULE_REDEFINITION(51, "rule '' redefinition; previous at line ", ErrorSeverity.ERROR), LEXER_RULES_NOT_ALLOWED(52, "lexer rule '' not allowed in parser", ErrorSeverity.ERROR), PARSER_RULES_NOT_ALLOWED(53, "parser rule '' not allowed in lexer", ErrorSeverity.ERROR), REPEATED_PREQUEL(54, "repeated grammar prequel spec (option, token, or import); please merge", ErrorSeverity.ERROR), diff --git a/tool/src/org/antlr/v4/tool/ast/RuleAST.java b/tool/src/org/antlr/v4/tool/ast/RuleAST.java index b394b493f..0db780a2b 100644 --- a/tool/src/org/antlr/v4/tool/ast/RuleAST.java +++ b/tool/src/org/antlr/v4/tool/ast/RuleAST.java @@ -36,9 +36,6 @@ import org.antlr.v4.parse.ANTLRParser; import org.antlr.v4.tool.Grammar; public class RuleAST extends GrammarASTWithOptions { - /** Kill redef of rules */ - public boolean dead; - public RuleAST(RuleAST node) { super(node); } diff --git a/tool/test/org/antlr/v4/test/TestToolSyntaxErrors.java b/tool/test/org/antlr/v4/test/TestToolSyntaxErrors.java index cc900af5d..e322c5a7a 100644 --- a/tool/test/org/antlr/v4/test/TestToolSyntaxErrors.java +++ b/tool/test/org/antlr/v4/test/TestToolSyntaxErrors.java @@ -261,4 +261,19 @@ public class TestToolSyntaxErrors extends BaseTest { }; super.testErrors(pair, true); } + + @Test public void testRuleRedefinition() { + String[] pair = new String[] { + "grammar Oops;\n" + + "\n" + + "ret_ty : A ;\n" + + "ret_ty : B ;\n" + + "\n" + + "A : 'a' ;\n" + + "B : 'b' ;\n", + + "error(" + ErrorType.RULE_REDEFINITION.code + "): Oops.g4:4:0: rule 'ret_ty' redefinition; previous at line 3\n" + }; + super.testErrors(pair, true); + } } From f4ae1cf471cb89abfcd3f417a9b1266bdd31d98c Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Thu, 4 Apr 2013 13:09:54 -0500 Subject: [PATCH 4/4] Updated documentation --- .../antlr/v4/codegen/model/CodeBlockForOuterMostAlt.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tool/src/org/antlr/v4/codegen/model/CodeBlockForOuterMostAlt.java b/tool/src/org/antlr/v4/codegen/model/CodeBlockForOuterMostAlt.java index ee5583847..856d15af3 100644 --- a/tool/src/org/antlr/v4/codegen/model/CodeBlockForOuterMostAlt.java +++ b/tool/src/org/antlr/v4/codegen/model/CodeBlockForOuterMostAlt.java @@ -32,12 +32,18 @@ package org.antlr.v4.codegen.model; import org.antlr.v4.codegen.OutputModelFactory; import org.antlr.v4.tool.Alternative; -/** The code associated with an outermost alternative overrule. +/** The code associated with the outermost alternative of a rule. * Sometimes we might want to treat them differently in the * code generation. */ public class CodeBlockForOuterMostAlt extends CodeBlockForAlt { + /** + * The label for the alternative; or null if the alternative is not labeled. + */ public String altLabel; + /** + * The alternative. + */ public Alternative alt; public CodeBlockForOuterMostAlt(OutputModelFactory factory, Alternative alt) {