Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EmptyLineSeparator check does not validate newlines before comments #5981

Closed
romani opened this issue Jun 28, 2018 · 6 comments

Comments

Projects
None yet
3 participants
@romani
Copy link
Member

commented Jun 28, 2018

Originally was reported at #2974

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

    public static final void method1() { }



    // Method 2 must fail
    public static final void method2() { }



    /**
     * Method 3 must fail
     */
    public static final void method3() { }
}


$ cat config.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
    "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
    "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">

<module name="Checker">
    <module name="TreeWalker">
        <module name="EmptyLineSeparator">
            <property name="allowNoEmptyLineBetweenFields" value="true"/>
            <property name="allowMultipleEmptyLines" value="false"/>
        </module>
    </module>
</module>


$ java -jar checkstyle-6.15-all.jar -c config.xml Test.java
Starting audit...
Audit done.

Expected:
method2 and method3 must fail with ... has more than 1 empty lines before. [EmptyLineSeparator]

Related tickets: #2067

http://checkstyle.sourceforge.net/config_whitespace.html#EmptyLineSeparator

@romani

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2018

What I see is ... this Check is comments aware module, https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/EmptyLineSeparatorCheck.java#L255 , so it mean that it traverse Tree with comments. So all that you need is already in ASTree.

example from issue:

$ cat Test.java 
public class Test {

    public static final void method1() { }



    // Method 2 must fail
    public static final void method2() { }



    /**
     * Method 3 must fail
     */
    public static final void method3() { }
}

