Skip to content

Commit 939113f

Browse files
KvanTTTKvanTTT
committed
New warning ACTION_SHOULD_BE_PLACED_AFTER_PREDICATES, fix antlr#3611 
1 parent 15e38b2 commit 939113f

File tree

3 files changed

+164
-8
lines changed

3 files changed

+164
-8
lines changed

‎tool-testsuite/test/org/antlr/v4/test/tool/TestSymbolIssues.java

+60
Original file line numberDiff line numberDiff line change
@@ -560,4 +560,64 @@ public void testLabelsForTokensWithMixedTypesLRWithoutLabels() {
560560

561561
testErrors(test, false);
562562
}
563+
564+
// ISSUE: https://github.com/antlr/antlr4/issues/3611
565+
@Test public void testActionShouldBePlacedAfterAllPredicates() {
566+
String grammar =
567+
"lexer grammar T;\n" +
568+
"@members {String s = \"\";\n" +
569+
"\n" +
570+
"// Action\n" +
571+
"void a(String x) {\n" +
572+
" s += x;\n" +
573+
"}\n" +
574+
"\n" +
575+
"// Predicate\n" +
576+
"boolean p() {\n" +
577+
" return !s.equals(\"\");\n" +
578+
"}}\n" +
579+
"\n" +
580+
"// No warnings\n" +
581+
"OR: 'or1' {p()}? {a(\"or1\");} | 'or2' {p()}? {a(\"or2\");};\n" +
582+
"NO_PREDICATE: 'no_predicate' {a(\"no_predicate\");};\n" +
583+
"PREDICATE: 'predicate' {p()}? {a(\"predicate\");};\n" +
584+
"ACTIONS: 'actions' {a(\"action1\");} {a(\"action2\");};\n" +
585+
"CLOSURE: ('closure' {a(\"closure\");})+;\n" +
586+
"OPTIONAL: 'optional' ('_' {a(\"optional\");})?;\n" +
587+
"USE_FRAGMENT: 'f1_' FRAGMENT {a(\"fragment\");};\n" +
588+
"RECURSIVE : '/*' {p()}? (RECURSIVE | .)*? {a(\"recursive\");}'*/';" +
589+
"\n" +
590+
"// Warnings\n" +
591+
"OR_W: 'or1' {a(\"or1_w\");} {p()}? | 'or2' {a(\"or2_w\");} {p()}?;\n" +
592+
"PREDICATE_W: 'predicate_w' {a(\"predicate_w\");} {p()}?;\n" +
593+
"CLOSURE_W1: ('closure_w1' {a(\"closure_w1\");} {p()}?)+;\n" +
594+
"CLOSURE_W2: ('closure_w2' {p()}? {a(\"closure_w2\");})+;\n" +
595+
"OPTIONAL_W: 'optional_w' ('_' {a(\"optional_w\");})? {p()}?;\n" +
596+
"USE_FRAGMENT_W: 'f2_' {a(\"fragment_w\");} FRAGMENT;\n" +
597+
"HIERARCHY_W: 'hierarchy_w' {a(\"hierarchy_w\");} ('_1' {p()}? | '_2');\n" +
598+
"RECURSIVE_W : '/*' {a(\"recursive_w\");} (RECURSIVE_W | .)*? {p()}? '*/';" +
599+
"\n" +
600+
"fragment FRAGMENT: 'fragment' {p()}?;\n" +
601+
"\n" +
602+
"WS: [ \\r\\n]+;";
603+
604+
int code = ErrorType.ACTION_SHOULD_BE_PLACED_AFTER_PREDICATES.code;
605+
String expected =
606+
"warning(" + code + "): T.g4:24:12: Action {a(\"or1_w\");} should be placed after all predicates because it's always executed after them\n" +
607+
"warning(" + code + "): T.g4:24:41: Action {a(\"or2_w\");} should be placed after all predicates because it's always executed after them\n" +
608+
"warning(" + code + "): T.g4:25:27: Action {a(\"predicate_w\");} should be placed after all predicates because it's always executed after them\n" +
609+
"warning(" + code + "): T.g4:26:26: Action {a(\"closure_w1\");} should be placed after all predicates because it's always executed after them\n" +
610+
"warning(" + code + "): T.g4:27:33: Action {a(\"closure_w2\");} should be placed after all predicates because it's always executed after them\n" +
611+
"warning(" + code + "): T.g4:28:30: Action {a(\"optional_w\");} should be placed after all predicates because it's always executed after them\n" +
612+
"warning(" + code + "): T.g4:29:22: Action {a(\"fragment_w\");} should be placed after all predicates because it's always executed after them\n" +
613+
"warning(" + code + "): T.g4:30:27: Action {a(\"hierarchy_w\");} should be placed after all predicates because it's always executed after them\n" +
614+
"warning(" + code + "): T.g4:31:19: Action {a(\"recursive_w\");} should be placed after all predicates because it's always executed after them\n";
615+
616+
String[] pair = new String[] {
617+
grammar,
618+
expected
619+
};
620+
621+
testErrors(pair, true);
622+
}
563623
}

‎tool/src/org/antlr/v4/semantics/SymbolChecks.java

+90-8
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,7 @@
2323
import org.antlr.v4.tool.LeftRecursiveRule;
2424
import org.antlr.v4.tool.LexerGrammar;
2525
import org.antlr.v4.tool.Rule;
26-
import org.antlr.v4.tool.ast.AltAST;
27-
import org.antlr.v4.tool.ast.GrammarAST;
28-
import org.antlr.v4.tool.ast.TerminalAST;
26+
import org.antlr.v4.tool.ast.*;
2927

3028
import java.util.ArrayList;
3129
import java.util.Collection;
@@ -70,12 +68,14 @@ public void process() {
7068
// methods affect fields, but no side-effects outside this object
7169
// So, call order sensitive
7270
// First collect all rules for later use in checkForLabelConflict()
73-
if (g.rules != null) {
74-
for (Rule r : g.rules.values()) nameToRuleMap.put(r.name, r);
75-
}
76-
checkReservedNames(g.rules.values());
71+
Collection<Rule> rules = g.rules.values();
72+
for (Rule r : rules) nameToRuleMap.put(r.name, r);
73+
checkReservedNames(rules);
7774
checkActionRedefinitions(collector.namedActions);
78-
checkForLabelConflicts(g.rules.values());
75+
checkForLabelConflicts(rules);
76+
if (g instanceof LexerGrammar) {
77+
checkActionLocation(rules);
78+
}
7979
}
8080

8181
public void checkActionRedefinitions(List<GrammarAST> actions) {
@@ -479,4 +479,86 @@ public void checkForQualifiedRuleIssues(Grammar g, List<GrammarAST> qualifiedRul
479479
}
480480
}
481481
}
482+
483+
private void checkActionLocation(Collection<Rule> rules) {
484+
HashMap<RuleAST, Boolean> calculatedRules = new HashMap<>();
485+
for (Rule rule : rules) {
486+
checkActionLocation(rule.ast, calculatedRules, false);
487+
}
488+
}
489+
490+
private void checkActionLocation(GrammarAST node, HashMap<RuleAST, Boolean> calculatedRules, boolean isPredicateDetected) {
491+
if (node instanceof RuleAST) {
492+
RuleAST ruleAST = (RuleAST) node;
493+
checkActionLocation((GrammarAST) ruleAST.getChild(ruleAST.getChildCount() - 1), calculatedRules, isPredicateDetected);
494+
}
495+
else if (node instanceof BlockAST) {
496+
for (Object child : node.getChildren()) {
497+
checkActionLocation((GrammarAST)child, calculatedRules, isPredicateDetected);
498+
}
499+
}
500+
else if (node instanceof AltAST) {
501+
List<?> children = node.getChildren();
502+
boolean isPredicateDetectedInSequence = isPredicateDetected;
503+
for (int i = children.size() - 1; i >= 0; i--) {
504+
GrammarAST child = (GrammarAST)children.get(i);
505+
checkActionLocation(child, calculatedRules, isPredicateDetectedInSequence);
506+
if (!isPredicateDetectedInSequence && containsSemanticsPredicate(child, calculatedRules)) {
507+
isPredicateDetectedInSequence = true;
508+
}
509+
}
510+
}
511+
else if (node instanceof PlusBlockAST || node instanceof StarBlockAST) {
512+
GrammarAST firstChild = (GrammarAST) node.getChild(0);
513+
// Warn about action in closure if there is predicate inside closure
514+
checkActionLocation(firstChild, calculatedRules,
515+
isPredicateDetected || containsSemanticsPredicate(firstChild, calculatedRules));
516+
}
517+
else if (node instanceof OptionalBlockAST) {
518+
checkActionLocation((GrammarAST) node.getChild(0), calculatedRules, isPredicateDetected);
519+
}
520+
else if (node instanceof ActionAST && !(node instanceof PredAST)) {
521+
if (isPredicateDetected) {
522+
ActionAST actionAST = (ActionAST) node;
523+
errMgr.grammarError(ErrorType.ACTION_SHOULD_BE_PLACED_AFTER_PREDICATES, g.fileName, actionAST.token, actionAST.token.getText());
524+
}
525+
}
526+
}
527+
528+
private boolean containsSemanticsPredicate(GrammarAST node, HashMap<RuleAST, Boolean> calculatedRules) {
529+
if (node instanceof RuleAST) {
530+
RuleAST ruleAST = (RuleAST) node;
531+
Boolean calculatedValue = calculatedRules.get(ruleAST);
532+
if (calculatedValue != null) {
533+
return calculatedValue;
534+
}
535+
536+
calculatedRules.put(ruleAST, false); // Prevent endless recursion
537+
boolean result = containsSemanticsPredicate((GrammarAST) ruleAST.getChild(ruleAST.getChildCount() - 1), calculatedRules);
538+
calculatedRules.put(ruleAST, result);
539+
}
540+
else if (node instanceof PredAST) {
541+
return true;
542+
}
543+
else if (node instanceof TerminalAST) {
544+
if (node.token.getType() == ANTLRLexer.TOKEN_REF) {
545+
Rule refRule = nameToRuleMap.get(node.token.getText());
546+
if (refRule != null) {
547+
return containsSemanticsPredicate(refRule.ast, calculatedRules);
548+
}
549+
}
550+
}
551+
else {
552+
List<?> children = node.getChildren();
553+
if (children != null) {
554+
for (Object child : children) {
555+
if (containsSemanticsPredicate((GrammarAST) child, calculatedRules)) {
556+
return true;
557+
}
558+
}
559+
}
560+
}
561+
562+
return false;
563+
}
482564
}

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

+14
Original file line numberDiff line numberDiff line change
@@ -1137,6 +1137,20 @@ public enum ErrorType {
11371137
ErrorSeverity.WARNING
11381138
),
11391139

1140+
/**
1141+
* <p>Action is always performed after semantics predicate execution</p>
1142+
*
1143+
* <pre>
1144+
* ACTION_BEFORE_PREDICATE: {action();} 'value' {predicate()}? // warning
1145+
* ACTION_AFTER_PREDICATE: {predicate()}? 'value' {action();} // ok
1146+
* <pre>
1147+
*/
1148+
ACTION_SHOULD_BE_PLACED_AFTER_PREDICATES(
1149+
188,
1150+
"Action <arg> should be placed after all predicates because it's always executed after them",
1151+
ErrorSeverity.WARNING
1152+
),
1153+
11401154
/*
11411155
* Backward incompatibility errors
11421156
*/

0 commit comments

Comments
 (0)