[incubator-kie-drools#5742] [new-parser] Broken inline cast #5806
Conversation
|
Before PR After PR |
| @@ -230,6 +230,7 @@ drlIdentifier returns [Token token] | |||
| | PERMITS | |||
| | RECORD | |||
| | VAR | |||
| | THIS | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- A slightly more permissive parser is not a big problem as opposed to a parser that's unable to accept a valid input.
- It's in line with the old parser's behavior (no
THIStoken =>thisrecognized asID=> accepted in the example above). - Yes, this is probably the most pragmatic approach. Simplicity over (maybe redundant) correctness.
So +0.9.
There was a problem hiding this comment.
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)
| @@ -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? | |||
There was a problem hiding this comment.
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.
| | inlineCast | ||
| ; | ||
|
|
||
| inlineCast : drlIdentifier HASH drlIdentifier ; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I also filed #5808 for further refactoring for the parsers.
|
@yurloc @mariofusco @gitgabrio Please review, thanks! |
| final String text = "package org.drools\n" + | ||
| "rule R1\n" + | ||
| "when\n" + | ||
| " $a : ICA( someB#ICB.someC#ICC.onlyConcrete() == \"Hello\" )\n" + |
There was a problem hiding this comment.
This is identical to the inlineCastMultiple() test below. Looks like a copy-paste error. Please fix.
There was a problem hiding this comment.
Thank you for spotting this!
|
Overall fixing progress: There were 270 failed tests. |
* [incubator-kie-drools#5742] [new-parser] Broken inline cast * fixed duplicate test
Issue:
How to replicate CI configuration locally?
Build Chain tool does "simple" maven build(s), the builds are just Maven commands, but because the repositories relates and depends on each other and any change in API or class method could affect several of those repositories there is a need to use build-chain tool to handle cross repository builds and be sure that we always use latest version of the code for each repository.
build-chain tool is a build tool which can be used on command line locally or in Github Actions workflow(s), in case you need to change multiple repositories and send multiple dependent pull requests related with a change you can easily reproduce the same build by executing it on Github hosted environment or locally in your development environment. See local execution details to get more information about it.
How to retest this PR or trigger a specific build:
for pull request and downstream checks
for a full downstream build
run_fdbfor Jenkins PR check only
Build Nowbutton.