This is an automated email from the ASF dual-hosted git repository. mariofusco pushed a commit to branch dev-new-parser in repository https://gitbox.apache.org/repos/asf/incubator-kie-drools.git
commit 4422e58ae5b12dc2e1be1c9a4cb8904ec5472be1 Author: Toshiya Kobayashi <[email protected]> AuthorDate: Fri Oct 27 15:53:09 2023 +0900 [DROOLS-7310] parse nested parentheses (#42) * [DROOLS-7310] parse nested parentheses * - Adding code commentes and explanations - Adding more nested level tests --- .../src/main/antlr4/org/drools/parser/DRLParser.g4 | 20 ++- .../java/org/drools/parser/DRLVisitorImpl.java | 148 ++++++++++++++++++--- .../java/org/drools/parser/MiscDRLParserTest.java | 85 ++++++++++-- 3 files changed, 225 insertions(+), 28 deletions(-) diff --git a/drools-drl/drools-drl10-parser/src/main/antlr4/org/drools/parser/DRLParser.g4 b/drools-drl/drools-drl10-parser/src/main/antlr4/org/drools/parser/DRLParser.g4 index 80774cd08e..933c013ee6 100644 --- a/drools-drl/drools-drl10-parser/src/main/antlr4/org/drools/parser/DRLParser.g4 +++ b/drools-drl/drools-drl10-parser/src/main/antlr4/org/drools/parser/DRLParser.g4 @@ -91,8 +91,9 @@ lhsExpression : LPAREN lhsExpression RPAREN #lhsExpressionEnclosed | lhsExpression (DRL_OR lhsExpression)+ #lhsOr ; -// lhsAnd is used as a label in lhsExpression rule. But some other rules also use it. -lhsAndDef : lhsUnary (DRL_AND lhsUnary)* +// lhsAnd is used as a label in lhsExpression rule. But some other rules explicitly use the def, so lhsAndDef is declared. +lhsAndDef : LPAREN lhsAndDef RPAREN + | lhsUnary (DRL_AND lhsUnary)* | LPAREN DRL_AND lhsUnary+ RPAREN ; @@ -380,7 +381,13 @@ fromEntryPoint : DRL_ENTRY_POINT stringId ; | lhsPatternBind ) */ -lhsExists : DRL_EXISTS lhsPatternBind ; +// Use lhsExpression instead of lhsOr because lhsExpression has good enough structure +lhsExists : DRL_EXISTS + ( LPAREN lhsExpression RPAREN + | lhsPatternBind + ) + ; + /* lhsNot := NOT ( (LEFT_PAREN (or_key|and_key))=> lhsOr // prevents '((' for prefixed and/or @@ -388,7 +395,12 @@ lhsExists : DRL_EXISTS lhsPatternBind ; | lhsPatternBind ) */ -lhsNot : DRL_NOT lhsPatternBind ; +// Use lhsExpression instead of lhsOr because lhsExpression has good enough structure +lhsNot : DRL_NOT + ( LPAREN lhsExpression RPAREN + | lhsPatternBind + ) + ; /** * lhsEval := EVAL LEFT_PAREN conditionalExpression RIGHT_PAREN diff --git a/drools-drl/drools-drl10-parser/src/main/java/org/drools/parser/DRLVisitorImpl.java b/drools-drl/drools-drl10-parser/src/main/java/org/drools/parser/DRLVisitorImpl.java index 3ca001611d..8212ac32be 100644 --- a/drools-drl/drools-drl10-parser/src/main/java/org/drools/parser/DRLVisitorImpl.java +++ b/drools-drl/drools-drl10-parser/src/main/java/org/drools/parser/DRLVisitorImpl.java @@ -40,7 +40,6 @@ import org.drools.drl.ast.descr.TypeDeclarationDescr; import org.drools.drl.ast.descr.TypeFieldDescr; import org.drools.drl.ast.descr.UnitDescr; import org.drools.drl.ast.descr.WindowDeclarationDescr; -import org.drools.util.StringUtils; import static org.drools.parser.DRLParserHelper.getTextWithoutErrorNode; import static org.drools.parser.ParserStringUtils.getTextPreservingWhitespace; @@ -577,16 +576,38 @@ public class DRLVisitorImpl extends DRLParserBaseVisitor<Object> { @Override public ExistsDescr visitLhsExists(DRLParser.LhsExistsContext ctx) { ExistsDescr existsDescr = new ExistsDescr(); - BaseDescr descr = visitLhsPatternBind(ctx.lhsPatternBind()); - existsDescr.addDescr(descr); + if (ctx.lhsExpression() != null) { + // exists( A() or B() ) + List<BaseDescr> baseDescrs = visitDescrChildren(ctx); + if (baseDescrs.size() == 1) { + existsDescr.addDescr(baseDescrs.get(0)); + } else { + throw new IllegalStateException("'exists()' children descr size must be 1 : " + ctx.getText()); + } + } else { + // exists A() + BaseDescr descr = visitLhsPatternBind(ctx.lhsPatternBind()); + existsDescr.addDescr(descr); + } return existsDescr; } @Override public NotDescr visitLhsNot(DRLParser.LhsNotContext ctx) { NotDescr notDescr = new NotDescr(); - BaseDescr descr = visitLhsPatternBind(ctx.lhsPatternBind()); - notDescr.addDescr(descr); + if (ctx.lhsExpression() != null) { + // not ( A() or B() ) + List<BaseDescr> baseDescrs = visitDescrChildren(ctx); + if (baseDescrs.size() == 1) { + notDescr.addDescr(baseDescrs.get(0)); + } else { + throw new IllegalStateException("'not()' children descr size must be 1 : " + ctx.getText()); + } + } else { + // not A() + BaseDescr descr = visitLhsPatternBind(ctx.lhsPatternBind()); + notDescr.addDescr(descr); + } return notDescr; } @@ -602,26 +623,87 @@ public class DRLVisitorImpl extends DRLParserBaseVisitor<Object> { @Override public BaseDescr visitLhsOr(DRLParser.LhsOrContext ctx) { - OrDescr orDescr = new OrDescr(); - List<BaseDescr> descrList = visitDescrChildren(ctx); - descrList.forEach(orDescr::addDescr); - return orDescr; + // For flatten nested OrDescr logic, we call visitDescrChildrenForDescrNodePair instead of usual visitDescrChildren + List<DescrNodePair> descrList = visitDescrChildrenForDescrNodePair(ctx); + if (descrList.size() == 1) { + // Avoid nested OrDescr + return descrList.get(0).getDescr(); + } else { + OrDescr orDescr = new OrDescr(); + // For example, in case of A() or B() or C(), + // Parser creates AST like this: + // lhsOr + // / \ + // A() lhsOr + // / \ + // B() C() + // So, we need to flatten it so that OrDescr has A(), B() and C() as children. + List<BaseDescr> flattenedDescrs = flattenOrDescr(descrList); + flattenedDescrs.forEach(orDescr::addDescr); + return orDescr; + } + } + + private List<BaseDescr> flattenOrDescr(List<DescrNodePair> descrList) { + List<BaseDescr> flattenedDescrs = new ArrayList<>(); + for (DescrNodePair descrNodePair : descrList) { + BaseDescr descr = descrNodePair.getDescr(); + ParseTree node = descrNodePair.getNode(); // parser node corresponding to the descr + if (descr instanceof OrDescr && !(node instanceof DRLParser.LhsExpressionEnclosedContext)) { + // sibling OrDescr should be flattened unless it's explicitly enclosed by parenthesis + flattenedDescrs.addAll(((OrDescr) descr).getDescrs()); + } else { + flattenedDescrs.add(descr); + } + } + return flattenedDescrs; } @Override public BaseDescr visitLhsAnd(DRLParser.LhsAndContext ctx) { - return createAndDescr(visitDescrChildren(ctx)); + return createAndDescr(ctx); + } + + private BaseDescr createAndDescr(ParserRuleContext ctx) { + // For flatten nested AndDescr logic, we call visitDescrChildrenForDescrNodePair instead of usual visitDescrChildren + List<DescrNodePair> descrList = visitDescrChildrenForDescrNodePair(ctx); + if (descrList.size() == 1) { + // Avoid nested AndDescr + return descrList.get(0).getDescr(); + } else { + AndDescr andDescr = new AndDescr(); + // For example, in case of A() and B() and C(), + // Parser creates AST like this: + // lhsAnd + // / \ + // A() lhsAnd + // / \ + // B() C() + // So, we need to flatten it so that AndDescr has A(), B() and C() as children. + List<BaseDescr> flattenedDescrs = flattenAndDescr(descrList); + flattenedDescrs.forEach(andDescr::addDescr); + return andDescr; + } } - private AndDescr createAndDescr(List<BaseDescr> descrList) { - AndDescr andDescr = new AndDescr(); - descrList.forEach(andDescr::addDescr); - return andDescr; + private List<BaseDescr> flattenAndDescr(List<DescrNodePair> descrList) { + List<BaseDescr> flattenedDescrs = new ArrayList<>(); + for (DescrNodePair descrNodePair : descrList) { + BaseDescr descr = descrNodePair.getDescr(); + ParseTree node = descrNodePair.getNode(); // parser node corresponding to the descr + if (descr instanceof AndDescr && !(node instanceof DRLParser.LhsExpressionEnclosedContext)) { + // sibling AndDescr should be flattened unless it's explicitly enclosed by parenthesis + flattenedDescrs.addAll(((AndDescr) descr).getDescrs()); + } else { + flattenedDescrs.add(descr); + } + } + return flattenedDescrs; } @Override public BaseDescr visitLhsAndDef(DRLParser.LhsAndDefContext ctx) { - return createAndDescr(visitDescrChildren(ctx)); + return createAndDescr(ctx); } @Override @@ -650,6 +732,42 @@ public class DRLVisitorImpl extends DRLParserBaseVisitor<Object> { return aggregator; } + // This method is used when the parent descr requires children parser node information for composing the descr. + // Ideally, we should use visitDescrChildren as possible and use the returned Descr object to compose the parent descr. + // However, for example, in flatten OrDescr/AndDescr logic, + // enhancing the returned Descr object of visitLhsExpressionEnclosed() (e.g. adding 'enclosed' flag to OrDescr/AndDescr) could be intrusive just for composing the parent descr. + private List<DescrNodePair> visitDescrChildrenForDescrNodePair(RuleNode node) { + List<DescrNodePair> aggregator = new ArrayList<>(); + int n = node.getChildCount(); + + for (int i = 0; i < n && this.shouldVisitNextChild(node, aggregator); ++i) { + ParseTree c = node.getChild(i); + Object childResult = c.accept(this); + if (childResult instanceof BaseDescr) { + aggregator.add(new DescrNodePair((BaseDescr) childResult, c)); // pairing the returned Descr and the parser node + } + } + return aggregator; + } + + private static class DescrNodePair { + private final BaseDescr descr; // returned Descr object + private final ParseTree node; // parser node corresponding to the descr. This is used for composing the parent descr. + + private DescrNodePair(BaseDescr descr, ParseTree node) { + this.descr = descr; + this.node = node; + } + + public BaseDescr getDescr() { + return descr; + } + + public ParseTree getNode() { + return node; + } + } + // leaves of constraint concatenate return Strings private String visitConstraintChildren(ParserRuleContext ctx) { return getTokenTextPreservingWhitespace(ctx, tokenStream); diff --git a/drools-drl/drools-drl10-parser/src/test/java/org/drools/parser/MiscDRLParserTest.java b/drools-drl/drools-drl10-parser/src/test/java/org/drools/parser/MiscDRLParserTest.java index 50e7f82f7a..229524867d 100644 --- a/drools-drl/drools-drl10-parser/src/test/java/org/drools/parser/MiscDRLParserTest.java +++ b/drools-drl/drools-drl10-parser/src/test/java/org/drools/parser/MiscDRLParserTest.java @@ -1477,7 +1477,7 @@ class MiscDRLParserTest { } @Test - void parenthesesAndOrOrOrAnd() throws Exception { + void multipleLevelNestAndOrOrOrAnd() throws Exception { final String drl = "rule and_or_or_or_and\n" + " when\n" + " (Foo(x == 1) and (Bar(x == 2) or Foo(x == 3))) or (Bar(x == 4) or (Foo(x == 5) and Bar(x == 6)))\n" + @@ -1516,6 +1516,79 @@ class MiscDRLParserTest { assertThat(bar3.getObjectType()).isEqualTo("Bar"); } + @Test + void multipleLevelNestWithThreeOrSiblings() throws Exception { + final String drl = "rule nest_or_siblings\n" + + " when\n" + + " (A() or (B() or C() or (D() and E())))\n" + + " then\n" + + "end"; + PackageDescr pkg = parser.parse(drl); + + assertThat(pkg.getRules()).hasSize(1); + final RuleDescr rule = (RuleDescr) pkg.getRules().get(0); + final AndDescr rootAnd = (AndDescr) rule.getLhs(); + assertThat(rootAnd.getDescrs()).hasSize(1); + + final OrDescr topOr = (OrDescr) rootAnd.getDescrs().get(0); + assertThat(topOr.getDescrs()).hasSize(2); + + final PatternDescr leftPattern = (PatternDescr) topOr.getDescrs().get(0); + assertThat(leftPattern.getObjectType()).isEqualTo("A"); + + final OrDescr rightOr = (OrDescr) topOr.getDescrs().get(1); + assertThat(rightOr.getDescrs()).as("top level Or has 3 sibling children").hasSize(3); + final PatternDescr bPattern = (PatternDescr) rightOr.getDescrs().get(0); + assertThat(bPattern.getObjectType()).isEqualTo("B"); + final PatternDescr cPattern = (PatternDescr) rightOr.getDescrs().get(1); + assertThat(cPattern.getObjectType()).isEqualTo("C"); + final AndDescr deAnd = (AndDescr) rightOr.getDescrs().get(2); + assertThat(deAnd.getDescrs()).hasSize(2); + + final PatternDescr dPattern = (PatternDescr) deAnd.getDescrs().get(0); + assertThat(dPattern.getObjectType()).isEqualTo("D"); + final PatternDescr ePattern = (PatternDescr) deAnd.getDescrs().get(1); + assertThat(ePattern.getObjectType()).isEqualTo("E"); + } + + @Test + public void existsMultipleLevelNestWithThreeOrSiblings() throws Exception { + final String drl = "rule nest_or_siblings\n" + + " when\n" + + " exists(A() or (B() or C() or (D() and E())))\n" + + " then\n" + + "end"; + PackageDescr pkg = parser.parse(drl); + + assertThat(pkg.getRules()).hasSize(1); + final RuleDescr rule = (RuleDescr) pkg.getRules().get(0); + final AndDescr rootAnd = (AndDescr) rule.getLhs(); + assertThat(rootAnd.getDescrs()).hasSize(1); + + final ExistsDescr topExists = (ExistsDescr) rootAnd.getDescrs().get(0); + assertThat(topExists.getDescrs()).hasSize(1); + + final OrDescr topOr = (OrDescr) topExists.getDescrs().get(0); + assertThat(topOr.getDescrs()).hasSize(2); + + final PatternDescr leftPattern = (PatternDescr) topOr.getDescrs().get(0); + assertThat(leftPattern.getObjectType()).isEqualTo("A"); + + final OrDescr rightOr = (OrDescr) topOr.getDescrs().get(1); + assertThat(rightOr.getDescrs()).hasSize(3); + final PatternDescr bPattern = (PatternDescr) rightOr.getDescrs().get(0); + assertThat(bPattern.getObjectType()).isEqualTo("B"); + final PatternDescr cPattern = (PatternDescr) rightOr.getDescrs().get(1); + assertThat(cPattern.getObjectType()).isEqualTo("C"); + final AndDescr deAnd = (AndDescr) rightOr.getDescrs().get(2); + assertThat(deAnd.getDescrs()).hasSize(2); + + final PatternDescr dPattern = (PatternDescr) deAnd.getDescrs().get(0); + assertThat(dPattern.getObjectType()).isEqualTo("D"); + final PatternDescr ePattern = (PatternDescr) deAnd.getDescrs().get(1); + assertThat(ePattern.getObjectType()).isEqualTo("E"); + } + @Test public void parse_EvalMultiple() throws Exception { final PackageDescr pkg = parseAndGetPackageDescrFromFile( @@ -1686,7 +1759,6 @@ class MiscDRLParserTest { assertThat(at.getValue()).isEqualTo("true"); } - @Disabled("Priority : High | Parse attribute with parentheses") @Test public void parse_Attributes2() throws Exception { final PackageDescr pkg = parseAndGetPackageDescrFromFile( @@ -2084,7 +2156,6 @@ class MiscDRLParserTest { assertThat((String) rule.getConsequence()).isEqualToIgnoringWhitespace( expected); } - @Disabled("Priority : High | parse nested parentheses") @Test public void parse_NestedCEs() throws Exception { final RuleDescr rule = parseAndGetFirstRuleDescrFromFile( @@ -2835,11 +2906,9 @@ class MiscDRLParserTest { } - @Disabled("Priority : High | Failed to parse or with parentheses in LHS") @Test - public void parse_RuleWithLHSNesting() throws Exception { - final PackageDescr pkg = parseAndGetPackageDescrFromFile( - "Rule_with_nested_LHS.drl" ); + public void parenthesesOneLevelNestWithThreeSiblings() throws Exception { + final PackageDescr pkg = parseAndGetPackageDescrFromFile( "Rule_with_nested_LHS.drl" ); assertThat(parser.hasErrors()).as(parser.getErrors().toString()).isFalse(); @@ -2941,7 +3010,6 @@ class MiscDRLParserTest { assertThat(descr.getParameters().get(0)).isEqualTo("10"); } - @Disabled("Priority : Mid | outmost parentheses") @Test public void parse_RuleOldSyntax1() throws Exception { final String source = "rule \"Test\" when ( not $r :LiteralRestriction( operator == Operator.EQUAL ) ) then end"; @@ -2963,7 +3031,6 @@ class MiscDRLParserTest { assertThat(fieldConstraintDescr.getExpression()).isEqualToIgnoringWhitespace("operator == Operator.EQUAL"); } - @Disabled("Priority : Mid | outmost parentheses") @Test public void parse_RuleOldSyntax2() throws Exception { final String source = "rule \"Test\" when ( $r :LiteralRestriction( operator == Operator.EQUAL ) ) then end"; --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
