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

Google Style: Incorrect ParenPad warning #4294

Closed
cushon opened this Issue Apr 25, 2017 · 9 comments

Comments

Projects
None yet
4 participants
@cushon
Contributor

cushon commented Apr 25, 2017

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

The Google Java Style Guide requires a space after ; [1], so the following warning is incorrect.

[1] https://google.github.io/styleguide/javaguide.html#s4.6.2-horizontal-whitespace

After ,:; or the closing parenthesis ')' of a cast

class T {
  {
    for (; ; ) {
      //
    }
    try (InputStream is = null; ) { // violation
      //
    }
  }
}
$ java -jar checkstyle-7.6-all.jar -c google_checks.xml T.java
[WARN] T.java:6:32: ')' is preceded with whitespace. [ParenPad]

@romani romani added the approved label Apr 28, 2017

@romani romani changed the title from Incorrect ParenPad warning to Google Style: Incorrect ParenPad warning Apr 28, 2017

@soon

This comment has been minimized.

Show comment
Hide comment
@soon

soon May 6, 2017

Contributor

@romani, The ParenPad check now contains only option parameter, which could be either SPACE or NOSPACE. I want to add another option like spaceAfterSemicolon (or a list of tokens, which should be followed with a space), which will force a user to put a space between a semicolon and a right parenthesis.

Is it acceptable, or there is another solution?

Contributor

soon commented May 6, 2017

@romani, The ParenPad check now contains only option parameter, which could be either SPACE or NOSPACE. I want to add another option like spaceAfterSemicolon (or a list of tokens, which should be followed with a space), which will force a user to put a space between a semicolon and a right parenthesis.

Is it acceptable, or there is another solution?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani May 7, 2017

Member

AST tree:

$ java -jar checkstyle-7.6.1-all.jar -t T.java
CLASS_DEF -> CLASS_DEF [1:0]
|--MODIFIERS -> MODIFIERS [1:0]
|--LITERAL_CLASS -> class [1:0]
|--IDENT -> T [1:6]
`--OBJBLOCK -> OBJBLOCK [1:16]
    |--LCURLY -> { [1:16]
    |--INSTANCE_INIT -> INSTANCE_INIT [2:2]
    |   `--SLIST -> { [2:2]
    |       |--LITERAL_FOR -> for [3:4]
    |       |   |--LPAREN -> ( [3:8]
    |       |   |--FOR_INIT -> FOR_INIT [3:9]
    |       |   |--SEMI -> ; [3:9]
    |       |   |--FOR_CONDITION -> FOR_CONDITION [3:11]
    |       |   |--SEMI -> ; [3:11]
    |       |   |--FOR_ITERATOR -> FOR_ITERATOR [3:13]
    |       |   |--RPAREN -> ) [3:13]
    |       |   `--SLIST -> { [3:15]
    |       |       `--RCURLY -> } [5:4]
    |       |--LITERAL_TRY -> try [6:4]
    |       |   |--RESOURCE_SPECIFICATION -> RESOURCE_SPECIFICATION [6:8]
    |       |   |   |--LPAREN -> ( [6:8]
    |       |   |   |--RESOURCES -> RESOURCES [6:9]
    |       |   |   |   `--RESOURCE -> RESOURCE [6:9]
    |       |   |   |       |--MODIFIERS -> MODIFIERS [6:9]
    |       |   |   |       |--TYPE -> TYPE [6:9]
    |       |   |   |       |   `--IDENT -> InputStream [6:9]
    |       |   |   |       |--IDENT -> is [6:21]
    |       |   |   |       `--ASSIGN -> = [6:24]
    |       |   |   |           `--EXPR -> EXPR [6:26]
    |       |   |   |               `--LITERAL_NULL -> null [6:26]
    |       |   |   |--SEMI -> ; [6:30]
    |       |   |   `--RPAREN -> ) [6:32]
    |       |   `--SLIST -> { [6:34]
    |       |       `--RCURLY -> } [8:4]
    |       `--RCURLY -> } [9:2]
    `--RCURLY -> } [10:0]

ParenPad is used in default mode in config - https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/google_checks.xml#L177

http://checkstyle.sourceforge.net/config_whitespace.html#ParenPad
default tokens are LITERAL_FOR and RESOURCE_SPECIFICATION.
default for option is nospace.

@soon , Why we violation only on RESOURCE_SPECIFICATION ?

Member

romani commented May 7, 2017

AST tree:

