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-5818] [new-parser] Parsing fails if a Java keyw… #5958

Merged
merged 2 commits into from
May 21, 2024
Merged

[incubator-kie-drools-5818] [new-parser] Parsing fails if a Java keyw… #5958

merged 2 commits into from
May 21, 2024

Conversation

tkobayas
Copy link
Contributor

…ord appears in a qualified name

Issue

Also

tkobayas
tkobayas commented May 17, 2024
View reviewed changes
...rl/drools-drl-parser-tests/src/test/java/org/drools/drl/parser/antlr4/DRLExprParserTest.java Outdated
Comment on lines 443 to 446
@Test
public void testNoViableAlt() {
String source = "x.int";
parser.parse(source);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because of this PR fix, this expression no longer causes an error. I couldn't figure out another expression to produce "no viable alternative", so removed this test for now. I think we can add the test later, but I think it's relatively low priority.

Copy link
Contributor

Choose a reason for hiding this comment

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

a~a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Added the test.

@yurloc yurloc mentioned this pull request May 17, 2024
Closed
yurloc
yurloc suggested changes May 17, 2024
View reviewed changes
Copy link
Contributor

@yurloc yurloc left a comment

Choose a reason for hiding this comment

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

I have to request a change. I don't have the time to investigate this fully but there is a problem with the new keyword. It might need a special treatment because at this state the PR breaks org.drools.mvel.integrationtests.JittingTest#testBigDecimalConstructorCoercion:

### parse : ANTLR4_PARSER_ENABLED = true
line 1:10 no viable alternative at input 'BigDecimal'
14:49:04.620 [main] ERROR o.d.c.k.b.impl.AbstractKieProject.buildKnowledgePackages:280 - Unable to build KieBaseModel:KieBaseModelName
Unable to parse pattern expression:
[ERR 101] Line 1:10 no viable alternative at input 'BigDecimal' : [Rule name='R1']


14:49:04.626 [main] ERROR o.d.testcoverage.common.util.KieUtil.getKieBuilderFromKieFileSystem:165 - KieBuilder errors: [Message [id=1, kieBase=KieBaseModelName, level=ERROR, path=rules1.drl, line=4, column=0
   text=Unable to parse pattern expression:
[ERR 101] Line 1:10 no viable alternative at input 'BigDecimal']]

java.lang.AssertionError: [[Message [id=1, kieBase=KieBaseModelName, level=ERROR, path=rules1.drl, line=4, column=0
   text=Unable to parse pattern expression:
[ERR 101] Line 1:10 no viable alternative at input 'BigDecimal']]] 
Expecting empty but was: [Message [id=1, kieBase=KieBaseModelName, level=ERROR, path=rules1.drl, line=4, column=0
   text=Unable to parse pattern expression:
[ERR 101] Line 1:10 no viable alternative at input 'BigDecimal']]

	at org.drools.testcoverage.common.util.KieUtil.getKieBuilderFromKieFileSystem(KieUtil.java:169)
	at org.drools.testcoverage.common.util.KieUtil.getKieBuilderFromKieFileSystem(KieUtil.java:134)
	at org.drools.testcoverage.common.util.KieUtil.buildAndInstallKieModuleIntoRepo(KieUtil.java:78)
	at org.drools.testcoverage.common.util.KieUtil.buildAndInstallKieModuleIntoRepo(KieUtil.java:72)
	at org.drools.testcoverage.common.util.KieUtil.getKieModuleFromResources(KieUtil.java:249)
	at org.drools.testcoverage.common.util.KieUtil.getKieModuleFromDrls(KieUtil.java:227)
	at org.drools.testcoverage.common.util.KieUtil.getKieModuleFromDrls(KieUtil.java:217)
	at org.drools.testcoverage.common.util.KieUtil.getKieModuleFromDrls(KieUtil.java:212)
	at org.drools.mvel.integrationtests.JittingTest.testBigDecimalConstructorCoercion(JittingTest.java:327)

...rl/drools-drl-parser-tests/src/test/java/org/drools/drl/parser/antlr4/DRLExprParserTest.java Outdated
Comment on lines 443 to 446
@Test
public void testNoViableAlt() {
String source = "x.int";
parser.parse(source);
Copy link
Contributor

Choose a reason for hiding this comment

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

a~a

@tkobayas tkobayas mentioned this pull request May 20, 2024
Open
@tkobayas
Copy link
Contributor Author

I have to request a change. I don't have the time to investigate this fully but there is a problem with the new keyword. It might need a special treatment because at this state the PR breaks org.drools.mvel.integrationtests.JittingTest#testBigDecimalConstructorCoercion:

Thanks @yurloc ! I excluded new from drlIdentifier. We may be able to solve the issue by modifying primary rule etc., but it would complicate the difference between the old and new parser, so it would make the future investigation harder. I think at this stage, it' better to go for a simple fix to pass existing unit tests and, list any backward compatibility issues in #5962 , so we can evaluate them in the next round.

yurloc
yurloc approved these changes May 20, 2024
View reviewed changes
Copy link
Contributor

@yurloc yurloc left a comment

Choose a reason for hiding this comment

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

LGTM now.

@yurloc
Copy link
Contributor

yurloc commented May 20, 2024

@mariofusco @gitgabrio please review and merge, thanks!

@mariofusco mariofusco merged commit 8997956 into apache:dev-new-parser May 21, 2024
0 of 3 checks passed
This was referenced May 21, 2024
Closed
Closed
Closed
tkobayas added a commit to tkobayas/drools that referenced this pull request Jun 11, 2024
@tkobayas
[incubator-kie-drools-5818] [new-parser] Parsing fails if a Java keyw… (
5e2a896 
apache#5958)

* [incubator-kie-drools-5818] [new-parser] Parsing fails if a Java keyword appears in a qualified name

* - Exclude 'new' from 'drlIdentifier' because of 'primary' ambiguity
- Add tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mariofusco mariofusco mariofusco approved these changes

@yurloc yurloc yurloc approved these changes

@gitgabrio gitgabrio Awaiting requested review from gitgabrio

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tkobayas @yurloc @mariofusco