False-positive RightCurly in google_checks since 8.20

[alexbde]

https://checkstyle.org/config_blocks.html#RightCurly
https://checkstyle.org/property_types.html#rcurly

/var/tmp $ javac RightCurly.java

/var/tmp $ cat RightCurly.java
public class RightCurly {
  public RightCurly() {}
}

/var/tmp $ RUN_LOCALE="-Duser.language=en -Duser.country=US"
/var/tmp $ java $RUN_LOCALE -jar checkstyle-8.21-all.jar -c /google_checks.xml RightCurly.java
Starting audit...
[WARN] /var/tmp/RightCurly.java:2:24: '}' at column 24 should be alone on a line. [RightCurlyAlone]
Audit done.

---

I expected no warning because of section 4.1.3 Empty blocks: may be concise in Google Java Style Guide.

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

---

Problem exists since Checkstyle 8.20, with Checkstyle 8.19 there is no such warning.

Checkstyle 8.20

$ java -Duser.language=en -Duser.country=US -jar checkstyle-8.20-all.jar -c /google_checks.xml RightCurly.java
Starting audit...
[WARN] /var/tmp/RightCurly.java:2:24: '}' at column 24 should be alone on a line. [RightCurlyAlone]
Audit done.

Checkstyle 8.19

$ java -Duser.language=en -Duser.country=US -jar checkstyle-8.19-all.jar -c /google_checks.xml RightCurly.java
Starting audit...
Audit done.

Google config:

checkstyle/src/main/resources/google_checks.xml

Lines 91 to 97 in dbbc42c

	<module name="RightCurly">
	<property name="id" value="RightCurlyAlone"/>
	<property name="option" value="alone"/>
	<property name="tokens"
	value="CLASS_DEF, METHOD_DEF, CTOR_DEF, LITERAL_FOR, LITERAL_WHILE, STATIC_INIT,
	INSTANCE_INIT, ANNOTATION_DEF, ENUM_DEF"/>
	</module>

        <module name="RightCurly">
            <property name="id" value="RightCurlyAlone"/>
            <property name="option" value="alone"/>
            <property name="tokens"
             value="CLASS_DEF, METHOD_DEF, CTOR_DEF, 
                        LITERAL_FOR, LITERAL_WHILE, STATIC_INIT,
                    INSTANCE_INIT"/>
</module>

https://checkstyle.org/config_blocks.html#RightCurly
https://checkstyle.org/property_types.html#rcurly

---

[rnveach]

@romani this is in response to #6345 .

[romani]

hmm, google style is precise on expected behavior, but our Check is not that configurable to allow this only by option change.
We have options:

• create new Check to cover such requirements. We can not extend current Check as it is already over complicated and hardly maintain it already.
• create Xpath suppression by mean of https://checkstyle.org/config_filters.html#SuppressionXpathSingleFilter

I would recommend to try Xpath first and create new Check as last option.
@alexbde and @ thenatog , please try to suppress all our false-positives. If it is possible we can such filter in google_style.xml

[romani]

this issue caused used to disable Check completely in google config.
See #6946 (comment)

[romani]

@rnveach , do you remember why https://checkstyle.org/property_types.html#rcurly alone_or_singleline is not ok to use for this problem ?

[rnveach]

I do not know.
If you feel it will work, lets test out the change in a PR and with regression to see what the differences will be.

[romani]

alone_or_singleline is not ok to use for this problem ?

