From 0d8f015e666e8bbbd3d059d4f0a0311bb6ec8be4 Mon Sep 17 00:00:00 2001 From: Minghao Li Date: Fri, 29 Mar 2024 02:05:41 -0700 Subject: [PATCH] Issue #13553: False positive in FallThroughCheck on last case --- .../checker-lock-tainting-suppressions.xml | 30 +++++ ...llness-optional-interning-suppressions.xml | 7 -- .../checks/coding/FallThroughCheck.java | 26 ++-- .../meta/checks/coding/FallThroughCheck.xml | 6 +- .../checks/coding/FallThroughCheckTest.java | 49 ++++++-- .../coding/fallthrough/InputFallThrough4.java | 2 +- .../coding/fallthrough/InputFallThrough8.java | 4 +- .../InputFallThroughInlineSingleCase.java | 15 +++ .../InputFallThroughLastLineCommentCheck.java | 4 +- ...nputFallThroughMultipleReliefPatterns.java | 26 ++++ .../InputFallThroughWithoutReliefPattern.java | 117 ++++++++++++++++++ .../coding/FallThroughCheckExamplesTest.java | 3 +- .../checks/coding/fallthrough/Example1.java | 3 +- src/xdocs/checks/coding/fallthrough.xml | 6 +- .../checks/coding/fallthrough.xml.template | 3 +- 15 files changed, 257 insertions(+), 44 deletions(-) create mode 100644 src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThroughInlineSingleCase.java create mode 100644 src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThroughMultipleReliefPatterns.java create mode 100644 src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThroughWithoutReliefPattern.java diff --git a/config/checker-framework-suppressions/checker-lock-tainting-suppressions.xml b/config/checker-framework-suppressions/checker-lock-tainting-suppressions.xml index f2f716dc335..98515577511 100644 --- a/config/checker-framework-suppressions/checker-lock-tainting-suppressions.xml +++ b/config/checker-framework-suppressions/checker-lock-tainting-suppressions.xml @@ -44,6 +44,36 @@ + + src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/FallThroughCheck.java + methodref.param + Incompatible parameter type for obj + .filter(Objects::nonNull) +
+ 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<@GuardedBy DetailAST> + @GuardedBy boolean test(@GuardedBy Predicate<@GuardedBy DetailAST> this, @GuardedBy DetailAST p0) +
+ + + + src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/FallThroughCheck.java + methodref.param + Incompatible parameter type for obj + Objects::nonNull, DetailAST::getPreviousSibling) +
+ 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<@GuardedBy DetailAST> + @GuardedBy boolean test(@GuardedBy Predicate<@GuardedBy DetailAST> this, @GuardedBy DetailAST p0) +
+ + src/main/java/com/puppycrawl/tools/checkstyle/Main.java method.guarantee.violated diff --git a/config/checker-framework-suppressions/checker-nullness-optional-interning-suppressions.xml b/config/checker-framework-suppressions/checker-nullness-optional-interning-suppressions.xml index 5604b4e3f15..45dd8b11028 100644 --- 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 @@ .isPresent(); - - src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/FallThroughCheck.java - introduce.eliminate - It is bad style to create an Optional just to chain methods to get a non-optional value. - .orElse(Boolean.FALSE); - - src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/FinalLocalVariableCheck.java dereference.of.nullable 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 index 5570a996c7a..0839fdc0a87 100644 --- 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). *

*

