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

MapIterationInForEachLoopCheck crashes on classes extending Maps #427

Closed
karlicoss opened this Issue Jan 25, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@karlicoss

karlicoss commented Jan 25, 2016

I managed to reproduce it in a unit test 0587683, but not really sure how it should be fixed.

Stacktrace:

java.lang.NullPointerException
    at com.github.sevntu.checkstyle.checks.coding.MapIterationInForEachLoopCheck.getKeySetOrEntrySetNode(MapIterationInForEachLoopCheck.java:400)
    at com.github.sevntu.checkstyle.checks.coding.MapIterationInForEachLoopCheck.validate(MapIterationInForEachLoopCheck.java:332)
    at com.github.sevntu.checkstyle.checks.coding.MapIterationInForEachLoopCheck.visitToken(MapIterationInForEachLoopCheck.java:306)
    at com.puppycrawl.tools.checkstyle.TreeWalker.notifyVisit(TreeWalker.java:388)
    at com.puppycrawl.tools.checkstyle.TreeWalker.processIter(TreeWalker.java:499)
    at com.puppycrawl.tools.checkstyle.TreeWalker.walk(TreeWalker.java:330)
    at com.puppycrawl.tools.checkstyle.TreeWalker.processFiltered(TreeWalker.java:203)
    at com.puppycrawl.tools.checkstyle.api.AbstractFileSetCheck.process(AbstractFileSetCheck.java:79)
    at com.puppycrawl.tools.checkstyle.Checker.process(Checker.java:265)
    at com.github.sevntu.checkstyle.BaseCheckTestSupport.verify(BaseCheckTestSupport.java:83)
    at com.github.sevntu.checkstyle.BaseCheckTestSupport.verify(BaseCheckTestSupport.java:75)
    at com.github.sevntu.checkstyle.BaseCheckTestSupport.verify(BaseCheckTestSupport.java:64)
    at com.github.sevntu.checkstyle.checks.coding.MapIterationInForEachLoopTest.extendsMapTest(MapIterationInForEachLoopTest.java:88)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:497)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:45)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:15)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:42)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:20)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:263)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:68)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:47)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:231)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:60)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:229)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:50)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:222)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:300)
    at org.junit.runner.JUnitCore.run(JUnitCore.java:157)
    at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:117)
    at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:234)
    at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:74)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:497)
    at com.intellij.rt.execution.application.AppMain.main(AppMain.java:144)

I could of course quickfix it by adding some null checks here https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/MapIterationInForEachLoopCheck.java#L400, but it looks hacky, maybe the fix should be more consistent? If it is okay though, I would be happy to fix it myself and open a PR.

@maxvetrenko I guess you are the author, what do you think?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 26, 2016

Member

@karlicoss , please be welcome with PR for fix.

extra check for null are ok if all branches/lines coveraged by UTs , coverage rate for this Check is not 100% unfortunately https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/sevntu-checks/pom.xml#L129 but as you improve coverage this will be awesome!

Member

romani commented Jan 26, 2016

@karlicoss , please be welcome with PR for fix.

extra check for null are ok if all branches/lines coveraged by UTs , coverage rate for this Check is not 100% unfortunately https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/sevntu-checks/pom.xml#L129 but as you improve coverage this will be awesome!

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Dec 5, 2016

Contributor

Full report:

$ cat TestClass.java
package com.github.sevntu.checkstyle.checks.coding;

import java.util.HashMap;

public class InputMapIterationInForEachLoopExtends {
    public static class TestMap extends HashMap<Integer, Integer> {
        public void test() {
            for (Entry<Integer, Integer> entry : this.entrySet()) {
            }
        }
    }
}


$ 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="com.github.sevntu.checkstyle.checks.coding.MapIterationInForEachLoopCheck">
  <property name="proposeValuesUsage" value="true"/>
  <property name="proposeKeySetUsage" value="true"/>
  <property name="proposeEntrySetUsage" value="true"/>
</module>
    </module>
</module>

$ java -jar checkstyle-sevntu-nightly-2016-12-03-all.jar -c TestConfig.xml TestClass.java
Starting audit...
com.puppycrawl.tools.checkstyle.api.CheckstyleException: Exception was thrown while processing TestClass.java
    at com.puppycrawl.tools.checkstyle.Checker.processFiles(Checker.java:287)
    at com.puppycrawl.tools.checkstyle.Checker.process(Checker.java:205)
    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.github.sevntu.checkstyle.checks.coding.MapIterationInForEachLoopCheck.getKeySetOrEntrySetNode(MapIterationInForEachLoopCheck.java:403)
    at com.github.sevntu.checkstyle.checks.coding.MapIterationInForEachLoopCheck.validate(MapIterationInForEachLoopCheck.java:332)
    at com.github.sevntu.checkstyle.checks.coding.MapIterationInForEachLoopCheck.visitToken(MapIterationInForEachLoopCheck.java:307)
    at com.puppycrawl.tools.checkstyle.TreeWalker.notifyVisit(TreeWalker.java:361)
    at com.puppycrawl.tools.checkstyle.TreeWalker.processIter(TreeWalker.java:498)
    at com.puppycrawl.tools.checkstyle.TreeWalker.walk(TreeWalker.java:303)
    at com.puppycrawl.tools.checkstyle.TreeWalker.processFiltered(TreeWalker.java:178)
    at com.puppycrawl.tools.checkstyle.api.AbstractFileSetCheck.process(AbstractFileSetCheck.java:78)
    at com.puppycrawl.tools.checkstyle.Checker.processFile(Checker.java:307)
    at com.puppycrawl.tools.checkstyle.Checker.processFiles(Checker.java:278)
    ... 4 more
Checkstyle ends with 1 errors.
Contributor

rnveach commented Dec 5, 2016

Full report:

$ cat TestClass.java
package com.github.sevntu.checkstyle.checks.coding;

import java.util.HashMap;

public class InputMapIterationInForEachLoopExtends {
    public static class TestMap extends HashMap<Integer, Integer> {
        public void test() {
            for (Entry<Integer, Integer> entry : this.entrySet()) {
            }
        }
    }
}


$ 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="com.github.sevntu.checkstyle.checks.coding.MapIterationInForEachLoopCheck">
  <property name="proposeValuesUsage" value="true"/>
  <property name="proposeKeySetUsage" value="true"/>
  <property name="proposeEntrySetUsage" value="true"/>
</module>
    </module>
</module>

$ java -jar checkstyle-sevntu-nightly-2016-12-03-all.jar -c TestConfig.xml TestClass.java
Starting audit...
com.puppycrawl.tools.checkstyle.api.CheckstyleException: Exception was thrown while processing TestClass.java
    at com.puppycrawl.tools.checkstyle.Checker.processFiles(Checker.java:287)
    at com.puppycrawl.tools.checkstyle.Checker.process(Checker.java:205)
    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.github.sevntu.checkstyle.checks.coding.MapIterationInForEachLoopCheck.getKeySetOrEntrySetNode(MapIterationInForEachLoopCheck.java:403)
    at com.github.sevntu.checkstyle.checks.coding.MapIterationInForEachLoopCheck.validate(MapIterationInForEachLoopCheck.java:332)
    at com.github.sevntu.checkstyle.checks.coding.MapIterationInForEachLoopCheck.visitToken(MapIterationInForEachLoopCheck.java:307)
    at com.puppycrawl.tools.checkstyle.TreeWalker.notifyVisit(TreeWalker.java:361)
    at com.puppycrawl.tools.checkstyle.TreeWalker.processIter(TreeWalker.java:498)
    at com.puppycrawl.tools.checkstyle.TreeWalker.walk(TreeWalker.java:303)
    at com.puppycrawl.tools.checkstyle.TreeWalker.processFiltered(TreeWalker.java:178)
    at com.puppycrawl.tools.checkstyle.api.AbstractFileSetCheck.process(AbstractFileSetCheck.java:78)
    at com.puppycrawl.tools.checkstyle.Checker.processFile(Checker.java:307)
    at com.puppycrawl.tools.checkstyle.Checker.processFiles(Checker.java:278)
    ... 4 more
Checkstyle ends with 1 errors.
@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach May 13, 2017

Contributor

There is a second NPE, in a different area, if you remove the this from the example I gave above.

$ cat TestClass.java
package com.github.sevntu.checkstyle.checks.coding;

import java.util.HashMap;

public class InputMapIterationInForEachLoopExtends {
    public static class TestMap extends HashMap<Integer, Integer> {
        public void test() {
            for (Entry<Integer, Integer> entry : entrySet()) {
            }
        }
    }
}

$ 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="com.github.sevntu.checkstyle.checks.coding.MapIterationInForEachLoopCheck">
  <property name="proposeValuesUsage" value="true"/>
  <property name="proposeKeySetUsage" value="true"/>
  <property name="proposeEntrySetUsage" value="true"/>