it is not ok, as it should allow only empty braces { } as defined in google style.
BUT alone_or_singleline (https://checkstyle.org/property_types.html#rcurly) will allow public long getId() { return id; } that is not acceptable by Google style

[romani]

Tree is like:

java -jar /var/tmp/checkstyle-8.28-all.jar -t Test.java CLASS_DEF -> CLASS_DEF [1:0]
|--MODIFIERS -> MODIFIERS [1:0]
|   `--LITERAL_PUBLIC -> public [1:0]
|--LITERAL_CLASS -> class [1:7]
|--IDENT -> Test [1:13]
`--OBJBLOCK -> OBJBLOCK [1:18]
    |--LCURLY -> { [1:18]
    |--CTOR_DEF -> CTOR_DEF [2:2]
    |   |--MODIFIERS -> MODIFIERS [2:2]
    |   |   `--LITERAL_PUBLIC -> public [2:2]
    |   |--IDENT -> Test [2:9]
    |   |--LPAREN -> ( [2:13]
    |   |--PARAMETERS -> PARAMETERS [2:14]
    |   |--RPAREN -> ) [2:14]
    |   `--SLIST -> { [2:16]
    |       `--RCURLY -> } [2:17]
    `--RCURLY -> } [3:0]

So we need to have Xpath that test that RCURLY is the only child of parent SLIST.
@timurt , do you know if it is possible to make such Xpath expression ?

[romani]

Looks like I got it ....
Here is Xpath:

$ cat config.xml 
<?xml version="1.0" encoding="UTF-8"?>
<!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="RightCurly">
             <property name="id" value="RightCurlyAlone"/>
             <property name="option" value="alone"/>
             <property name="tokens"
              value="CLASS_DEF, METHOD_DEF, CTOR_DEF, 
                     LITERAL_FOR, LITERAL_WHILE, STATIC_INIT,
                     INSTANCE_INIT, ANNOTATION_DEF, ENUM_DEF"/>
        </module>
        <module name="SuppressionXpathSingleFilter">
          <property name="id" value="RightCurlyAlone"/>
          <property name="query" value="//RCURLY[parent::SLIST[count(./*)=1]]"/>
        </module>
  </module>
</module>

$ cat Test.java 
public class Test {
  public Test() {}
  public Test(int a) {;}
}

$ java -jar /var/tmp/checkstyle-8.29-all.jar -c config.xml Test.java 
Starting audit...
[ERROR] /var/tmp/Test.java:3:24: '}' at column 24 should be alone on a line. [RightCurlyAlone]
Audit done.
Checkstyle ends with 1 errors.

@timurt , am I right in my xpath ? if so we can embed this filter in config so it will ease a problem.

[timurt]

@romani
I checked your xpath query with XpathMapperTest and it passed

You can see my test here

[rdiachenko]

@romani , replying to your comment #6946 (comment):

I checked few projects with the proposed suppression above. The only one violation was detected:

$ cat config.xml 
<?xml version="1.0" encoding="UTF-8"?>
<!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="RightCurly">
             <property name="id" value="RightCurlyAlone"/>
             <property name="option" value="alone"/>
             <property name="tokens"
              value="CLASS_DEF, METHOD_DEF, CTOR_DEF, 
                     LITERAL_FOR, LITERAL_WHILE, STATIC_INIT,
                     INSTANCE_INIT, ANNOTATION_DEF, ENUM_DEF"/>
        </module>
        <module name="SuppressionXpathSingleFilter">
          <property name="id" value="RightCurlyAlone"/>
          <property name="query" value="//RCURLY[parent::SLIST[count(./*)=1]]"/>
        </module>
  </module>
</module>

$ cat Test.java 
@Component
@ConfigurationProperties
@EnableConfigurationProperties
@Data
public class Test {}

$ java -jar checkstyle-8.29-all.jar -c config.xml Test.java 
Starting audit...
[ERROR] /tmp/checkstyle/Test.java:5:20: '}' at column 20 should be alone on a line. [RightCurlyAlone]
Audit done.
Checkstyle ends with 1 errors.

PS: don't ask me why that class is empty, I have no idea, just found it in one of the projects :)

[romani]

structure of AST is a bit different for empty classes:

$ cat Test1.java 
public class Test1 {}
class Test12 { int i; }

$ java -jar /var/tmp/checkstyle-8.29-all.jar -t Test1.java 
CLASS_DEF -> CLASS_DEF [1:0]
|--MODIFIERS -> MODIFIERS [1:0]
|   `--LITERAL_PUBLIC -> public [1:0]
|--LITERAL_CLASS -> class [1:7]
|--IDENT -> Test1 [1:13]
`--OBJBLOCK -> OBJBLOCK [1:19]
    |--LCURLY -> { [1:19]
    `--RCURLY -> } [1:20]
CLASS_DEF -> CLASS_DEF [2:0]
|--MODIFIERS -> MODIFIERS [2:0]
|--LITERAL_CLASS -> class [2:0]
|--IDENT -> Test12 [2:6]
`--OBJBLOCK -> OBJBLOCK [2:13]
    |--LCURLY -> { [2:13]
    |--VARIABLE_DEF -> VARIABLE_DEF [2:15]
    |   |--MODIFIERS -> MODIFIERS [2:15]
    |   |--TYPE -> TYPE [2:15]
    |   |   `--LITERAL_INT -> int [2:15]
    |   |--IDENT -> i [2:19]
    |   `--SEMI -> ; [2:20]
    `--RCURLY -> } [2:22]

$ java -jar /var/tmp/checkstyle-8.29-all.jar -b "//RCURLY[parent::OBJBLOCK[count(./*)=2]]" Test1.java 
CLASS_DEF -> CLASS_DEF [1:0]
`--OBJBLOCK -> OBJBLOCK [1:19]
    `--RCURLY -> } [1:20]

[romani]

Here is working example:

$ javac Test1.java
$ cat Test1.java 
public class Test1 {

    {}
    static {}

    void method6(int a) {
          {}; //ok
    }

    void method7(int a) {} //ok
    void method8(int a) {/*:)*/} //ok, should be violation

    class TestClass4 {} //ok

    enum Test {}

    interface Interface {}

    @interface ClassPreamble { } //ok, should be violation, as there is space
}

$ cat config.xml 
<?xml version="1.0" encoding="UTF-8"?>
<!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="RightCurly">
             <property name="id" value="RightCurlyAlone"/>
             <property name="option" value="alone"/>
             <property name="tokens"
              value="CLASS_DEF, METHOD_DEF, CTOR_DEF, LITERAL_FOR, 
                     LITERAL_WHILE, STATIC_INIT,
                     INSTANCE_INIT, ANNOTATION_DEF, ENUM_DEF"/>
        </module>
        <module name="SuppressionXpathSingleFilter">
          <property name="id" value="RightCurlyAlone"/>
          <property name="query" value="//RCURLY[parent::SLIST[count(./*)=1]
                                                 or parent::OBJBLOCK[count(./*)=2]]"/>
        </module>
  </module>
</module>

$ java -jar /var/tmp/checkstyle-8.29-all.jar -c config.xml Test1.java 
Starting audit...
Audit done.

@rdiachenko , please try to put in your config filter, this version of filter, it should suppress most false positives, BUT it will cause false-negatives on cases like void method8(int a) {/*:)*/} //ok, should be violation .... false-negative is better than false-positive and will be addressed only by custom Check.
If you find no bad effects, we can put such filter in google config and keep it till this issue is resolved.

new Check with possible name as RightCurlyAloneOrEmpty should be separate from RightCurly but can copy any functionality it needs and cover singe case, ones implemented it should replace existing RightCurly instance in google config.

[rdiachenko]

@romani thanks a lot for the suggestion. Works fine for me, none suspicious behavior was detected.

[rnveach]

@romani Your xpath fails for one case once we introduce comment aware checks into the configuration which brings up an issue with our xpath implementation.

$ cat TestClass.java
public class Test1 {

    {}
    static {}

    void method6(int a) {
          {}; //ok
    }

    void method7(int a) {} //ok
    void method8(int a) {/*:)*/} //ok, should be violation

    class TestClass4 {} //ok

    enum Test {}

    interface Interface {}

    @interface ClassPreamble { } //ok, should be violation, as there is space
}

$ cat TestConfig.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "https://checkstyle.org/dtds/configuration_1_3.dtd">

<module name="Checker">
  <module name="TreeWalker">
        <module name="RightCurly">
             <property name="id" value="RightCurlyAlone"/>
             <property name="option" value="alone"/>
             <property name="tokens"
              value="CLASS_DEF, METHOD_DEF, CTOR_DEF, LITERAL_FOR, 
                     LITERAL_WHILE, STATIC_INIT,
                     INSTANCE_INIT, ANNOTATION_DEF, ENUM_DEF"/>
        </module>
        <module name="TodoComment" />
        <module name="SuppressionXpathSingleFilter">
          <property name="id" value="RightCurlyAlone"/>
          <property name="query" value="//RCURLY[parent::SLIST[count(./*)=1]
                                                 or parent::OBJBLOCK[count(./*)=2]]"/>
        </module>
  </module>
</module>

$ java -jar checkstyle-8.29-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:11:32: '}' at column 32 should be alone on a line. [RightCurlyAlone]
Audit done.
Checkstyle ends with 1 errors.

[romani]

#7531 is valid issue, we need to think about it.

Xpath that I provided is still good for Google style as style does not allow any content in {}, so missed match by xpath is valid violation.

[romani]

Workaround is merged.
Actual fix will be by new check implementation at #7541

[romani]

additional fix for xpath is required to keep violation for empty initilaizers , see discussion and explanation at #7543

PR with fix: #7676