checkstyle / checkstyle

Checkstyle is a development tool to help programmers write Java code that adheres to a coding standard. By default it supports the Google Java Style Guide and Sun Code Conventions, but is highly configurable. It can be invoked with an ANT task and a command line program.
https://checkstyle.org
GNU Lesser General Public License v2.1
8.3k stars 3.66k forks source link

WhitespaceAfter should support `ELLIPSIS` token #11116

Closed nrmancuso closed 2 years ago

nrmancuso commented 2 years ago

I have downloaded the latest cli 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

Check documentation at https://checkstyle.sourceforge.io/config_whitespace.html#WhitespaceAfter

How it works Now:


➜  javac Test.java
➜  cat config.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">
    <property name="charset" value="UTF-8"/>

    <module name="TreeWalker">
        <module name="WhitespaceAfter">
          <property name="tokens" value="ELLIPSIS"/>
        </module>
    </module>
</module>

➜  cat Test.java  
public class Test {
    void method1(int...myVarargs) { // expected violation

    }
    void method2(int... myVarargs) { // ok

    }
}

➜  java -jar checkstyle-9.2-all.jar -c config.xml Test.java
com.puppycrawl.tools.checkstyle.api.CheckstyleException: cannot initialize module TreeWalker - Token "ELLIPSIS" was not found in Acceptable tokens list in check com.puppycrawl.tools.checkstyle.checks.whitespace.WhitespaceAfterCheck
        at com.puppycrawl.tools.checkstyle.Checker.setupChild(Checker.java:473)
        at com.puppycrawl.tools.checkstyle.api.AutomaticBean.configure(AutomaticBean.java:201)
        at com.puppycrawl.tools.checkstyle.Main.runCheckstyle(Main.java:405)
        at com.puppycrawl.tools.checkstyle.Main.runCli(Main.java:332)
        at com.puppycrawl.tools.checkstyle.Main.execute(Main.java:191)
        at com.puppycrawl.tools.checkstyle.Main.main(Main.java:126)
Caused by: com.puppycrawl.tools.checkstyle.api.CheckstyleException: Token "ELLIPSIS" was not found in Acceptable tokens list in check com.puppycrawl.tools.checkstyle.checks.whitespace.WhitespaceAfterCheck
        at com.puppycrawl.tools.checkstyle.TreeWalker.registerCheck(TreeWalker.java:223)
        at com.puppycrawl.tools.checkstyle.TreeWalker.setupChild(TreeWalker.java:133)
        at com.puppycrawl.tools.checkstyle.api.AutomaticBean.configure(AutomaticBean.java:201)
        at com.puppycrawl.tools.checkstyle.Checker.setupChild(Checker.java:468)
        ... 5 more
Checkstyle ends with 1 errors.

However, WhitespaceAroundCheck does support ELLIPSIS. But, this is not a frequently used style. In fact, the JLS specifically mentions that

Note that the ellipsis (...) is a token unto itself (§3.11). It is possible to put whitespace between it and the type, but this is discouraged as a matter of style.

Is your feature request related to a problem? Please describe. I would expect ... to be a valid token for WhitespaceAfterCheck, and for it to enforce style noted above.

Describe the solution you'd like

[ERROR] Test.java:2:21: '...' is not followed by whitespace. [WhitespaceAfter]

Additional context

Noticed at https://github.com/checkstyle/checkstyle/pull/11094#discussion_r775249437, WhitespaceAfterCheck does not currently support ELLIPSIS token.

ELLIPSIS should be a default token for WhitespaceAfterCheck. Since we use default module for this check in checkstyle_checks.xml, we may find some violations in the project.

pbludov commented 2 years ago

Some teams may prefer whitespace around style:

https://github.com/treeform/orekit/blob/master/src/main/java/org/orekit/data/PolynomialParser.java#L256

strkkk commented 2 years ago

void method1(int...myVarargs) { // expected violation

in case this code is compliable (it is a surprise for me), https://checkstyle.org/config_whitespace.html#NoWhitespaceAfter check can be enhanced with ellipsis token as well (not default ofc)

nrmancuso commented 2 years ago

Some teams may prefer whitespace around style...

@pbludov do you think we should not make ELLIPSIS a default token?

pbludov commented 2 years ago

@pbludov do you think we should not make ELLIPSIS a default token?

Let's make it and look at the regression tests.

Vyom-Yadav commented 2 years ago

On it.