Jsr305AnnotationsCheck is crashing on checkstyle sources

[romani]

details at https://travis-ci.org/checkstyle/checkstyle/jobs/547533522#L415

Caused by: java.lang.NullPointerException
    at com.github.sevntu.checkstyle.checks.coding.Jsr305AnnotationsCheck$AbstractJsr305Handler.isPrimitiveType (Jsr305AnnotationsCheck.java:939)
    at com.github.sevntu.checkstyle.checks.coding.Jsr305AnnotationsCheck$ParameterJsr305Handler.runHandler (Jsr305AnnotationsCheck.java:603)
    at com.github.sevntu.checkstyle.checks.coding.Jsr305AnnotationsCheck$AbstractJsr305Handler.handle (Jsr305AnnotationsCheck.java:807)
    at com.github.sevntu.checkstyle.checks.coding.Jsr305AnnotationsCheck.visitToken (Jsr305AnnotationsCheck.java:371)

diff: https://github.com/checkstyle/checkstyle/pull/6836/files

command in checkstyle repo (with change): ./.ci/travis/travis.sh checkstyle-and-sevntu

ATTENTION: to unblock checkstyle master build I reverted upgrade to sevntu 1.34.0 . I will not do any public notifications on new senvtu release till issue is resolved.

It is crashing not only on checkstyle sources: sevntu CI master build is red now - https://app.wercker.com/checkstyle/sevntu.checkstyle/runs/build/5d09aa43105780001c558e65?step=5d09aa88322dac0009d85364 null pointer at "Apache Struts" sources.

[romani]

@mbert , please address ASAP. and we need to investigate how did we miss so obvious problem.

[romani]

wercker was green on PR, as it takes master contribution repo, where new Check did not present, as PR is hanging, and it is failing right after merge .... ok. So new Checks are skipped from such regression testing .... I updated release notes with ATTENTION.

Looks like we need to be even more demanding and require update(in separate commit) at https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/sevntu-checks/.ci/wercker.sh#L31 to reference users fork/branch https://github.com/mbert/contribution as independent prove that it has no exceptions. We cannot trust contributors....

[romani]

https://travis-ci.org/checkstyle/checkstyle/jobs/547533522#L462 Caused by: com.puppycrawl.tools.checkstyle.api.CheckstyleException: Exception was thrown while processing /home/travis/build/checkstyle/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/filters/SuppressWithPlainTextCommentFilter.java

This file passed during regresson testing: https://mbert.github.io/sevntu.checkstyle/reports/diff/checkstyle/index.html#A3008 on April 16 https://github.com/sevntu-checkstyle/sevntu.checkstyle/pull/742#issuecomment-483360036

there was regression testing in May 16 - https://github.com/sevntu-checkstyle/sevntu.checkstyle/pull/742#issuecomment-493099527

@mbert , how we end up NPE ? at the end

[mbert]

I'm going to look at this as first when at my desk.

[romani]

@rnveach , @pbludov , please review this incident. Even we were demanding as usual, exception is passed through our CI and review process, we have a hole, please read my proposal above. It is better to automatically do this, Is there any proposals ?

[mbert]

@mbert , how we end up NPE ? at the end

OK, I see what caused this. Methods like DetailAST.findFirstToken() or DetailAST.getFirstChild() can return null and one actually does here. In the old code there were "better safe than sorry" null checks, some of which leading to untested if branches. As we discussed back then "unnecessary" checks should be removed and rather use unit tests to assert that even in the absence of nullnness guards the code is "safe".

So here we have a case where a nullness guard was necessary. I will fix this of course. I will have to find out how to construct a source file that runs into this.

I am however a bit concerned about this probably being the "tip of the iceberg". Having run the new check against a good number of annotated and unannotated code without any issues, still running into an issue here seems rather alarming to me.

Since I have less experience with the CheckStyle code than you, do you have any advice how to deal with all the uses of DetailAST.findFirstToken() and the like? I would normally just use reflection-based unit tests to be very safe, but I understand that this is not wanted here.

Maybe, from your experience, you can give me a hint on how to construct test files that do reproduce the case that lead to the problem here?

[romani]

Since I have less experience with the CheckStyle code than you, do you have any advice how to deal with all the uses of DetailAST.findFirstToken() and the like?

Token is something from source file. So it is mostly knowledge on what possible to write in java sources. We also have problems sometime with this, but execution of our checkstyle-tester on opensource projects find us such cases.

Some cases unfortunately found by users, as practice show it is so easy to fix and again be confident in your code. Extra null checks is a sign that engineer do not know or lazy to think, in practice it is error prone approach.

[mbert]

Since I have less experience with the CheckStyle code than you, do you have any advice how to deal with all the uses of DetailAST.findFirstToken() and the like?

Token is something from source file. So it is mostly knowledge on what possible to write in java sources. We also have problems sometime with this, but execution of our checkstyle-tester on opensource projects find us such cases.

Some cases unfortunately found by users, as practice show it is so easy to fix and again be confident in your code. Extra null checks is a sign that engineer do not know or lazy to think, in practice it is error prone approach.

Hmm, not sure whether I can agree here. A check operates on a DetailAST which depends on the checked source file. What we had today is a very rare pattern (nested annotation, actually the first time I've seen this). When it comes to nullness checks I prefer to guard everything that technically can become null. Test coverage provides only limited protection.

When deploying code on a customer's system the impact of having been wrong is far bigger than the impact of being able to make a point that a programmer really knows where to do null checks and where not.

Anyway, your coding conventions are as they are, and I will of course respect this.

[rnveach]

new Checks are skipped from such regression testing Looks like we need to be even more demanding and require update(in separate commit) Is there any proposals ?

Regression would not work for this check. The module has to be configured specifically for the packages you want. By default it produces no violations and that is why travis' dynamic regression didn't pick up anything. There is no way to be sure this check passes all our normal regression unless it is customized for each project.

[mbert]

You'll already cover pretty much everything if you set packages to com,org,net.

[romani]

Just launch such config on massive sources amount. If case is not found, please remove extra null check.

[mbert]

Just launch such config on massive sources amount. If case is not found, please remove extra null check.

I have already done that. Please take a look at the PR.

[rnveach]

Fix was merged