ParenPad: thinks precedence parens are a METHOD_CALL instead of an EXPR, new token TokenTypes.DOT should be supported #3048

Closed
WadeWalker opened this Issue Mar 20, 2016 · 16 comments

Comments

Projects
None yet
5 participants
@WadeWalker

WadeWalker commented Mar 20, 2016

Hi guys! Firstly, thank you for all your efforts over the years with Checkstyle -- I've gotten a lot of use out of it, both personally and at work :)

Secondly, I'd like to report what looks like a bug. When upgrading from Checkstyle 5.6 to 6.16.1, I noticed a bunch of new ParenPad warnings. They were triggered by things like this:

E:\Users\Wade\Downloads\Checkstyle bug report>javac Test.java

E:\Users\Wade\Downloads\Checkstyle bug report>cat Test.java
package test;

public class Test {
    static void main( String [] args ) {
        String s = "test";
        Object o = s;
        ((String)o).length();
    }
}
E:\Users\Wade\Downloads\Checkstyle bug report>cat config.xml
<?xml version="1.0" encoding="UTF-8"?>
<!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"> <property name="severity" value="warning"/>
        <module name="ParenPad">
            <property name="tokens" value="METHOD_CALL"/>
            <property name="option" value="space"/>
        </module>
    </module>
</module>

E:\Users\Wade\Downloads\Checkstyle bug report>java -jar checkstyle-6.17-all.jar -c config.xml Test.java
Starting audit...
[WARN] E:\Users\Wade\Downloads\Checkstyle bug report\Test.java:7:10: '(' is not followed by whitespace. [ParenPad]
[WARN] E:\Users\Wade\Downloads\Checkstyle bug report\Test.java:7:19: ')' is not preceded with whitespace. [ParenPad]
Audit done.

Here's the additional data you wanted. The expected output is no warnings. The actual output incorrectly shows the parens around "(String)o" to be a METHOD_CALL.

It seems like Checkstyle now thinks the parens around (Wavefront)oElement are a method call instead of just an expression. If I remove METHOD_CALL from the ParenPad configuration, the warning goes away. Or if I put spaces inside like this
( (Wavefront)oElement ).getType()
the warning also goes away. Sorry, but I don't know exactly which version of Checkstyle this change occurred in, since I hadn't upgraded for a while.

Please let me know if you need any additional information, and thanks again!

@WadeWalker WadeWalker changed the title from ParenPad thinks precenence parens are a METHOD_CALL instead of an EXPR to ParenPad thinks precedence parens are a METHOD_CALL instead of an EXPR Mar 20, 2016

@gsmet

This comment has been minimized.

Show comment
Hide comment
@gsmet

gsmet Sep 23, 2016

This one is still a problem in Checkstyle 7.1.1. It hit us on the Hibernate projects and we had to disable the METHOD_CALL element for the ParenPad rule.

Looks like this bug report is complete now but if anything is missing, I can provide further information.

Any chance it could get some attention?

Thanks!

gsmet commented Sep 23, 2016

This one is still a problem in Checkstyle 7.1.1. It hit us on the Hibernate projects and we had to disable the METHOD_CALL element for the ParenPad rule.

Looks like this bug report is complete now but if anything is missing, I can provide further information.

Any chance it could get some attention?

Thanks!

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 14, 2017

Member

Issue is approved, sorry for delay

Member

romani commented Mar 14, 2017

Issue is approved, sorry for delay

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 14, 2017

Member

pre-assigned to @subkrish

Member

romani commented Mar 14, 2017

pre-assigned to @subkrish

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Mar 14, 2017

Contributor

I am on it.

Contributor

subkrish commented Mar 14, 2017

I am on it.

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Mar 17, 2017

Contributor

Regarding this issue, is there anything specific I should be looking at? I have gone through the files including and related to ParenPadCheck. Also adding to this, when I added another module TypecastParenPad to the config file it gave another warning from the Test.java file for typecast white space.

Contributor

subkrish commented Mar 17, 2017

Regarding this issue, is there anything specific I should be looking at? I have gone through the files including and related to ParenPadCheck. Also adding to this, when I added another module TypecastParenPad to the config file it gave another warning from the Test.java file for typecast white space.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 18, 2017

Member

please add user code to input file, make UT method that have the same config as in issue - do debuging.

Member

romani commented Mar 18, 2017

please add user code to input file, make UT method that have the same config as in issue - do debuging.

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Mar 20, 2017

Contributor

