Skip to content

Commit

Permalink
Issue checkstyle#13553: Finished false positive in FallThroughCheck o…
Browse files Browse the repository at this point in the history
…n last case
  • Loading branch information
SteLeo1602 authored and romani committed Sep 8, 2024
1 parent 27b7ee9 commit e3d5e59
Show file tree
Hide file tree
Showing 15 changed files with 248 additions and 34 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,31 @@
</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>type.arguments.not.inferred</specifier>
<message>Could not infer type arguments for Stream.iterate</message>
<lineContent>result = Stream.iterate(nonCommentAst.getPreviousSibling(),</lineContent>
<details>
unsatisfiable constraint: @GuardedBy DetailAST &lt;: @GuardSatisfied Object
</details>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<fileName>src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/ModifiedControlVariableCheck.java</fileName>
<specifier>methodref.param</specifier>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1878,13 +1878,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>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -45,6 +47,7 @@
* 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).
* Any other comment may follow on the same line.
* </p>
* <p>
* Note: The check assumes that there is no unreachable code in the {@code case}.
Expand Down Expand Up @@ -454,11 +457,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;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
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).
Any other comment may follow on the same line.
&lt;/p&gt;
&lt;p&gt;
Note: The check assumes that there is no unreachable code in the {@code case}.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down Expand Up @@ -332,10 +331,8 @@ 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"),
Expand All @@ -355,12 +352,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);
Expand All @@ -383,12 +375,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);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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 */
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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.'
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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

}
}
}
}
Original file line number Diff line number Diff line change
@@ -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++;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Loading

0 comments on commit e3d5e59

Please sign in to comment.