Skip to content

Commit

Permalink
- Adding code commentes and explanations
Browse files Browse the repository at this point in the history 
- Adding more nested level tests
  • Loading branch information
@tkobayas
tkobayas committed Oct 26, 2023
1 parent 4473ecd commit 5eace20
Show file tree
Hide file tree
Showing 3 changed files with 130 additions and 27 deletions.
16 changes: 7 additions & 9 deletions drools-parser/src/main/antlr4/org/drools/parser/DRLParser.g4
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,13 +91,9 @@ lhsExpression : LPAREN lhsExpression RPAREN #lhsExpressionEnclosed
| lhsExpression (DRL_OR lhsExpression)+ #lhsOr
;

// lhsOr is used as a label in lhsExpression rule. But some other rules also use it.
lhsOrDef : lhsAndDef (DRL_OR lhsAndDef)*
| LPAREN DRL_OR lhsAndDef+ RPAREN
;

// 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
;

Expand Down Expand Up @@ -385,8 +381,9 @@ fromEntryPoint : DRL_ENTRY_POINT stringId ;
| lhsPatternBind
)
*/
// Use lhsExpression instead of lhsOr because lhsExpression has good enough structure
lhsExists : DRL_EXISTS
( LPAREN lhsOrDef RPAREN
( LPAREN lhsExpression RPAREN
| lhsPatternBind
)
;
Expand All @@ -398,8 +395,9 @@ lhsExists : DRL_EXISTS
| lhsPatternBind
)
*/
// Use lhsExpression instead of lhsOr because lhsExpression has good enough structure
lhsNot : DRL_NOT
( LPAREN lhsOrDef RPAREN
( LPAREN lhsExpression RPAREN
| lhsPatternBind
)
;
Expand Down
61 changes: 47 additions & 14 deletions drools-parser/src/main/java/org/drools/parser/DRLVisitorImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -576,10 +576,16 @@ public String visitDrlIdentifier(DRLParser.DrlIdentifierContext ctx) {
@Override
public ExistsDescr visitLhsExists(DRLParser.LhsExistsContext ctx) {
ExistsDescr existsDescr = new ExistsDescr();
if (ctx.lhsOrDef() != null) {
BaseDescr descr = visitDescrChildren(ctx.lhsOrDef()).get(0);
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);
}
Expand All @@ -589,10 +595,16 @@ public ExistsDescr visitLhsExists(DRLParser.LhsExistsContext ctx) {
@Override
public NotDescr visitLhsNot(DRLParser.LhsNotContext ctx) {
NotDescr notDescr = new NotDescr();
if (ctx.lhsOrDef() != null) {
BaseDescr descr = visitDescrChildren(ctx.lhsOrDef()).get(0);
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);
}
Expand All @@ -611,12 +623,21 @@ public BaseDescr visitLhsExpressionEnclosed(DRLParser.LhsExpressionEnclosedConte

@Override
public BaseDescr visitLhsOr(DRLParser.LhsOrContext ctx) {
// 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;
Expand All @@ -627,9 +648,9 @@ private List<BaseDescr> flattenOrDescr(List<DescrNodePair> descrList) {
List<BaseDescr> flattenedDescrs = new ArrayList<>();
for (DescrNodePair descrNodePair : descrList) {
BaseDescr descr = descrNodePair.getDescr();
ParseTree node = descrNodePair.getNode();
ParseTree node = descrNodePair.getNode(); // parser node corresponding to the descr
if (descr instanceof OrDescr && !(node instanceof DRLParser.LhsExpressionEnclosedContext)) {
// sibling OrDescr should be flattened
// sibling OrDescr should be flattened unless it's explicitly enclosed by parenthesis
flattenedDescrs.addAll(((OrDescr) descr).getDescrs());
} else {
flattenedDescrs.add(descr);
Expand All @@ -644,12 +665,21 @@ public BaseDescr visitLhsAnd(DRLParser.LhsAndContext 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;
Expand All @@ -660,9 +690,9 @@ private List<BaseDescr> flattenAndDescr(List<DescrNodePair> descrList) {
List<BaseDescr> flattenedDescrs = new ArrayList<>();
for (DescrNodePair descrNodePair : descrList) {
BaseDescr descr = descrNodePair.getDescr();
ParseTree node = descrNodePair.getNode();
ParseTree node = descrNodePair.getNode(); // parser node corresponding to the descr
if (descr instanceof AndDescr && !(node instanceof DRLParser.LhsExpressionEnclosedContext)) {
// sibling AndDescr should be flattened
// sibling AndDescr should be flattened unless it's explicitly enclosed by parenthesis
flattenedDescrs.addAll(((AndDescr) descr).getDescrs());
} else {
flattenedDescrs.add(descr);
Expand Down Expand Up @@ -702,7 +732,10 @@ private List<BaseDescr> visitDescrChildren(RuleNode node) {
return aggregator;
}

// This method is used when the parent descr requires children node information for composing the descr.
// 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();
Expand All @@ -711,15 +744,15 @@ private List<DescrNodePair> visitDescrChildrenForDescrNodePair(RuleNode node) {
ParseTree c = node.getChild(i);
Object childResult = c.accept(this);
if (childResult instanceof BaseDescr) {
aggregator.add(new DescrNodePair((BaseDescr) childResult, c));
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;
private final ParseTree node;
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;
Expand Down
80 changes: 76 additions & 4 deletions drools-parser/src/test/java/org/drools/parser/MiscDRLParserTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1477,7 +1477,7 @@ void parenthesesOrAndAnd() {
}

@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" +
Expand Down Expand Up @@ -1516,6 +1516,79 @@ void parenthesesAndOrOrOrAnd() throws Exception {
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(
Expand Down Expand Up @@ -2834,9 +2907,8 @@ public void parse_TypeDeclarationWithFields() throws Exception {
}

@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();

Expand Down

0 comments on commit 5eace20

Please sign in to comment.