$ java -jar checkstyle-7.6.1-all.jar -t T.java
CLASS_DEF -> CLASS_DEF [1:0]
|--MODIFIERS -> MODIFIERS [1:0]
|--LITERAL_CLASS -> class [1:0]
|--IDENT -> T [1:6]
`--OBJBLOCK -> OBJBLOCK [1:16]
    |--LCURLY -> { [1:16]
    |--INSTANCE_INIT -> INSTANCE_INIT [2:2]
    |   `--SLIST -> { [2:2]
    |       |--LITERAL_FOR -> for [3:4]
    |       |   |--LPAREN -> ( [3:8]
    |       |   |--FOR_INIT -> FOR_INIT [3:9]
    |       |   |--SEMI -> ; [3:9]
    |       |   |--FOR_CONDITION -> FOR_CONDITION [3:11]
    |       |   |--SEMI -> ; [3:11]
    |       |   |--FOR_ITERATOR -> FOR_ITERATOR [3:13]
    |       |   |--RPAREN -> ) [3:13]
    |       |   `--SLIST -> { [3:15]
    |       |       `--RCURLY -> } [5:4]
    |       |--LITERAL_TRY -> try [6:4]
    |       |   |--RESOURCE_SPECIFICATION -> RESOURCE_SPECIFICATION [6:8]
    |       |   |   |--LPAREN -> ( [6:8]
    |       |   |   |--RESOURCES -> RESOURCES [6:9]
    |       |   |   |   `--RESOURCE -> RESOURCE [6:9]
    |       |   |   |       |--MODIFIERS -> MODIFIERS [6:9]
    |       |   |   |       |--TYPE -> TYPE [6:9]
    |       |   |   |       |   `--IDENT -> InputStream [6:9]
    |       |   |   |       |--IDENT -> is [6:21]
    |       |   |   |       `--ASSIGN -> = [6:24]
    |       |   |   |           `--EXPR -> EXPR [6:26]
    |       |   |   |               `--LITERAL_NULL -> null [6:26]
    |       |   |   |--SEMI -> ; [6:30]
    |       |   |   `--RPAREN -> ) [6:32]
    |       |   `--SLIST -> { [6:34]
    |       |       `--RCURLY -> } [8:4]
    |       `--RCURLY -> } [9:2]
    `--RCURLY -> } [10:0]

ParenPad is used in default mode in config - https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/google_checks.xml#L177

http://checkstyle.sourceforge.net/config_whitespace.html#ParenPad
default tokens are LITERAL_FOR and RESOURCE_SPECIFICATION.
default for option is nospace.

@soon , Why we violation only on RESOURCE_SPECIFICATION ?

@soon

This comment has been minimized.

Show comment
Hide comment
@soon

soon May 7, 2017

Contributor

