LITERAL_WHEN and LAND tokens in switch statements should have EXPR child

[nrmancuso]

Originally posted by @rnveach in #15044 (comment)

Expressions used in case of switch are not caught by BooleanExpressionComplexity.

        switch (obj) {
            case ColoredPoint(boolean a, boolean b, boolean _) // line 4
                    when (a ^ (a || b) ^ (b || a) & (a | b)) -> {  // expected violation

                if (a ^ (a || b) ^ (b || a) & (a | b)) { }   // violation
                boolean c = (a ^ (a || b) ^ (b || a) & (a | b));   // violation

            }

This is the issue. We only check the count when we finish an expression. However, this when isn't an expression.

        |      |  |  |--LITERAL_CASE -> case [4:12]
        |      |  |  |  `--PATTERN_DEF -> PATTERN_DEF [5:20]
        |      |  |  |      `--LITERAL_WHEN -> when [5:20]
        |      |  |  |          |--RECORD_PATTERN_DEF -> RECORD_PATTERN_DEF [4:17]
...
        |      |  |  |          |--LPAREN -> ( [5:25]
        |      |  |  |          |--BXOR -> ^ [5:39]   <-- no expression
...
        |      |  |  |          `--RPAREN -> ) [5:59]

the same problem is for case with && or any other expression like operator:

        switch (o) {
            case String s && s.length() > 4: // guarded pattern, `PATTERN_DEF`
                break;

According to the JLS (unless I am looking at the wrong area), this guard says it is an expression. Is this a bug in our grammar that we are missing an expression token?
https://docs.oracle.com/javase/specs/jls/se22/html/jls-14.html#jls-14.11.1

SwitchLabel:
case CaseConstant {, CaseConstant}
case null [, default]
case CasePattern {, CasePattern} [Guard]
default
CaseConstant:
ConditionalExpression
CasePattern:
Pattern
Guard:
when Expression

Edit: It does look like our grammar does say expression.

checkstyle/src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java/JavaLanguageParser.g4

Lines 913 to 914 in 11dcd18

	guardedPattern
	: primaryPattern guard expr

Edit 2: It looks like it is an issue using expr over expression in the grammar.

checkstyle/src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java/JavaLanguageParser.g4

Lines 683 to 687 in 11dcd18

	// We will use this rule to make sure that we have an EXPR node
	// at the top of all expression subtrees.
	expression
	: expr
	;

examples of EXPR in our tree:
https://checkstyle.sourceforge.io/apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#SLIST
https://checkstyle.sourceforge.io/apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#EXPR
location of EXPR is odd when comparing to ASSIGN:

  `--SLIST -> {
      |--EXPR -> EXPR
      |   `--ASSIGN -> =

vs

 |--VARIABLE_DEF -> VARIABLE_DEF
 ...
 |   |--IDENT -> x
 |   `--ASSIGN -> =
 |       `--EXPR -> EXPR
 |           `--MINUS -> -

[rnveach]

@nrmancuso Are you able to look at other expr instances and see if they are valid or not? I did a scan of the grammar and these were what I saw.

checkstyle/src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java/JavaLanguageParser.g4

Line 503 in 11dcd18

| expr DOT typeArguments? LITERAL_SUPER arguments SEMI #primaryCtorCall

explicitConstructorInvocation

checkstyle/src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java/JavaLanguageParser.g4

Line 625 in 11dcd18

: expr DOT

fieldAccessNoIdent

expr can be ignored since it is just reflecting on itself.

checkstyle/src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java/JavaLanguageParser.g4

Line 750 in 11dcd18

| LPAREN expr RPAREN #parenPrimary

primary

checkstyle/src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java/JavaLanguageParser.g4

Lines 770 to 776 in 11dcd18

	stringTemplate
	: stringTemplateBegin expr? stringTemplateMiddle* stringTemplateEnd
	;
	stringTemplateMiddle
	: stringTemplateMid expr?
	;

String templates will be removed in #14805 , so this can also be ignored.

I will see if I can create some code to make this easier.

[rnveach]

Code examples from #15082 (comment) :

public class TestClass {
    void method() {
        // primary
        assert(false);

        // fieldAccessNoIdent
        try (InputAntlr4AstRegressionUncommon3.this) {
        }
    }
}
// explicitConstructorInvocation
class c5 extends c4.c4a {
    c5() { new c4().super(); }
}

Tree:

|      |      |--SINGLE_LINE_COMMENT -> // [3:8]
|      |      |  `--COMMENT_CONTENT ->  primary\n [3:10]
|      |      |--LITERAL_ASSERT -> assert [4:8]
|      |      |  |--EXPR -> EXPR [4:14]
|      |      |  |  |--LPAREN -> ( [4:14]
|      |      |  |  |--LITERAL_FALSE -> false [4:15]
|      |      |  |  `--RPAREN -> ) [4:20]
|      |      |  `--SEMI -> ; [4:21]
|      |      |--SINGLE_LINE_COMMENT -> // [6:8]
|      |      |  `--COMMENT_CONTENT ->  fieldAccessNoIdent\n [6:10]
|      |      |--LITERAL_TRY -> try [7:8]
|      |      |  |--RESOURCE_SPECIFICATION -> RESOURCE_SPECIFICATION [7:12]
|      |      |  |  |--LPAREN -> ( [7:12]
|      |      |  |  |--RESOURCES -> RESOURCES [7:46]
|      |      |  |  |  `--RESOURCE -> RESOURCE [7:46]
|      |      |  |  |      `--DOT -> . [7:46]
|      |      |  |  |          |--IDENT -> InputAntlr4AstRegressionUncommon3 [7:13]
|      |      |  |  |          `--LITERAL_THIS -> this [7:47]
|      |      |  |  `--RPAREN -> ) [7:51]
|      |      |  `--SLIST -> { [7:53]
|      |      |      `--RCURLY -> } [8:8]
...
    |--SINGLE_LINE_COMMENT -> // [11:0]
    |  `--COMMENT_CONTENT ->  explicitConstructorInvocation\n [11:2]
...
        |  `--SLIST -> { [13:9]
        |      |--SUPER_CTOR_CALL -> super [13:20]
        |      |  |--LITERAL_NEW -> new [13:11]
        |      |  |  |--IDENT -> c4 [13:15]
        |      |  |  |--LPAREN -> ( [13:17]
        |      |  |  |--ELIST -> ELIST [13:18]
        |      |  |  `--RPAREN -> ) [13:18]
        |      |  |--DOT -> . [13:19]
        |      |  |--LPAREN -> ( [13:25]
        |      |  |--ELIST -> ELIST [13:26]
        |      |  |--RPAREN -> ) [13:26]
        |      |  `--SEMI -> ; [13:27]
        |      `--RCURLY -> } [13:29]

JLS:
assert - https://docs.oracle.com/javase/specs/jls/se22/html/jls-14.html#jls-14.10
try-with-resource - https://docs.oracle.com/javase/specs/jls/se22/html/jls-14.html#jls-14.20.3
super call - https://docs.oracle.com/javase/specs/jls/se22/html/jls-15.html#jls-15.12

[nrmancuso]

These were all existing oddities (pre ANTLR4), which was one of the reasons why I had the two different rules in the first place.

We could certainly open tickets for each one of these to discuss if we should change our grammar, but these will be massively breaking at this point.

[rnveach]

We could certainly open tickets for each one of these to discuss if we should change our grammar, but these will be massively breaking at this point.

I agree, I will open the tickets to discuss.

[rnveach]

primary

I did not create an issue for this. This is a sub-set of the expr even though it is not named similarly.

You can see in our grammar only expr is calling it.

checkstyle/src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java/JavaLanguageParser.g4

Lines 689 to 690 in 11dcd18

	expr
	: primary #primaryExp

You can see in the tree, EXPR token is already listed, just above the LITERAL_FALSE which is the expr of the primary.

|      |      |  `--COMMENT_CONTENT ->  primary\n [3:10]
|      |      |--LITERAL_ASSERT -> assert [4:8]
|      |      |  |--EXPR -> EXPR [4:14]
|      |      |  |  |--LPAREN -> ( [4:14]
|      |      |  |  |--LITERAL_FALSE -> false [4:15]
|      |      |  |  `--RPAREN -> ) [4:20]

JLS:
https://docs.oracle.com/javase/specs/jls/se22/html/jls-15.html#jls-Primary
See PrimaryNoNewArray.

[romani]

@nrmancuso , please update issue title to reference that we will add expressions to case too. Title are used in release notes.

[nrmancuso]

@nrmancuso , please update issue title to reference that we will add expressions to case too. Title are used in release notes.

Not sure what you mean by this, the issue description accurately describes exactly what is changing here. We aren't adding imaginary EXPR nodes directly under the LITERAL_CASE token.

[romani]

I was about title, as the only part that is visible in release notes.

[nrmancuso]

Sorry, typo on my part, title describes what is changing here, we aren't adding an imaginary EXPR node to LITERAL_CASE.

[rnveach]

@romani There is no case group token here.

        |      |--LITERAL_SWITCH -> switch [3:8]
        |      |  |--LPAREN -> ( [3:15]
        |      |  |--EXPR -> EXPR [3:16]
        |      |  |  `--IDENT -> obj [3:16]
        |      |  |--RPAREN -> ) [3:19]
        |      |  |--LCURLY -> { [3:21]
        |      |  |--SWITCH_RULE -> SWITCH_RULE [4:12]
        |      |  |  |--LITERAL_CASE -> case [4:12]
        |      |  |  |  `--PATTERN_DEF -> PATTERN_DEF [5:20]
        |      |  |  |      |--SINGLE_LINE_COMMENT -> // [4:63]
        |      |  |  |      |  `--COMMENT_CONTENT ->  line 4\n [4:65]
        |      |  |  |      `--LITERAL_WHEN -> when [5:20]

[romani]

There is no when at all at https://github.com/checkstyle/checkstyle/blob/0c13df61a826ad81668aab4c790a6342454de976/src/test/resources-noncompilable/com/puppycrawl/tools/checkstyle/grammar/antlr4/ExpectedAntlr4AstRegressionCaseDefault.txt

But all switches got new EXPR token.

We detected issue on when but result fix for more wide and affected other places. I updated ticket to reflect what user will see in difference.

@nrmancuso , @rnveach , please consider renaming title.
User should find his problem in release notes and relatively easily match to his problem.
Release notes https://github.com/checkstyle/checkstyle/actions/runs/9685995933

[rnveach]

But all switches got new EXPR token.

This is false, because...

There is no when at all at https://github.com/checkstyle/checkstyle/blob/0c13df61a826ad81668aab4c790a6342454de976/src/test/resources-noncompilable/com/puppycrawl/tools/checkstyle/grammar/antlr4/ExpectedAntlr4AstRegressionCaseDefault.txt

checkstyle/src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java/JavaLanguageParser.g4

Line 925 in 9d33dde

guard: ( LAND | LITERAL_WHEN );

Guard appears only on when and && (we may update to guardedPattern). We shouldn't be updating switches with normal/old cases. I feel this is the issue with saying "case" without any additional context.

@nrmancuso can probably understand the grammar better if we can say "pattern guards" but it seems like the issue should be titled Guard tokens should have... since there are 2 type of guards.

[romani]

I look at it not from grammar perspective, but from resulted AST. Our API is AST, not a grammar, even they are related.
Users/Checks knows and cares only about this
https://checkstyle.sourceforge.io/apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html

[rnveach]

Even still, expr is not added to case. It is added to && (LAND). Saying we added to case is misleading and untrue in some cases.

[nrmancuso]

I've updated the title to be as correct and google-able as possible, anyone that has an issue with it can change the title again themselves. Users can come to this issue, see the painful discussion here, and understand all details about this change and the associated PR if they need to.

The following statements are not accurate:

But all switches got new EXPR token.

we will add expressions to case too