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

MethodCountCheck: wrong counting with nested interface definition #4540

Closed
rnveach opened this Issue Jun 27, 2017 · 2 comments

Comments

Projects
None yet
2 participants
@rnveach
Member

rnveach commented Jun 27, 2017

Identified in regression at #4535 after PR was merged :

$ cat TestClass.java
public class TestClass {
    void method1() {
    }

    private @interface Generates {}

    void method2() {
    }
}

$ 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="MethodCountCheck">
  <property name="maxTotal" value="1" />
</module>
    </module>
</module>

$ java -jar checkstyle-7.8.2-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:1: Total number of methods is 2 (max allowed is 1). [MethodCount]
Audit done.
Checkstyle ends with 1 errors.

As shown previous release had 1 violation.
However running with latest master after fix now produce no violation.

$ java -jar checkstyle-nightly-2017-06-27-all.jar -c TestConfig.xml TestClass.java
Starting audit...
Audit done.

This new behavior is incorrect and there should still be 1 violations.
Fix should describe how counting was broken in previous PR as it went unnoticed by 2 reviewers.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jun 28, 2017

Member

@romani I see the issue now after debugging it.
When we reach new class/interface/etc definiton, we push new counter onto scope and continue on.
However, when we leave scope, we much check if we are specifically leaving a class/etc definition, and only than do we pop the scope and resume on previous scope if definition is nested.

The issue that the previous PR created is that the new interface definition token was not added to the list in leaveToken. It thinks it is still in the old scope, hence why regression showed counts decreasing in these places.

The check is behaving as if the class is written like this, with the example in first post:

public class TestClass {
    void method1() {
    }

    private @interface Generates {
        void method2() { // I'm inside interface definition instead of inside class
        }
    }
}

I will provide a fix shortly.

Member

rnveach commented Jun 28, 2017

@romani I see the issue now after debugging it.
When we reach new class/interface/etc definiton, we push new counter onto scope and continue on.
However, when we leave scope, we much check if we are specifically leaving a class/etc definition, and only than do we pop the scope and resume on previous scope if definition is nested.

The issue that the previous PR created is that the new interface definition token was not added to the list in leaveToken. It thinks it is still in the old scope, hence why regression showed counts decreasing in these places.

The check is behaving as if the class is written like this, with the example in first post:

public class TestClass {
    void method1() {
    }

    private @interface Generates {
        void method2() { // I'm inside interface definition instead of inside class
        }
    }
}

I will provide a fix shortly.

rnveach added a commit to rnveach/checkstyle that referenced this issue Jun 28, 2017

@rnveach rnveach changed the title from MethodCountCheck: counting broken from latest master fix to MethodCountCheck: wrong counting with nested interface definition Jun 28, 2017

romani added a commit that referenced this issue Jun 28, 2017

@romani romani added the bug label Jun 28, 2017

@romani romani added this to the 8.0 milestone Jun 28, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 28, 2017

Member

fix is merged.

Member

romani commented Jun 28, 2017

fix is merged.

@romani romani closed this Jun 28, 2017

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