From 385e96784db04a18cb7c8ff7edb6ea85a29cdbaf Mon Sep 17 00:00:00 2001 From: Ivan Kochurkin Date: Sat, 7 Jan 2017 18:16:22 +0300 Subject: [PATCH 1/6] Initial fix for labels in left recursive rules: https://github.com/antlr/antlr4/pull/1570 & https://github.com/antlr/antlr4/issues/1543 --- .../org/antlr/v4/semantics/SymbolChecks.java | 82 ++++++++++++++++--- 1 file changed, 69 insertions(+), 13 deletions(-) diff --git a/tool/src/org/antlr/v4/semantics/SymbolChecks.java b/tool/src/org/antlr/v4/semantics/SymbolChecks.java index 5caba656d..67abe2125 100644 --- a/tool/src/org/antlr/v4/semantics/SymbolChecks.java +++ b/tool/src/org/antlr/v4/semantics/SymbolChecks.java @@ -6,6 +6,7 @@ package org.antlr.v4.semantics; +import org.antlr.runtime.tree.CommonTree; import org.antlr.v4.automata.LexerATNFactory; import org.antlr.v4.parse.ANTLRParser; import org.antlr.v4.runtime.Token; @@ -18,6 +19,7 @@ import org.antlr.v4.tool.Grammar; import org.antlr.v4.tool.LabelElementPair; import org.antlr.v4.tool.LexerGrammar; import org.antlr.v4.tool.Rule; +import org.antlr.v4.tool.ast.AltAST; import org.antlr.v4.tool.ast.GrammarAST; import org.antlr.v4.tool.LabelType; import org.antlr.v4.tool.LeftRecursiveRule; @@ -28,6 +30,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.ArrayList; /** Check for symbol problems; no side-effects. Inefficient to walk rules * and such multiple times, but I like isolating all error checking outside @@ -128,43 +131,96 @@ public class SymbolChecks { public void checkForLabelConflicts(Collection rules) { for (Rule r : rules) { checkForAttributeConflicts(r); + + boolean leftRecursiveRule = false; if (r instanceof LeftRecursiveRule) { // Label conflicts for left recursive rules need to be checked // prior to the left recursion elimination step. - continue; + leftRecursiveRule = true; } - Map labelNameSpace = - new HashMap(); + Map labelNameSpace = new HashMap(); for (int i = 1; i <= r.numberOfAlts; i++) { + boolean altLabels = false; if (r.hasAltSpecificContexts()) { labelNameSpace.clear(); + altLabels = true; } Alternative a = r.alt[i]; for (List pairs : a.labelDefs.values()) { - for (LabelElementPair p : pairs) { - checkForLabelConflict(r, p.label); - String name = p.label.getText(); - LabelElementPair prev = labelNameSpace.get(name); - if (prev == null) labelNameSpace.put(name, p); - else checkForTypeMismatch(prev, p); + if (leftRecursiveRule && altLabels) { + Map> labelPairs = new HashMap>(); + for (LabelElementPair p : pairs) { + String labelName = findAltLabel(p.label); + List list; + if (labelPairs.containsKey(labelName)) { + list = labelPairs.get(labelName); + } else { + list = new ArrayList(); + labelPairs.put(labelName, list); + } + list.add(p); + } + + for (List internalPairs : labelPairs.values()) { + labelNameSpace.clear(); + checkLabelPairs(r, labelNameSpace, internalPairs); + } + } + else { + checkLabelPairs(r, labelNameSpace, pairs); } } } } } - void checkForTypeMismatch(LabelElementPair prevLabelPair, LabelElementPair labelPair) { + void checkLabelPairs(Rule r, Map labelNameSpace, List pairs) { + for (LabelElementPair p : pairs) { + checkForLabelConflict(r, p.label); + String name = p.label.getText(); + LabelElementPair prev = labelNameSpace.get(name); + if (prev == null) { + labelNameSpace.put(name, p); + } else { + checkForTypeMismatch(r, prev, p); + } + } + } + + String findAltLabel(CommonTree label) { + if (label == null) { + return null; + } else if (label instanceof AltAST) { + AltAST altAST = (AltAST) label; + if (altAST.altLabel != null) { + return altAST.altLabel.toString(); + } else if (altAST.leftRecursiveAltInfo != null) { + return altAST.leftRecursiveAltInfo.altLabel.toString(); + } else { + return findAltLabel(label.parent); + } + } else { + return findAltLabel(label.parent); + } + } + + void checkForTypeMismatch(Rule r, LabelElementPair prevLabelPair, LabelElementPair labelPair) { // label already defined; if same type, no problem if (prevLabelPair.type != labelPair.type) { - String typeMismatchExpr = labelPair.type + "!=" + prevLabelPair.type; + org.antlr.runtime.Token token; + if (r instanceof LeftRecursiveRule) { + token = ((GrammarAST) r.ast.getChild(0)).getToken(); + } else { + token = labelPair.label.token; + } errMgr.grammarError( ErrorType.LABEL_TYPE_CONFLICT, g.fileName, - labelPair.label.token, + token, labelPair.label.getText(), - typeMismatchExpr); + labelPair.type + "!=" + prevLabelPair.type); } if (!prevLabelPair.element.getText().equals(labelPair.element.getText()) && (prevLabelPair.type.equals(LabelType.RULE_LABEL) || prevLabelPair.type.equals(LabelType.RULE_LIST_LABEL)) && From a46db4b0b7d129907c8880e96a4e7e193d170b52 Mon Sep 17 00:00:00 2001 From: Ivan Kochurkin Date: Sat, 7 Jan 2017 18:17:40 +0300 Subject: [PATCH 2/6] Updated testLabelsForTokensWithMixedTypes. Removed excess testLabelsForTokensWithMixedTypesLRWithLabels and testLabelsForTokensWithMixedTypesLRWithoutLabels. --- .../antlr/v4/test/tool/TestSymbolIssues.java | 78 +++++++------------ 1 file changed, 27 insertions(+), 51 deletions(-) diff --git a/tool-testsuite/test/org/antlr/v4/test/tool/TestSymbolIssues.java b/tool-testsuite/test/org/antlr/v4/test/tool/TestSymbolIssues.java index 16f98d639..eda776584 100644 --- a/tool-testsuite/test/org/antlr/v4/test/tool/TestSymbolIssues.java +++ b/tool-testsuite/test/org/antlr/v4/test/tool/TestSymbolIssues.java @@ -292,21 +292,36 @@ public class TestSymbolIssues extends BaseJavaToolTest { String[] test = { "grammar L;\n" + "\n" + - "rule1 // Correct (Alternatives)\n" + - " : t1 = a #aLabel\n" + - " | t1 = b #bLabel\n" + + "rule1 // Correct (Alternatives)\n" + + " : t1=a #aLabel\n" + + " | t1=b #bLabel\n" + " ;\n" + "rule2 //Incorrect type casting in generated code (RULE_LABEL)\n" + - " : t2 = a | t2 = b\n" + + " : t2=a | t2=b\n" + " ;\n" + "rule3\n" + - " : t3 += a+ b t3 += c+ //Incorrect type casting in generated code (RULE_LIST_LABEL)\n" + + " : t3+=a+ b t3+=c+ //Incorrect type casting in generated code (RULE_LIST_LABEL)\n" + " ;\n" + "rule4\n" + - " : a t4 = A b t4 = B c // Correct (TOKEN_LABEL)\n" + + " : a t4=A b t4=B c // Correct (TOKEN_LABEL)\n" + " ;\n" + "rule5\n" + - " : a t5 += A b t5 += B c // Correct (TOKEN_LIST_LABEL)\n" + + " : a t5+=A b t5+=B c // Correct (TOKEN_LIST_LABEL)\n" + + " ;\n" + + "rule6 // Correct (https://github.com/antlr/antlr4/issues/1543)\n" + + " : t6=a #t6_1_Label\n" + + " | t6=rule6 b (t61=c)? t62=rule6 #t6_2_Label\n" + + " | t6=A a (t61=B)? t62=A #t6_3_Label\n" + + " ;\n" + + "rule7 // Incorrect (https://github.com/antlr/antlr4/issues/1543)\n" + + " : a\n" + + " | t7=rule7 b (t71=c)? t72=rule7 \n" + + " | t7=A a (t71=B)? t72=A \n" + + " ;\n" + + "rule8 // Correct (https://github.com/antlr/antlr4/issues/1543)\n" + + " : a\n" + + " | t8=rule8 a t8=rule8\n" + + " | t8=rule8 b t8=rule8\n" + " ;\n" + "a: A;\n" + "b: B;\n" + @@ -315,51 +330,12 @@ public class TestSymbolIssues extends BaseJavaToolTest { "B: 'b';\n" + "C: 'c';\n", - "error(" + ErrorType.LABEL_TYPE_CONFLICT.code + "): L.g4:8:15: label t2=b type mismatch with previous definition: t2=a\n" + - "error(" + ErrorType.LABEL_TYPE_CONFLICT.code + "): L.g4:11:17: label t3+=c type mismatch with previous definition: t3+=a\n" - }; + "error(" + ErrorType.LABEL_TYPE_CONFLICT.code + "): L.g4:8:13: label t2=b type mismatch with previous definition: t2=a\n" + + "error(" + ErrorType.LABEL_TYPE_CONFLICT.code + "): L.g4:11:15: label t3+=c type mismatch with previous definition: t3+=a\n" + - testErrors(test, false); - } - - // https://github.com/antlr/antlr4/issues/1543 - @Test public void testLabelsForTokensWithMixedTypesLRWithLabels() { - String[] test = { - "grammar L;\n" + - "\n" + - "expr\n" + - " : left=A '+' right=A #primary\n" + - " | left=expr '-' right=expr #sub\n" + - " ;\n" + - "\n" + - "A: 'a';\n" + - "B: 'b';\n" + - "C: 'c';\n", - - "" - }; - - testErrors(test, false); - } - - // https://github.com/antlr/antlr4/issues/1543 - @Test - @Ignore - public void testLabelsForTokensWithMixedTypesLRWithoutLabels() { - String[] test = { - "grammar L;\n" + - "\n" + - "expr\n" + - " : left=A '+' right=A\n" + - " | left=expr '-' right=expr\n" + - " ;\n" + - "\n" + - "A: 'a';\n" + - "B: 'b';\n" + - "C: 'c';\n", - - "error(" + ErrorType.LABEL_TYPE_CONFLICT.code + "): L.g4:5:7: label left=expr type mismatch with previous definition: left=A\n" + - "error(" + ErrorType.LABEL_TYPE_CONFLICT.code + "): L.g4:5:19: label right=expr type mismatch with previous definition: right=A\n" + "error(" + ErrorType.LABEL_TYPE_CONFLICT.code + "): L.g4:24:0: label t7 type mismatch with previous definition: TOKEN_LABEL!=RULE_LABEL\n" + + "error(" + ErrorType.LABEL_TYPE_CONFLICT.code + "): L.g4:24:0: label t71 type mismatch with previous definition: RULE_LABEL!=TOKEN_LABEL\n" + + "error(" + ErrorType.LABEL_TYPE_CONFLICT.code + "): L.g4:24:0: label t72 type mismatch with previous definition: RULE_LABEL!=TOKEN_LABEL\n" }; testErrors(test, false); From a558334a010a527d452feab0a28dc53743db1444 Mon Sep 17 00:00:00 2001 From: Ivan Kochurkin Date: Sat, 7 Jan 2017 18:26:22 +0300 Subject: [PATCH 3/6] Code cleared, added comments. --- .../org/antlr/v4/semantics/SymbolChecks.java | 31 ++++++------------- 1 file changed, 10 insertions(+), 21 deletions(-) diff --git a/tool/src/org/antlr/v4/semantics/SymbolChecks.java b/tool/src/org/antlr/v4/semantics/SymbolChecks.java index 67abe2125..37a632960 100644 --- a/tool/src/org/antlr/v4/semantics/SymbolChecks.java +++ b/tool/src/org/antlr/v4/semantics/SymbolChecks.java @@ -132,32 +132,20 @@ public class SymbolChecks { for (Rule r : rules) { checkForAttributeConflicts(r); - boolean leftRecursiveRule = false; - if (r instanceof LeftRecursiveRule) { - // Label conflicts for left recursive rules need to be checked - // prior to the left recursion elimination step. - leftRecursiveRule = true; - } - - Map labelNameSpace = new HashMap(); + Map labelNameSpace = new HashMap<>(); for (int i = 1; i <= r.numberOfAlts; i++) { - boolean altLabels = false; - if (r.hasAltSpecificContexts()) { - labelNameSpace.clear(); - altLabels = true; - } - Alternative a = r.alt[i]; for (List pairs : a.labelDefs.values()) { - if (leftRecursiveRule && altLabels) { - Map> labelPairs = new HashMap>(); + if (r.hasAltSpecificContexts()) { + // Collect labelName-labeledRules map for rule with alternative labels. + Map> labelPairs = new HashMap<>(); for (LabelElementPair p : pairs) { - String labelName = findAltLabel(p.label); + String labelName = findAltLabelName(p.label); List list; if (labelPairs.containsKey(labelName)) { list = labelPairs.get(labelName); } else { - list = new ArrayList(); + list = new ArrayList<>(); labelPairs.put(labelName, list); } list.add(p); @@ -189,7 +177,7 @@ public class SymbolChecks { } } - String findAltLabel(CommonTree label) { + String findAltLabelName(CommonTree label) { if (label == null) { return null; } else if (label instanceof AltAST) { @@ -199,10 +187,10 @@ public class SymbolChecks { } else if (altAST.leftRecursiveAltInfo != null) { return altAST.leftRecursiveAltInfo.altLabel.toString(); } else { - return findAltLabel(label.parent); + return findAltLabelName(label.parent); } } else { - return findAltLabel(label.parent); + return findAltLabelName(label.parent); } } @@ -211,6 +199,7 @@ public class SymbolChecks { if (prevLabelPair.type != labelPair.type) { org.antlr.runtime.Token token; if (r instanceof LeftRecursiveRule) { + // TODO: replace rule token for left-recursive rule with correct token. token = ((GrammarAST) r.ast.getChild(0)).getToken(); } else { token = labelPair.label.token; From 4a5717162e64c776498223f29e627a1ad49a11bc Mon Sep 17 00:00:00 2001 From: Ivan Kochurkin Date: Sat, 7 Jan 2017 18:37:17 +0300 Subject: [PATCH 4/6] Fixed indents for else statements. Fixed correct token in checkForTypeMismatch. --- .../org/antlr/v4/semantics/SymbolChecks.java | 40 +++++++++++-------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/tool/src/org/antlr/v4/semantics/SymbolChecks.java b/tool/src/org/antlr/v4/semantics/SymbolChecks.java index 37a632960..dcaae508d 100644 --- a/tool/src/org/antlr/v4/semantics/SymbolChecks.java +++ b/tool/src/org/antlr/v4/semantics/SymbolChecks.java @@ -144,7 +144,8 @@ public class SymbolChecks { List list; if (labelPairs.containsKey(labelName)) { list = labelPairs.get(labelName); - } else { + } + else { list = new ArrayList<>(); labelPairs.put(labelName, list); } @@ -164,46 +165,48 @@ public class SymbolChecks { } } - void checkLabelPairs(Rule r, Map labelNameSpace, List pairs) { + private void checkLabelPairs(Rule r, Map labelNameSpace, List pairs) { for (LabelElementPair p : pairs) { checkForLabelConflict(r, p.label); String name = p.label.getText(); LabelElementPair prev = labelNameSpace.get(name); if (prev == null) { labelNameSpace.put(name, p); - } else { + } + else { checkForTypeMismatch(r, prev, p); } } } - String findAltLabelName(CommonTree label) { + private String findAltLabelName(CommonTree label) { if (label == null) { return null; - } else if (label instanceof AltAST) { + } + else if (label instanceof AltAST) { AltAST altAST = (AltAST) label; if (altAST.altLabel != null) { return altAST.altLabel.toString(); - } else if (altAST.leftRecursiveAltInfo != null) { + } + else if (altAST.leftRecursiveAltInfo != null) { return altAST.leftRecursiveAltInfo.altLabel.toString(); - } else { + } + else { return findAltLabelName(label.parent); } - } else { + } + else { return findAltLabelName(label.parent); } } - void checkForTypeMismatch(Rule r, LabelElementPair prevLabelPair, LabelElementPair labelPair) { + private void checkForTypeMismatch(Rule r, LabelElementPair prevLabelPair, LabelElementPair labelPair) { // label already defined; if same type, no problem if (prevLabelPair.type != labelPair.type) { - org.antlr.runtime.Token token; - if (r instanceof LeftRecursiveRule) { - // TODO: replace rule token for left-recursive rule with correct token. - token = ((GrammarAST) r.ast.getChild(0)).getToken(); - } else { - token = labelPair.label.token; - } + // TODO: replace rule token for left-recursive rule with correct token. + org.antlr.runtime.Token token = r instanceof LeftRecursiveRule + ? ((GrammarAST) r.ast.getChild(0)).getToken() + : labelPair.label.token; errMgr.grammarError( ErrorType.LABEL_TYPE_CONFLICT, g.fileName, @@ -215,12 +218,15 @@ public class SymbolChecks { (prevLabelPair.type.equals(LabelType.RULE_LABEL) || prevLabelPair.type.equals(LabelType.RULE_LIST_LABEL)) && (labelPair.type.equals(LabelType.RULE_LABEL) || labelPair.type.equals(LabelType.RULE_LIST_LABEL))) { + org.antlr.runtime.Token token = r instanceof LeftRecursiveRule + ? ((GrammarAST) r.ast.getChild(0)).getToken() + : labelPair.label.token; String prevLabelOp = prevLabelPair.type.equals(LabelType.RULE_LIST_LABEL) ? "+=" : "="; String labelOp = labelPair.type.equals(LabelType.RULE_LIST_LABEL) ? "+=" : "="; errMgr.grammarError( ErrorType.LABEL_TYPE_CONFLICT, g.fileName, - labelPair.label.token, + token, labelPair.label.getText() + labelOp + labelPair.element.getText(), prevLabelPair.label.getText() + prevLabelOp + prevLabelPair.element.getText()); } From c929469c5f36fb2094ab2f494fbba7f29f24242b Mon Sep 17 00:00:00 2001 From: Ivan Kochurkin Date: Sat, 7 Jan 2017 18:47:08 +0300 Subject: [PATCH 5/6] Restored old testLabelsForTokensWithMixedTypesLRWithLabels & testLabelsForTokensWithMixedTypesLRWithoutLabels tests. --- .../antlr/v4/test/tool/TestSymbolIssues.java | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/tool-testsuite/test/org/antlr/v4/test/tool/TestSymbolIssues.java b/tool-testsuite/test/org/antlr/v4/test/tool/TestSymbolIssues.java index eda776584..8ac0f958f 100644 --- a/tool-testsuite/test/org/antlr/v4/test/tool/TestSymbolIssues.java +++ b/tool-testsuite/test/org/antlr/v4/test/tool/TestSymbolIssues.java @@ -341,6 +341,48 @@ public class TestSymbolIssues extends BaseJavaToolTest { testErrors(test, false); } + // https://github.com/antlr/antlr4/issues/1543 + @Test public void testLabelsForTokensWithMixedTypesLRWithLabels() { + String[] test = { + "grammar L;\n" + + "\n" + + "expr\n" + + " : left=A '+' right=A #primary\n" + + " | left=expr '-' right=expr #sub\n" + + " ;\n" + + "\n" + + "A: 'a';\n" + + "B: 'b';\n" + + "C: 'c';\n", + + "" + }; + + testErrors(test, false); + } + + // https://github.com/antlr/antlr4/issues/1543 + @Test + public void testLabelsForTokensWithMixedTypesLRWithoutLabels() { + String[] test = { + "grammar L;\n" + + "\n" + + "expr\n" + + " : left=A '+' right=A\n" + + " | left=expr '-' right=expr\n" + + " ;\n" + + "\n" + + "A: 'a';\n" + + "B: 'b';\n" + + "C: 'c';\n", + + "error(" + ErrorType.LABEL_TYPE_CONFLICT.code + "): L.g4:3:0: label left type mismatch with previous definition: TOKEN_LABEL!=RULE_LABEL\n" + + "error(" + ErrorType.LABEL_TYPE_CONFLICT.code + "): L.g4:3:0: label right type mismatch with previous definition: RULE_LABEL!=TOKEN_LABEL\n" + }; + + testErrors(test, false); + } + @Test public void testCharsCollision() throws Exception { String[] test = { "lexer grammar L;\n" + From d71c2157cec0245df8a2ec0a3c29e3a7188d4a05 Mon Sep 17 00:00:00 2001 From: Ivan Kochurkin Date: Sun, 8 Jan 2017 15:08:48 +0300 Subject: [PATCH 6/6] Fixed @sharwell note about comment. --- .../org/antlr/v4/semantics/SymbolChecks.java | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/tool/src/org/antlr/v4/semantics/SymbolChecks.java b/tool/src/org/antlr/v4/semantics/SymbolChecks.java index dcaae508d..cf21ddf57 100644 --- a/tool/src/org/antlr/v4/semantics/SymbolChecks.java +++ b/tool/src/org/antlr/v4/semantics/SymbolChecks.java @@ -141,15 +141,16 @@ public class SymbolChecks { Map> labelPairs = new HashMap<>(); for (LabelElementPair p : pairs) { String labelName = findAltLabelName(p.label); - List list; - if (labelPairs.containsKey(labelName)) { - list = labelPairs.get(labelName); + if (labelName != null) { + List list; + if (labelPairs.containsKey(labelName)) { + list = labelPairs.get(labelName); + } else { + list = new ArrayList<>(); + labelPairs.put(labelName, list); + } + list.add(p); } - else { - list = new ArrayList<>(); - labelPairs.put(labelName, list); - } - list.add(p); } for (List internalPairs : labelPairs.values()) { @@ -203,7 +204,11 @@ public class SymbolChecks { private void checkForTypeMismatch(Rule r, LabelElementPair prevLabelPair, LabelElementPair labelPair) { // label already defined; if same type, no problem if (prevLabelPair.type != labelPair.type) { - // TODO: replace rule token for left-recursive rule with correct token. + // Current behavior: take a token of rule declaration in case of left-recursive rule + // Desired behavior: take a token of proper label declaration in case of left-recursive rule + // See https://github.com/antlr/antlr4/pull/1585 + // Such behavior is referring to the fact that the warning is typically reported on the actual label redefinition, + // but for left-recursive rules the warning is reported on the enclosing rule. org.antlr.runtime.Token token = r instanceof LeftRecursiveRule ? ((GrammarAST) r.ast.getChild(0)).getToken() : labelPair.label.token;