Skip to content

Commit

Permalink
Issue #13553: False positive in FallThroughCheck on last case
Browse files Browse the repository at this point in the history
  • Loading branch information
Lmh-java committed Mar 30, 2024
1 parent e20f9e2 commit 06a1b46
Show file tree
Hide file tree
Showing 14 changed files with 231 additions and 44 deletions.
Expand Up @@ -1733,13 +1733,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
Expand Up @@ -42,9 +42,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}.
Expand Down Expand Up @@ -454,11 +454,25 @@ 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);
DetailAST nonCommentAst = getNextNonCommentAst(ast);
boolean hasReliefComment = false;

while (nonCommentAst != null) {
DetailAST previousSibling = nonCommentAst.getPreviousSibling();
final int prevLineNumber = previousSibling.getLineNo();

while (previousSibling != null && previousSibling.getLineNo() == prevLineNumber) {
final DetailAST firstChild = previousSibling.getFirstChild();
if (firstChild != null && reliefPattern.matcher(firstChild.getText()).find()) {
hasReliefComment = true;
break;
}
previousSibling = previousSibling.getPreviousSibling();
}
nonCommentAst = getNextNonCommentAst(nonCommentAst.getParent());
}

return hasReliefComment;
}

}
Expand Up @@ -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}.
Expand Down
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 @@ -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"),
Expand All @@ -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);
Expand All @@ -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);
}
}
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
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
@@ -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.'
}
}
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
@@ -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

}
}
}
}
@@ -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++;
}
}
}
}
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
Expand Up @@ -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:
Expand Down
6 changes: 3 additions & 3 deletions src/xdocs/checks/coding/fallthrough.xml
Expand Up @@ -22,8 +22,7 @@
&quot;fallthru&quot;, &quot;fall thru&quot;, &quot;fall-thru&quot;,
&quot;fallthrough&quot;, &quot;fall through&quot;, &quot;fall-through&quot;
&quot;fallsthrough&quot;, &quot;falls through&quot;, &quot;falls-through&quot; (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
<code>case</code> triggering the warning or on
the same line before the <code>case</code>
(ugly, but possible).
Expand Down Expand Up @@ -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:
Expand Down

0 comments on commit 06a1b46

Please sign in to comment.