* 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 index 470ba3c9a73..e5c2bbcd0c8 100644 --- 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). </p> <p> 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 index 85cde1500c3..96ba8ddf518 100644 --- 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/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThrough4.java b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThrough4.java index 0c2c78d53b1..73e4773f2e4 100644 --- a/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThrough4.java +++ b/src/test/resources/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/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThrough8.java b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThrough8.java index 370d36096a7..fc2488e96dd 100644 --- a/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThrough8.java +++ b/src/test/resources/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/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThroughInlineSingleCase.java b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThroughInlineSingleCase.java new file mode 100644 index 00000000000..b11051cc86b --- /dev/null +++ b/src/test/resources/com/puppycrawl/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/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThroughLastLineCommentCheck.java b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThroughLastLineCommentCheck.java index bca74ffbf73..5b6caf71f50 100644 --- a/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThroughLastLineCommentCheck.java +++ b/src/test/resources/com/puppycrawl/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/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThroughMultipleReliefPatterns.java b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThroughMultipleReliefPatterns.java new file mode 100644 index 00000000000..0b522ba6949 --- /dev/null +++ b/src/test/resources/com/puppycrawl/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 + + } + } + } +} diff --git a/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThroughWithoutReliefPattern.java b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThroughWithoutReliefPattern.java new file mode 100644 index 00000000000..504437cae86 --- /dev/null +++ b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThroughWithoutReliefPattern.java @@ -0,0 +1,117 @@ +/* +FallThrough +checkLastCaseGroup = (default)false +reliefPattern = Continue with next case + + +*/ + +package com.puppycrawl.tools.checkstyle.checks.coding.fallthrough; + +public class InputFallThroughWithoutReliefPattern { + void method(int i, boolean cond) { + while (true) { + switch (i) { + case 0: + case 1: + i++; + break; + case 2: + i++; + case 3: // violation 'Fall\ through from previous branch of the switch statement.' + i++; + break; + case 4: + return; + case 5: + throw new RuntimeException(""); + case 6: + continue; + case 7: { + break; + } + case 8: { + return; + } + case 9: { + throw new RuntimeException(""); + } + case 10: { + continue; + } + case 11: { + i++; + } + case 12: // violation 'Fall\ through from previous branch of the switch statement.' + if (false) + break; + else + break; + case 13: + if (true) { + return; + } + case 14: // violation 'Fall\ through from previous branch of the switch statement.' + if (true) { + return; + } else { + //do nothing + } + case 15: // violation 'Fall\ through from previous branch of the switch statement.' + do { + System.identityHashCode("something"); + return; + } while (true); + case 16: + for (int j1 = 0; j1 < 10; j1++) { + String.valueOf("something"); + return; + } + case 17: + while (true) + throw new RuntimeException(""); + case 18: + while (cond) { + break; + } + case 19: // violation 'Fall\ through from previous branch of the switch statement.' + try { + i++; + break; + } catch (RuntimeException e) { + break; + } catch (Error e) { + return; + } + case 20: + try { + i++; + break; + } catch (RuntimeException e) { + } catch (Error e) { + return; + } + case 21: // violation 'Fall\ through from previous branch of the switch statement.' + try { + i++; + } catch (RuntimeException e) { + i--; + } finally { + break; + } + case 22: + try { + i++; + break; + } catch (RuntimeException e) { + i--; + break; + } finally { + i++; + } + default: + i++; + } + } + } +} diff --git a/src/xdocs-examples/java/com/puppycrawl/tools/checkstyle/checks/coding/FallThroughCheckExamplesTest.java b/src/xdocs-examples/java/com/puppycrawl/tools/checkstyle/checks/coding/FallThroughCheckExamplesTest.java index 8bc60f4d59c..81dd48fd662 100644 --- a/src/xdocs-examples/java/com/puppycrawl/tools/checkstyle/checks/coding/FallThroughCheckExamplesTest.java +++ b/src/xdocs-examples/java/com/puppycrawl/tools/checkstyle/checks/coding/FallThroughCheckExamplesTest.java @@ -36,8 +36,7 @@ protected String getPackageLocation() { @Test public void testExample1() throws Exception { final String[] expected = { - "21:9: " + getCheckMessage(MSG_FALL_THROUGH), - "32:9: " + getCheckMessage(MSG_FALL_THROUGH), + "33:9: " + getCheckMessage(MSG_FALL_THROUGH), }; verifyWithInlineConfigParser(getPath("Example1.java"), expected); diff --git a/src/xdocs-examples/resources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/Example1.java b/src/xdocs-examples/resources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/Example1.java index c173b75a818..5dc07622645 100644 --- a/src/xdocs-examples/resources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/Example1.java +++ b/src/xdocs-examples/resources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/Example1.java @@ -18,7 +18,8 @@ public void foo() throws Exception { switch (i) { case 1: i++; - case 2: // violation 'Fall\ through from previous branch of the switch' + /* block */ /* fallthru */ // comment + case 2: // ok, ReliefPattern is present in above line. i++; break; case 3: diff --git a/src/xdocs/checks/coding/fallthrough.xml b/src/xdocs/checks/coding/fallthrough.xml index 8c5afd1a682..ba446d034d3 100644 --- a/src/xdocs/checks/coding/fallthrough.xml +++ b/src/xdocs/checks/coding/fallthrough.xml @@ -22,8 +22,7 @@ "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 + The comment containing these words 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). @@ -84,7 +83,8 @@ class Example1 { switch (i) { case 1: i++; - case 2: // violation 'Fall\ through from previous branch of the switch' + /* block */ /* fallthru */ // comment + case 2: // ok, ReliefPattern is present in above line. i++; break; case 3: diff --git a/src/xdocs/checks/coding/fallthrough.xml.template b/src/xdocs/checks/coding/fallthrough.xml.template index a2bfc88d8fa..7a5c1087716 100644 --- a/src/xdocs/checks/coding/fallthrough.xml.template +++ b/src/xdocs/checks/coding/fallthrough.xml.template @@ -22,8 +22,7 @@ "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 + The comment containing these words 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).