Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[incubator-kie-drools#5742] [new-parser] Broken inline cast #5806

Merged
merged 2 commits into from
Mar 28, 2024
Merged

[incubator-kie-drools#5742] [new-parser] Broken inline cast #5806

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8ddc472
[incubator-kie-drools#5742] [new-parser] Broken inline cast
tkobayas Mar 27, 2024
12468d9
fixed duplicate test
tkobayas Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions ...drools-drl-parser-tests/src/test/java/org/drools/drl/parser/antlr4/MiscDRLParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3870,4 +3870,43 @@ void ooPathMixedWithStandardConstraint() {
assertThat(constraintDescr.toString())
.isEqualToIgnoringWhitespace("/wife[$age : age] && age > $age");
}

@Test
void inlineCast() {
final String text = "package org.drools\n" +
"rule R1\n" +
"when\n" +
" $a : ICA( someB#ICB.someC#ICC.onlyConcrete() == \"Hello\" )\n" +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is identical to the inlineCastMultiple() test below. Looks like a copy-paste error. Please fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for spotting this!

"then\n" +
"end\n";
PackageDescr packageDescr = parser.parse(text);
ExprConstraintDescr constraintDescr = getFirstExprConstraintDescr(packageDescr);
assertThat(constraintDescr.toString()).isEqualToIgnoringWhitespace("someB#ICB.someC#ICC.onlyConcrete() == \"Hello\"");
}

@Test
void inlineCastMultiple() {
final String text = "package org.drools\n" +
"rule R1\n" +
"when\n" +
" $a : ICA( someB#ICB.someC#ICC.onlyConcrete() == \"Hello\" )\n" +
"then\n" +
"end\n";
PackageDescr packageDescr = parser.parse(text);
ExprConstraintDescr constraintDescr = getFirstExprConstraintDescr(packageDescr);
assertThat(constraintDescr.toString()).isEqualToIgnoringWhitespace("someB#ICB.someC#ICC.onlyConcrete() == \"Hello\"");
}

@Test
void inlineCastThis() {
final String text = "package org.drools\n" +
"rule R1\n" +
"when\n" +
" $o : Object( this#Person.name == \"Mark\" )\n" +
"then\n" +
"end\n";
PackageDescr packageDescr = parser.parse(text);
ExprConstraintDescr constraintDescr = getFirstExprConstraintDescr(packageDescr);
assertThat(constraintDescr.toString()).isEqualToIgnoringWhitespace("this#Person.name == \"Mark\"");
}
}
2 changes: 1 addition & 1 deletion drools-drl/drools-drl-parser/src/main/antlr4/org/drools/drl/parser/antlr4/DRL6Expressions.g4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ drlIdentifier returns [Token token]
| PERMITS
| RECORD
| VAR
| THIS
Copy link
Contributor Author

@tkobayas tkobayas Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, I add THIS to drlIdentifier (in both DRLParser.g4 and DRL6Expressions.g4) because the original DRL6Expressions.g treated this as ID in primary rule (remember that | this_key ... line was commented out). this is involved with DRL specific syntax like inline cast (this#Person) and null dereference (this.address!.city). So I think it's better to include it in drlIdentifier so that we can keep parser rules simple.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with this is that the parser will now allow this to appear at impossible places, for example, Person( age.this.getSomething() ). I feel slightly uncomfortable about this 🙂

But on second thought, I think it's OK because:

  1. A slightly more permissive parser is not a big problem as opposed to a parser that's unable to accept a valid input.
  2. It's in line with the old parser's behavior (no THIS token => this recognized as ID => accepted in the example above).
  3. Yes, this is probably the most pragmatic approach. Simplicity over (maybe redundant) correctness.

So +0.9.

Copy link
Contributor Author

@tkobayas tkobayas Mar 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, especially the reason "2" is the point. We'd like to minimize deviation from the old parser behaviour at this stage. (Hoping cleaner refactoring in the future)

;

// --------------------------------------------------------
Expand Down Expand Up @@ -671,7 +672,6 @@ primary returns [BaseDescr result]
: expr=parExpression { if( buildDescr ) { $result = $expr.result; } }
| nonWildcardTypeArguments (explicitGenericInvocationSuffix | this_key arguments)
| literal { if( buildDescr ) { $result = new AtomicExprDescr( $literal.text, true ); } }
| this_key (DOT drlIdentifier)* identifierSuffix?
Copy link
Contributor Author

@tkobayas tkobayas Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line was added by #5791 , but now this is handled by the later lines | i1=drlIdentifier ... which can process DOT, HASH and NULL_SAFE_DOT.

| super_key superSuffix
| new_key creator
| primitiveType (LBRACK RBRACK)* DOT class_key
Expand Down
5 changes: 5 additions & 0 deletions drools-drl/drools-drl-parser/src/main/antlr4/org/drools/drl/parser/antlr4/DRLParser.g4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ drlIdentifier
| PERMITS
| RECORD
| VAR
| THIS
;

drlKeywords
Expand Down Expand Up @@ -258,6 +259,7 @@ drlExpression
| NEW nonWildcardTypeArguments? innerCreator
| SUPER superSuffix
| explicitGenericInvocation
| inlineCast
)
| drlExpression NULL_SAFE_DOT ( drlIdentifier | drlMethodCall )
| drlExpression LBRACK drlExpression RBRACK
Expand Down Expand Up @@ -321,8 +323,11 @@ drlPrimary
| nonWildcardTypeArguments (explicitGenericInvocationSuffix | THIS arguments)
| inlineListExpression
| inlineMapExpression
| inlineCast
;

inlineCast : drlIdentifier HASH drlIdentifier ;
Comment on lines +326 to +329
Copy link
Contributor Author

@tkobayas tkobayas Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In DRLParser.g4, we mimic the parser syntax of DRL6Expressions.g4. However, here looks a little different. DRL6Expressions.g4's primary rule handles more complex structure (e.g. DOT chaining) while such a structure is handled in drlExpression in DRLParser.g4 because drlExpression is inherited from JavaParser.g4's expression.

This time, I prioritized simplicity for the changes in DRLParser.g4. But I filed #5807 to review drlExpression and drlPrimary to make them closer to DRL6Expressions.g4.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also filed #5808 for further refactoring for the parsers.


/* extending JavaParser literal */
drlLiteral
: integerLiteral
Expand Down
Loading