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

[new-parser] Allow package to be used as an identifier #5819

Closed
wants to merge 1 commit into from
Closed

[new-parser] Allow package to be used as an identifier #5819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 20 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 @@ -3978,4 +3978,24 @@ public void queryNoArgument() throws Exception {
assertThat(query.getParameterTypes()).isEmpty();
assertThat(query.getParameters()).isEmpty();
}

@Test
public void packageInQualifiedName() throws Exception {
String pkgName = "org.drools.package";
String text = "package " + pkgName + "\n" +
"rule R\n" +
"when\n" +
" $p : Person()\n" +
"then\n" +
"end\n";


PackageDescr pkg = parser.parse(text);
assertThat(parser.hasErrors()).as(parser.getErrorMessages().toString()).isFalse();

assertThat(pkg.getName()).isEqualTo(pkgName);
assertThat(pkg.getRules()).hasSize(1);

assertThat(pkg.getRules().get(0).getName()).isEqualTo("R");
}
}
1 change: 1 addition & 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 @@ -198,6 +198,7 @@ drlIdentifier
| RECORD
| VAR
| THIS
| PACKAGE
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tkobayas @mariofusco @gitgabrio Together with THIS on the line above, this is just the tip of the iceberg. If we want to keep the new parser 100% backward compatible, we need to A) also add all the other Java keywords, for example, long, break, for, etc. or B) revert closer to the old approach and stop using JavaLexer that defines all these keyword tokens.

B) means using just a few tokens for operators, parentheses, and literals of various types, and IDENTIFIER for everything else on the Lexer level. The parser then needs to use semantic predicates (e.g. helper.validateIdentifierKey(DroolsSoftKeywords.PACKAGE) to see whether the next IDENTIFIER token actually contains package).

A) means listing all JavaLexer tokens for various Java keywords here and also replacing nearly all usages of IDENTIFIER with drlIdentifier.

Copy link
Contributor

@tkobayas tkobayas Apr 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think the original test failure #5818 org.kie.declarativetypes.JavaBeansEventRoleTest#testImportBean is a very exceptional case.

        String drl = "package some.other.package;\n" +
                "import org.drools.compiler.Bean;\n" +
                "declare Bean\n" +
                "  @role(event)\n" +
                "end";

The test doesn't fail with the old parser, because the package some.other.package doesn't have anything to compile actually. If a user adds a rule to the package, it would fail on the java compilation phase even with the old parser.

DRL is more or less based on Java syntax, so I think, theoretically, it's fine to fail such a java keyword usage (which normally fails with java compiler) at the parse phase with the new parser (As written in #5818, no-error-report is another issue, though). In other words, fix the test case not to use package in package.

C) Keep excluding java keywords from drlIdentifier

It's against the policy "100% backward compatible with the old parser", but sounds acceptable. WDYT?

(We may face a situation that we will need to choose option A or B in the future, though)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @yurloc @tkobayas @mariofusco
sorry I could provide only very general observations:

  1. I would stick with 100% backward compatibility; without that, something that was working previously, simply stop to work, and it is not clear who/how would fix the issues that will popup, nor how much time this could take
  2. I wonder (again, just asking) if there is not a programmatic way to automatically add all the java keywords from JavaLexer to drlIdentifier : drlKeywords - and then keep going with option A (IIUC)

Wdyt ? Am I missing something ?

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. I would stick with 100% backward compatibility; without that, something that was working previously, simply stop to work, and it is not clear who/how would fix the issues that will popup, nor how much time this could take

Okay, I'm not opinionated on this. We can go with Option A and leave Option C as a future enhancement possibility.

  1. I wonder (again, just asking) if there is not a programmatic way to automatically add all the java keywords from JavaLexer to drlIdentifier : drlKeywords - and then keep going with option A (IIUC)

There is no programmatic way because those Java keywords are individually declared in JavaLexer.g4. I think explicitly listing them in drlIdentifier is readable enough.

Thanks,

Copy link
Contributor

Choose a reason for hiding this comment

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

ok @tkobayas
Anyway, just remember that copying/duplicating stuff creates problem of maintanability, not readability.
Anyway, fine for me 👍

;

drlKeywords
Expand Down
Loading