Issue #5685: Fix false positive indentation for try/catch with body on same line#19562
Conversation
c5a6ffc to
97737ee
Compare
|
GitHub, generate report for Indentation/Example3 |
97737ee to
06920e9
Compare
|
GitHub, generate report for Indentation/Example3 |
|
it looks good i guess, i'm not sure about the added violation should it be reported or no . @romani can you please review |
|
@Carbon14-48 , please resolve conflict. |
i will resolve the conflict and also look into the new violations that appeared in the last report because i verified now and it should not be reported |
13de087 to
69208e3
Compare
|
GitHub, generate report for Indentation/Example3 |
4c1b6de to
c7507af
Compare
|
GitHub, generate report for Indentation/Example3 |
2a7315b to
36741ab
Compare
|
GitHub, generate report for Indentation/Example3 |
| } //indent:8 exp:8 | ||
| } //indent:4 exp:4 | ||
| void method3() { //indent:4 exp:4 | ||
| try { invokeAndWait(new Runnable() { @Override //indent:8 exp:8 |
There was a problem hiding this comment.
Not clear why this code is not violation
But
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/36741ab4af1ebf03d8c57f55ef343c029282f294_2026230430/reports/diff/openjdk25/index.html#A5 is still violation.
Please explain
There was a problem hiding this comment.
both snippets go through the same LineWrappingHandler path; the difference is the config: the test uses forceStrictCondition=false, so it only flags when indent is below expected, and column 46 is ≥ 8 so no violation. The OpenJDK diff uses forceStrictCondition=true, which i guess requires exact alignment, so column 46 becomes a violation.
There was a problem hiding this comment.
Please create new input with such property as true, copy jdk code snippet in it.
Let's see what is going on.
Something strange, it use to be violation on 'try' now it is on '@'.
forceStrictCondition=true only applied to line wrapping, not sure how it becomes here involved
There was a problem hiding this comment.
Something strange, it use to be violation on 'try' now it is on '@'.
i think that comes down to the fact that checkstyle report one violation per line .before the fix #A4 (the try child violation) was reported first, which claimed line 643,That prevented #A5 (the @ violation) from being reported , After our fix, #A4 is correctly removed ( the false positive). But now line 643 is never claimed by TryHandler, so LineWrappingHandler freely reports #A5, which was always there, just previously masked.
i previously runned the all jar on this
import java.awt.Component;
public class Test {
// Thread-safe Object.equals() called from native
public static boolean doEquals(final Object a, final Object b, Component c) {
if (a == b) return true;
final boolean[] ret = new boolean[1];
try {
invokeAndWait(new Runnable() { @Override
public void run() {
synchronized (ret) {
ret[0] = a.equals(b);
}
}
}, c);
}
catch (Exception e) {
e.printStackTrace();
}
synchronized(ret) { return ret[0]; }
}
private static void invokeAndWait(Runnable r, Object o) {
}
}with this confix.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="Indentation">
<property name="forceStrictCondition" value="true"/>
</module>
</module>
</module>
and i get
~/gsoc/checkstyle > java -jar target/checkstyle-13.4.3-SNAPSHOT-all.jar -c config.xml Test.java
Starting audit...
[ERROR] /home/carbon14/gsoc/checkstyle/Test.java:11:44: '@' has incorrect indentation level 43, expected level should be 12. [Indentation]
[ERROR] /home/carbon14/gsoc/checkstyle/Test.java:13:51: 'synchronized' has incorrect indentation level 50, expected level should be one of the following: 20, 24. [Indentation]
[ERROR] /home/carbon14/gsoc/checkstyle/Test.java:14:55: 'synchronized' child has incorrect indentation level 54, expected level should be one of the following: 24, 28. [Indentation]
[ERROR] /home/carbon14/gsoc/checkstyle/Test.java:15:51: 'synchronized rcurly' has incorrect indentation level 50, expected level should be one of the following: 20, 24. [Indentation]
[ERROR] /home/carbon14/gsoc/checkstyle/Test.java:16:47: 'method def rcurly' has incorrect indentation level 46, expected level should be one of the following: 16, 20. [Indentation]
Audit done.
Checkstyle ends with 5 errors.
the first violation should not be reported right ?
There was a problem hiding this comment.
Please share output in same input but released version of Checkstyle
There was a problem hiding this comment.
Let's add this input to tests, bit fix all violations except for line with @
There was a problem hiding this comment.
Please share output in same input but released version of Checkstyle
i fixed the input to have only the @ violation
import java.awt.Component;
public class Test {
public static boolean doEquals(final Object a, final Object b, Component c) {
if (a == b) return true;
final boolean[] ret = new boolean[1];
try {
invokeAndWait(new Runnable() { @Override
public void run() {
synchronized (ret) {
ret[0] = a.equals(b);
}
}
}, c);
}
catch (Exception e) {
e.printStackTrace();
}
synchronized (ret) {
return ret[0];
}
}
private static void invokeAndWait(Runnable r, Object o) {
}
}with latest released we get :
~ > java -jar checkstyle-13.4.2-all.jar -c config.xml Test.java
Starting audit...
[ERROR] /home/carbon14/Test.java:8:44: '@' has incorrect indentation level 43, expected level should be 12. [Indentation]
Audit done.
Checkstyle ends with 1 errors.
…ith body on same line
36741ab to
752b989
Compare
romani
left a comment
There was a problem hiding this comment.
Ok. Let's move forward, this edge case is very rare and nobody writes code like this in modern world
closes : #5685
Build Success with
mvn clean verify