I wanted to add something I noticed. The warning is first caused in checkstyle-6.8. Also in the config.xml file if the property value for 'option' is changed from 'space' to 'nospace', the warning disappears. Also when I had a look at the code for checkstyle-6.7 it looked like there was no direct check for METHOD_CALL.

Contributor

subkrish commented Mar 20, 2017

I wanted to add something I noticed. The warning is first caused in checkstyle-6.8. Also in the config.xml file if the property value for 'option' is changed from 'space' to 'nospace', the warning disappears. Also when I had a look at the code for checkstyle-6.7 it looked like there was no direct check for METHOD_CALL.

subkrish pushed a commit to subkrish/checkstyle that referenced this issue Mar 20, 2017

subkrish pushed a commit to subkrish/checkstyle that referenced this issue Mar 20, 2017

subkrish pushed a commit to subkrish/checkstyle that referenced this issue Mar 21, 2017

subkrish pushed a commit to subkrish/checkstyle that referenced this issue Mar 21, 2017

subkrish pushed a commit to subkrish/checkstyle that referenced this issue Mar 22, 2017

subkrish pushed a commit to subkrish/checkstyle that referenced this issue Mar 23, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 24, 2017

Member
$ cat MyClass.java 
public class MyClass {
    static void main( String [] args ) {
        String s = "test";
        Object o = s;
        ((String)o).equals(s);
    }
}

