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

JavadocPackage: NullPointerException when relative path is used to run checkstyle CLI #5127

Closed
chris21k opened this Issue Sep 19, 2017 · 13 comments

Comments

Projects
None yet
5 participants
@chris21k

chris21k commented Sep 19, 2017

/var/tmp $ javac HelloWorld.java
/var/tmp $ cat HelloWorld.java
public class HelloWorld {
    public static void main(String[] args) {
        System.out.println("Hello, World!");
    }
}
/var/tmp $ java -Duser.language=en -Duser.country=US -jar checkstyle-8.2-all.jar \
-c /sun_checks.xml HelloWorld.java
Starting audit...
com.puppycrawl.tools.checkstyle.api.CheckstyleException: Exception was thrown while processing HelloWorld.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:523)
        at com.puppycrawl.tools.checkstyle.Main.runCli(Main.java:437)
        at com.puppycrawl.tools.checkstyle.Main.main(Main.java:210)
Caused by: java.lang.NullPointerException
        at java.util.concurrent.ConcurrentHashMap.putVal(ConcurrentHashMap.java:1011)
        at java.util.concurrent.ConcurrentHashMap$KeySetView.add(ConcurrentHashMap.java:4595)
        at com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocPackageCheck.processFiltered(JavadocPackageCheck.java:73)
        at com.puppycrawl.tools.checkstyle.api.AbstractFileSetCheck.process(AbstractFileSetCheck.java:79)
        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.

With checkstyle 8.2 the above using the provided sun_checks.xml throws a NullPointerException . 8.1 does not schow this problem. Same behavior under win and linux.

$ java -Duser.language=en -Duser.country=US -jar checkstyle-8.1-all.jar -c /sun_checks.xml HelloWorld.java
Starting audit...
[ERROR] /var/tmp/HelloWorld.java:0: File does not end with a newline. [NewlineAtEndOfFile]
[ERROR] /var/tmp/HelloWorld.java:0: Missing package-info.java file. [JavadocPackage]
[ERROR] /var/tmp/HelloWorld.java:1: Missing a Javadoc comment. [JavadocType]
[ERROR] /var/tmp/HelloWorld.java:1:1: Utility classes should not have a public or default constructor. [HideUtilityClassConstructor]
[ERROR] /var/tmp/HelloWorld.java:2:5: Missing a Javadoc comment. [JavadocMethod]
[ERROR] /var/tmp/HelloWorld.java:2:29: Parameter args should be final. [FinalParameters]
Audit done.
Checkstyle ends with 6 errors.

Thx,
Chris

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Sep 19, 2017

Member

https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ConcurrentHashMap.html#put(K,%20V)

put
Throws:
NullPointerException - if the specified key or value is null

final File dir = file.getParentFile();
final boolean isDirChecked = !directoriesChecked.add(dir);

Since the file is in a directory with a parent, I think the exception occurs if you give a short relative path.
@chris21k Do you still get an exception if instead of giving it the file HelloWorld.java, you give it the full path /var/tmp/HelloWorld.java?

Member

rnveach commented Sep 19, 2017

https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ConcurrentHashMap.html#put(K,%20V)

put
Throws:
NullPointerException - if the specified key or value is null

final File dir = file.getParentFile();
final boolean isDirChecked = !directoriesChecked.add(dir);

Since the file is in a directory with a parent, I think the exception occurs if you give a short relative path.
@chris21k Do you still get an exception if instead of giving it the file HelloWorld.java, you give it the full path /var/tmp/HelloWorld.java?

@chris21k

This comment has been minimized.

Show comment
Hide comment
@chris21k

chris21k Sep 20, 2017

Providing the full path for HelloWorld.java helps (under Linux and Win). However, the relative path should suffice, as it did with Checkstyle versions before 8.2, as far as I know. The relative path also is ok while using google_checks.xml. Please fix.

Thanks, Chris

chris21k commented Sep 20, 2017

Providing the full path for HelloWorld.java helps (under Linux and Win). However, the relative path should suffice, as it did with Checkstyle versions before 8.2, as far as I know. The relative path also is ok while using google_checks.xml. Please fix.

