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

Incorrect warning for empty lambda bodies with google_checks.xml #6381

Closed
cushon opened this Issue Jan 16, 2019 · 4 comments

Comments

Projects
None yet
4 participants
@cushon
Copy link
Contributor

cushon commented Jan 16, 2019

class T {
  Runnable runnable = () -> {};
}
$ java -jar checkstyle-8.16-all.jar -c google_checks.xml T.java
Starting audit...
[WARN] /tmp/T.java:2:29: WhitespaceAround: '{' is not followed by whitespace. Empty blocks may only be represented as {} when not part of a multi-block statement (4.1.3) [WhitespaceAround]
[WARN] /tmp/T.java:2:30: WhitespaceAround: '}' is not preceded with whitespace. [WhitespaceAround]
Audit done.

I used google_checks.xml from: https://raw.githubusercontent.com/checkstyle/checkstyle/master/src/main/resources/google_checks.xml

Both warnings are incorrect.

The first warning cites §4.1.3, but the style guide clarifies that 'multi-block statement' refers to if/else or try/catch/finally. An empty lambda like () -> {} doesn't contain multiple blocks (and also isn't a statement).

The style guide provides a complete list of places where horizontal whitespace is required in §4.6.2 and does not mention lambda bodies.

@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented Jan 17, 2019

It seems that this was added with #2924 when the token was added to the default tokens as google XML config uses the default tokens. I am seeing no conversations about google config in the issue or PR, so it could have been by accident.

We have had issues with the style guide as it isn't always complete and is sometimes ambiguous and google has never responded to issues we have raised with their wording.

§4.1.3, but the style guide clarifies that 'multi-block statement' refers to if/else or try/catch/finally. An empty lambda like () -> {} doesn't contain multiple blocks

@romani I agree with the reading of the guide here and the issue stated.
I also think we should remove all default token usage in google's config and hardcode the list of tokens it uses for all checks. This way, if a new default token is added to a check then AllChecksTest.testAllCheckTokensAreReferencedInGoogleConfigFile(link) should ping it as not being used for google and force us to review it.

If you agree with everything, please approve the issue.

@romani

This comment has been minimized.

Copy link
Member

romani commented Jan 17, 2019

as @rnveach stated, workaround is to update tokens and remove LAMBDA from it. Full list of tokens is at https://checkstyle.org/config_whitespace.html#WhitespaceAround_Properties

should ping it as not being used for google and force us to review it

good idea. But should be register in separate issue.

@ehiggs

This comment has been minimized.

Copy link

ehiggs commented Jan 28, 2019

@rnveach rnveach added the easy label Jan 28, 2019

fzdy1914 added a commit to fzdy1914/checkstyle that referenced this issue Feb 3, 2019

fzdy1914 added a commit to fzdy1914/checkstyle that referenced this issue Feb 3, 2019

fzdy1914 added a commit to fzdy1914/checkstyle that referenced this issue Feb 12, 2019

fzdy1914 added a commit to fzdy1914/checkstyle that referenced this issue Feb 12, 2019

fzdy1914 added a commit to fzdy1914/checkstyle that referenced this issue Feb 12, 2019

fzdy1914 added a commit to fzdy1914/checkstyle that referenced this issue Feb 13, 2019

fzdy1914 added a commit to fzdy1914/checkstyle that referenced this issue Feb 17, 2019

romani added a commit that referenced this issue Feb 20, 2019

@romani romani added the bug label Feb 20, 2019

@romani romani added this to the 8.18 milestone Feb 20, 2019

@romani

This comment has been minimized.

Copy link
Member

romani commented Feb 20, 2019

Fix is merged.
@fzdy1914, thanks a lot for your contribution!

@romani romani closed this Feb 20, 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.