Issue #13553: False positive in FallThroughCheck on last case

checkstyle · Apr 26, 2024 · 0d8f015 · 0d8f015
1 parent ef2f38a
commit 0d8f015
Show file tree

Hide file tree

Showing 15 changed files with 257 additions and 44 deletions.
diff --git a/config/checker-framework-suppressions/checker-lock-tainting-suppressions.xml b/config/checker-framework-suppressions/checker-lock-tainting-suppressions.xml
@@ -44,6 +44,36 @@
     </details>
   </checkerFrameworkError>
 
+  <checkerFrameworkError unstable="false">
+    <fileName>src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/FallThroughCheck.java</fileName>
+    <specifier>methodref.param</specifier>
+    <message>Incompatible parameter type for obj</message>
+    <lineContent>.filter(Objects::nonNull)</lineContent>
+    <details>
+      found   : @GuardSatisfied Object
+      required: @GuardedBy DetailAST
+      Consequence: method in @GuardedBy Objects
+      @GuardedBy boolean nonNull(@GuardSatisfied Object p0)
+      is not a valid method reference for method in @GuardedBy Predicate&lt;@GuardedBy DetailAST&gt;
+      @GuardedBy boolean test(@GuardedBy Predicate&lt;@GuardedBy DetailAST&gt; this, @GuardedBy DetailAST p0)
+    </details>
+  </checkerFrameworkError>
+
+  <checkerFrameworkError unstable="false">
+    <fileName>src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/FallThroughCheck.java</fileName>
+    <specifier>methodref.param</specifier>
+    <message>Incompatible parameter type for obj</message>
+    <lineContent>Objects::nonNull, DetailAST::getPreviousSibling)</lineContent>
+    <details>
+      found   : @GuardSatisfied Object
+      required: @GuardedBy DetailAST
+      Consequence: method in @GuardedBy Objects
+      @GuardedBy boolean nonNull(@GuardSatisfied Object p0)
+      is not a valid method reference for method in @GuardedBy Predicate&lt;@GuardedBy DetailAST&gt;
+      @GuardedBy boolean test(@GuardedBy Predicate&lt;@GuardedBy DetailAST&gt; this, @GuardedBy DetailAST p0)
+    </details>
+  </checkerFrameworkError>
+
   <checkerFrameworkError unstable="false">
     <fileName>src/main/java/com/puppycrawl/tools/checkstyle/Main.java</fileName>
     <specifier>method.guarantee.violated</specifier>

diff --git a/config/checker-framework-suppressions/checker-nullness-optional-interning-suppressions.xml b/config/checker-framework-suppressions/checker-nullness-optional-interning-suppressions.xml
@@ -1766,13 +1766,6 @@
     <lineContent>.isPresent();</lineContent>
   </checkerFrameworkError>
 
-  <checkerFrameworkError unstable="false">
-    <fileName>src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/FallThroughCheck.java</fileName>
-    <specifier>introduce.eliminate</specifier>
-    <message>It is bad style to create an Optional just to chain methods to get a non-optional value.</message>
-    <lineContent>.orElse(Boolean.FALSE);</lineContent>
-  </checkerFrameworkError>
-
   <checkerFrameworkError unstable="false">
     <fileName>src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/FinalLocalVariableCheck.java</fileName>
     <specifier>dereference.of.nullable</specifier>

diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/FallThroughCheck.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/FallThroughCheck.java
@@ -20,9 +20,11 @@
 package com.puppycrawl.tools.checkstyle.checks.coding;
 
 import java.util.HashSet;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.regex.Pattern;
+import java.util.stream.Stream;
 
 import com.puppycrawl.tools.checkstyle.StatelessCheck;
 import com.puppycrawl.tools.checkstyle.api.AbstractCheck;
@@ -42,9 +44,9 @@
  * "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 {@code case} triggering
- * the warning or on the same line before the {@code case}(ugly, but possible).
+ * The comment containing these words must be on the last non-empty line
+ * before the {@code case} triggering the warning or on the same line before
+ * the {@code case}(ugly, but possible).
  * </p>
  * <p>
  * Note: The check assumes that there is no unreachable code in the {@code case}.
