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

OneStatementPerLine: false-positive on try-with-resource when objects just referenced #6125

Closed
boris-petrov opened this issue Sep 20, 2018 · 9 comments

Comments

Projects
None yet
3 participants
@boris-petrov
Copy link

commented Sep 20, 2018

$ cat TestClass.java
public class TestClass {
    void method() {
        var stream1 = new PipedOutputStream();
        var stream2 = new PipedOutputStream();
        try (stream1; stream2; var stream3 = new PipedOutputStream()) {
        }
    }
}

$ cat TestConfig.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">
    <property name="charset" value="UTF-8"/>

    <module name="TreeWalker">
       <module name="OneStatementPerLineCheck" />
    </module>
</module>

$ java -jar checkstyle-8.12-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:5:30: Only one statement per line allowed. [OneStatementPerLine]
Audit done.
Checkstyle ends with 1 errors.

This gives a OneStatementPerLine warning which is wrong I think. Checkstyle 8.12, Java 10.0.2.

@romani

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

This is interesting case ... this Check what it to be like:

$ cat TestClass.java
public class TestClass {
    void method() {
        var stream1 = new PipedOutputStream();
        var stream2 = new PipedOutputStream();
        try (stream1;
             stream2;
             var stream3 = new PipedOutputStream()) {
        }
    }
}

$ java -jar checkstyle-8.13-all.jar -c TestConfig.xml TestClass.java
Starting audit...
Audit done.

and it is the same as :

//Each line causes violation:
int var1; int var2;
var1 = 1; var2 = 2;

from https://checkstyle.org/config_coding.html#OneStatementPerLine
So technically Check works properly, as use can put a lot of code in try, and it is not clear when to demand all be on their own line or not.
@rnveach , @pbludov , please share your ideas on this.

@boris-petrov , to avoid violation please try to use https://checkstyle.org/config_filters.html#SuppressionXpathFilter and share here configuration, some other people might find it useful too.

@rnveach

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

@romani This seems to me like it might be a false positive.
According to https://docs.oracle.com/javase/specs/jls/se7/html/jls-14.html#jls-14.20.3 , Resource is not a statement and the check is clearly named Statement. The only thing that is a statement here is the full try/catch/finally, not the resources by themselves.

//Each line causes violation:
int var1; int var2;

Your analogy is incorrect. int var is a statement by itself and is not similar to the resources in try-with-resource.
For comparison, the correct analogy for try-with-resource would be int var1, var2; which does not cause a violation in this check.

@romani

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

@rnveach , here is Input with many cases (and not only try .... ) - https://github.com/checkstyle/checkstyle/blob/master/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/onestatementperline/InputOneStatementPerLineMultiline.java

Check is not designed to be configured for different tokens - https://checkstyle.org/config_coding.html#OneStatementPerLine

We already did fix for similar cases - #2211 , our test inputs are explicitly focused to catch similar cases - 10f348b

Looks like there is a slight conflict of interests..... BUT it looks like we need update Check to allow single-line if no "=" is present between ";".
try (s1; s2; var s3 = new PipedOutputStream()) // OK
try (s1=new PipedOutputStream(); s2; var s3 = new PipedOutputStream()) // VIOLATION

@rnveach and @boris-petrov is it ok ?

@boris-petrov

This comment has been minimized.

Copy link
Author

commented Oct 23, 2018

@rnveach

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

I already stated that I think this check should ignore resources in try. The same way we ignore multiple variables in a single variable declaration statement (int var1, var2;)
They aren't statements.

@romani

This comment has been minimized.

Copy link
Member

commented Oct 27, 2018

Yes, that’s exactly what I think this rule should do.

Ok, issue is approved.

(int var1, var2;) They aren't statements

Ok, but we need to give users some new check to cover such case, before, we start to ignore this case in this Check.

@romani romani added the approved label Oct 27, 2018

@romani romani changed the title OneStatementPerLine false-positive OneStatementPerLine: false-positive on try-with-resource when objects just referenced Oct 27, 2018

strkkk added a commit to strkkk/checkstyle that referenced this issue May 8, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue May 8, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue May 8, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue May 8, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue May 9, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue May 9, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue May 9, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue May 10, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue May 10, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue May 11, 2019

@rnveach

This comment has been minimized.

Copy link
Member

commented May 11, 2019

@romani When we approved the issue, did you look into the impact with the google config as it is listed there?
http://checkstyle.sourceforge.net/google_style.html#a4.3

One statement per line
Each statement is followed by a line break.

Since this isn't a statement we aren't violating the guide, but we are placing a violation in a spot that has nothing to do with statements per the JLS. Is it ok for a check to do more then what the guide says by default?
I myself am fine to continue, I just want to make sure it is brought up and you agree.

strkkk added a commit to strkkk/checkstyle that referenced this issue May 11, 2019

@romani

This comment has been minimized.

Copy link
Member

commented May 15, 2019

I understand your concerns, this is hard decision.

I think we should continue with special treatment of "try-with-resources" expressions as statements when they have = sign. I am pretty sure there will be bunch of users who disagree. But lets give them a chance to request new feature/property in Check to no treat "try-with-resources" expressions as statements at all.
I am ok to have our google config to do a bit more, it might bring attention from community on this if somebody is disagree. If new request come up we will need to open ticket against google style guide for clarification (just to know how to setup Check in config).

strkkk added a commit to strkkk/checkstyle that referenced this issue May 15, 2019

romani added a commit that referenced this issue May 16, 2019

@romani romani added the bug label May 16, 2019

@romani romani added this to the 8.21 milestone May 16, 2019

@romani

This comment has been minimized.

Copy link
Member

commented May 16, 2019

Fix is merged.
@strkkk , thanks a lot for fix

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.