@romani The ParenPad does not check right parenthesis in the for loop if the iterator part is empty (

). The javadoc refers to the EmptyForIteratorPad check (https://github.com/checkstyle/checkstyle/blob/ce21086e087661553f3a774c38362327ee88996a/src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/EmptyForIteratorPadCheck.java)

Contributor

soon commented May 7, 2017

@romani The ParenPad does not check right parenthesis in the for loop if the iterator part is empty (

). The javadoc refers to the EmptyForIteratorPad check (https://github.com/checkstyle/checkstyle/blob/ce21086e087661553f3a774c38362327ee88996a/src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/EmptyForIteratorPadCheck.java)

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani May 7, 2017

Member

ParenPad Check is not able to cover specific nuances language constructions with delimiters inside and optional parts.
Unfortunately in try-with-resources, it is only one part, last separation by ";" is not required so user can put it or skip it..... that is why we have a problem here.

@cushon , what is a reason to keep extra ; in try ?

Member

romani commented May 7, 2017

ParenPad Check is not able to cover specific nuances language constructions with delimiters inside and optional parts.
Unfortunately in try-with-resources, it is only one part, last separation by ";" is not required so user can put it or skip it..... that is why we have a problem here.

@cushon , what is a reason to keep extra ; in try ?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani May 7, 2017

Member

Looks like we have two ways to resolve nuance:

  1. ignore try-with-resourse by ParenPad as we already have for FOR and Typecast structure. Create special Check with option=[space|nospace] and special value 'noSpaceExceptAfterSemicolon' (it will be different type from pad policy) that will cover all nuances of try-with-resources that user would like to have.
  2. some changes in ParenPad ..... let me think .... as we have option that define a way of validation. So we need new pad policy 'noSpaceExceptInnerSeparator' , separators we take from tokens of http://checkstyle.sourceforge.net/config_whitespace.html#SeparatorWrap (no need to let user define this list for now). List java separators: https://docs.oracle.com/javase/specs/jls/se8/html/jls-3.html#jls-3.11 but only ; and , could be somehow relevant here. So idea is weak ....

so decision is make option ugly to cover this conner case that to my mind should be resolved by removal extra ";" or place this case as limitation and coverage report should mention that we cover point partly. Google style need to be fixed to mention that in this particular case ";" is not required, so result space is not required too.

All Checks EmptyForIteratorPad, EmptyForInitializerPad, TypecastParenPad - are created for non avoidable syntax elements, case of this issue is very conner case that could be easily avoidable by removal of non required '';" (I see no benefit from usage of ";", one day smb create issue on us to catch such cases to forbid them).

Request to keep ; could be result of desire to have already in place separator to ease next item insertion. The same issue as users demand for ArrayTrailingComma . And as we can see there bunch of demands on how it should look like from users.

lets wait for @cushon comments on this.

Member

romani commented May 7, 2017

Looks like we have two ways to resolve nuance:

  1. ignore try-with-resourse by ParenPad as we already have for FOR and Typecast structure. Create special Check with option=[space|nospace] and special value 'noSpaceExceptAfterSemicolon' (it will be different type from pad policy) that will cover all nuances of try-with-resources that user would like to have.
  2. some changes in ParenPad ..... let me think .... as we have option that define a way of validation. So we need new pad policy 'noSpaceExceptInnerSeparator' , separators we take from tokens of http://checkstyle.sourceforge.net/config_whitespace.html#SeparatorWrap (no need to let user define this list for now). List java separators: https://docs.oracle.com/javase/specs/jls/se8/html/jls-3.html#jls-3.11 but only ; and , could be somehow relevant here. So idea is weak ....

so decision is make option ugly to cover this conner case that to my mind should be resolved by removal extra ";" or place this case as limitation and coverage report should mention that we cover point partly. Google style need to be fixed to mention that in this particular case ";" is not required, so result space is not required too.

All Checks EmptyForIteratorPad, EmptyForInitializerPad, TypecastParenPad - are created for non avoidable syntax elements, case of this issue is very conner case that could be easily avoidable by removal of non required '';" (I see no benefit from usage of ";", one day smb create issue on us to catch such cases to forbid them).

Request to keep ; could be result of desire to have already in place separator to ease next item insertion. The same issue as users demand for ArrayTrailingComma . And as we can see there bunch of demands on how it should look like from users.

lets wait for @cushon comments on this.

@cushon

This comment has been minimized.

Show comment
Hide comment
@cushon

cushon Sep 2, 2017

Contributor

what is a reason to keep extra ; in try ?

I'm not aware of any reasons to prefer the extra ;. When we see instances of this bug I've been recommending removing it as a work-around.

But it's valid Java code that shows up in the wild, and there are no plans to add a special case to the style guide for this, so it would be nice if checkstyle didn't warn about it.

Contributor

cushon commented Sep 2, 2017

what is a reason to keep extra ; in try ?

I'm not aware of any reasons to prefer the extra ;. When we see instances of this bug I've been recommending removing it as a work-around.

But it's valid Java code that shows up in the wild, and there are no plans to add a special case to the style guide for this, so it would be nice if checkstyle didn't warn about it.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 6, 2017

Member

@rnveach and @Vladlis (as you are mentioned in authors of this check)
we already got PR , but we did not decided how it should be resolved.

please review my comment #4294 (comment) with explanation of problem and possible scenarios to resolve.
The more I look to this the more I understand that try-with-resources is the same special braced element of java. In all other such java elements, trailing separator is not allowed at all.
I more in favor of option 1).

Please share ides or vote.

Member

romani commented Sep 6, 2017

@rnveach and @Vladlis (as you are mentioned in authors of this check)
we already got PR , but we did not decided how it should be resolved.

please review my comment #4294 (comment) with explanation of problem and possible scenarios to resolve.
The more I look to this the more I understand that try-with-resources is the same special braced element of java. In all other such java elements, trailing separator is not allowed at all.
I more in favor of option 1).

Please share ides or vote.

@Vladlis

This comment has been minimized.

Show comment
Hide comment
@Vladlis

Vladlis Sep 6, 2017

Member

@romani
I like the first option.

Member

Vladlis commented Sep 6, 2017

@romani
I like the first option.

cushon added a commit to cushon/checkstyle that referenced this issue Sep 6, 2017

cushon added a commit to cushon/checkstyle that referenced this issue Sep 6, 2017

cushon added a commit to cushon/checkstyle that referenced this issue Sep 7, 2017

cushon added a commit to cushon/checkstyle that referenced this issue Sep 7, 2017

cushon added a commit to cushon/checkstyle that referenced this issue Sep 8, 2017

romani added a commit that referenced this issue Sep 8, 2017

@romani romani added the bug label Sep 8, 2017

@romani romani added this to the 8.3 milestone Sep 8, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 8, 2017

Member

fix is merged.

Member

romani commented Sep 8, 2017

fix is merged.

@romani romani closed this Sep 8, 2017

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