@@ -454,11 +456,19 @@ private boolean hasFallThroughComment(DetailAST currentCase) {
      * @return true if relief comment found
      */
     private boolean hasReliefComment(DetailAST ast) {
-        return Optional.ofNullable(getNextNonCommentAst(ast))
-                .map(DetailAST::getPreviousSibling)
-                .map(previous -> previous.getFirstChild().getText())
-                .map(text -> reliefPattern.matcher(text).find())
-                .orElse(Boolean.FALSE);
+        final DetailAST nonCommentAst = getNextNonCommentAst(ast);
+        boolean result = false;
+        if (nonCommentAst != null) {
+            final int prevLineNumber = nonCommentAst.getPreviousSibling().getLineNo();
+            result = Stream.iterate(nonCommentAst.getPreviousSibling(),
+                            Objects::nonNull,
+                            DetailAST::getPreviousSibling)
+                    .takeWhile(sibling -> sibling.getLineNo() == prevLineNumber)
+                    .map(DetailAST::getFirstChild)
+                    .filter(Objects::nonNull)
+                    .anyMatch(firstChild -> reliefPattern.matcher(firstChild.getText()).find());
+        }
+        return result;
     }
 
 }
diff --git a/src/main/resources/com/puppycrawl/tools/checkstyle/meta/checks/coding/FallThroughCheck.xml b/src/main/resources/com/puppycrawl/tools/checkstyle/meta/checks/coding/FallThroughCheck.xml
@@ -15,9 +15,9 @@
  "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 {@code case} triggering
- the warning or on the same line before the {@code case}(ugly, but possible).
+ The comment containing these words must be on the last non-empty line
+ before the {@code case} triggering the warning or on the same line before
+ the {@code case}(ugly, but possible).
  &lt;/p&gt;
  &lt;p&gt;
  Note: The check assumes that there is no unreachable code in the {@code case}.

diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/FallThroughCheckTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/FallThroughCheckTest.java
@@ -252,7 +252,6 @@ public void testLastCase() throws Exception {
         final String[] expected = {
             "48:11: " + getCheckMessage(MSG_FALL_THROUGH_LAST),
             "83:11: " + getCheckMessage(MSG_FALL_THROUGH_LAST),
-            "112:11: " + getCheckMessage(MSG_FALL_THROUGH_LAST),
         };
         verifyWithInlineConfigParser(
                 getPath("InputFallThrough4.java"),
@@ -331,11 +330,7 @@ public void testFallThrough7() throws Exception {
     public void testLastLine() throws Exception {
         final String[] expected = {
             "21:13: " + getCheckMessage(MSG_FALL_THROUGH),
-            // until https://github.com/checkstyle/checkstyle/issues/13553
-            "33:13: " + getCheckMessage(MSG_FALL_THROUGH),
             "99:39: " + getCheckMessage(MSG_FALL_THROUGH_LAST),
-            // until https://github.com/checkstyle/checkstyle/issues/13553
-            "107:11: " + getCheckMessage(MSG_FALL_THROUGH_LAST),
         };
         verifyWithInlineConfigParser(
                 getPath("InputFallThroughLastLineCommentCheck.java"),
@@ -355,12 +350,7 @@ public void testLastLine2() throws Exception {
 
     @Test
     public void testReliefCommentBetweenMultipleComment() throws Exception {
-        final String[] expected = {
-            // until https://github.com/checkstyle/checkstyle/issues/13553
-            "25:17: " + getCheckMessage(MSG_FALL_THROUGH),
-            // until https://github.com/checkstyle/checkstyle/issues/13553
-            "34:13: " + getCheckMessage(MSG_FALL_THROUGH_LAST),
-        };
+        final String[] expected = {};
         verifyWithInlineConfigParser(
                 getPath("InputFallThrough8.java"),
                 expected);
@@ -383,12 +373,45 @@ public void testLabeledBreak() throws Exception {
 
     @Test
     public void testSwitchLabeledRules() throws Exception {
-        final String[] expected = {
+        final String[] expected = {};
+
+        verifyWithInlineConfigParser(
+                getNonCompilablePath("InputFallThroughSwitchRules.java"),
+                expected);
+    }
 
+    @Test
+    public void testInlineSingleCase() throws Exception {
+        final String[] expected = {
+            "12:17: " + getCheckMessage(MSG_FALL_THROUGH_LAST),
         };
 
         verifyWithInlineConfigParser(
-                getNonCompilablePath("InputFallThroughSwitchRules.java"),
+                getPath("InputFallThroughInlineSingleCase.java"),
+                expected);
+    }
+
+    @Test
+    public void testInlineMultipleComment() throws Exception {
+        final String[] expected = {};
+
+        verifyWithInlineConfigParser(
+                getPath("InputFallThroughMultipleReliefPatterns.java"),
+                expected);
+    }
+
+    @Test
+    public void testFallThroughWithoutReliefPattern() throws Exception {
+        final String[] expected = {
+            "21:9: " + getCheckMessage(MSG_FALL_THROUGH),
+            "45:9: " + getCheckMessage(MSG_FALL_THROUGH),
+            "54:9: " + getCheckMessage(MSG_FALL_THROUGH),
+            "60:9: " + getCheckMessage(MSG_FALL_THROUGH),
+            "77:9: " + getCheckMessage(MSG_FALL_THROUGH),
+            "94:9: " + getCheckMessage(MSG_FALL_THROUGH),
+        };
+        verifyWithInlineConfigParser(
+                getPath("InputFallThroughWithoutReliefPattern.java"),
                 expected);
     }
 }
diff --git a/...esources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThrough4.java b/...esources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThrough4.java
@@ -109,7 +109,7 @@ void method4(int i, int j, boolean cond) {
    void method5(int i, int j, boolean cond) {
       while (true) {
           switch (i){
-          case 5: // violation 'Fall\ through from the last branch of the switch statement'
+          case 5:
               i++;
               /* block */ /* fallthru */ // comment
           }

diff --git a/...esources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThrough8.java b/...esources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThrough8.java
@@ -22,7 +22,7 @@ void methodLastLine(int i) {
                 case 2:
                     i++;
                     /* comment */ /* fall thru */ /* comment */
-                case 3: // violation 'Fall\ through from previous branch of the switch statement'
+                case 3:
                     i--;
                     break;
             }
@@ -31,7 +31,7 @@ void methodLastLine(int i) {
 
     void testLastCase(int i) {
         switch (i) {
-            case 0: // violation 'Fall\ through from the last branch of the switch statement'
+            case 0:
                 i++;
                 /* comment */ /* fall thru */ /* comment */
         }

diff --git a/...ppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThroughInlineSingleCase.java b/...ppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThroughInlineSingleCase.java
@@ -0,0 +1,15 @@
+/*
+FallThrough
+checkLastCaseGroup = true
+reliefPattern = (default)falls?[ -]?thr(u|ough)
+
+
+*/
+package com.puppycrawl.tools.checkstyle.checks.coding.fallthrough;
+
+public class InputFallThroughInlineSingleCase{
+  void method(int a) {
+    switch (a) {case 1:;}
+    // violation above 'Fall\ through from the last branch of the switch statement.'
+  }
+}
diff --git a/...rawl/tools/checkstyle/checks/coding/fallthrough/InputFallThroughLastLineCommentCheck.java b/...rawl/tools/checkstyle/checks/coding/fallthrough/InputFallThroughLastLineCommentCheck.java
@@ -30,7 +30,7 @@ public void method2(int i) {
             case 1:
                 i++;
                 /* block */ /* fallthru */ // comment
-            case 2: // violation 'Fall\ through from previous branch of the switch statement'
+            case 2:
                 // this is comment
                 i++;
                 // fall through
@@ -104,7 +104,7 @@ void method6(String str) {
    void method7(int i, int j, boolean cond) {
       while (true) {
           switch (i){
-          case 5: // violation 'Fall\ through from the last branch of the switch statement'
+          case 5:
               i++;
               /* block */ i++; /* fallthru */ // comment
           }

diff --git a/...wl/tools/checkstyle/checks/coding/fallthrough/InputFallThroughMultipleReliefPatterns.java b/...wl/tools/checkstyle/checks/coding/fallthrough/InputFallThroughMultipleReliefPatterns.java
@@ -0,0 +1,26 @@
+/*
+FallThrough
+checkLastCaseGroup = true
+reliefPattern = (default)falls?[ -]?thr(u|ough)
+
+
+*/
+package com.puppycrawl.tools.checkstyle.checks.coding.fallthrough;
+
+public class InputFallThroughMultipleReliefPatterns {
+
+  void method(int i) {
+    while (true) {
+      switch (i) {
+        case 5: {
+          i++;
+        }
+        /* block */ /* fallthru */ // comment
+        case 6:
+          i++;
+          /* block */ /* fallthru */ // comment
+
+      }
+    }
+  }
+}