$ java -jar checkstyle-7.6-all.jar -t MyClass.java | grep "\[5:"
    |       |--EXPR -> EXPR [5:26]
    |       |   `--METHOD_CALL -> ( [5:26]
    |       |       |--DOT -> . [5:19]
    |       |       |   |--LPAREN -> ( [5:8]
    |       |       |   |--TYPECAST -> ( [5:9]
    |       |       |   |   |--TYPE -> TYPE [5:10]
    |       |       |   |   |   `--IDENT -> String [5:10]
    |       |       |   |   |--RPAREN -> ) [5:16]
    |       |       |   |   `--IDENT -> o [5:17]
    |       |       |   |--RPAREN -> ) [5:18]
    |       |       |   `--IDENT -> equals [5:20]
    |       |       |--ELIST -> ELIST [5:27]
    |       |       |   `--EXPR -> EXPR [5:27]
    |       |       |       `--IDENT -> s [5:27]
    |       |       `--RPAREN -> ) [5:28]
    |       |--SEMI -> ; [5:29]

So as you see from AST, METHOD_CALL token is contains methods call and expression of typecast and parentheses around them.
What we can do is skip completely DOT sub tree when user define METHOD_CALL tag and add new token DOT to set of tokens.
This will allow users to define rules separately for METHOD_CALL and any expression/parentheses

@subkrish , please redo PR (you can close PR as fix will be completely different) and add new token to Check, METHOD_CALL should skip validation non its parentheses.

@gsmet, we did investigation report on hibernate-orm project https://subkrish.github.io/hibernate-orm/index.html , and looks like code should use "space" option and code is formatted with spaces for expressions on type cast. Please point us in what hibernate project(+ link to config) you disabled a rule, we will test fix on your code.

Member

romani commented Mar 24, 2017

$ cat MyClass.java 
public class MyClass {
    static void main( String [] args ) {
        String s = "test";
        Object o = s;
        ((String)o).equals(s);
    }
}

$ java -jar checkstyle-7.6-all.jar -t MyClass.java | grep "\[5:"
    |       |--EXPR -> EXPR [5:26]
    |       |   `--METHOD_CALL -> ( [5:26]
    |       |       |--DOT -> . [5:19]
    |       |       |   |--LPAREN -> ( [5:8]
    |       |       |   |--TYPECAST -> ( [5:9]
    |       |       |   |   |--TYPE -> TYPE [5:10]
    |       |       |   |   |   `--IDENT -> String [5:10]
    |       |       |   |   |--RPAREN -> ) [5:16]
    |       |       |   |   `--IDENT -> o [5:17]
    |       |       |   |--RPAREN -> ) [5:18]
    |       |       |   `--IDENT -> equals [5:20]
    |       |       |--ELIST -> ELIST [5:27]
    |       |       |   `--EXPR -> EXPR [5:27]
    |       |       |       `--IDENT -> s [5:27]
    |       |       `--RPAREN -> ) [5:28]
    |       |--SEMI -> ; [5:29]

So as you see from AST, METHOD_CALL token is contains methods call and expression of typecast and parentheses around them.
What we can do is skip completely DOT sub tree when user define METHOD_CALL tag and add new token DOT to set of tokens.
This will allow users to define rules separately for METHOD_CALL and any expression/parentheses

@subkrish , please redo PR (you can close PR as fix will be completely different) and add new token to Check, METHOD_CALL should skip validation non its parentheses.

@gsmet, we did investigation report on hibernate-orm project https://subkrish.github.io/hibernate-orm/index.html , and looks like code should use "space" option and code is formatted with spaces for expressions on type cast. Please point us in what hibernate project(+ link to config) you disabled a rule, we will test fix on your code.

@gsmet

This comment has been minimized.

Show comment
Hide comment

gsmet commented Mar 24, 2017

Hi @romani ,

The issue is with Hibernate Search: https://github.com/hibernate/hibernate-search .

More specifically, you can find our checkstyle config here: https://github.com/hibernate/hibernate-search/blob/master/build-config/src/main/resources/checkstyle.xml#L131 .

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 24, 2017

Member

@subkrish , please add hibernate-search repo to your regression testing.

Member

romani commented Mar 24, 2017

@subkrish , please add hibernate-search repo to your regression testing.

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Mar 25, 2017

Contributor

@romani I will add it during regression testing.

Contributor

subkrish commented Mar 25, 2017

@romani I will add it during regression testing.

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Mar 25, 2017

Contributor

@romani
Referring to what you said

"METHOD_CALL should skip validation non its parentheses"

Does that mean it should not raise a warning like this which was raised after I added the the token DOT:

java -jar checkstyle-7.7-SNAPSHOT-all.jar -c config.xml MyClass.java Starting audit... [WARN] /Users/subbudantu/Downloads/MyClass.java:5:28: '(' is not followed by whitespace. [ParenPad] [WARN] /Users/subbudantu/Downloads/MyClass.java:5:29: ')' is not preceded with whitespace. [ParenPad] Audit done.

Contributor

subkrish commented Mar 25, 2017

@romani
Referring to what you said

"METHOD_CALL should skip validation non its parentheses"

Does that mean it should not raise a warning like this which was raised after I added the the token DOT:

java -jar checkstyle-7.7-SNAPSHOT-all.jar -c config.xml MyClass.java Starting audit... [WARN] /Users/subbudantu/Downloads/MyClass.java:5:28: '(' is not followed by whitespace. [ParenPad] [WARN] /Users/subbudantu/Downloads/MyClass.java:5:29: ')' is not preceded with whitespace. [ParenPad] Audit done.

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Mar 27, 2017

Contributor

@romani I ran into another issue. Once I added DOT tokentype to the set of tokens, a NullPointerException was raised when I was testing the method testNospaceWithComplexInput() in ParenPadCheckTest.java. I can't seem to find why this exception is being raised.

Contributor

subkrish commented Mar 27, 2017

@romani I ran into another issue. Once I added DOT tokentype to the set of tokens, a NullPointerException was raised when I was testing the method testNospaceWithComplexInput() in ParenPadCheckTest.java. I can't seem to find why this exception is being raised.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Mar 28, 2017

Member

@subkrish We can't see your code until you push it into a PR, especially without a stacktrace.
Trying debugging the test where the stacktrace points to and see why it is null. Don't expect code to work as-is without any changes for that specific token.
If you still have issues, post code and problem to mailing list.

Member

rnveach commented Mar 28, 2017

@subkrish We can't see your code until you push it into a PR, especially without a stacktrace.
Trying debugging the test where the stacktrace points to and see why it is null. Don't expect code to work as-is without any changes for that specific token.
If you still have issues, post code and problem to mailing list.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Apr 9, 2017

Member

Current PR for this issue will be a partial fix.
Main issue with check was identified and be finally fixed in Issue #4175.

Member

rnveach commented Apr 9, 2017

Current PR for this issue will be a partial fix.
Main issue with check was identified and be finally fixed in Issue #4175.

subkrish pushed a commit to subkrish/checkstyle that referenced this issue Apr 13, 2017

subkrish pushed a commit to subkrish/checkstyle that referenced this issue Apr 13, 2017

romani added a commit that referenced this issue Apr 16, 2017

@romani romani added the new feature label Apr 16, 2017

@romani romani added this to the 7.7 milestone Apr 16, 2017

@romani romani changed the title from ParenPad thinks precedence parens are a METHOD_CALL instead of an EXPR to ParenPad: thinks precedence parens are a METHOD_CALL instead of an EXPR, new token TokenTypes.DOT should be supported Apr 16, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Apr 16, 2017

Member

fix is merged

Member

romani commented Apr 16, 2017

fix is merged

@romani romani closed this Apr 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment