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 with wrong tokens crashes Checkstyle #4556

Closed
denizarsan opened this Issue Jun 29, 2017 · 7 comments

Comments

Projects
None yet
3 participants
@denizarsan

denizarsan commented Jun 29, 2017

$ javac C.java

$ cat C.java

class C {
   void m() {}
}

$ cat config.xml

<!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="MethodCount">
      <property name="tokens" value="ENUM_DEF"/>
    </module>
  </module>
</module>

$ java -jar checkstyle-7.8.2-all.jar -c config.xml C.java

Starting audit...
com.puppycrawl.tools.checkstyle.api.CheckstyleException: Exception was thrown while processing C.java
        at com.puppycrawl.tools.checkstyle.Checker.processFiles(Checker.java:295)
        at com.puppycrawl.tools.checkstyle.Checker.process(Checker.java:213)
        at com.puppycrawl.tools.checkstyle.Main.runCheckstyle(Main.java:425)
        at com.puppycrawl.tools.checkstyle.Main.runCli(Main.java:359)
        at com.puppycrawl.tools.checkstyle.Main.main(Main.java:174)
Caused by: java.lang.NullPointerException
        at com.puppycrawl.tools.checkstyle.checks.sizes.MethodCountCheck$MethodCounter.access$000(MethodCountCheck.java:218)
        at com.puppycrawl.tools.checkstyle.checks.sizes.MethodCountCheck.raiseCounter(MethodCountCheck.java:140)
        at com.puppycrawl.tools.checkstyle.checks.sizes.MethodCountCheck.visitToken(MethodCountCheck.java:112)
        at com.puppycrawl.tools.checkstyle.TreeWalker.notifyVisit(TreeWalker.java:370)
        at com.puppycrawl.tools.checkstyle.TreeWalker.processIter(TreeWalker.java:507)
        at com.puppycrawl.tools.checkstyle.TreeWalker.walk(TreeWalker.java:312)
        at com.puppycrawl.tools.checkstyle.TreeWalker.processFiltered(TreeWalker.java:184)
        at com.puppycrawl.tools.checkstyle.api.AbstractFileSetCheck.process(AbstractFileSetCheck.java:78)
        at com.puppycrawl.tools.checkstyle.Checker.processFile(Checker.java:316)
        at com.puppycrawl.tools.checkstyle.Checker.processFiles(Checker.java:286)
        ... 4 more
Checkstyle ends with 1 errors.

The tool crashes with a NPE for the given config and class. The problem is that the "tokens" property is missing CLASS_DEF in this case (and INTERFACE_DEF when that is the case). While this can be viewed as a bug in the config, Checkstyle should gracefully report a warning instead of crashing with an uncaught NPE.

Please let me know if you want me to help with fixing this.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jun 30, 2017

Member

@denizarsan The same here too. It is better to report using release versions.
We have had similar issues with this module in this release (#4534) so please confirm what commit your using and that it exists in latest master.

Fee free to provide a fix if you can, but this might not be that simple.
We expect no warnings on config since the tokens are acceptable.
Just checking for null might not be enough. Ignoring a parent of the method will cause the method to be seen under a different class definition. So the method's parent has to be tracked for each method.

Member

rnveach commented Jun 30, 2017

@denizarsan The same here too. It is better to report using release versions.
We have had similar issues with this module in this release (#4534) so please confirm what commit your using and that it exists in latest master.

Fee free to provide a fix if you can, but this might not be that simple.
We expect no warnings on config since the tokens are acceptable.
Just checking for null might not be enough. Ignoring a parent of the method will cause the method to be seen under a different class definition. So the method's parent has to be tracked for each method.

@rnveach rnveach added the approved label Jun 30, 2017

@denizarsan

This comment has been minimized.

Show comment
Hide comment
@denizarsan

denizarsan Jun 30, 2017

@rnveach I have updated my post. I'm using the latest release (7.8.2) from https://sourceforge.net/projects/checkstyle/files/checkstyle/

denizarsan commented Jun 30, 2017

@rnveach I have updated my post. I'm using the latest release (7.8.2) from https://sourceforge.net/projects/checkstyle/files/checkstyle/

@denizarsan

This comment has been minimized.

Show comment
Hide comment
@denizarsan

denizarsan Jun 30, 2017

I agree fixing this is not as simple as it looked initially. I will defer looking into it until after I finish sending some more bug reports.

Let me know if you want bug reports in some other format, e.g., PRs with failing unit tests if that's more convenient.

denizarsan commented Jun 30, 2017

I agree fixing this is not as simple as it looked initially. I will defer looking into it until after I finish sending some more bug reports.

Let me know if you want bug reports in some other format, e.g., PRs with failing unit tests if that's more convenient.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 3, 2017

Member

Let me know if you want bug reports in some other format, e.g., PRs with failing unit tests if that's more convenient.

thanks a lot for your reports, it is better to stay with format that we demand for issue.
We have strict policy on PR live period, we close PRs that are incomplete and has no activity and give smb else right to finish fix or do it from scratch.

Member

romani commented Jul 3, 2017

Let me know if you want bug reports in some other format, e.g., PRs with failing unit tests if that's more convenient.

thanks a lot for your reports, it is better to stay with format that we demand for issue.
We have strict policy on PR live period, we close PRs that are incomplete and has no activity and give smb else right to finish fix or do it from scratch.

@rnveach rnveach self-assigned this Jul 4, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Jul 4, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 4, 2017

Member

@romani If we end up fixing this bug correctly, than it also includes the fixes for Issue #4539.
Is that ok or should we do a simple NPE fix here and a more complex fix in #4539?
Without full fix, inner classes will be counted to wrong areas like anonymous classes.

Member

rnveach commented Jul 4, 2017

@romani If we end up fixing this bug correctly, than it also includes the fixes for Issue #4539.
Is that ok or should we do a simple NPE fix here and a more complex fix in #4539?
Without full fix, inner classes will be counted to wrong areas like anonymous classes.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 15, 2017

Member

Is that ok or should we do a simple NPE fix here and a more complex fix in #4539?

I prefer to fix Exceptions quicker and then thoroughly make Check more correct from validation perspective. Users are more affected by Exceptions rather than by false-positives.

Member

romani commented Jul 15, 2017

Is that ok or should we do a simple NPE fix here and a more complex fix in #4539?

I prefer to fix Exceptions quicker and then thoroughly make Check more correct from validation perspective. Users are more affected by Exceptions rather than by false-positives.

rnveach added a commit to rnveach/checkstyle that referenced this issue Jul 19, 2017

romani added a commit that referenced this issue Jul 19, 2017

@romani romani added the bug label Jul 19, 2017

@romani romani added this to the 8.1 milestone Jul 19, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 19, 2017

Member

fix is merged

Member

romani commented Jul 19, 2017

fix is merged

@romani romani closed this Jul 19, 2017

sergileon added a commit to sergileon/checkstyle that referenced this issue Aug 30, 2017

sergileon added a commit to sergileon/checkstyle that referenced this issue Aug 31, 2017

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