From 204c2817728f3fab3cc9d48e8a06486396fe9772 Mon Sep 17 00:00:00 2001
From: Sam Harwell <sam@tunnelvisionlabs.com>
Date: Fri, 6 Jan 2017 00:11:11 -0600
Subject: [PATCH 1/2] Fix optional element analysis

* Properly handle elements that are optional in some alts but not others
* Properly handle block sets (a group of terminals producing a SetTransition)
* Properly handle OPTIONAL subrule
---
 .../model/ElementFrequenciesVisitor.java      | 21 +++++++++++++++++--
 .../antlr/v4/codegen/model/RuleFunction.java  | 20 +++++++++++++-----
 2 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/tool/src/org/antlr/v4/codegen/model/ElementFrequenciesVisitor.java b/tool/src/org/antlr/v4/codegen/model/ElementFrequenciesVisitor.java
index 1c9e70436..94deddbac 100644
--- a/tool/src/org/antlr/v4/codegen/model/ElementFrequenciesVisitor.java
+++ b/tool/src/org/antlr/v4/codegen/model/ElementFrequenciesVisitor.java
@@ -96,7 +96,7 @@ protected static FrequencySet<String> combineMin(FrequencySet<String> a, Frequen
 		}
 
 		assert a != SENTINEL;
-		FrequencySet<String> result = combineAndClip(a, b, 1);
+		FrequencySet<String> result = combineAndClip(a, b, Integer.MAX_VALUE);
 		for (Map.Entry<String, MutableInt> entry : result.entrySet()) {
 			entry.getValue().v = Math.min(a.count(entry.getKey()), b.count(entry.getKey()));
 		}
@@ -177,6 +177,23 @@ protected void exitElement(GrammarAST tree) {
 		minFrequencies.push(combineAndClip(minFrequencies.pop(), minFrequencies.pop(), 2));
 	}
 
+	@Override
+	protected void enterBlockSet(GrammarAST tree) {
+		frequencies.push(new FrequencySet<String>());
+		minFrequencies.push(new FrequencySet<String>());
+	}
+
+	@Override
+	protected void exitBlockSet(GrammarAST tree) {
+		if (minFrequencies.peek().size() > 1) {
+			// Everything is optional
+			minFrequencies.peek().clear();
+		}
+
+		frequencies.push(combineAndClip(frequencies.pop(), frequencies.pop(), 2));
+		minFrequencies.push(combineAndClip(minFrequencies.pop(), minFrequencies.pop(), 2));
+	}
+
 	@Override
 	protected void exitSubrule(GrammarAST tree) {
 		if (tree.getType() == CLOSURE || tree.getType() == POSITIVE_CLOSURE) {
@@ -185,7 +202,7 @@ protected void exitSubrule(GrammarAST tree) {
 			}
 		}
 
-		if (tree.getType() == CLOSURE) {
+		if (tree.getType() == CLOSURE || tree.getType() == OPTIONAL) {
 			// Everything inside a closure is optional, so the minimum
 			// number of occurrences for all elements is 0.
 			minFrequencies.peek().clear();
diff --git a/tool/src/org/antlr/v4/codegen/model/RuleFunction.java b/tool/src/org/antlr/v4/codegen/model/RuleFunction.java
index ad6f049ad..c211720a9 100644
--- a/tool/src/org/antlr/v4/codegen/model/RuleFunction.java
+++ b/tool/src/org/antlr/v4/codegen/model/RuleFunction.java
@@ -171,8 +171,9 @@ public void fillNamedActions(OutputModelFactory factory, Rule r) {
 	 */
 	public Set<Decl> getDeclsForAllElements(List<AltAST> altASTs) {
 		Set<String> needsList = new HashSet<String>();
-		Set<String> optional = new HashSet<String>();
+		Set<String> nonOptional = new HashSet<String>();
 		List<GrammarAST> allRefs = new ArrayList<GrammarAST>();
+		boolean firstAlt = true;
 		for (AltAST ast : altASTs) {
 			IntervalSet reftypes = new IntervalSet(RULE_REF, TOKEN_REF);
 			List<GrammarAST> refs = ast.getNodesWithType(reftypes);
@@ -182,13 +183,22 @@ public Set<Decl> getDeclsForAllElements(List<AltAST> altASTs) {
 			FrequencySet<String> altFreq = minAndAltFreq.b;
 			for (GrammarAST t : refs) {
 				String refLabelName = t.getText();
-				if (minFreq.count(refLabelName) == 0) {
-					optional.add(refLabelName);
-				}
 				if ( altFreq.count(refLabelName)>1 ) {
 					needsList.add(refLabelName);
 				}
+
+				if (firstAlt && minFreq.count(refLabelName) != 0) {
+					nonOptional.add(refLabelName);
+				}
 			}
+
+			for (String ref : nonOptional.toArray(new String[nonOptional.size()])) {
+				if (minFreq.count(ref) == 0) {
+					nonOptional.remove(ref);
+				}
+			}
+
+			firstAlt = false;
 		}
 		Set<Decl> decls = new LinkedHashSet<Decl>();
 		for (GrammarAST t : allRefs) {
@@ -196,7 +206,7 @@ public Set<Decl> getDeclsForAllElements(List<AltAST> altASTs) {
 			List<Decl> d = getDeclForAltElement(t,
 												refLabelName,
 												needsList.contains(refLabelName),
-												optional.contains(refLabelName));
+												!nonOptional.contains(refLabelName));
 			decls.addAll(d);
 		}
 		return decls;

From 587ea3646c17e36f37f1bd3a1ab32609f4dc3ef2 Mon Sep 17 00:00:00 2001
From: Sam Harwell <sam@tunnelvisionlabs.com>
Date: Fri, 6 Jan 2017 00:21:42 -0600
Subject: [PATCH 2/2] Don't use list labels for elements that appear twice in a
 block set

---
 .../antlr/v4/codegen/model/ElementFrequenciesVisitor.java | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tool/src/org/antlr/v4/codegen/model/ElementFrequenciesVisitor.java b/tool/src/org/antlr/v4/codegen/model/ElementFrequenciesVisitor.java
index 94deddbac..f61ce8abb 100644
--- a/tool/src/org/antlr/v4/codegen/model/ElementFrequenciesVisitor.java
+++ b/tool/src/org/antlr/v4/codegen/model/ElementFrequenciesVisitor.java
@@ -185,6 +185,14 @@ protected void enterBlockSet(GrammarAST tree) {
 
 	@Override
 	protected void exitBlockSet(GrammarAST tree) {
+		for (Map.Entry<String, MutableInt> entry : frequencies.peek().entrySet()) {
+			// This visitor counts a block set as a sequence of elements, not a
+			// sequence of alternatives of elements. Reset the count back to 1
+			// for all items when leaving the set to ensure duplicate entries in
+			// the set are treated as a maximum of one item.
+			entry.getValue().v = 1;
+		}
+
 		if (minFrequencies.peek().size() > 1) {
 			// Everything is optional
 			minFrequencies.peek().clear();