VariableDeclarationUsageDistance: false negative when processing SWITCH_RULE

[stoyanK7]

I have read check documentation: https://checkstyle.org/config_coding.html#VariableDeclarationUsageDistance
I have downloaded the latest checkstyle from: https://checkstyle.org/cmdline.html#Download_and_Run
I have executed the cli and showed it below, as cli describes the problem better than 1,000 words

Discovered while working on #12052.

stoyan@ZenBook:~/Desktop/checkstyle$ cat config.xml 

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC "-//Puppy Crawl//DTD Check Configuration 1.3//EN" 
     "https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">
  <module name="TreeWalker">
    <module name="VariableDeclarationUsageDistance">
      <property name="validateBetweenScopes" value="true"/>
      <property name="allowedDistance" value="1"/>
    </module>
  </module>
</module>

stoyan@ZenBook:~/Desktop/checkstyle$ cat Test.java 

public class Test {
    
    int meth1(int a) {
        int method1Variable = 0;
        return switch(a) {
            case 1 -> method1Variable; // this should be ok
        };
    }

    int meth2(int a) {
        int method2Variable = 0;
        return switch(a) {
            case 2 -> {
                yield method2Variable; // this should be ok
            }
        };
    }

    int meth3(int a) {
        int method3Variable = 0;
        return switch(a) {
            case 3 -> {
                a++;
                yield a + method3Variable; // this should raise violation
            }
        };
    }


    int meth4(int a) {
        int method4Variable = 0;
        return switch(a) {
            case 4:
                yield method4Variable; // this should be ok
        }; 
    }

    int meth5(int a) {
        int method5Variable = 0;
        return switch(a) {
            case 5:
                a++;
                yield a + method5Variable; // this should raise violation
        };
    }

    int meth6(int a) {
        int method6Variable = 0;
        return switch(a) {
            case 1 -> method6Variable; // this should be ok
            case 2 -> {
                yield method6Variable; // this should be ok
            }
            case 3 -> {
                a++;
                yield a + method6Variable; // this should raise violation
            }
        };
    }

    int meth7(int a) {
        int method7Variable = 0;
        return switch(a) {
            case 4:
                yield method7Variable; // this should be ok
            case 5:
                a++;
                yield a + method7Variable; // this should raise violation
        };
    }

}

Current output

stoyan@ZenBook:~/Desktop/checkstyle$ RUN_LOCALE="-Duser.language=en -Duser.country=US"
stoyan@ZenBook:~/Desktop/checkstyle$ java $RUN_LOCALE -jar checkstyle-10.3.2-all.jar -c config.xml Test.java 
Starting audit...
Audit done.

Expected output

stoyan@ZenBook:~/Desktop/checkstyle$ RUN_LOCALE="-Duser.language=en -Duser.country=US"
stoyan@ZenBook:~/Desktop/checkstyle$ java $RUN_LOCALE -jar checkstyle-10.3.2-all.jar -c config.xml Test.java 
Starting audit...
[ERROR] /home/stoyan/Desktop/checkstyle/Test.java:20:9: Distance between variable 'method3Variable' declaration and its first usage is 2, but allowed 1.  Consider making that variable final if you still need to store its value in advance (before method calls that might have side effects on the original value). [VariableDeclarationUsageDistance]
[ERROR] /home/stoyan/Desktop/checkstyle/Test.java:39:9: Distance between variable 'method5Variable' declaration and its first usage is 2, but allowed 1.  Consider making that variable final if you still need to store its value in advance (before method calls that might have side effects on the original value). [VariableDeclarationUsageDistance]
[ERROR] /home/stoyan/Desktop/checkstyle/Test.java:48:9: Distance between variable 'method6Variable' declaration and its first usage is 2, but allowed 1.  Consider making that variable final if you still need to store its value in advance (before method calls that might have side effects on the original value). [VariableDeclarationUsageDistance]
[ERROR] /home/stoyan/Desktop/checkstyle/Test.java:62:9: Distance between variable 'method7Variable' declaration and its first usage is 2, but allowed 1.  Consider making that variable final if you still need to store its value in advance (before method calls that might have side effects on the original value). [VariableDeclarationUsageDistance]
Audit done.
Checkstyle ends with 1 errors.

