Issue #14620: LITERAL_CASE token support in RightCurlyCheck

[mahfouz72]

Resolves #14620 :

Diff Regression config: https://gist.githubusercontent.com/mahfouz72/c82dbc1b4a05567e245729eccfc6cf62/raw/c4958b5ee00ded9c8f89d3af0a50fa871b352205/rcurlybase.xml

Diff Regression patch config: https://gist.githubusercontent.com/mahfouz72/fb26eda6b5a3264c0c42351e612cc4f6/raw/daf318509889f654cd61d58c9e039c9ba1505716/rcurlypatch.xml

Site (link)

Reports

• alone option (link)

• alone_or_singleline option (link)

• same option (link)

[mahfouz72]

@nrmancuso @rnveach can you guide me on how to solve this CI failure related to checker I am not familiar with it

also the purpose of this function is to skip switch expressions when we getDetailsForSwitch() token why is that ? and should I do the same here in this PR

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/blocks/RightCurlyCheck.java

Lines 462 to 472 in 7e9960b

	private static boolean isSwitchExpression(DetailAST switchNode) {
	DetailAST currentNode = switchNode;
	boolean ans = false;
	while (currentNode != null) {
	if (currentNode.getType() == TokenTypes.EXPR) {
	ans = true;
	}
	currentNode = currentNode.getParent();
	}
	return ans;

and please review changes if all is good

[nrmancuso]

@nrmancuso @rnveach can you guide me on how to solve this CI failure related to checker I am not familiar with it

Let's wait until we are getting close to merge to deal with this, code might change by then :) But what Checker is telling us is that basically it expects the DetailAST to be non-null, and you are passing in a potentially null object instead.

also the purpose of this function is to skip switch expressions when we getDetailsForSwitch() token why is that ? and should I do the same here in this PR

Ah, I had forgotten about this. I do not totally understand this "limitation", but since we have an open issue on it, let's also avoid switch expressions here. You can use git blame on this method to find the associated PR and read more if you are interested.

So, the answer to

should I do the same here in this PR

is yes. Make sure we have some in our inputs to verify this behavior.

Looking good so far!

[nrmancuso]

@mahfouz72 I am going to place this PR into draft mode while you keep working :)

[mahfouz72]

Github, generate report

[mahfouz72]

Github, generate site

[github-actions]

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/8119c30_2024144719/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/8119c30_2024144719/checks/blocks/rightcurly.html#Description

[github-actions]

Report generation failed on phase "make_report",
step "Generate report".
Link: https://github.com/checkstyle/checkstyle/actions/runs/8215154947

[mahfouz72]

Github, generate report

[github-actions]

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/8119c30_2024174438/reports/diff/index.html

[mahfouz72]

Github, generate report

[github-actions]

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/bb961ec_2024223609/reports/diff/index.html

[mahfouz72]

Github, generate report

[nrmancuso]

@mahfouz72 let's try to make Checker happy and place @Nullable annotation on Details constructor arguments lcurly and rcurly. If this doesn't help, we can just update suppression file.

[github-actions]

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/bb961ec_2024002916/reports/diff/index.html

[mahfouz72]

Github, generate report

[mahfouz72]

If this doesn't help, we can just update suppression file.

this works but produced new survivals

<checkerFrameworkError unstable="false">
    <fileName>src/main/java/com/puppycrawl/tools/checkstyle/checks/blocks/RightCurlyCheck.java</fileName>
    <specifier>assignment</specifier>
    <message>incompatible types in assignment.</message>
    <lineContent>this.lcurly = lcurly;</lineContent>
    <details>
      found   : @Initialized @Nullable DetailAST
      required: @Initialized @NonNull DetailAST
    </details>
  </checkerFrameworkError>

  <checkerFrameworkError unstable="false">
    <fileName>src/main/java/com/puppycrawl/tools/checkstyle/checks/blocks/RightCurlyCheck.java</fileName>
    <specifier>assignment</specifier>
    <message>incompatible types in assignment.</message>
    <lineContent>this.rcurly = rcurly;</lineContent>
    <details>
      found   : @Initialized @Nullable DetailAST
      required: @Initialized @NonNull DetailAST
    </details>
  </checkerFrameworkError>

[github-actions]

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/bb961ec_2024075000/reports/diff/index.html

[mahfouz72]

@nrmancuso checker is falling even after adding suppression build successbut process exit with code 1 idk why. also Please Check PR description for report and site

[github-actions]

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/a77fb5d_2024132409/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/a77fb5d_2024132409/checks/blocks/rightcurly.html#Description

[github-actions]

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/a77fb5d_2024140817/reports/diff/index.html

[mahfouz72]

@rnveach ping

[rnveach]

I see production code changes after #14623 (comment) , so we need a new report.

[mahfouz72]

Github, generate report

[rnveach]

I think I am good with the PR. I want to review the latest report first.

[mahfouz72]

@rnveach here is the latest report
to ease review for you. I reviewed it first and all lgtm expect this I am not sure about it. the thing is that according to AST both LITERAL_CASE and LITERAL_DEFAULT are under the same parent (CASE_GROUP) and the { } are for both of them

        |       |   |--CASE_GROUP -> CASE_GROUP [6:14]
        |       |   |   |--LITERAL_CASE -> case [6:14]
        |       |   |   |   |--EXPR -> EXPR [6:19]
        |       |   |   |   |   `--NUM_INT -> 1 [6:19]
        |       |   |   |   `--COLON -> : [6:21]
        |       |   |   |--LITERAL_DEFAULT -> default [7:14]
        |       |   |   |   `--COLON -> : [7:21]
        |       |   |   `--SLIST -> SLIST [7:23]
        |       |   |       `--SLIST -> { [7:23]
        |       |   |           `--RCURLY -> } [8:14]

this will be more reasonable when we add support for default. I opened an issue for this and will add support for default after merging this PR

what do you think is it normal to validate this }? As it is technically a brace for the case, not the default only

[github-actions]

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/da58ea6_2024154338/reports/diff/index.html

[rnveach]

expect this I am not sure about it

How does LeftCurly handle this case if only LITERAL_CASE token is added? We are basing all the logic here off of duplicating Left's logic just for Rights.

[mahfouz72]

Github, generate report

[rnveach]

Please let me know what to expect from #14623 (comment) .

[mahfouz72]

leftCurlyCheck handles it differently. the braces in our example are not considered with case they are considered with the default only and this is correct IMO, So I made a small change to handle it as leftCurlyCheck does

for leftCurlyCheck when LITERAL_CASE only is added

case 0:
default: 
{               // no violation 
} ; 

so we should expect now after the last change. that this should have no violation

[github-actions]

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/ddf54a1_2024001633/reports/diff/index.html

[mahfouz72]

@rnveach report is good. The violation was removed from #14623 (comment) and there are no differences between the last report and the previous one rather than this case. I think we are good to go now

[rnveach]

@mahfouz72 Did you regenerate all 3 reports in #14623 (comment) ? I would want to see all 3 since this is a new token and it has many different options.

[mahfouz72]

Github, generate report

[github-actions]

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/ddf54a1_2024014514/reports/diff/index.html

[mahfouz72]

Github, generate report

[github-actions]

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/ddf54a1_2024024511/reports/diff/index.html

[mahfouz72]

@rnveach please check #14623 (comment) reports for all 3 options has been regenerated and all lgtm
I updated PR description it has the final reports now