Merge pull request #215 from sharwell/error-testing

Error testing

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
+	 * Important enough to avoid multiple definitions that we do very early,
-	 *  field to true. Also check for undefined rules in parser/lexer to
+	 * right after AST construction. Also check for undefined rules in
-	 *  avoid exceptions later. Return true if we find an undefined rule.
+	 * 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<String, RuleAST> ruleToAST = new HashMap<String, RuleAST>();
 		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<GrammarRootAST> sortGrammarByTokenVocab(List<String> fileNames) {
@@ -527,7 +530,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 &&

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) {

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

tool/src/org/antlr/v4/tool/ErrorType.java

@@ -69,7 +69,7 @@ public enum ErrorType {
 
 	// Grammar errors
 	SYNTAX_ERROR(50, "syntax error: <arg>", ErrorSeverity.ERROR),
-	RULE_REDEFINITION(51, "rule '<arg>' redefinition (ignoring); previous at line <arg2>", ErrorSeverity.ERROR),
+	RULE_REDEFINITION(51, "rule '<arg>' redefinition; previous at line <arg2>", ErrorSeverity.ERROR),
 	LEXER_RULES_NOT_ALLOWED(52, "lexer rule '<arg>' not allowed in parser", ErrorSeverity.ERROR),
 	PARSER_RULES_NOT_ALLOWED(53, "parser rule '<arg>' not allowed in lexer", ErrorSeverity.ERROR),
     REPEATED_PREQUEL(54, "repeated grammar prequel spec (option, token, or import); please merge", ErrorSeverity.ERROR),

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);
 	}

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) {
+	protected ErrorQueue antlr(String fileName, String grammarFileName, String grammarStr, boolean defaultListener, String... extraOptions) {
-		boolean allIsWell = true;
 		System.out.println("dir "+tmpdir);
 		mkdir(tmpdir);
 		writeFile(tmpdir, fileName, grammarStr);
-		ErrorQueue equeue = new ErrorQueue();
 		final List<String> options = new ArrayList<String>();
 		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;
+			String[] lines = input.split("\n");
-            try {
+			String fileName = getFilenameFromFirstLineOfGrammar(lines[0]);
-                String[] lines = input.split("\n");
+			ErrorQueue equeue = antlr(fileName, fileName, input, false);
-				String fileName = getFilenameFromFirstLineOfGrammar(lines[0]);
+
-				if (input.startsWith("lexer ")) {
+			String actual = equeue.toString(true);
-					g = new LexerGrammar(fileName, input, equeue);
+			actual = actual.replace(tmpdir + File.separator, "");
-				} 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());
 			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>";
+		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 = "<string>";
+		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());
 		}
 	}
 

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<String> infos = new ArrayList<String>();
+	public final Tool tool;
-	public List<ANTLRMessage> errors = new ArrayList<ANTLRMessage>();
+	public final List<String> infos = new ArrayList<String>();
-	public List<ANTLRMessage> warnings = new ArrayList<ANTLRMessage>();
+	public final List<ANTLRMessage> errors = new ArrayList<ANTLRMessage>();
-    public List<ANTLRMessage> all = new ArrayList<ANTLRMessage>();
+	public final List<ANTLRMessage> warnings = new ArrayList<ANTLRMessage>();
+	public final List<ANTLRMessage> all = new ArrayList<ANTLRMessage>();
+
+	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();
 	}
 

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);
+		ErrorQueue equeue = antlr("M.g4", "M.g4", master, false);
-		boolean expecting = true; // should be ok
+		int expecting = 0; // should be ok
-		assertEquals(expecting, ok);
+		assertEquals(expecting, equeue.errors.size());
 	}
 
 	@Test public void testImportedRuleWithAction() throws Exception {

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 + "): <string>: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 + "): <string>: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 + "): <string>: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"+
@@ -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);
+	}
 }