</module>
    </module>
</module>

$ java -jar checkstyle-7.3-sevntu-1.23.0-all.jar -c TestConfig.xml TestClass.java
Starting audit...
com.puppycrawl.tools.checkstyle.api.CheckstyleException: Exception was thrown while processing TestClass.java
    at com.puppycrawl.tools.checkstyle.Checker.processFiles(Checker.java:287)
    at com.puppycrawl.tools.checkstyle.Checker.process(Checker.java:205)
    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.github.sevntu.checkstyle.checks.coding.MapIterationInForEachLoopCheck.getKeySetOrEntrySetNode(MapIterationInForEachLoopCheck.java:406)
    at com.github.sevntu.checkstyle.checks.coding.MapIterationInForEachLoopCheck.validate(MapIterationInForEachLoopCheck.java:332)
    at com.github.sevntu.checkstyle.checks.coding.MapIterationInForEachLoopCheck.visitToken(MapIterationInForEachLoopCheck.java:307)
    at com.puppycrawl.tools.checkstyle.TreeWalker.notifyVisit(TreeWalker.java:361)
    at com.puppycrawl.tools.checkstyle.TreeWalker.processIter(TreeWalker.java:498)
    at com.puppycrawl.tools.checkstyle.TreeWalker.walk(TreeWalker.java:303)
    at com.puppycrawl.tools.checkstyle.TreeWalker.processFiltered(TreeWalker.java:178)
    at com.puppycrawl.tools.checkstyle.api.AbstractFileSetCheck.process(AbstractFileSetCheck.java:78)
    at com.puppycrawl.tools.checkstyle.Checker.processFile(Checker.java:307)
    at com.puppycrawl.tools.checkstyle.Checker.processFiles(Checker.java:278)
    ... 4 more
Checkstyle ends with 1 errors.
Contributor

rnveach commented May 13, 2017

There is a second NPE, in a different area, if you remove the this from the example I gave above.

$ cat TestClass.java
package com.github.sevntu.checkstyle.checks.coding;

import java.util.HashMap;

public class InputMapIterationInForEachLoopExtends {
    public static class TestMap extends HashMap<Integer, Integer> {
        public void test() {
            for (Entry<Integer, Integer> entry : entrySet()) {
            }
        }
    }
}

$ 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="com.github.sevntu.checkstyle.checks.coding.MapIterationInForEachLoopCheck">
  <property name="proposeValuesUsage" value="true"/>
  <property name="proposeKeySetUsage" value="true"/>
  <property name="proposeEntrySetUsage" value="true"/>
</module>
    </module>
</module>

$ java -jar checkstyle-7.3-sevntu-1.23.0-all.jar -c TestConfig.xml TestClass.java
Starting audit...
com.puppycrawl.tools.checkstyle.api.CheckstyleException: Exception was thrown while processing TestClass.java
    at com.puppycrawl.tools.checkstyle.Checker.processFiles(Checker.java:287)
    at com.puppycrawl.tools.checkstyle.Checker.process(Checker.java:205)
    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.github.sevntu.checkstyle.checks.coding.MapIterationInForEachLoopCheck.getKeySetOrEntrySetNode(MapIterationInForEachLoopCheck.java:406)
    at com.github.sevntu.checkstyle.checks.coding.MapIterationInForEachLoopCheck.validate(MapIterationInForEachLoopCheck.java:332)
    at com.github.sevntu.checkstyle.checks.coding.MapIterationInForEachLoopCheck.visitToken(MapIterationInForEachLoopCheck.java:307)
    at com.puppycrawl.tools.checkstyle.TreeWalker.notifyVisit(TreeWalker.java:361)
    at com.puppycrawl.tools.checkstyle.TreeWalker.processIter(TreeWalker.java:498)
    at com.puppycrawl.tools.checkstyle.TreeWalker.walk(TreeWalker.java:303)
    at com.puppycrawl.tools.checkstyle.TreeWalker.processFiltered(TreeWalker.java:178)
    at com.puppycrawl.tools.checkstyle.api.AbstractFileSetCheck.process(AbstractFileSetCheck.java:78)
    at com.puppycrawl.tools.checkstyle.Checker.processFile(Checker.java:307)
    at com.puppycrawl.tools.checkstyle.Checker.processFiles(Checker.java:278)
    ... 4 more
Checkstyle ends with 1 errors.
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani May 13, 2017

Member

fix is merged

Member

romani commented May 13, 2017

fix is merged

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