Use nullability annotation to enforce/document API contract

Details

Description

In a discussion about exception handling on the dev list [1] Julian brough up the idea of using nullability annotations in APIs. I think we should decide on which one to use and start using them whereever apropriate.

Julian Reschke
added a comment - 09/May/12 13:17 I just checked with FindBugs 2.01, and annotating methods with @CheckForNull and @Nonnull seemed to have the desired effect.
That being said, this appears to be a can of worms; JSR-305 is dormant; FindBugs supports the annotation using it's own package + javax.annotation.
IntelliJ is said to support them in "all" packages, but I can't test that.
There's also a Eclipse JDT extension in the works which appears to be configurable with respect to package names ( http://wiki.eclipse.org/JDT_Core/Null_Analysis )
In the end we need something that has an acceptable license, and can be automatically checked; FindBugs is probably a candidate for that.

FindBugs annotations sound good to me as long as they don't introduce a mandatory runtime dependency on the code (which I don't think they do).

IDE support (IntelliJ, Eclipse, etc.) is nice, but IMHO not essential. The important bit is that we're able to configure our Maven build to check such annotations and fail the build if the explicitly declared rules are broken.

Jukka Zitting
added a comment - 16/May/12 11:41 FindBugs annotations sound good to me as long as they don't introduce a mandatory runtime dependency on the code (which I don't think they do).
IDE support (IntelliJ, Eclipse, etc.) is nice, but IMHO not essential. The important bit is that we're able to configure our Maven build to check such annotations and fail the build if the explicitly declared rules are broken.

FindBugs annotations sound good to me as long as they don't introduce a mandatory runtime dependency on the code (which I don't think they do).

For intra-subproject checking they are only needed at compile time. However, I'm not sure how it will work when we use a JAR from another subproject. But let's start simple.

IDE support (IntelliJ, Eclipse, etc.) is nice, but IMHO not essential. The important bit is that we're able to configure our Maven build to check such annotations and fail the build if the explicitly declared rules are broken.

That should be possible but maybe I'll need some hand-holding with that.

Julian Reschke
added a comment - 16/May/12 12:30 FindBugs annotations sound good to me as long as they don't introduce a mandatory runtime dependency on the code (which I don't think they do).
For intra-subproject checking they are only needed at compile time. However, I'm not sure how it will work when we use a JAR from another subproject. But let's start simple.
IDE support (IntelliJ, Eclipse, etc.) is nice, but IMHO not essential. The important bit is that we're able to configure our Maven build to check such annotations and fail the build if the explicitly declared rules are broken.
That should be possible but maybe I'll need some hand-holding with that.

Julian Reschke
added a comment - 16/May/12 16:33 Minimal demonstration, showing how to embed the annotations.
Note:
cross project checking with the FindBugs plugin in Eclipse indeed works
FindBugs report:
Possible null pointer dereference in org.apache.jackrabbit.oak.jcr.security.user.UserManagerImpl.removeInternalProperty(Node, String) due to return value of called method UserManagerImpl.java /oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/security/user line 352 FindBugs Problem (Troubling)
I believe this is something we should do; running FindBugs from the actual Maven build would be the next step

PS. The Maven dependencies should have a <scope>provided</scope> entry in them. Otherwise they'll show up as transitive dependencies in downstream components. See for example the bndlib dependency for an example of another annotation library we're using.

Jukka Zitting
added a comment - 16/May/12 16:42 +1 great stuff!
PS. The Maven dependencies should have a <scope>provided</scope> entry in them. Otherwise they'll show up as transitive dependencies in downstream components. See for example the bndlib dependency for an example of another annotation library we're using.

PS. The Maven dependencies should have a <scope>provided</scope> entry in them. Otherwise they'll show up as transitive dependencies in downstream components. See for example the bndlib dependency for an example of another annotation library we're using.

Done; see attachment. Should I go ahead and commit this, and see how it works in practice once we add more annotations?

Julian Reschke
added a comment - 16/May/12 17:00 PS. The Maven dependencies should have a <scope>provided</scope> entry in them. Otherwise they'll show up as transitive dependencies in downstream components. See for example the bndlib dependency for an example of another annotation library we're using.
Done; see attachment. Should I go ahead and commit this, and see how it works in practice once we add more annotations?

I added the findbugs and checkstyle plugins to the pedantic profile of our Maven build in revision 1365017. For now they are configured to simply output their findings without causing the build to fail.

Jukka Zitting
added a comment - 24/Jul/12 13:53 I added the findbugs and checkstyle plugins to the pedantic profile of our Maven build in revision 1365017. For now they are configured to simply output their findings without causing the build to fail.