False positive in FallThroughCheck on last case

[Kevin222004]

I have read check documentation: https://checkstyle.org/checks/coding/fallthrough.html#FallThrough
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

---

Config

kevin@inspiron-15-5510:~/Desktop/check_style$ cat config.xml 
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN" 
      "https://checkstyle.org/dtds/configuration_1_3.dtd">

<module name="Checker">
    <module name="TreeWalker">
        <module name="FallThrough">
            <property name="checkLastCaseGroup" value="true"/>
        </module>
    </module>
</module>

---

code

kevin@inspiron-15-5510:~/Desktop/check_style$ cat Test.java
class Test {
   void method5(int i, int j, boolean cond) {
      while (true) {
          switch (i){
          case 5:
               i++;
               /* block */ /* fallthru */ // comment
           }
       }
    }
}

---

output

$ java -jar checkstyle-10.12.4-all.jar -c config.xml Test.java
Starting audit...
[ERROR] /var/tmp/Test.java:5:11: Fall through from the last branch of the switch statement. [FallThrough]
Audit done.
Checkstyle ends with 1 errors.

---

Expected

No violation

Update in doc to remove:

The comment containing these words must be all on one line

[AayushSaini101]

I am on it

[AayushSaini101]

@romani @Kevin222004 So we have to resolve that issue: when only one case in switch and checklist property is set true and check style must not return any error? Is it

[romani]

Cases in switch can be few.
But last case should not be highlighted with violation.

[AayushSaini101]

Cases in switch can be few. But last case should not be highlighted with violation.

@romani can you please highlight this better ? The checklastgroup property is used to check the last group but according to you we need to remove the violation for the last group that is check then what is need to of checklastgroup property?

[Kevin222004]

@AayushSaini101
https://checkstyle.sourceforge.io/checks/coding/fallthrough.html#FallThrough:~:text=The%20check%20honors,ugly%2C%20but%20possible).
We have to remove that violation because a relief comment(fallthru) is there. if we remove that comment then only we expect the violation.
It is not only for last case group you have to do it for all case
For same config in issue description

kevin@kevin-Inspiron-15-5510:~/Desktop/check_style$ cat -n Test.java 
     1  class Test {
     2      void method5(int i, int j, boolean cond) {
     3         while (true) {
     4             switch (i){
     5             case 5:
     6                  i++;
     7                  /* block */ /* fallthru */ // comment
     8              case 3:
     9              i++;
    10              /* block */ /* fallthru */ // comment
    11          }
    12          }
    13       }
    14   }kevin@kevin-Inspiron-15-5510:~/Desktop/check_style$ java -jar /home/kevin/Downloads/checkstyle-10.12.4-all.jar -c config.xml Test.java
Starting audit...
[ERROR] /home/kevin/Desktop/check_style/Test.java:8:13: Fall through from previous branch of the switch statement. [FallThrough]
[ERROR] /home/kevin/Desktop/check_style/Test.java:8:13: Fall through from the last branch of the switch statement. [FallThrough]
Audit done.
Checkstyle ends with 2 errors.
kevin@kevin-Inspiron-15-5510:~/Desktop/check_style$ 

Both the violation needs to remove.

[rnveach]

I need a reminder why the violation needs to be removed.

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

The comment containing these words must be all on one line, and must be on the last non-empty line before the case triggering the warning or on the same line before the case (ugly, but possible).

So we are saying the suppression comment can have another comment before and after it on the same line as long as the other criteria matches? If so, we should add this to the documentation with an example.

Both the violation needs to remove.

Isn't that means this issue is a false positive, and not a negative as it is titled.

[Kevin222004]

@rnveach This issue is opened during #12966
So before that when the check is non-ast based like before the release of 10.2.4 https://checkstyle.org/releasenotes.html#:~:text=Fix%20existing%20cases%20of%20%60getFileContents()%60%20usage.%20Author%3A%20Kevin222004%20%2311166 in older version we are not throwing any violation

For example in 10.12.2 or 10.2.3
for same config in issue and for code

     1  class Test {
     2      void method5(int i, int j, boolean cond) {
     3         while (true) {
     4             switch (i){
     5             case 5:
     6                  i++;
     7                  /* block */ /* fallthru */ // comment
     8              case 3:
     9              i++;
    10              /* block */ /* fallthru */ // comment
    11          }
    12          }
    13       }
    14   }

for version 10.2.3 and 10.2.2

