RequireThis treats local variables as properties #3423

Closed
artembilan opened this Issue Sep 1, 2016 · 11 comments

Comments

Projects
None yet
6 participants
@artembilan

artembilan commented Sep 1, 2016

/var/tmp $ javac Test.java

/var/tmp $ cat Test.java

public class Test {

    private final String s = "foo";

    public String getS() {
        String s = null;
        s = "bar"; // ??? Reference to instance variable 's' needs "this.". [RequireThis] !!!
        return s; 
    }

    public String getS(String s) {
        s = "bar"; // no violation
        return s;
    }

}

/var/tmp $ cat config.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>

/var/tmp $ java -jar checkstyle-7.1-all.jar -c config.xml Test.java

Starting audit...
[ERROR] d:\tmp\Test.java:7:17: Reference to instance variable 's' needs "this.". [RequireThis]
Audit done.
Checkstyle ends with 1 errors.

I really expect that local variable doesn't overlap to the property.
If I'd use property I'd specify this. for it.

That works correctly before version 6.17.


Thanks

@romani romani added the approved label Sep 10, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
Member

romani commented Sep 10, 2016

@averza

This comment has been minimized.

Show comment
Hide comment
@averza

averza Feb 22, 2017

Hi,
It is still not fixed in the latest Checkstyle 7.5.1...

averza commented Feb 22, 2017

Hi,
It is still not fixed in the latest Checkstyle 7.5.1...

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Feb 22, 2017

Contributor

@averza
Yes. The issue is not closed, so it has not fixed yet.

Contributor

MEZk commented Feb 22, 2017

@averza
Yes. The issue is not closed, so it has not fixed yet.

@rnveach rnveach added the medium label Mar 2, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 3, 2017

Member

pre-assigned to @sagar-shah94

Member

romani commented Mar 3, 2017

pre-assigned to @sagar-shah94

@ps-sp

This comment has been minimized.

Show comment
Hide comment
@ps-sp

ps-sp Mar 3, 2017

Collaborator

I am on it too.

Collaborator

ps-sp commented Mar 3, 2017

I am on it too.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 3, 2017

Member

@sagar-shah94 please choose another issue.

Member

romani commented Mar 3, 2017

@sagar-shah94 please choose another issue.

@ps-sp

This comment has been minimized.

Show comment
Hide comment
@ps-sp

ps-sp Mar 8, 2017

Collaborator
class InputAllowNonOverlappingLocalVars {
    String s3 = null;
    void foo() {
        try {
            if (true) {
                String s3 = "Hello, World!";
                if (true) {
                    s3 = new String(); // No violation ?
                }
                else {
                    s3 = new String(); // No violation ?
                }
            }
        }
        catch (Exception ex) {
        }
    }
}

This would be the acceptable behavior right ?

Collaborator

ps-sp commented Mar 8, 2017

class InputAllowNonOverlappingLocalVars {
    String s3 = null;
    void foo() {
        try {
            if (true) {
                String s3 = "Hello, World!";
                if (true) {
                    s3 = new String(); // No violation ?
                }
                else {
                    s3 = new String(); // No violation ?
                }
            }
        }
        catch (Exception ex) {
        }
    }
}

This would be the acceptable behavior right ?

@ps-sp

This comment has been minimized.

Show comment
Hide comment
@ps-sp

ps-sp Mar 8, 2017

Collaborator

Also, when variableDeclarationFrame would be BLOCK_FRAME and there's no overlapping then there can never be a violation ! Correct me if I am wrong. Thanks.

Collaborator

ps-sp commented Mar 8, 2017

Also, when variableDeclarationFrame would be BLOCK_FRAME and there's no overlapping then there can never be a violation ! Correct me if I am wrong. Thanks.

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Mar 8, 2017

Issue #3423: Allow non-overlapping overshadowing local vars without a…
…ny violations from RequireThisCheck. Added UT. Fixed related UT

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Mar 8, 2017

Issue #3423: Allow non-overlapping overshadowing local vars without a…
…ny violations from RequireThisCheck. Added UT. Fixed related UT
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 9, 2017

Member

This would be the acceptable behavior right ?

No violations in this case as Check is enforcing "this." To appear. There is no way to but "this." In your example.

Member

romani commented Mar 9, 2017

This would be the acceptable behavior right ?

No violations in this case as Check is enforcing "this." To appear. There is no way to but "this." In your example.

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Mar 10, 2017

Issue #3423: Allow local vars without any unnecessary violations from…
… RequireThisCheck. Added UT. Fixed related UT

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Mar 18, 2017

Issue #3423: Allow local vars without any unnecessary violations from…
… RequireThisCheck. Added UT. Fixed related UT

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Mar 20, 2017

Issue #3423: Allow local vars without any unnecessary violations from…
… RequireThisCheck. Added UT. Fixed related UT

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

Issue #3423: Allow local vars without any unnecessary violations from…
… RequireThisCheck. Added UT. Fixed related UT

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

Issue #3423: Allow local vars without any unnecessary violations from…
… RequireThisCheck. Added UT. Fixed related UT

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

Issue #3423: Allow local vars without any unnecessary violations from…
… RequireThisCheck. Added UT. Fixed related UT

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

Issue #3423: Allow local vars without any unnecessary violations from…
… RequireThisCheck. Added UT. Fixed related UT

@romani romani added the bug label Apr 13, 2017

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

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Apr 13, 2017

Member

fix is merged

Member

romani commented Apr 13, 2017

fix is merged

@romani romani closed this Apr 13, 2017

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

Issue #3423: Allow local vars without any unnecessary violations from…
… RequireThisCheck. Added UT. Fixed related UT
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Apr 13, 2017

Member

For some reason I failed to actually press merge from web.
Now PR is merged.

Member

romani commented Apr 13, 2017

For some reason I failed to actually press merge from web.
Now PR is merged.

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