false-negative in google_checks.xml for not being able to detect requirement of K & R style for FINALLY #15664
Description
I have read documentation: https://google.github.io/styleguide/javaguide.html#s4.1.2-blocks-k-r-style, https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/dbe82ad_2024161500/google_style.html#a4.1.3
I have downloaded the latest checkstyle from: https://checkstyle.org/cmdline.html#Download_and_Run
I have executed the cli and showed it below, as cli describes the problem better than 1,000 words
From: 4.1.3 Empty blocks: may be concise:
An empty block or block-like construct may be in K & R style (as described in Section 4.1.2). Alternatively, it may be closed immediately after it is opened, with no characters or line break in between ({}), unless it is part of a multi-block statement (one that directly contains multiple blocks: if/else or try/catch/finally).
Rule states that if an empty block is part of multi-block statement then it should follow K & R style blocks.
$ cat Test.java
/** some. */
public class Test {
void foo() throws Exception {
try (MyResource r = new MyResource()) {}
try (MyResource r = new MyResource()) {
} finally { after = true; }
try (MyResource r = new MyResource()) {
} catch (Exception expected) {
} finally { boolean after = true; }
try (MyResource r = new MyResource()) {
} finally {
boolean after = true;
}
}
/** some. */
public class MyResource implements AutoCloseable {
/** some. */
@Override
public void close() throws Exception {
System.out.println("Closed MyResource");
}
}
}
$ java -jar checkstyle-10.19.0-SNAPSHOT-all.jar -c /google_checks.xml Test.java
Starting audit...
[WARN] /var/tmp/Test.java:8:15: '{' at column 15 should have line break after. [LeftCurlyEol]
[WARN] /var/tmp/Test.java:13:15: '{' at column 15 should have line break after. [LeftCurlyEol]
Audit done.
Expected: violations for FINALLY blocks on } on line 8 and 13 as it should be on new line.
$ cat EmptyBlocks.java
/** some. */
public class EmptyBlocks {
void foo() throws Exception {
int a = 90;
if (a == 1) {
} else {} // false-negative
if (a == 1) {
} else { } // false-negative
try (MyResource r = new MyResource()) { }
try (MyResource r = new MyResource()) {}
try (MyResource r = new MyResource()) {} catch (Exception expected) {} // false-negative
try (MyResource r = new MyResource()) {} catch (Exception expected) { } // false-negative
try (MyResource r = new MyResource()) {
} catch (Exception expected) {} // false-negative
try (MyResource r = new MyResource()) {
} catch (Exception expected) { } // false-negative
try (MyResource r = new MyResource()) {;}
}
/** some. */
public class MyResource implements AutoCloseable {
/** some. */
@Override
public void close() throws Exception {
System.out.println("Closed MyResource");
}
}
}Output:
$ java -jar checkstyle-10.18.1-all.jar -c google_checks.xml EmptyBlocks.java
Starting audit...
[WARN] C:\checkstyle testing\EmptyBlocks.java:10: Empty blocks should have no spaces. Empty blocks may only be represented as {} when not part of a multi-block statement (4.1.3) [RegexpSinglelineJava]
[WARN] C:\checkstyle testing\EmptyBlocks.java:12: Empty blocks should have no spaces. Empty blocks may only be represented as {} when not part of a multi-block statement (4.1.3) [RegexpSinglelineJava]
[WARN] C:\checkstyle testing\EmptyBlocks.java:15: Empty blocks should have no spaces. Empty blocks may only be represented as {} when not part of a multi-block statement (4.1.3) [RegexpSinglelineJava]
[WARN] C:\checkstyle testing\EmptyBlocks.java:21: Empty blocks should have no spaces. Empty blocks may only be represented[WARN] C:\checkstyle testing\EmptyBlocks.java:15: Empty blocks should have no spaces. Empty blocks may only be represented as {} when not part of a multi-block statement (4.1.3) [RegexpSinglelineJava]
[WARN] C:\checkstyle testing\EmptyBlocks.java:21: Empty blocks should have no spaces. Empty blocks may only be represented as {} when not part of a multi-block statement (4.1.3) [RegexpSinglelineJava]
[WARN] C:\checkstyle testing\EmptyBlocks.java:21: Empty blocks should have no spaces. Empty blocks may only be represented as {} when not part of a multi-block statement (4.1.3) [RegexpSinglelineJava]
[WARN] C:\checkstyle testing\EmptyBlocks.java:22:43: '{' at column 43 should have line break after. [LeftCurly]
as {} when not part of a multi-block statement (4.1.3) [RegexpSinglelineJava]
[WARN] C:\checkstyle testing\EmptyBlocks.java:22:43: '{' at column 43 should have line break after. [LeftCurly]
[WARN] C:\checkstyle testing\EmptyBlocks.java:22:43: '{' at column 43 should have line break after. [LeftCurly]
[WARN] C:\checkstyle testing\EmptyBlocks.java:22:43: WhitespaceAround: '{' is not followed by whitespace. Empty blocks may only be represented as {} when not part of a multi-block statement (4.1.3) [WhitespaceAround]
[WARN] C:\checkstyle testing\EmptyBlocks.java:22:43: WhitespaceAround: '{' is not followed by whitespace. Empty blocks may only be represented as {} when not part of a multi-block statement (4.1.3) [WhitespaceAround]
as {} when not part of a multi-block statement (4.1.3) [WhitespaceAround]
[WARN] C:\checkstyle testing\EmptyBlocks.java:22:45: WhitespaceAround: '}' is not preceded with whitespace. [WhitespaceAround]
[WARN] C:\checkstyle testing\EmptyBlocks.java:22:45: WhitespaceAround: '}' is not preceded with whitespace. [WhitespaceAround]
Audit done.
We're missing violations for not following K&R style blocks though we're able to detect other mis-followed rules like having { } in-place of {}, in current style guide implementation we don't have support to check for multi-block statements.
As part of this issue, we need to add support to detect multi-block statements and give violations if rule isn't being followed properly.
The solution I can think of is modifying following suppression:
checkstyle/src/main/resources/google_checks.xml
Lines 107 to 112 in 888ac42
and check if we're in multi-block statement then we should give violation '}' should be alone on the line for else/catch/finally's right curly }