$ java -jar checkstyle-8.10.1-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]
    |--METHOD_DEF -> METHOD_DEF [3:4]
    |   |--MODIFIERS -> MODIFIERS [3:4]
    |   |   |--LITERAL_PUBLIC -> public [3:4]
    |   |   |--LITERAL_STATIC -> static [3:11]
    |   |   `--FINAL -> final [3:18]
    |   |--TYPE -> TYPE [3:24]
    |   |   `--LITERAL_VOID -> void [3:24]
    |   |--IDENT -> method1 [3:29]
    |   |--LPAREN -> ( [3:36]
    |   |--PARAMETERS -> PARAMETERS [3:37]
    |   |--RPAREN -> ) [3:37]
    |   `--SLIST -> { [3:39]
    |       `--RCURLY -> } [3:41]
    |--METHOD_DEF -> METHOD_DEF [8:4]
    |   |--MODIFIERS -> MODIFIERS [8:4]
    |   |   |--SINGLE_LINE_COMMENT -> // [7:4]
    |   |   |   `--COMMENT_CONTENT ->  Method 2 must fail\n [7:6]
    |   |   |--LITERAL_PUBLIC -> public [8:4]
    |   |   |--LITERAL_STATIC -> static [8:11]
    |   |   `--FINAL -> final [8:18]
    |   |--TYPE -> TYPE [8:24]
    |   |   `--LITERAL_VOID -> void [8:24]
    |   |--IDENT -> method2 [8:29]
    |   |--LPAREN -> ( [8:36]
    |   |--PARAMETERS -> PARAMETERS [8:37]
    |   |--RPAREN -> ) [8:37]
    |   `--SLIST -> { [8:39]
    |       `--RCURLY -> } [8:41]
    |--METHOD_DEF -> METHOD_DEF [15:4]
    |   |--MODIFIERS -> MODIFIERS [15:4]
    |   |   |--BLOCK_COMMENT_BEGIN -> /* [12:4]
    |   |   |   |--COMMENT_CONTENT -> *\n     * Method 3 must fail\n      [12:6]
    |   |   |   `--BLOCK_COMMENT_END -> */ [14:5]
    |   |   |--LITERAL_PUBLIC -> public [15:4]
    |   |   |--LITERAL_STATIC -> static [15:11]
    |   |   `--FINAL -> final [15:18]
    |   |--TYPE -> TYPE [15:24]
    |   |   `--LITERAL_VOID -> void [15:24]
    |   |--IDENT -> method3 [15:29]
    |   |--LPAREN -> ( [15:36]
    |   |--PARAMETERS -> PARAMETERS [15:37]
    |   |--RPAREN -> ) [15:37]
    |   `--SLIST -> { [15:39]
    |       `--RCURLY -> } [15:41]
    `--RCURLY -> } [16:0]

attention to (there is clear gap between line 3 and 7-8, comments over method can be located a bit dipper in tree, but this is stable behavior):

    |       `--RCURLY -> } [3:41]
    |--METHOD_DEF -> METHOD_DEF [8:4]
    |   |--MODIFIERS -> MODIFIERS [8:4]
    |   |   |--SINGLE_LINE_COMMENT -> // [7:4]
    |   |   |   `--COMMENT_CONTENT ->  Method 2 must fail\n [7:6]

We should simply go over tree and find a gap that is more than 1. in example above gap is 3.

Ok, make example a bit complicated

$ cat Test.java 
public class Test {

    public void method1() { }

    // single line comment

    /** Some comment with empty line

    */
    // comment over method2
    public void method2() { }
}

$ java -jar checkstyle-8.10.1-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]
    |--METHOD_DEF -> METHOD_DEF [3:4]
    |   |--MODIFIERS -> MODIFIERS [3:4]
    |   |   `--LITERAL_PUBLIC -> public [3:4]
    |   |--TYPE -> TYPE [3:11]
    |   |   `--LITERAL_VOID -> void [3:11]
    |   |--IDENT -> method1 [3:16]
    |   |--LPAREN -> ( [3:23]
    |   |--PARAMETERS -> PARAMETERS [3:24]
    |   |--RPAREN -> ) [3:24]
    |   `--SLIST -> { [3:26]
    |       `--RCURLY -> } [3:28]
    |--METHOD_DEF -> METHOD_DEF [11:4]
    |   |--MODIFIERS -> MODIFIERS [11:4]
    |   |   |--SINGLE_LINE_COMMENT -> // [5:4]
    |   |   |   `--COMMENT_CONTENT ->  single line comment\n [5:6]
    |   |   |--BLOCK_COMMENT_BEGIN -> /* [7:4]
    |   |   |   |--COMMENT_CONTENT -> * Some comment with empty line\n\n     [7:6]
    |   |   |   `--BLOCK_COMMENT_END -> */ [9:4]
    |   |   |--SINGLE_LINE_COMMENT -> // [10:4]
    |   |   |   `--COMMENT_CONTENT ->  comment over method2\n [10:6]
    |   |   `--LITERAL_PUBLIC -> public [11:4]
    |   |--TYPE -> TYPE [11:11]
    |   |   `--LITERAL_VOID -> void [11:11]
    |   |--IDENT -> method2 [11:16]
    |   |--LPAREN -> ( [11:23]
    |   |--PARAMETERS -> PARAMETERS [11:24]
    |   |--RPAREN -> ) [11:24]
    |   `--SLIST -> { [11:26]
    |       `--RCURLY -> } [11:28]
    `--RCURLY -> } [12:0]

tracing a case:

    |--METHOD_DEF -> METHOD_DEF [3:4]
    ......
    |       `--RCURLY -> } [3:28]
    |--METHOD_DEF -> METHOD_DEF [11:4]
    |   |   |--SINGLE_LINE_COMMENT -> // [5:4]
    |   |   |   `--COMMENT_CONTENT ->  single line comment\n [5:6]
    |   |   |--BLOCK_COMMENT_BEGIN -> /* [7:4]
    |   |   |   |--COMMENT_CONTENT -> * Some comment with empty line\n\n     [7:6]
    |   |   |   `--BLOCK_COMMENT_END -> */ [9:4]
    |   |   |--SINGLE_LINE_COMMENT -> // [10:4]
    |   |   |   `--COMMENT_CONTENT ->  comment over method2\n [10:6]

previous method in on line 3
next method is on line 11
11 - 3 = 8 lines are some where
look at comments
there is smth on line 5 ---- so line 4 is EMPTY !
theres is smth on line 7 ...... but nothing is on line 6 , so 6 is EMPTY !
block from line 7 finishes at line 9 , so line 8 is empty but is inside some block - skip
there is smth on line 10, ok
method is on line 11, ok
SO there is 2 EMPTY lines

@romani

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2018

@jjlharrison , fyi .

@kazachka

This comment has been minimized.

Copy link
Collaborator

commented Nov 7, 2018

I'm on it

@perlun

This comment has been minimized.

Copy link

commented Apr 1, 2019

@kazachka How are you doing with the fix? I personally would find it great to be able to enforce a (single) blank line before comments, because I think it's easier to read code formatted that way. Not sure if your PR will help accomplish this though?

kazachka added a commit to kazachka/checkstyle that referenced this issue Apr 1, 2019

@kazachka

This comment has been minimized.

Copy link
Collaborator

commented Apr 1, 2019

@perlun Changes to checkstyle code are done, but I need one more PR to pgjdbc to be merged in order to pass CI.
In the PR EmptyLineSeparatorCheck has been changed to not disallow to have more that one empty line between different comments, so hopefully it will help.

kazachka added a commit to kazachka/checkstyle that referenced this issue Apr 2, 2019

kazachka added a commit to kazachka/checkstyle that referenced this issue Apr 2, 2019

kazachka added a commit to kazachka/checkstyle that referenced this issue Apr 2, 2019

kazachka added a commit to kazachka/checkstyle that referenced this issue Apr 3, 2019

kazachka added a commit to kazachka/checkstyle that referenced this issue Apr 5, 2019

romani added a commit that referenced this issue Apr 27, 2019

@romani romani added the bug label Apr 27, 2019

@romani romani added this to the 8.20 milestone Apr 27, 2019

@romani

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2019

fix is merged, but implementation is based not on AST, it will not be very reliable, but at least we got significant step forward.

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue Apr 27, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue Apr 28, 2019

@romani romani closed this Apr 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.