Issue #13086: fix InnerAssignmentCheck In SwitchRuleAndExpression

[mahfouz72]

Resolves #13086 :

Diff Regression config: https://gist.githubusercontent.com/mahfouz72/70e0fb4b76668949fd9e0fd3744deddd/raw/0b035184445fee1009e49543af7b1daa6b52f79f/innerass.xml

Diff Regression projects: https://gist.githubusercontent.com/mahfouz72/297d7353d2d0bcec178e1b098109cd9e/raw/546dcdbdff363110fa265ea80d70e36f079c6ee8/projects-to-test-on.properties

[mahfouz72]

Github, generate report

[mahfouz72]

Github, generate report

[github-actions]

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

[mahfouz72]

report is good

[romani]

Question

[nrmancuso]

Please review documentation to see if we need to update there as well.

Item:

[mahfouz72]

Github, generate site

[github-actions]

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

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/32b2013_2024154039/checks/coding/innerassignment.html#Example1-code

[nrmancuso]

@mahfouz72 please generate a regression report against openjdk21

[mahfouz72]

Github, generate report

[github-actions]

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

[mahfouz72]

Github, generate report

[github-actions]

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

[romani]

Ok to merge

[rnveach]

There is not really any descriptions update in this PR.
https://checkstyle.org/checks/coding/innerassignment.html#Description

Except for the loop idioms, all assignments should occur in their own top-level statement to increase readability.

Is this wording technically not correct anymore? I wouldn't really consider cases to be top level statements.

[rnveach]

So why does this create violations but something like test 2 doesn't? Is this because it is basically nested inside an assignment already?

Is there somewhere (official?) that shows what inner assignments are in this case?

What if we had a case like (also add it):

System.out.println(switch (mode) {
            case 2 -> {
                yield x = 2;

This wouldn't be an violation?

[mahfouz72]

So why does this create violations but something like test 2 doesn't

because this is an assignment inside expressions ( switch expressions ), while test2 is an assignment inside normal switch statements

Is there somewhere (official?)

I didn't find any but what I know from our docs is that inner assignments are the assignments inside expressions. So basically the assignments inside switch expressions x = switch(){....} should be avoided

What if we had a case like (also add it):

Yes, violation and added

[nrmancuso]

Is there somewhere (official?) that shows what inner assignments are in this case?

Doubt it. A better way to describe what this check does is "place violations (mostly) anywhere that assigned values are used directly". Ideally, we should only do one thing at a time; assign a value, or use it, not both.

[rnveach]

yield x = 2;

Just to confirm, since I am not use to this syntax, is this the same as x = 2; yield x;?
If we had y = 2 before this line would that be a violation too?

@nrmancuso @romani Are we ok that test8 is a violation for this check?

[mahfouz72]

Just to confirm, since I am not use to this syntax, is this the same as x = 2; yield x;?

Yes I think So

If we had y = 2 before this line would that be a violation too?

this will have no violation due to

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/InnerAssignmentCheck.java

Line 94 in 405db25

{TokenTypes.EXPR, TokenTypes.SLIST},

I am not sure if this is the correct behavior. but I assume it is valid because we can assign a value inside the case block but also without using this assigned value at the same time, so we are not violating this

we should only do one thing at a time; assign a value, or use it, not both.

example

System.out.println(switch (x) {
            case 1 -> x = 1;   // violation
            case 2 -> {
                  y = 2;   // assign only but not used
                 yield x = 2;      // violation
            }

this is not the case when we have something like

System.out.println(switch (x) {
            case 1 -> x = 1;   // violation
            case 2 -> {
                 yield x = 2;      // violation
            }

here in both cases, we assigned a value and and used it directly in the print

[nrmancuso]

@nrmancuso @romani Are we ok that test8 is a violation for this check?

Yes, assigned values are used directly in all three cases. I encourage everyone involved here to practice using Java 21 language features to understand how they work.

[romani]

Yes, I am ok with test8 to be violation .
Old switch structure always did side effect to outside variables.
Switch like expression, that return single result, should not do side effects (changing outside of switch variables).

I reviewed one more time this PR, I am still ok to merge.

[rnveach]

Diff Regression projects: https://gist.githubusercontent.com/mahfouz72/297d7353d2d0bcec178e1b098109cd9e/raw/546dcdbdff363110fa265ea80d70e36f079c6ee8/projects-to-test-on.properties

We normally provide regression on all projects. Even if no others have this form of code, it will show this doesn't impact older code. Please update the file.

[mahfouz72]

We normally provide regression on all projects. Even if no others have this form of code, it will show this doesn't impact older code. Please update the file.

Yes this is only to generate another report I generated two

all projects: #14804 (comment)
java 21: #14804 (comment)

[mahfouz72]

Is this wording technically not correct anymore?

I don't see anything wrong. Can you please point to an example that you think violates this wording

x = switch(..){ case 1 -> x = 3} (assignment is not in its own top level (nested inside another assignment) so violation
switch(..){ case 1 -> x = 3} ( I guess this is considered as a top-level so no violation )

[nrmancuso]

There is not really any descriptions update in this PR. https://checkstyle.org/checks/coding/innerassignment.html#Description

Except for the loop idioms, all assignments should occur in their own top-level statement to increase readability.

Is this wording technically not correct anymore? I wouldn't really consider cases to be top level statements.

I think this still holds for switch expressions; the only assignment happening should be to a variable declared outside(top level) of the switch expression, from a value yielded by the switch expression.

[rnveach]

I think I need some clarification then on what is a "top level statement" is.

I tried google. The first thing said Top-level statements allows you to write executable code directly at the root of a file, eliminating the need for wrapping your code in a class or method. . However that seems to be a C# link. Then I got a reddit for Java help where someone replied to the poster to clarify this exact statement and it was never answered. Is there someplace official that has the exact definition?

I always considered this check to mean something like "the statement must only contain the assignment itself", but then the whole switch is considered a single statement (unless I have the Java grammar wrong).

[romani]

but then the whole switch is considered a single statement (unless I have the Java grammar wrong).

It is probably very confusing to consider switch expressions as statement. It better to think of this like inlined function in such cases.

Defect that we have is that we report violation on switch that statement like.
For now users are ok with our decision that assignments in expression like structures should be violation, so let's keep it as is, we are ok to reconsider this is user come up with good reasons later on.

[rnveach]

I am not calling into question what we print out as violations. My whole comments was the documentation and is it clear enough on what we say this check does.

[romani]

https://checkstyle.org/checks/coding/innerassignment.html#Description

I don't know how to improve our doc, we have have not very strict target of validation, and nice exception structure that is ignored.
This should not block us to merge this PR. One day in future some contributor will share good wording.