Skip to content

Issue #6615: Add support for Java 14 switch and yield#8449

Merged
romani merged 3 commits into
checkstyle:masterfrom
nrmancuso:enhanced-switch
Aug 11, 2020
Merged

Issue #6615: Add support for Java 14 switch and yield#8449
romani merged 3 commits into
checkstyle:masterfrom
nrmancuso:enhanced-switch

Conversation

@nrmancuso

@nrmancuso nrmancuso commented Jul 14, 2020

Copy link
Copy Markdown
Contributor

Issue #6615: Add support for Java 14 switch and yield

This PR will introduce support for Java 14 switch expressions, and yield statements.


Relevant JLS grammar for switch expressions:

UnaryExpression:
PreIncrementExpression
PreDecrementExpression
+ UnaryExpression
- UnaryExpression
UnaryExpressionNotPlusMinus

PreIncrementExpression:
++ UnaryExpression

PreDecrementExpression:
-- UnaryExpression

UnaryExpressionNotPlusMinus:
PostfixExpression
~ UnaryExpression
! UnaryExpression
CastExpression
SwitchExpression

SwitchExpression:
switch ( Expression ) SwitchBlock

SwitchBlock:
{ SwitchLabeledRule {SwitchLabeledRule} }
{ {SwitchLabeledStatementGroup} {SwitchLabel :} }

SwitchLabeledRule:
SwitchLabeledExpression
SwitchLabeledBlock
SwitchLabeledThrowStatement

SwitchLabeledExpression:
SwitchLabel -> Expression ;

SwitchLabeledBlock:
SwitchLabel -> Block

SwitchLabeledThrowStatement:
SwitchLabel -> ThrowStatement

SwitchLabel:
case CaseConstant {, CaseConstant}
default

CaseConstant:
ConditionalExpression

SwitchLabeledStatementGroup:
SwitchLabel : {SwitchLabel :} BlockStatements


Relevant grammar for yield statements:

YieldStatement:
yield Expression ;


I have introduced the new token, SWITCH_RULE, for the following reasons:

  1. From JLS: When a selector expression matches a switch label for a switch labeled rule, the labeled expression or statement is executed and nothing else.

    This means that there is no reason to check for fall-through, as in the case of switch case groups. In checks, we can simply conclude that we are in a switch rule, and know that there will be definite assignment and no fall through.

  2. The java grammar differentiates between the two, so we should also.

    The body of both a switch statement and a switch expression (15.28) is known as a switch block. This block can consist of either:

    Switch labeled rules, which use -> to introduce either a switch labeled expression, switch labeled block, or a switch labeled throw statement; or

    Switch labeled statement groups, which use : to introduce switch labeled block statements.

@nrmancuso

This comment has been minimized.

@nrmancuso nrmancuso marked this pull request as draft July 14, 2020 16:25
@nrmancuso nrmancuso force-pushed the enhanced-switch branch 6 times, most recently from 20a6016 to 96c9237 Compare July 17, 2020 03:08
@nrmancuso nrmancuso marked this pull request as ready for review July 17, 2020 03:08
Comment thread config/ant-phase-verify.xml Outdated
@nrmancuso nrmancuso force-pushed the enhanced-switch branch 4 times, most recently from 08f32ec to 0f0f626 Compare July 18, 2020 03:14
@nrmancuso nrmancuso marked this pull request as draft July 19, 2020 19:16
@nrmancuso nrmancuso force-pushed the enhanced-switch branch 3 times, most recently from 5e4d6b5 to 8ccdf55 Compare July 26, 2020 01:22
@nrmancuso nrmancuso marked this pull request as ready for review July 26, 2020 12:18

@nrmancuso nrmancuso left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Some items to discuss:

Comment thread src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java.g Outdated
Comment thread src/main/java/com/puppycrawl/tools/checkstyle/api/TokenTypes.java

@pbludov pbludov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@esilkensen @romani @rnveach
I agree with @nmancus1 on the idea of a separate token for the SWITCH_RULE. Please share your thoughts.

Comment thread src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java.g Outdated

@esilkensen esilkensen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is great, nice work again Nick.

Comment thread src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java.g Outdated
Comment thread src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java.g Outdated
@pbludov pbludov assigned esilkensen and unassigned pbludov Aug 9, 2020
@esilkensen esilkensen assigned rnveach and unassigned esilkensen Aug 9, 2020
Comment thread src/main/java/com/puppycrawl/tools/checkstyle/api/TokenTypes.java Outdated
Comment thread src/main/java/com/puppycrawl/tools/checkstyle/api/TokenTypes.java Outdated
Comment thread src/main/java/com/puppycrawl/tools/checkstyle/api/TokenTypes.java
@rnveach

rnveach commented Aug 10, 2020

Copy link
Copy Markdown
Contributor

Has there been any time regression done for antlr changes to see if changes had any negative impact on time taken to parse?

@nrmancuso

Copy link
Copy Markdown
Contributor Author

Has there been any time regression done for antlr changes to see if changes had any negative impact on time taken to parse?

Not since full records support. Should I run trials against 8.34?

@romani

romani commented Aug 10, 2020

Copy link
Copy Markdown
Member

Let's move performance consideration to separate issue. It should not block ability to parse in general.

@rnveach

rnveach commented Aug 11, 2020

Copy link
Copy Markdown
Contributor

Can be merged when CI passes.

@nrmancuso

nrmancuso commented Aug 11, 2020

Copy link
Copy Markdown
Contributor Author

Has there been any time regression done for antlr changes to see if changes had any negative impact on time taken to parse?

Before my changes:
image

After my changes:
image

Run time for guava report for Checkstyle 8.34: 23.460 seconds
Run time for guava report for Checkstyle 8.36-SNAPSHOT (built from this PR): 23.910 seconds

Difference in report generation times: -0.45

There is no noticeable difference between the execution times of these two runs. The number of files in the guava repo:

➜  repositories git:(master) ✗ ls
guava
➜  repositories git:(master) ✗ find . -type f | wc -l
3267

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for Java 14 switch/yield expression

5 participants