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

Issue #15082:LITERAL_WHEN token should have EXPR child #15084

Merged
merged 1 commit into from
Jun 25, 2024
Merged

Issue #15082:LITERAL_WHEN token should have EXPR child #15084

merged 1 commit into from
Jun 25, 2024

Conversation

nrmancuso
Copy link
Member

@nrmancuso nrmancuso commented Jun 21, 2024 •
edited
Loading

Closes #15082.

Link to report workflow: https://github.com/nrmancuso/checkstyle-diff-report-generator/actions/runs/9614065671

Projects file: projects-to-test-on.properties
Patch branch: issue-15082

https://checkstyle-reports.s3.us-east-2.amazonaws.com/reports/issue-15082/2024-06-21-T-15-06-41/part1/index.html

No diff.

https://checkstyle-reports.s3.us-east-2.amazonaws.com/reports/issue-15082/2024-06-21-T-15-06-41/part4/index.html

We will need to explore BooleanExpressionComplexity in #15044

https://checkstyle-reports.s3.us-east-2.amazonaws.com/reports/issue-15082/2024-06-21-T-15-06-41/part5/index.html

No diff

https://checkstyle-reports.s3.us-east-2.amazonaws.com/reports/issue-15082/2024-06-21-T-15-06-41/part3/index.html

We will need to explore UnnecessaryParentheses in #14972, looks like we fixed some false positives and false negatives

https://checkstyle-reports.s3.us-east-2.amazonaws.com/reports/issue-15082/2024-06-21-T-15-06-41/part2/index.html

No diff

https://checkstyle-reports.s3.us-east-2.amazonaws.com/reports/issue-15082/2024-06-21-T-15-06-41/part6/index.html

No diff

https://checkstyle-reports.s3.us-east-2.amazonaws.com/reports/issue-15082/2024-06-21-T-15-06-41/sevntu-check-regression_part_1/index.html

No diff

https://checkstyle-reports.s3.us-east-2.amazonaws.com/reports/issue-15082/2024-06-21-T-15-06-41/sevntu-check-regression_part_2/index.html

No diff

https://checkstyle-reports.s3.us-east-2.amazonaws.com/reports/issue-15082/2024-06-21-T-15-06-41/antlr-report-checkstyle/index.html

All expected diffs

@nrmancuso nrmancuso self-assigned this Jun 21, 2024
@nrmancuso nrmancuso force-pushed the issue-15082 branch 2 times, most recently from c1f145e to f02062d Compare June 21, 2024 13:28
@rnveach rnveach requested review from romani and rnveach June 21, 2024 15:25
@nrmancuso nrmancuso force-pushed the issue-15082 branch 2 times, most recently from 5949b3a to 76a6b96 Compare June 22, 2024 14:17
@nrmancuso nrmancuso requested a review from mahfouz72 June 22, 2024 15:22
@nrmancuso nrmancuso assigned mahfouz72 and rnveach and unassigned nrmancuso Jun 22, 2024
@nrmancuso nrmancuso marked this pull request as ready for review June 22, 2024 15:24
nrmancuso
nrmancuso commented Jun 22, 2024
View reviewed changes
Copy link
Member Author

@nrmancuso nrmancuso left a comment

Choose a reason for hiding this comment

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

Comments:

src/test/java/com/puppycrawl/tools/checkstyle/JavaAstVisitorTest.java
* @throws IOException if file does not exist
*/
@Test
public void countExprUsagesInParserGrammar() throws IOException {
Copy link
Member Author

Choose a reason for hiding this comment

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

Hopefully we can prevent this in the future. I had considered using a custom check for this, and a few other things that included individual suppressions, but really we should not need to get this crazy. As long as we can explain why we increase the number of expected usages of the expr rule, we should be fine. I placed a comment directly above this value so that we will notice updates to this number and catch it in review.

...le/com/puppycrawl/tools/checkstyle/grammar/antlr4/ExpectedAntlr4AstRegressionCaseDefault.txt
| | | | | | | |--ELIST -> ELIST [10:39]
| | | | | | | `--RPAREN -> ) [10:39]
| | | | | | `--NUM_INT -> 6 [10:43]
| | | | | | `--EXPR -> EXPR [10:41]
Copy link
Member Author

Choose a reason for hiding this comment

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

All diffs in AST files are expected, we have just added a new EXPR node parent to each.

rnveach
rnveach approved these changes Jun 22, 2024
View reviewed changes
Copy link
Member

@rnveach rnveach left a comment

Choose a reason for hiding this comment

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

My concerns are being addressed in new issues.

...ble/com/puppycrawl/tools/checkstyle/grammar/java19/ExpectedJava19PatternsTrickyWhenUsage.txt
| | | | | | `--NUM_INT -> 0 [20:43]
| | | | | | `--EXPR -> EXPR [20:40]
| | | | | | `--GE -> >= [20:40]
| | | | | | |--IDENT -> when [20:35]
Copy link
Member

Choose a reason for hiding this comment

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

I see a concern and will put it in a new issue.

Copy link
Member

Choose a reason for hiding this comment

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

checkstyle/src/test/resources-noncompilable/com/puppycrawl/tools/checkstyle/grammar/java19/InputJava19PatternsTrickyWhenUsage.java

Lines 18 to 22 in d951e08

return switch (x) {
// this is horrible, but it compiles
case Integer when when when >= 0 -> when.toString();
default -> "2";
};

@rnveach rnveach removed their assignment Jun 22, 2024
@rnveach rnveach mentioned this pull request Jun 22, 2024
Open
mahfouz72
mahfouz72 approved these changes Jun 22, 2024
View reviewed changes
Copy link
Member

@mahfouz72 mahfouz72 left a comment

Choose a reason for hiding this comment

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

Thank a lot for the quick fix!

@rnveach rnveach assigned romani and unassigned mahfouz72 Jun 22, 2024
@romani
Copy link
Member

romani commented Jun 23, 2024

This PR needs laptop time.
Pattern changed, and I don't see why it should.

romani
romani reviewed Jun 25, 2024
View reviewed changes
...ncompilable/com/puppycrawl/tools/checkstyle/grammar/java21/ExpectedUnnamedVariableSwitch.txt
Comment on lines +719 to +726
| | | | | | `--EXPR -> EXPR [84:44]
| | | | | | `--LITERAL_INSTANCEOF -> instanceof [84:44]
| | | | | | |--IDENT -> o2 [84:41]
| | | | | | `--PATTERN_VARIABLE_DEF -> PATTERN_VARIABLE_DEF [84:55]
| | | | | | |--MODIFIERS -> MODIFIERS [84:55]
| | | | | | |--TYPE -> TYPE [84:55]
| | | | | | | `--IDENT -> String [84:55]
| | | | | | `--IDENT -> s [84:62]
Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/checkstyle/checkstyle/blob/d951e083e6f631937374678adfbeafc67fe45424/src/test/resources-noncompilable/com/puppycrawl/tools/checkstyle/grammar/java21/InputUnnamedVariableSwitch.java#L84C42-L84C73

romani
romani approved these changes Jun 25, 2024
View reviewed changes
Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

ok to merge.

@romani romani merged commit 0c13df6 into checkstyle:master Jun 25, 2024
111 checks passed
@nrmancuso nrmancuso deleted the issue-15082 branch June 25, 2024 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romani romani romani approved these changes

@rnveach rnveach rnveach approved these changes

@mahfouz72 mahfouz72 mahfouz72 approved these changes

Assignees

@romani romani

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

LITERAL_WHEN and LAND tokens in switch statements should have EXPR child
4 participants
@nrmancuso @romani @rnveach @mahfouz72