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

RequireThis: False positive for lambda parameters #4207

Closed
MEZk opened this Issue Apr 12, 2017 · 4 comments

Comments

Projects
None yet
4 participants
@MEZk
Contributor

MEZk commented Apr 12, 2017

$ cat TestClass.java
class InputAllowLocalVars {

    private String s1 = "foo1";

    void foo1() {
        final java.util.List<String> strings = new java.util.ArrayList<>();
            strings.add("foo1");
            strings.stream().filter(s1 -> s1.contains("f"))
            .collect(java.util.stream.Collectors.toList());
    }
}

$ 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">
    <module name="TreeWalker">
        <module name="RequireThis">
            <property name="checkMethods" value="false" />
            <property name="validateOnlyOverlapping" value="false" />
        </module>
    </module>
</module>


$ java -jar checkstyle-7.6.1-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:8:37: Reference to instance variable 's1' needs "this.". [RequireThis]
[ERROR] TestClass.java:8:43: Reference to instance variable 's1' needs "this.". [RequireThis]
Audit done.
Checkstyle ends with 2 errors.

http://rveach.no-ip.org/checkstyle/checkstyle.php?action=view&config=2b75d3a2ecffeba47690e0416ccd9906&code=c5b8991fb27367d9bfbf7d120993a7cb&checkstyle=checkstyle-7.6.1-all.jar&printTree=


Expected: no violations as this cannot be used for s1 on line 8 and column 37 (compile time error); s1 on line 8 and column 43 is a lambda parameter and it does not make sense to prefix it with this.

Note, that if validateOnlyOverlapping is set to true, the violations disappear.

@ps-sp

This comment has been minimized.

Show comment
Hide comment
@ps-sp

ps-sp Apr 13, 2017

Collaborator

I am on it.

Collaborator

ps-sp commented Apr 13, 2017

I am on it.

@ps-sp

This comment has been minimized.

Show comment
Hide comment
@ps-sp

ps-sp Apr 14, 2017

Collaborator
class foo {   

     int x;
     int z;
     void methodInFirstLevel(int x) {
        
         //There would be no compiler errors for the statement A
         //because the parameter x is effectively final.
         
         Consumer<Integer> myConsumer = (y) -> 
         {
             System.out.println("x = " + x);         // Statement A
             System.out.println("y = " + y);
             System.out.println("this.x = " + this.x);
             System.out.println("LambdaScopeTest.this.x = " +
                 LambdaScopeTest.this.x);
             z++;                //Statement B
         };

         myConsumer.accept(x);

     }
}

Would it be desirable for RequireThis to report an error for statement A ? Currently there's no violation for x in statement A

@MEZk @romani please address. Thanks.

UPDATE: there's no violation for statement A because it's previous sibling isn't null and neither it's parent an assignment token. This policy has been maintained throughout RequireThis and holds for all the blocks and not just lambdas. Therefore I feel there shouldn't be any violation for statement A.

Also, in the current form, RequireThis points a violation for statement B. That is violation is acceptable, right ?

@rnveach Please address. Thanks

Collaborator

ps-sp commented Apr 14, 2017

class foo {   

     int x;
     int z;
     void methodInFirstLevel(int x) {
        
         //There would be no compiler errors for the statement A
         //because the parameter x is effectively final.
         
         Consumer<Integer> myConsumer = (y) -> 
         {
             System.out.println("x = " + x);         // Statement A
             System.out.println("y = " + y);
             System.out.println("this.x = " + this.x);
             System.out.println("LambdaScopeTest.this.x = " +
                 LambdaScopeTest.this.x);
             z++;                //Statement B
         };

         myConsumer.accept(x);

     }
}

Would it be desirable for RequireThis to report an error for statement A ? Currently there's no violation for x in statement A

@MEZk @romani please address. Thanks.

UPDATE: there's no violation for statement A because it's previous sibling isn't null and neither it's parent an assignment token. This policy has been maintained throughout RequireThis and holds for all the blocks and not just lambdas. Therefore I feel there shouldn't be any violation for statement A.

Also, in the current form, RequireThis points a violation for statement B. That is violation is acceptable, right ?

@rnveach Please address. Thanks

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Apr 15, 2017

Contributor

@ps-sp

there's no violation for statement A because it's previous sibling isn't null and neither it's parent an assignment token.

Yes, this is not an assignment.

Also, in the current form, RequireThis points a violation for statement B. That is violation is acceptable, right ?

Yes, this is an assignment.

Contributor

MEZk commented Apr 15, 2017

@ps-sp

there's no violation for statement A because it's previous sibling isn't null and neither it's parent an assignment token.

Yes, this is not an assignment.

Also, in the current form, RequireThis points a violation for statement B. That is violation is acceptable, right ?

Yes, this is an assignment.

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Apr 15, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue May 22, 2017

rnveach added a commit that referenced this issue May 23, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach May 23, 2017

Member

Fix is merged

Member

rnveach commented May 23, 2017

Fix is merged

@rnveach rnveach closed this May 23, 2017

@rnveach rnveach added the bug label May 23, 2017

@rnveach rnveach added this to the 7.8 milestone May 23, 2017

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