-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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.
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
Issue #12507: Support record patterns preview #12516
Conversation
bdc5d58
to
9e53eda
Compare
...noncompilable/com/puppycrawl/tools/checkstyle/grammar/java19/InputRecordPatternsPreview.java
Show resolved
Hide resolved
9e53eda
to
e3a8156
Compare
src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java/JavaLanguageParser.g4
Outdated
Show resolved
Hide resolved
2d72848
to
ca6d66c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
...om/puppycrawl/tools/checkstyle/grammar/java19/ExpectedGenericRecordDeconstructionPattern.txt
Outdated
Show resolved
Hide resolved
ca6d66c
to
71827a6
Compare
212165d
to
cd12e33
Compare
Notes to self:
Example:
|
...om/puppycrawl/tools/checkstyle/grammar/java19/ExpectedGenericRecordDeconstructionPattern.txt
Outdated
Show resolved
Hide resolved
@nrmancuso Do you have a link to the JLS that your changes cover? |
No, since this is a preview feature, the only specification is in the JEP. |
The only thing I noticed to bring up is I question the need for |
| | | | | | | |--TYPE -> TYPE [46:35] | ||
| | | | | | | | `--IDENT -> ColoredPoint [46:35] | ||
| | | | | | | |--LPAREN -> ( [46:47] | ||
| | | | | | | |--RECORD_PATTERN_COMPONENTS -> RECORD_PATTERN_COMPONENTS [46:48] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rnveach example of multiple components
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks I figured I was missing it. That was my only real concern. I feel fine with what you are doing.
3dc7ed7
to
3c5d304
Compare
3c5d304
to
1e493ed
Compare
| | `--IDENT -> r [45:29] | ||
| |--RPAREN -> ) [45:30] | ||
| `--SLIST -> { [45:32] | ||
| |--LITERAL_IF -> if [46:8] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the beginning of a complicated case of record pattern decomposition:
if (r instanceof Rectangle(ColoredPoint(Point p1,Color c1),
ColoredPoint lr1)
&& r instanceof Rectangle(ColoredPoint(Point p2,Color c2),
ColoredPoint lr2) && lr2.c == Color.BLUE) {
System.out.println(r);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is easier to read at https://github.com/checkstyle/checkstyle/wiki/Record-Pattern-Preview---AST-strucuture
| | | | `--IDENT -> b [13:23] | ||
| | | |--RPAREN -> ) [13:24] | ||
| | | |--LCURLY -> { [13:26] | ||
| | | |--SWITCH_RULE -> SWITCH_RULE [14:12] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Start of another nested record decomposition, with when
usage:
case Box<Box<String>>(Box<String>(String s)box)
when (("test".equals(s) && box.x != 7)) -> 1;
@romani left some comments, but basically we are only adding record patterns to existing pattern grammar. It is a tricky syntax for parsing, which is why I have all the crazy inputs. But - from a reviewing perspective a great deal of the input here very similar. |
Also, To help others to understand what is happening with record decomposition:
Of course, I made the actual input files terrible to make sure we can parse all such code, but basically all the inputs are variations on the above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
items:
src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java/JavaLanguageLexer.g4
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no problems with AST, all good.
https://github.com/checkstyle/checkstyle/wiki/Record-Pattern-Preview---AST-strucuture
I think we can finalize this PR.
config/projects-to-test/openjdk19-excluded.files
jdk19 to no-execution to make sure we can parse all record patterns that openjdk have in its test inputs - GOOD.
items:
src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java/JavaLanguageParser.g4
Outdated
Show resolved
Hide resolved
4f04299
to
4acbcc2
Compare
8806ec8
to
2f569ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last minor:
src/main/java/com/puppycrawl/tools/checkstyle/api/TokenTypes.java
Outdated
Show resolved
Hide resolved
0bd3eab
to
28515c4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok to merge
closes #12507
Compact view of AST: https://github.com/checkstyle/checkstyle/wiki/Record-Pattern-Preview---AST-strucuture
Reports
Check regression reports:
https://nrmancuso.github.io/reports/issue-12507/2023-04-21-T-13-58-36/part1-report/index.html
https://nrmancuso.github.io/reports/issue-12507/2023-04-21-T-13-58-36/part4-report/index.html
https://nrmancuso.github.io/reports/issue-12507/2023-04-21-T-13-58-36/part5-report/index.html
https://nrmancuso.github.io/reports/issue-12507/2023-04-21-T-13-58-36/sevntu-check-regression_part_2-report/index.html
https://nrmancuso.github.io/reports/issue-12507/2023-04-21-T-13-58-36/part2-report/index.html
https://nrmancuso.github.io/reports/issue-12507/2023-04-21-T-13-58-36/part6-report/index.html
https://nrmancuso.github.io/reports/issue-12507/2023-04-21-T-13-58-36/part3-report/index.html
https://nrmancuso.github.io/reports/issue-12507/2023-04-21-T-13-58-36/sevntu-check-regression_part_1-report/index.html
No differences in check behavior ^
ANTLR regression report:
https://nrmancuso.github.io/reports/issue-12507/2023-04-21-T-13-58-36/antlr-report/index.html
Only change is in exception line number in non-compilable files ^