Thanks, Chris

@rnveach rnveach added the approved label Sep 20, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Sep 20, 2017

Member

Thanks for confirming, JavadocPackage needs to be fixed at the code mentioned above.

The relative path also is ok while using google_checks.xml.

Google doesn't use JavadocPackage, which is why it isn't affected.

Member

rnveach commented Sep 20, 2017

Thanks for confirming, JavadocPackage needs to be fixed at the code mentioned above.

The relative path also is ok while using google_checks.xml.

Google doesn't use JavadocPackage, which is why it isn't affected.

@romani romani changed the title from provided sun_checks.xml triggers NullPointerException to JavadocPackage: NullPointerException when relative path is used to run checkstyle CLI Sep 21, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 21, 2017

Member

@chris21k , how did you find such problem ?

Member

romani commented Sep 21, 2017

@chris21k , how did you find such problem ?

@chris21k

This comment has been minimized.

Show comment
Hide comment
@chris21k

chris21k Sep 22, 2017

@romani, I am maintaining a (strongly modified) Praktomat (https://github.com/KITPraktomatTeam/Praktomat/) web instance for advanced programming courses at an university. There Checkstyle is one Plugin/Checker which is used in a sandbox of each code submission to give feedback to the submitter, decline acceptance of the submission, or/and to give feedback to me about code quality for giving grades. Thereby we use the Sun Code Conventions. How Praktomat calls Checkstyle (console command) and prepares the sandbox directed me to the above problem. I never have any control about a (good) package structure of the submissions, as I can give only advices to the students, which they don't always follow :)

Checkstyle is great! Many thanks!

Chris

chris21k commented Sep 22, 2017

@romani, I am maintaining a (strongly modified) Praktomat (https://github.com/KITPraktomatTeam/Praktomat/) web instance for advanced programming courses at an university. There Checkstyle is one Plugin/Checker which is used in a sandbox of each code submission to give feedback to the submitter, decline acceptance of the submission, or/and to give feedback to me about code quality for giving grades. Thereby we use the Sun Code Conventions. How Praktomat calls Checkstyle (console command) and prepares the sandbox directed me to the above problem. I never have any control about a (good) package structure of the submissions, as I can give only advices to the students, which they don't always follow :)

Checkstyle is great! Many thanks!

Chris

@LeeiFrankJaw

This comment has been minimized.

Show comment
Hide comment
@LeeiFrankJaw

LeeiFrankJaw Oct 7, 2017

I found the same problem when configuring checkstyle for jdee. I commented issue 58 and created a workaround pull request 134 (by preceding filename with ./), both at that repository.

Actually, simply preceding ./ will work around the problem and the following works.

 java -jar checkstyle-8.2-all.jar -c ./sun_checks.xml ./HelloWorld.java

LeeiFrankJaw commented Oct 7, 2017

I found the same problem when configuring checkstyle for jdee. I commented issue 58 and created a workaround pull request 134 (by preceding filename with ./), both at that repository.

Actually, simply preceding ./ will work around the problem and the following works.

 java -jar checkstyle-8.2-all.jar -c ./sun_checks.xml ./HelloWorld.java
@chris21k

This comment has been minimized.

Show comment
Hide comment
@chris21k

chris21k Oct 8, 2017

Unfortunately, the new release 8.3 does not fix the problem.

chris21k commented Oct 8, 2017

Unfortunately, the new release 8.3 does not fix the problem.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Oct 8, 2017

Member

@chris21k No one has worked on this issue, so of course it isn't fixed.
If you want to, you are free to fix this and submit a PR to have it fixed sooner.

Member

rnveach commented Oct 8, 2017

@chris21k No one has worked on this issue, so of course it isn't fixed.
If you want to, you are free to fix this and submit a PR to have it fixed sooner.

@AADudkin

This comment has been minimized.

Show comment
Hide comment
@AADudkin

AADudkin commented Oct 16, 2017

I'm on it

AADudkin pushed a commit to epam/checkstyle that referenced this issue Oct 17, 2017

AADudkin pushed a commit to epam/checkstyle that referenced this issue Oct 18, 2017

AADudkin pushed a commit to epam/checkstyle that referenced this issue Oct 18, 2017

AADudkin pushed a commit to epam/checkstyle that referenced this issue Oct 19, 2017

AADudkin pushed a commit to epam/checkstyle that referenced this issue Oct 20, 2017

AADudkin pushed a commit to epam/checkstyle that referenced this issue Oct 20, 2017

AADudkin added a commit to epam/checkstyle that referenced this issue Oct 21, 2017

AADudkin added a commit to epam/checkstyle that referenced this issue Oct 22, 2017

AADudkin pushed a commit to epam/checkstyle that referenced this issue Oct 23, 2017

AADudkin pushed a commit to epam/checkstyle that referenced this issue Oct 23, 2017

AADudkin pushed a commit to epam/checkstyle that referenced this issue Oct 24, 2017

@Vladlis Vladlis changed the title from JavadocPackage: NullPointerException when relative path is used to run checkstyle CLI to Misusage of input File in FileSet checks leads to NPE Oct 25, 2017

AADudkin pushed a commit to epam/checkstyle that referenced this issue Oct 25, 2017

AADudkin pushed a commit to epam/checkstyle that referenced this issue Oct 25, 2017

AADudkin pushed a commit to epam/checkstyle that referenced this issue Oct 26, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 26, 2017

Member

@AADudkin , as such NPE was introduced from 8.1 to 8.2, do you know what change caused it ?

Member

romani commented Oct 26, 2017

@AADudkin , as such NPE was introduced from 8.1 to 8.2, do you know what change caused it ?

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Oct 26, 2017

Member

@romani Look at the blame for the 2 lines I posted as the cause for the NPE.
The combination of both caused the exception. One change was more recent, but the other one slightly hid it as it didn't use the null.

Member

rnveach commented Oct 26, 2017

@romani Look at the blame for the 2 lines I posted as the cause for the NPE.
The combination of both caused the exception. One change was more recent, but the other one slightly hid it as it didn't use the null.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 26, 2017

Member

problem is due to inaccurate refactoring at ff85e71
ConcurrentHashMap collection throw NPE, but previous collection HashSet, worked fine with nulls, and before this commit all was fine.
sources of ConcurrentHashMap - http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/tip/src/share/classes/java/util/concurrent/ConcurrentHashMap.java#l1010
sources of HashMap method that is called by "HashMap.contains()" - http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/tip/src/share/classes/java/util/HashMap.java#l566

@AADudkin , are sure that we can reproduce this problem at any other FileSet Check ? please prove by CLI.

Member

romani commented Oct 26, 2017

problem is due to inaccurate refactoring at ff85e71
ConcurrentHashMap collection throw NPE, but previous collection HashSet, worked fine with nulls, and before this commit all was fine.
sources of ConcurrentHashMap - http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/tip/src/share/classes/java/util/concurrent/ConcurrentHashMap.java#l1010
sources of HashMap method that is called by "HashMap.contains()" - http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/tip/src/share/classes/java/util/HashMap.java#l566

@AADudkin , are sure that we can reproduce this problem at any other FileSet Check ? please prove by CLI.

AADudkin pushed a commit to epam/checkstyle that referenced this issue Oct 26, 2017

AADudkin pushed a commit to epam/checkstyle that referenced this issue Oct 30, 2017

AADudkin pushed a commit to epam/checkstyle that referenced this issue Oct 30, 2017

@Vladlis Vladlis changed the title from Misusage of input File in FileSet checks leads to NPE to JavadocPackage: NullPointerException when relative path is used to run checkstyle CLI Oct 31, 2017

AADudkin pushed a commit to epam/checkstyle that referenced this issue Oct 31, 2017

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

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Nov 23, 2017

Member

Fix was merged

Member

rnveach commented Nov 23, 2017

Fix was merged

@rnveach rnveach closed this Nov 23, 2017

@rnveach rnveach added the bug label Nov 23, 2017

@rnveach rnveach added this to the 8.5 milestone Nov 23, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Dec 19, 2017

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