kevin@kevin-Inspiron-15-5510:~/Desktop/check_style$ java -jar /home/kevin/Downloads/checkstyle-10.12.3-all.jar -c config.xml Test.java
Starting audit...
Audit done.
kevin@kevin-Inspiron-15-5510:~/Desktop/check_style$ java -jar /home/kevin/Downloads/checkstyle-10.12.2-all.jar -c config.xml Test.java
Starting audit...
Audit done.
kevin@kevin-Inspiron-15-5510:~/Desktop/check_style$ 

for 10.2.4

kevin@kevin-Inspiron-15-5510:~/Desktop/check_style$ java -jar /home/kevin/Downloads/checkstyle-10.12.4-all.jar -c config.xml Test.java
Starting audit...
[ERROR] /home/kevin/Desktop/check_style/Test.java:8:13: Fall through from previous branch of the switch statement. [FallThrough]
[ERROR] /home/kevin/Desktop/check_style/Test.java:8:13: Fall through from the last branch of the switch statement. [FallThrough]
Audit done.
Checkstyle ends with 2 errors.
kevin@kevin-Inspiron-15-5510:~/Desktop/check_style$ 

So the issue is opened becuase of this changes.

---

After again looking on docs

must be on the last non-empty line before the case triggering the warning or on the same line before the case (ugly, but possible)

Yes this is actually not an issue. it is as similar as docs.

@rnveach I request you to provide a feedback in #13545

[rnveach]

Docs don't clear say no other comments can be before or after. With this specific declaration missing, this issue can go either way and is a call for @romani to decide. Either documentation is missing saying that this is not an issue , or it is an issue and both code and documentation needs to be updated.

I remember the PR to change the code, but I don't remember how we came to this issue.

[AayushSaini101]

Docs don't clear say no other comments can be before or after. With this specific declaration missing, this issue can go either way and is a call for @romani to decide. Either documentation is missing saying that this is not an issue , or it is an issue and both code and documentation needs to be updated.

I remember the PR to change the code, but I don't remember how we came to this issue.

@romani Can you please check this ? Should i start on this issue

[AayushSaini101]

I am on it

[rnveach]

@romani Please provide clarity on which way this issue is going. Also, is this going to be the default or with an option?

[romani]

from doc:

The check honors special comments to suppress the warning. By default, the texts "fallthru", "fall thru", "fall-thru", "fallthrough", "fall through", "fall-through" "fallsthrough", "falls through", "falls-through" (case-sensitive). The comment containing these words must be all on one line, and must be on the last non-empty line before the case triggering the warning or on the same line before the case (ugly, but possible).

specifically:

The comment containing these words must be all on one line

this limitation is from time when we had no ability to deal with comments except for loading whole file again as text and get line content by line number. After migration to AST this is not a problem anymore. So I am ok to remove it limitation from doc.

and must be on the last non-empty line before the case triggering the warning

this requirement is ok to make code looks like:

          switch (i){
            case 4:
                 i++;
                 /* fallthru */
            case 5:
                 i++;
                 /* block */ /* fallthru */ // comment
           }

where comment is like "statement" that do move execution point to end of switch (break/continue/.....) but in fact explicit declaration of continue to execute.

I am confirming that issue declare expected behavior AND doc needs to be updated to remove extra limitation.
I removing aprooved label to let @rnveach confirm that logic is good.

[rnveach]

@rnveach confirm that logic is good.

I am fine with your decision.

[romani]

Issue description is updated to mention update of doc.

[romani]

@AayushSaini101, issue is ready to be fixed.

[AayushSaini101]

Thanks @romani

[AayushSaini101]

@romani So Multiline comments are also allowed in that case like

void method5(int i, int j, boolean cond) {
      while (true) {
          switch (i){
          case 5:
               i++;
                // fallthru 
          case 6:
               i++;
               /* 
                 continue fallthru
               */

               

           }
       }
    }

Output:
[ERROR] /Users/aaayush/Desktop/GFG/Main.java:9:11: Fall through from the last branch of the switch statement. [FallThrough]
Audit done.
Excepted:
No violation

Is this correct ?

[romani]

We need that whole comment is matching our word.

continue fallthru is not matching. We do this because our keyword can part of some explanation of code, not a reason of case falll through.

[AayushSaini101]

@romani Is this violation correct, After removing the limitation

public void method(int i) {
      switch (i) {
          case 1:
              if (true) {
              }
              // comment
              // fall through
              // check
          case 2: // violation 'Fall\ through from previous branch of the switch statement'
              break;
          case 3:
              break;
      

[romani]

Yes, as allowance comment is not the last comment before next case.

[romani]

code in #14016 can be reused to finish this issue, majority of work is done, averyone is welcome to finish this work

[Lmh-java]

I am on this