Skip to content
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

Jsr305AnnotationsCheck is crashing on checkstyle sources #751

Closed
romani opened this issue Jun 19, 2019 · 13 comments

Comments

@romani
Copy link
Member

commented Jun 19, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

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

@romani romani added the approved label Jun 19, 2019

@romani

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

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 #742 (comment)

there was regression testing in May 16 - #742 (comment)

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

@mbert

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

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

@romani

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

@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?

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue Jun 19, 2019

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue Jun 19, 2019

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue Jun 19, 2019

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue Jun 19, 2019

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue Jun 19, 2019

@romani

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

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.

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue Jun 19, 2019

@rnveach

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

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

@romani

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

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

@mbert

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

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.

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue Jun 21, 2019

rnveach added a commit that referenced this issue Jun 22, 2019

@rnveach

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2019

Fix was merged

@rnveach rnveach closed this Jun 22, 2019

@rnveach rnveach added the bug label Jun 22, 2019

@rnveach rnveach added this to the 1.35.0 milestone Jun 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.