Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue #13553: False positive in FallThroughCheck on last case #14734

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Issue #13553: False positive in FallThroughCheck on last case #14734

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 30 additions & 0 deletions config/checker-framework-suppressions/checker-lock-tainting-suppressions.xml
View file
Open in desktop
Expand Up @@ -452,6 +452,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,</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/ModifiedControlVariableCheck.java</fileName>
<specifier>methodref.param</specifier>
Expand Down
7 changes: 0 additions & 7 deletions config/checker-framework-suppressions/checker-nullness-optional-interning-suppressions.xml
View file
Open in desktop
Expand Up @@ -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>
Expand Down
26 changes: 18 additions & 8 deletions src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/FallThroughCheck.java
View file
Open in desktop
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 @@ -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}.
Expand Down Expand Up @@ -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);
rnveach marked this conversation as resolved.
Show resolved Hide resolved
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;
}

}
6 changes: 3 additions & 3 deletions src/main/resources/com/puppycrawl/tools/checkstyle/meta/checks/coding/FallThroughCheck.xml
View file
Open in desktop
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).
Copy link
Contributor Author

@Lmh-java Lmh-java Mar 30, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the sentence in the doc as required

Copy link
Member

@rnveach rnveach Apr 20, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update in doc to remove:

The comment containing these words must be all on one line

I don't remember this conversation regarding this but does this mean if I have a suppression word as "suppress me", it will match things like "suppress \n me"? Do you have a test example showing this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your updated documentation says:

...on the same line before the ...

How is that different then ...must be all on one line...?

Copy link
Contributor Author

@Lmh-java Lmh-java Apr 26, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update in doc to remove:

The comment containing these words must be all on one line

I don't remember this conversation regarding this but does this mean if I have a suppression word as "suppress me", it will match things like "suppress \n me"? Do you have a test example showing this?

No, I don't think "suppress \n me" will match.
This is matching by the pattern

    private Pattern reliefPattern = Pattern.compile("falls?[ -]?thr(u|ough)");

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your updated documentation says:

...on the same line before the ...

How is that different then ...must be all on one line...?

I didn't see the conversion about this part either, but I think "must be all on one line" is redundant and confusing. I don't really understand what it means. The subject of this sentence is "the comment containing these words". However, we only need one "comment" containing these words to suppress this check for that branch.

If we remove this "must be all on one line.", and updated it to

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).

I think this might be able to explain everything?

Copy link
Member

@rnveach rnveach May 12, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like comment in issue came from #12966 (comment) . This was about "the last comment on that line". This is why the issue has /* block */ /* fallthru */ // comment.

If you agree, please restore original wording and add some wording that it can be any comment on that line.

Can you point to keywords where the suppression text is not the last comment on the line. If we don't have one, then create it and point to it.

Example:
/* trigger */ /* comment */ // last
/* comment */ /* fallthru */ // last

&lt;/p&gt;
&lt;p&gt;
Note: The check assumes that there is no unreachable code in the {@code case}.
Expand Down
49 changes: 36 additions & 13 deletions src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/FallThroughCheckTest.java
View file
Open in desktop
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"),
rnveach marked this conversation as resolved.
Show resolved Hide resolved
expected);
}
}
2 changes: 1 addition & 1 deletion ...esources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThrough4.java
View file
Open in desktop
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
4 changes: 2 additions & 2 deletions ...esources/com/puppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThrough8.java
View file
Open in desktop
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
15 changes: 15 additions & 0 deletions ...ppycrawl/tools/checkstyle/checks/coding/fallthrough/InputFallThroughInlineSingleCase.java
View file
Open in desktop
@@ -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.'
}
}
4 changes: 2 additions & 2 deletions ...rawl/tools/checkstyle/checks/coding/fallthrough/InputFallThroughLastLineCommentCheck.java
View file
Open in desktop
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
26 changes: 26 additions & 0 deletions ...wl/tools/checkstyle/checks/coding/fallthrough/InputFallThroughMultipleReliefPatterns.java
View file
Open in desktop
@@ -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

}
}
}
}