---

I believe this issue needs to be resolved before killing the surviving mutation in #12052. A test case already exists for CASE_GROUP that kills a mutation similar to the surviving one:

checkstyle/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/variabledeclarationusagedistance/InputVariableDeclarationUsageDistanceGeneral.java

Lines 299 to 305 in 83c91b1

	int sw; // violation
	switch (i) {
	case 0:
	k++;
	sw = 0; // DECLARATION OF VARIABLE 'sw' SHOULD BE HERE (distance = 2)
	break;
	case 1:

[nrmancuso]

@stoyanK7 please add to your issue description actual/ expected for the following:

1. Switch expressions with multiple cases, with both switch rules and switch labels
2. Switch statements with single/ multiple cases, with both switch rules and switch labels

[stoyanK7]

@nick-mancuso Added more examples. I hope this is what you meant. I have not added examples of switch statements because they are handled properly by the check as shown in the test above. The problem is with switch expressions in particular.

[nrmancuso]

I hope this is what you meant. I have not added examples of switch statements because they are handled properly by the check as shown in the test above.

Prove via CLI output that switch statement behavior is what you expect with variable initialization outside of the switch statement.

[stoyanK7]

@nick-mancuso

stoyan@ZenBook:~/Desktop/checkstyle$ cat config.xml

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC "-//Puppy Crawl//DTD Check Configuration 1.3//EN" "https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">
  <module name="TreeWalker">
    <module name="VariableDeclarationUsageDistance">
      <property name="validateBetweenScopes" value="true"/>
      <property name="allowedDistance" value="1"/>
    </module>
  </module>
</module>

stoyan@ZenBook:~/Desktop/checkstyle$ cat Test1.java 

class Test {
    void switchExample(int x) {
        int variable = 0;
        switch(x) {
            case 1:
                x++;
                x++;
                x++;
                x++;
                variable++;
                break;
        }
    }
}

stoyan@ZenBook:~/Desktop/checkstyle$RUN_LOCALE="-Duser.language=en -Duser.country=US"
stoyan@ZenBook:~/Desktop/checkstyle$ java $RUN_LOCALE -jar checkstyle-10.3.2-all.jar -c config.xml Test1.java 
Starting audit...
[ERROR] /home/stoyan/Desktop/checkstyle/Test1.java:3:9: Distance between variable 'variable' declaration and its first usage is 5, but allowed 1.  Consider making that variable final if you still need to store its value in advance (before method calls that might have side effects on the original value). [VariableDeclarationUsageDistance]
Audit done.
Checkstyle ends with 1 errors.

[nrmancuso]

                x++;
                x++;
                x++;
                x++;

@stoyanK7 why did we extend original example?

➜  java git:(master) ✗ cat config.xml                                           
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC "-//Puppy Crawl//DTD Check Configuration 1.3//EN" "https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">
    <module name="TreeWalker">
        <module name="VariableDeclarationUsageDistance">
            <property name="validateBetweenScopes" value="true"/>
            <property name="allowedDistance" value="1"/>
        </module>
    </module>
</module>                                                                                                    

➜  java git:(master) ✗ cat Test.java                                            
public class Test {
    int m1(int x) {
        int y = 5; // if you expect a violation here
        return switch (x) {
            case 0:
                x++;
                yield x + y;
            default: yield 2;
        };
    }

    int m2(int x) {
        int y = 5; // why shouldn't we expect one here?
        switch (x) {
            case 0:
                x++;
                y = x + y;
                break;
            default:
                y = 2;
        }
        return y;
    }
}                                                                                                             

➜  java git:(master) 

✗ javac Test.java      
                                      
➜  java git:(master) ✗ java -jar checkstyle-10.3.1-all.jar -c config.xml Test.java
Starting audit...
Audit done.

[stoyanK7]

@nick-mancuso

Alright, this now seems more messed up than before.
Turns out this would not raise a violation

    int m2(int x) {
        int y = 5; // no violation
        switch (x) {
            case 0:
                x++;
                y = 0;
                break;
        }
        return y;
    }

But if we remove y from the return, it will

    int m2(int x) {
        int y = 5; // violation here
        switch (x) {
            case 0:
                x++;
                y = 0;
                break;
        }
        return 5;
    }

This behavior is not expected, right?

[nrmancuso]

This behavior is not expected, right?

I think it makes sense to not violate variable if we use it after whatever the "inner scope" is. But - I wonder if this is by accident or design; I am not sure if this logic is included in the check. It looks like we need to do some investigation to identify what the real issue here is. Please continue to test different constructs (loops, static blocks, etc.) and variable usage before/after "inner scope" and refine the PR description as you discover real issue.

[romani]

I think it makes sense to not violate variable if we use it after whatever the "inner scope" is.

yes. I pretty sure it is by design as scopes significantly increase complexity of this validation. If there is usage after nested scope, we can not do violation on variable.

lets check on code like:

int a = 0;
if () {
  do();
  do();
  do();
  a++;
}

and

int a = 0;
if () {
  do();
  do();
  do();
  a++;
}
a++;

on second there should be no violation, so first case might also fall in this pattern. Can you double check by CLI ?

May be it is good extension in validaiton, but it is more like problem of reducing scope/visibility of variable rather then Distance.
so it might be goal for different Check.

[stoyanK7]

@romani apparently, both are violation

stoyan@ZenBook:~/Desktop/checkstyle$ cat config.xml 

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC "-//Puppy Crawl//DTD Check Configuration 1.3//EN" 
   "https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">
  <module name="TreeWalker">
    <module name="VariableDeclarationUsageDistance">
      <property name="validateBetweenScopes" value="true"/>
      <property name="allowedDistance" value="1"/>
    </module>
  </module>
</module>

stoyan@ZenBook:~/Desktop/checkstyle$ cat Test1.java 

class Test1 {
    void doo() { }
    void example1() {
        int a = 0;
        if (true) {
            doo();
            doo();
            doo();
            a++;
        }
    }

    void example2() {
        int a = 0;
        if (true) {
            doo();
            doo();
            doo();
            a++;
        }
    }
}

stoyan@ZenBook:~/Desktop/checkstyle$ java -jar checkstyle-10.3.2-all.jar -c config.xml Test1.java

Starting audit...
[ERROR] /home/stoyan/Desktop/checkstyle/Test1.java:4:9: Distance between variable 'a' declaration and its first usage is 4, but allowed 1.  Consider making that variable final if you still need to store its value in advance (before method calls that might have side effects on the original value). [VariableDeclarationUsageDistance]
[ERROR] /home/stoyan/Desktop/checkstyle/Test1.java:14:9: Distance between variable 'a' declaration and its first usage is 4, but allowed 1.  Consider making that variable final if you still need to store its value in advance (before method calls that might have side effects on the original value). [VariableDeclarationUsageDistance]
Audit done.
Checkstyle ends with 2 errors.

[romani]

you did mistake in Input, that is why, see a difference:

$ cat config.xml 
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
   "https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">
  <module name="TreeWalker">
    <module name="VariableDeclarationUsageDistance">
      <property name="validateBetweenScopes" value="true"/>
      <property name="allowedDistance" value="1"/>
    </module>
  </module>
</module>

$ cat Test.java 
class Test {
    void doo() { }
    void example1() {
        int a = 0;
        if (true) {
            doo();
            doo();
            doo();
            a++;
        }
    }

    void example2() {
        int a = 0;
        if (true) {
            doo();
            doo();
            doo();
            a++;
        }
        a++;
    }
}

$ java -classpath checkstyle-10.2-all.jar \
     com.puppycrawl.tools.checkstyle.Main         -c config.xml Test.java 
Starting audit...
[ERROR] /var/tmp/Test.java:4:9: Distance between variable 'a' declaration 
and its first usage is 4, but allowed 1.  Consider making that variable final if
 you still need to store its value in advance (before method calls that might have 
side effects on the original value). [VariableDeclarationUsageDistance]
Audit done.
Checkstyle ends with 1 errors.

So logic is smart to handle scopes already for IF, so any other scope should be handled kind of the same.
So according to this logic, all cases in your Inputs in issue descrption should be violations.