From eb53e70e397b24731ad28a0b547d36fbfefe4c6a Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Wed, 4 Jan 2017 09:43:20 -0600 Subject: [PATCH 1/2] Implement support for optional getters This analysis is required for proper code generation in the TypeScript target when strict null checks are enabled. It also applies to targets intending to differentiate optional values from required values. --- .../model/ElementFrequenciesVisitor.java | 63 +++++++++++++++++++ .../antlr/v4/codegen/model/RuleFunction.java | 25 +++++--- .../model/decl/ContextRuleGetterDecl.java | 5 +- .../model/decl/ContextTokenGetterDecl.java | 5 +- 4 files changed, 87 insertions(+), 11 deletions(-) diff --git a/tool/src/org/antlr/v4/codegen/model/ElementFrequenciesVisitor.java b/tool/src/org/antlr/v4/codegen/model/ElementFrequenciesVisitor.java index ab5b86dfe..d70ec5bb9 100644 --- a/tool/src/org/antlr/v4/codegen/model/ElementFrequenciesVisitor.java +++ b/tool/src/org/antlr/v4/codegen/model/ElementFrequenciesVisitor.java @@ -21,12 +21,30 @@ import java.util.Deque; import java.util.Map; public class ElementFrequenciesVisitor extends GrammarTreeVisitor { + /** + * This special value means "no set", and is used by {@link #minFrequencies} + * to ensure that {@link #combineMin} doesn't merge an empty set (all zeros) + * with the results of the first alternative. + */ + private static final FrequencySet SENTINEL = new FrequencySet(); + final Deque> frequencies; + private final Deque> minFrequencies; public ElementFrequenciesVisitor(TreeNodeStream input) { super(input); frequencies = new ArrayDeque>(); frequencies.push(new FrequencySet()); + minFrequencies = new ArrayDeque>(); + minFrequencies.push(SENTINEL); + } + + FrequencySet getMinFrequencies() { + assert minFrequencies.size() == 1; + assert minFrequencies.peek() != SENTINEL; + assert SENTINEL.isEmpty(); + + return minFrequencies.peek(); } /** During code gen, we can assume tree is in good shape */ @@ -61,6 +79,31 @@ public class ElementFrequenciesVisitor extends GrammarTreeVisitor { return result; } + /** + * Generate a frequency set as the union of two input sets. If an + * element is contained in both sets, the value for the output will be + * the minimum of the two input values. + * + * @param a The first set. + * @param b The second set. If this set is {@link #SENTINEL}, it is treated + * as though no second set were provided. + * @return The union of the two sets, with the minimum value chosen + * whenever both sets contain the same key. + */ + protected static FrequencySet combineMin(FrequencySet a, FrequencySet b) { + if (b == SENTINEL) { + return a; + } + + assert a != SENTINEL; + FrequencySet result = combineAndClip(a, b, 1); + for (Map.Entry entry : result.entrySet()) { + entry.getValue().v = Math.min(a.count(entry.getKey()), b.count(entry.getKey())); + } + + return result; + } + /** * Generate a frequency set as the union of two input sets, with the * values clipped to a specified maximum value. If an element is @@ -97,11 +140,13 @@ public class ElementFrequenciesVisitor extends GrammarTreeVisitor { @Override public void tokenRef(TerminalAST ref) { frequencies.peek().add(ref.getText()); + minFrequencies.peek().add(ref.getText()); } @Override public void ruleRef(GrammarAST ref, ActionAST arg) { frequencies.peek().add(ref.getText()); + minFrequencies.peek().add(ref.getText()); } /* @@ -111,21 +156,25 @@ public class ElementFrequenciesVisitor extends GrammarTreeVisitor { @Override protected void enterAlternative(AltAST tree) { frequencies.push(new FrequencySet()); + minFrequencies.push(new FrequencySet()); } @Override protected void exitAlternative(AltAST tree) { frequencies.push(combineMax(frequencies.pop(), frequencies.pop())); + minFrequencies.push(combineMin(minFrequencies.pop(), minFrequencies.pop())); } @Override protected void enterElement(GrammarAST tree) { frequencies.push(new FrequencySet()); + minFrequencies.push(new FrequencySet()); } @Override protected void exitElement(GrammarAST tree) { frequencies.push(combineAndClip(frequencies.pop(), frequencies.pop(), 2)); + minFrequencies.push(combineAndClip(minFrequencies.pop(), minFrequencies.pop(), 2)); } @Override @@ -134,6 +183,11 @@ public class ElementFrequenciesVisitor extends GrammarTreeVisitor { for (Map.Entry entry : frequencies.peek().entrySet()) { entry.getValue().v = 2; } + + int multiplier = tree.getType() == POSITIVE_CLOSURE ? 1 : 0; + for (Map.Entry entry : minFrequencies.peek().entrySet()) { + entry.getValue().v *= multiplier; + } } } @@ -144,21 +198,25 @@ public class ElementFrequenciesVisitor extends GrammarTreeVisitor { @Override protected void enterLexerAlternative(GrammarAST tree) { frequencies.push(new FrequencySet()); + minFrequencies.push(new FrequencySet()); } @Override protected void exitLexerAlternative(GrammarAST tree) { frequencies.push(combineMax(frequencies.pop(), frequencies.pop())); + minFrequencies.push(combineMin(minFrequencies.pop(), minFrequencies.pop())); } @Override protected void enterLexerElement(GrammarAST tree) { frequencies.push(new FrequencySet()); + minFrequencies.push(new FrequencySet()); } @Override protected void exitLexerElement(GrammarAST tree) { frequencies.push(combineAndClip(frequencies.pop(), frequencies.pop(), 2)); + minFrequencies.push(combineAndClip(minFrequencies.pop(), minFrequencies.pop(), 2)); } @Override @@ -167,6 +225,11 @@ public class ElementFrequenciesVisitor extends GrammarTreeVisitor { for (Map.Entry entry : frequencies.peek().entrySet()) { entry.getValue().v = 2; } + + int multiplier = tree.getType() == POSITIVE_CLOSURE ? 1 : 0; + for (Map.Entry entry : minFrequencies.peek().entrySet()) { + entry.getValue().v *= multiplier; + } } } } diff --git a/tool/src/org/antlr/v4/codegen/model/RuleFunction.java b/tool/src/org/antlr/v4/codegen/model/RuleFunction.java index d6e165e2c..ad6f049ad 100644 --- a/tool/src/org/antlr/v4/codegen/model/RuleFunction.java +++ b/tool/src/org/antlr/v4/codegen/model/RuleFunction.java @@ -171,14 +171,20 @@ public class RuleFunction extends OutputModelObject { */ public Set getDeclsForAllElements(List altASTs) { Set needsList = new HashSet(); + Set optional = new HashSet(); List allRefs = new ArrayList(); for (AltAST ast : altASTs) { IntervalSet reftypes = new IntervalSet(RULE_REF, TOKEN_REF); List refs = ast.getNodesWithType(reftypes); allRefs.addAll(refs); - FrequencySet altFreq = getElementFrequenciesForAlt(ast); + Pair, FrequencySet> minAndAltFreq = getElementFrequenciesForAlt(ast); + FrequencySet minFreq = minAndAltFreq.a; + FrequencySet 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); } @@ -189,31 +195,32 @@ public class RuleFunction extends OutputModelObject { String refLabelName = t.getText(); List d = getDeclForAltElement(t, refLabelName, - needsList.contains(refLabelName)); + needsList.contains(refLabelName), + optional.contains(refLabelName)); decls.addAll(d); } return decls; } /** Given list of X and r refs in alt, compute how many of each there are */ - protected FrequencySet getElementFrequenciesForAlt(AltAST ast) { + protected Pair, FrequencySet> getElementFrequenciesForAlt(AltAST ast) { try { ElementFrequenciesVisitor visitor = new ElementFrequenciesVisitor(new CommonTreeNodeStream(new GrammarASTAdaptor(), ast)); visitor.outerAlternative(); if (visitor.frequencies.size() != 1) { factory.getGrammar().tool.errMgr.toolError(ErrorType.INTERNAL_ERROR); - return new FrequencySet(); + return new Pair<>(new FrequencySet(), new FrequencySet()); } - return visitor.frequencies.peek(); + return new Pair<>(visitor.getMinFrequencies(), visitor.frequencies.peek()); } catch (RecognitionException ex) { factory.getGrammar().tool.errMgr.toolError(ErrorType.INTERNAL_ERROR, ex); - return new FrequencySet(); + return new Pair<>(new FrequencySet(), new FrequencySet()); } } - public List getDeclForAltElement(GrammarAST t, String refLabelName, boolean needList) { + public List getDeclForAltElement(GrammarAST t, String refLabelName, boolean needList, boolean optional) { List decls = new ArrayList(); if ( t.getType()==RULE_REF ) { Rule rref = factory.getGrammar().getRule(t.getText()); @@ -225,7 +232,7 @@ public class RuleFunction extends OutputModelObject { decls.add( new ContextRuleListIndexedGetterDecl(factory, refLabelName, ctxName) ); } else { - decls.add( new ContextRuleGetterDecl(factory, refLabelName, ctxName) ); + decls.add( new ContextRuleGetterDecl(factory, refLabelName, ctxName, optional) ); } } else { @@ -235,7 +242,7 @@ public class RuleFunction extends OutputModelObject { decls.add( new ContextTokenListIndexedGetterDecl(factory, refLabelName) ); } else { - decls.add( new ContextTokenGetterDecl(factory, refLabelName) ); + decls.add( new ContextTokenGetterDecl(factory, refLabelName, optional) ); } } return decls; diff --git a/tool/src/org/antlr/v4/codegen/model/decl/ContextRuleGetterDecl.java b/tool/src/org/antlr/v4/codegen/model/decl/ContextRuleGetterDecl.java index b76396533..c2fdbe5a7 100644 --- a/tool/src/org/antlr/v4/codegen/model/decl/ContextRuleGetterDecl.java +++ b/tool/src/org/antlr/v4/codegen/model/decl/ContextRuleGetterDecl.java @@ -11,8 +11,11 @@ import org.antlr.v4.codegen.OutputModelFactory; /** {@code public XContext X() { }} */ public class ContextRuleGetterDecl extends ContextGetterDecl { public String ctxName; - public ContextRuleGetterDecl(OutputModelFactory factory, String name, String ctxName) { + public boolean optional; + + public ContextRuleGetterDecl(OutputModelFactory factory, String name, String ctxName, boolean optional) { super(factory, name); this.ctxName = ctxName; + this.optional = optional; } } diff --git a/tool/src/org/antlr/v4/codegen/model/decl/ContextTokenGetterDecl.java b/tool/src/org/antlr/v4/codegen/model/decl/ContextTokenGetterDecl.java index d66318141..5cd4fd5ad 100644 --- a/tool/src/org/antlr/v4/codegen/model/decl/ContextTokenGetterDecl.java +++ b/tool/src/org/antlr/v4/codegen/model/decl/ContextTokenGetterDecl.java @@ -10,7 +10,10 @@ import org.antlr.v4.codegen.OutputModelFactory; /** {@code public Token X() { }} */ public class ContextTokenGetterDecl extends ContextGetterDecl { - public ContextTokenGetterDecl(OutputModelFactory factory, String name) { + public boolean optional; + + public ContextTokenGetterDecl(OutputModelFactory factory, String name, boolean optional) { super(factory, name); + this.optional = optional; } } From 9009a1b98901553aafcfb70e3b8d93c0bcbc1f30 Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Wed, 4 Jan 2017 11:46:47 -0600 Subject: [PATCH 2/2] Simplify logic leaving a subrule in ElementFrequenciesVisitor --- .../model/ElementFrequenciesVisitor.java | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/tool/src/org/antlr/v4/codegen/model/ElementFrequenciesVisitor.java b/tool/src/org/antlr/v4/codegen/model/ElementFrequenciesVisitor.java index d70ec5bb9..1c9e70436 100644 --- a/tool/src/org/antlr/v4/codegen/model/ElementFrequenciesVisitor.java +++ b/tool/src/org/antlr/v4/codegen/model/ElementFrequenciesVisitor.java @@ -183,11 +183,12 @@ public class ElementFrequenciesVisitor extends GrammarTreeVisitor { for (Map.Entry entry : frequencies.peek().entrySet()) { entry.getValue().v = 2; } + } - int multiplier = tree.getType() == POSITIVE_CLOSURE ? 1 : 0; - for (Map.Entry entry : minFrequencies.peek().entrySet()) { - entry.getValue().v *= multiplier; - } + if (tree.getType() == CLOSURE) { + // Everything inside a closure is optional, so the minimum + // number of occurrences for all elements is 0. + minFrequencies.peek().clear(); } } @@ -225,11 +226,12 @@ public class ElementFrequenciesVisitor extends GrammarTreeVisitor { for (Map.Entry entry : frequencies.peek().entrySet()) { entry.getValue().v = 2; } + } - int multiplier = tree.getType() == POSITIVE_CLOSURE ? 1 : 0; - for (Map.Entry entry : minFrequencies.peek().entrySet()) { - entry.getValue().v *= multiplier; - } + if (tree.getType() == CLOSURE) { + // Everything inside a closure is optional, so the minimum + // number of occurrences for all elements is 0. + minFrequencies.peek().clear(); } } }