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

Move away from Findbugs (unofficial JSR-305) annotations #165

Closed
bbottema opened this issue Sep 2, 2018 · 10 comments
Closed

Move away from Findbugs (unofficial JSR-305) annotations #165

bbottema opened this issue Sep 2, 2018 · 10 comments

Comments

@bbottema
Copy link
Owner

bbottema commented Sep 2, 2018

The widely used but aging Findbugs annotations are not compatible with Java 9 due to split package (Findbugs choose to use the same package as an existing package in the JDK). The Findbug annotations were never made official and as the RFC died (dormant), there's no hope of fixing this with JSR-305 (which never defined annotations).

The solution is to move to an alternative, which begs the question: which alternative is still maintained, compatible with Java 9 and also has runtime retention, actually has all the annotations currently used, doesn't bring transitive baggage and is supported by analyses tools and on top of all that supports a compatible license?

Currently, nobody has a clear answer:

To make some sense out of all the options available to use, here's an aspect matrix for the main contenders to help us decide:

Library Purpose License Compiles in Java 9? Runteim Ret.? Active? Clean dependency Robust null support Tool support
javax Findbugs Static analyses LGPL ✔️ ✔️ ✔️ ✔️
Spotbugs Static analyses LGPL ✔️ ✔️ ✔️ ?
JetBrains Static analyses Apache ✔️ ✔️ ✔️ ✔️
Checker framework Static analyses MIT ✔️ ✔️ ✔️ ✔️ ✔️ ✔️
Eclipse JDT Static analyses EPL ✔️ ? ✔️ ✔️ ✔️
Google's ErrorProne Static analyses Apache ✔️ ? ✔️ ✔️ ?
Lombok Code generation MIT ✔️ ? ✔️
Java Bean Validation* Runtime integrity Apache ✔️ ? ✔️
edu.umd.cs Findbugs Static analyses LGPL ✔️ ✔️ ✔️ ?
Android Static analyses ? ✔️ ✔️
Netbeans ? ? ? ? ? ? ? ?
Spring based on findbugs ?

*Java Bean Validation (JSR-380) is implemented by Hibernate Validator under Apache v2
*Checker framework (and probably other) supports Lombok's null annotations

Other alternatives are switching to strict non-null Option types using some variation of Optional or the Null-object pattern, or even force Java 9 to work with Findbugs with some hackery.

Since Simple Java Mail only uses @NonNull and @Nullable, I'm thinking the Jetbrains annotations would be a comfortable transition.

@bbottema bbottema added this to the 6.0.0 milestone Sep 2, 2018
@bbottema bbottema self-assigned this Sep 2, 2018
@bbottema
Copy link
Owner Author

Until there's a clearly better alternative, for now we're going with the Intellij nullability annotations.

@bbottema
Copy link
Owner Author

bbottema commented Jan 1, 2019

Unfortunately, the Jetbrains annotations have a different and incompatible retention with Simple Java Mail compared to the ones from Findbugs / Spotbugs. Simple Java Mail relies on runtime availability of this information to analyse CLI requirements.

Updated the matrix.

The Checkers Framework seemed like a viable option, but it seems you need minimum Java 8 for that, while Simple Java Mail is on Java 7.

@bbottema
Copy link
Owner Author

bbottema commented Jan 1, 2019

If it was only for use with Spotbugs code analyses during builds, we could have rolled our own annotation classes as long as they are name Nonnull or NotNull and Nullable (since Spotbugs checks the name, not the package per se).

However, I prefer Simple Java Mail to be analyzed externally as well by something like Codacy or Code Climate which might use their own heuristics for analyses rather than Spotbugs'.

On that note, we're back to square one. Which annotation fits the bill as described in the initial post?

@bbottema bbottema removed their assignment Jan 3, 2019
@bbottema
Copy link
Owner Author

bbottema commented Jan 4, 2019

The thread over at google/guava#2960 mentions an example of a JDK 6 project using checker-qual with replacement annotations. But this is still rather hacky.

@bbottema bbottema pinned this issue Feb 6, 2019
@bbottema
Copy link
Owner Author

bbottema commented Oct 27, 2019

I just noticed a post by Mark Reinhold, one of the Java architects, that the old jsr305 library actually won't result in the split package problem, because they took the wide spread use of it into consideration and basically their modular structure around it.

Sooo... is this problem still a problem then.

@bbottema
Copy link
Owner Author

For now I'm going to go with "No". Closing until it becomes relevant again.

@bbottema bbottema removed this from the 6.0.0 milestone Oct 27, 2019
@bbottema bbottema unpinned this issue Oct 27, 2019
@bbottema
Copy link
Owner Author

bbottema commented Oct 27, 2019

Aaaand it's back.

@bbottema bbottema reopened this Oct 27, 2019
@bbottema bbottema added this to the 6.0.0 milestone Oct 27, 2019
@bbottema
Copy link
Owner Author

bbottema commented Nov 9, 2019

I've finally decided on an acceptable solution. As the Jetbrains developers suggested back in 2008, I've released an adapted version of the Jetbrains annotations (NotNull and Nullable), with retention changed to RUNTIME.

This solves the compatibility issue with a lightweight runtime annotations, released as a separate tiny library: https://github.com/bbottema/jetbrains-runtime-nullability-annotations.

I've already made the move, I just want to regression-test the CLI module, which depends on the runtime availability of these annotations (for generating the CLI options).

@bbottema
Copy link
Owner Author

Released in 6.0.0-rc1.

@bbottema
Copy link
Owner Author

6.0.0 has released as well, finally.

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

No branches or pull requests

1 participant