checkstyle
checkstyle copied to clipboard
checkstyle
FallThrough check doesn't handle infinite loops
None of the statement groups in this switch can complete normally, so checkstyle shouldn't warn about fall through.
class T {
void main(int i) {
switch (i) {
case 1:
for (;;) {}
case 2:
while (true) {}
case 3:
do {
} while (true);
default:
break;
}
}
}
java -jar checkstyle-7.6-all.jar -c google_checks.xml T.java
Starting audit...
[WARN] T.java:6:7: Fall through from previous branch of the switch statement. [FallThrough]
[WARN] T.java:8:7: Fall through from previous branch of the switch statement. [FallThrough]
[WARN] T.java:11:7: Fall through from previous branch of the switch statement. [FallThrough]
Audit done.
unfortunately this is conner cases. It is hard to maintain all variation of them. Cases like following is hard to analyse, to make it worse imagine that TRUE is defined in other class method - so that become completely impossible to detect by checkstyle.
class T {
private boolean TRUE = true;
void main(int i) {
switch (i) {
case 1:
for (;;) {}
case 2:
while (TRUE) {}
case 3:
do {
} while (TRUE);
default:
break;
}
}
}
as we can not cover all cases, should we start coverage of certain strict code structures ?
@rnveach , please share your thoughts on this issue.
@romani I agree.
My thoughts are I am ok with only handling the most obvious, no denying cases if they can be achieved without issue. If they become too complex, than let's not handle them.
Example: for (;;) {} and while (true).
There is no way they could be misunderstood that I can see and they are probably the most common ways of coding infinite loops. We still have to examine for breaks inside the infinite loops, but we should already be handling that.
Lets stop short at examining variables and negations and equalities and and/or/xor operators for this check and make that clear in documentation. There are numerous ways to break your example that we could never handle.
For example, your TRUE isn't final and could be changed by another method call somewhere to false.
Even if it was final, there are other ways to rewrite the condition like negation or weird expressions.
We still have to examine for breaks inside the infinite loops, but we should already be handling that.
this is additional bag of problems and nuances, we will never be successful to cover all.
I become even more in position to not support this at all as this coding style is error prone.
If user so like it and want to keep it - he should use suppressions.
Extra break till not damage the code that much, extra verbosity, but it clearly show that no matter what happening inside of case, switch will continue to work as expected.
@cushon , please provide some comments or examples of real code to show real necessity of your cases.
I don't have any open source code I can point to, but we've run into this a handful of times. I think there's value in staying as close as possible to the definition in JLS 14.21.
And for what it's worth, none of the examples I've seen use a constant field as the condition.
@cushon,
I don't have any open source code I can point to
Can you provide chunk of code with abfuscation to not reveal any secret ?
but we've run into this a handful of times.
I can not take this as prove of valid code. In this world so much code bad code. Non engineer was born ideal and left no bad code after him.
I think there's value in staying as close as possible to the definition in JLS 14.21.
Checkstyle is working only on compilable sources. We are single file validation tool.
And for what it's worth, none of the examples I've seen use a constant field as the condition.
But a soon it could be, users will start to request to support that too.
I could agree on idea to support this only if you provide a prove that code that you have can NOT be refactored to be better. Sorry that I am demanding.
I dug up a couple of examples.
case WIRETYPE_FIXED64:
while (true) {
target.add(input.readDouble());
if (input.isAtEnd()) {
return;
}
int nextTag = input.readTag();
if (nextTag != tag) {
this.nextTag = nextTag;
return;
}
}
case '/':
while (true) {
eat();
switch (ch) {
case '\n':
case '\r':
eat();
continue OUTER;
case ASCII_SUB:
if (reader.done()) {
return Token.EOF;
}
eat();
break;
}
}
The second one is from here.
None of the statement groups in this switch can complete normally, so checkstyle shouldn't warn about fall through.
class T { void main(int i) { switch (i) { case 1: function1(); break; case 2: function2(); break; case 3: function3(); break; default: break; } } }java -jar checkstyle-7.6-all.jar -c google_checks.xml T.java Starting audit... [WARN] T.java:6:7: Fall through from previous branch of the switch statement. [FallThrough] [WARN] T.java:8:7: Fall through from previous branch of the switch statement. [FallThrough] [WARN] T.java:11:7: Fall through from previous branch of the switch statement. [FallThrough] Audit done.
You can try to wrap the code in a function. And used function replace the code. It would be successed which I had tried used that.
Moving to function is good alternative but it will not always be good approach. Looks like such code is reasonable to appear in very optimized code (in favor of CPU father than human ability read), in such case style should definitely suffer, so violation from checkstyle in unavoidable .
Infinite loops can have break/continue in addition to returns - so they could cause "Fall through" violation. If only "returns" are present in loop it is ok to think that it will never reach end of case block.
FallThrough is supported by XpathFilter, such filter require separate configuration for this, but very soon(PR is already in review) such suppression filter could be done directly in main config file.
UPDATE: at least at 8.30 version, users can use Xpath suppressions for such corner cases, Checks is now supported by Filter, filter can be set for google style as https://checkstyle.org/google_style.html#Google_Suppressions
/var/tmp/cs $ cat -n TestClass.java
1 class TestClass {
2 void main(int i) {
3 switch (i) {
4 case -1:
5 //return;
6 case 0:
7 print(i);
8 //return;
9 case 1:
10 for (;;) {
11 return;
12 }
13 case 2:
14 while (true) {
15 return;
16 }
17 case 3:
18 do {
19 return;
20 } while (true);
21 default:
22 break;
23 }
24 }
25 }
$ java -jar checkstyle-8.15-all.jar -c /google_checks.xml TestClass.java
Starting audit...
[WARN] /private/var/tmp/cs/TestClass.java:9:7: Fall through from previous branch of the switch statement. [FallThrough]
Audit done.
works as expected on test case provided in Description. BUT it does not work on real life cases:
$ cat config.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
"-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
"https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">
<module name="TreeWalker">
<module name="FallThrough"/>
</module>
</module>
✔ /var/tmp/cs $ cat TestClassReal.java
class TestClassReal {
void main(int i) {
switch (i) {
case WIRETYPE_FIXED64:
while (true) {
target.add(input.readDouble());
if (input.isAtEnd()) {
return;
}
int nextTag = input.readTag();
if (nextTag != tag) {
this.nextTag = nextTag;
return;
}
}
case 5:
while (true) {
eat();
switch (ch) {
case '\n':
case '\r':
eat();
continue OUTER;
case ASCII_SUB:
if (reader.done()) {
return Token.EOF;
}
eat();
break;
}
}
default:
break;
}
}
}
✔ /var/tmp/cs$ java -jar checkstyle-8.15-all.jar -c config.xml TestClassReal.java
Starting audit...
[ERROR] /private/var/tmp/cs/TestClassReal.java:16:7: Fall through from previous branch of the switch statement. [FallThrough]
[ERROR] /private/var/tmp/cs/TestClassReal.java:32:7: Fall through from previous branch of the switch statement. [FallThrough]
Audit done.
Checkstyle ends with 2 errors.
@rnveach , we already support some cases with return - https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/FallThroughCheck.java#L157
Should we update Check to search of return in whole code of CASE ?
BUT it does not work on real life cases:
@romani If you are referring to this case that we don't support some returns, then this issue doesn't look related to returns but the while (true).
We are looking for an absolute termination of the case. The example you provided has returns surrounded by if conditions. The if conditions means that it is not an absolute that the returns will hit.
It only becomes an absolute because you have while (true) with no other breaks, so that means the loop must continue to execute until the if conditions are satisfied thereby creating an absolute termination.
To detect your case, we need to identify the infinite loop.
Let me know if I got what you are asking wrong.
continue OUTER;
I don't see OUTER in the example.
I don't see OUTER in the example.
I just used sources from https://github.com/checkstyle/checkstyle/issues/3885#issuecomment-283454252
To detect your case, we need to identify the infinite loop.
yes, we can support simple cases as provided in issue description. It will not be very reliable implementation. So I just trying to decide what to do with such issue/request: approve it or close it and wait for more feedback from users with cases why xpath and extraction to function is not OK. Please share your thought, last time.
Like last time, I am fine supporting the most clear cut cases. See https://github.com/checkstyle/checkstyle/issues/3885#issuecomment-283345102 . My stand hasn't changed any.
Are there any rules for empty, infinite loops? Do we need a rule to handle them?
@mohitsatr , Issue is approved, but lets treat with caution. If implementation will go wild, we can abandon this issue. Lets try to support only simple cases that shown in such issue only.
we will not support code like while(returnTrueAlways()) {}.