Provide JSR-305 support

[Stephan202]

trafficstars

I'd like to analyze my source code with the Checker Framework, in particular using its Nullness checker. Some of my code interfaces with JAXB and JAXB-basics generated code. Now, I can add a package-level annotation for the generated code, specifying that all parameters, return types and fields are @Nullable (or @Nonnull), but in fact there are both @Nullable and @Nonnull parameters and return types, so that doesn't actually work.

Hence my question: would you be open to adding the JSR-305 @Nullable and @Nonnull annotations to the interfaces and classes of the jaxb2-basics-runtme library, as well as the code generated by the jaxb2-basics plugin?

The change would introduce a provided-scope dependency on com.google.code.findbugs:jsr305:3.0.0 and thus be backwards-compatible. (Because Maven won't add it to the classpath of downstream dependencies and Java will simply ignore annotations it can't find on the classpath.)

I'd be willing to do the actual work and open a PR. Let me know if you have questions or concerns.

[highsource]

NB: http://stackoverflow.com/questions/3567413/why-doesnt-a-missing-annotation-cause-a-classnotfoundexception-at-runtime

[highsource]

@Stephan202 I think this would be a good feature.

[Stephan202]

Cool. I'll open a PR once I have an initial prototype (which won't be until next weekend, I expect). My tentative plan of attack is as follows:

1. Manually annotate the jaxb2-basics-runtime library, as mentioned above.
2. Introduce a new plugin, to be enabled using -Xjsr305, that will augment generated code with @Nullable and @Nonnull annotations.

Augmenting the generated code must be optional, for otherwise users can't actually compile the code without adding the JSR-305 library to their classpath. That would obviously be a backwards-incompatible change.

As for how the plugin would go about annotating: I'm currently investigating a procedure whereby the plugin will try to match generated methods with interface/super class definitions, upon which the annotations found in the super type definitions can be inherited. (Thus relying on the result of step 1 above.)

If you see problems with this approach or have suggestions for an alternative, I'm all ears :)

[highsource]

Planned for 0.10.0. Documentation (TBD).

[highsource]

(1) should be no big deal, feel free to ask if something is @Nullable or @Nonnull.

(2) the -Xjsr305 switch is OK, should enable JSR-305 for JAXB2-Basics plugin. We could have additional -Xequals-jsr305 switches but I think it does not really make sense. If someone wants JSR-305 then globally, turning it on for one plugin and off for another wouldn't make much sense.

Now about the mechanics. I actually would suggest a simpler approach. -Xjsr305 just turns on some global switch. The other plugins check this switch and generate the code accordingly. No matching of inherited/implemented methods, nothing fancy. Just checks of some global switch and then either add JSR-305 annotation or not.

This wiill require changing plugins, but this is much easier and much more reliable than matching method signatures in the generated code. What do you think?

If this suggestion is OK then let's file one issue per plugin. I think only the following ones are in scope:

• SimpleEquals Plugin
• SimpleHashCode Plugin
• Equals Plugin
• HashCode Plugin
• ToString Plugin
• Copyable Plugin
• Mergeable Plugin
• Inheritance Plugin (maybe this one will need "magic" signature matching)
• AutoInheritance Plugin (this, maybe, too)
• Setters Plugin
• EnumValue Plugin
• FixJAXB1058 Plugin

[Stephan202]

@highsource, thanks for the quick feedback! I agree that your approach is simpler. I considered it as well, but initially dismissed it because I assumed there should be a one-to-one mapping between XJC extension flags and plugins (not sure why I thought that).

Still a bit uneasy about this, though: the annotation injection is a cross-cutting concern (an "aspect", if you will), and the simpler route will require changes is many places. Normally I'd argue that it is therefore error-prone, but in this case the Checker Framework has our back (i.e., we should be able to prove completeness of the annotations). Still, the generalization of this approach (by which I mean future additions of other cross-cutting concerns) would surely have a negative impact on the overall code quality.

On the other hand: reliability of my initial approach could indeed be a valid concern. I'll have to sleep over this. (I have a busy day job, so no coding on this until next weekend, I suspect.)

NB: If the (auto)inheritance plugins indeed need "magic" signature matching, we might as well apply it across the board, no?

[highsource]

@Stephan202

1:M mapping in plugins will work somehow. I'm not yet quite sure how, but I'll get it done.

I understand your point with the cross-cutting concern etc., this is valid. Still I would suggest to not overengineer the solution from the very start but rather "grow" it. Let's start with one plugin (or a pair equals/hashCode) and see how it works. I guess we'll already see the pattern (more or less the same code in two places) and then be able to refactor it accordingly.

Inheritance/Autoinheritance plugins - I'm not quite sure about methods matching. The AutoInheritance plugin does some automagic signature matching, the Inheritance plugin does not. This may be also a bit too complex as the Inheritance plugin also supports generics (you can "implement" Comparable<Foo>). Matching generic methods might be quite difficult to do.

So I'd suggest to approoach the whole thing gradually. Let's do a couple of plugins first, have something which works and see what we've learned. Validate the approach.

[Stephan202]

Alright, the safe route it is :). Two questions about annotating jaxb2-basics-runtime:

1. Do you prefer method annotations on a line by themselves (as for method foo below), or "before" the return type (as with bar)? (I generally use the former, but have no strong opinion.)

   @Nullable
   Object foo() {
       return null;
   }
   
   @Nullable Object bar() {
       return null;
   }
   

2. Mind if I also add @Override where applicable?

[highsource]

1. Have no preference. Whatever the Eclipse default formatting is. :)
2. Sure. I didn't do it because of 1.5 compat, but now it's dropped anyway, so go ahead.

[Stephan202]

Alright, so I hit a bit of a snag. Basically, the JSR-305 annotations don't support the full expressiveness described by JSR-308, which is something that the Checker Framework relies on/builds on top of. Related to this is JSR-305 ticket #25.

In particular, jaxb2-basics-runtime defines methods that accept and return arrays for which both the arrays themselves, and the elements they contain may be null. Assuming objects are non-null by default (as the Checker Framework assumes), the Object[] type may be declared in one of four ways (see section 27.5.2 and section 27.5.3 of the Checker Framework manual):

1. A non-null array of non-null elements:

   Object[]
   

2. A non-null array of possibly-null elements:

   @Nullable Object[]
   

3. A possibly-null array of non-null elements:

   Object @Nullable []
   

4. A possibly-null array of possibly-null elements:

   @Nullable Object @Nullable []
   

Now, the third and fourth variant require Java 8, but for compatibility reasons the Checker Framework allows one to write these "as a comment". I.e:

Object /*@Nullable*/ []
@Nullable Object /*@Nullable*/ []

This is not enough, however, because the JSR-305 definition of @Nullable does not specify its java.lang.annotation.Target, and is thus not legal in this position in any case.

The solution to that problem is would be to use org.checkerframework.checker.nullness.qual.Nullable. My current understanding is that we can achieve this in one of the following ways:

1. Require a provided-scope dependency on org.checkerframework:checker-qual. I'd say we should avoid using two kinds of @Nullable (especially since that requires specifying at least one using its FQCN). As such this option would entail that we do not depend on/use com.google.code.findbugs:jsr305 at all, and use the Checker Framework version of @Nullable throughout.
2. Use "commented-out" import statements as described in section 23.3.1 of the Checker Framework manual. This is would definitely require using the FQCN for the Checker Framework version of @Nullable, and so is unsatisfactory, if you ask me.
3. Use stub classes as described in section 24.2 of the Checker Framework manual. Perhaps these stubs can be part of the JAR (don't think so, though), or we could contribute them to the Checker Framework project (they already ship with JDK and Guava stub classes). All in all I'm not a fan of this approach.

I haven't completely made up my mind about what all this means for the feasibility and/or attractiveness of the feature, compared to this ticket's original project proposal.

This might all boil down to two questions, though:

1. How useful are JSR-305 annotations if we can't use then "out of the box" using the Checker Framework (or any other JSR-308 compatible tool)? (For me personally the answer is "not very", but I'm just one guy.)
2. Should we focus on a -Xcheckerframework-nullness plugin, rather than a -Xjsr305 plugin? (I'm starting to think "yes".)

Would love to hear your thoughts on this!

NB: I'm aware of #21, but any decision on this topic might carry-through to subsequent tickets, so I decided to keep this discussion central, i.e., in this ticket.

[zman0900]

Version 0.12.0 release notes say this is included, but it doesn't appear to work and I can't find anything in the source that looks like a new plugin. Are the release note just wrong?

[highsource]

Sorry, was a mistake in the release notes.

[seanf]

I'd love to be able to use generated classes in a null-safe way from Kotlin. For interop with Java classes, Kotlin looks at both nullable and non-null annotations to distinguish between nullable, non-nullable and unknown nullability (neither annotation present).

I was thinking about a more general solution for the problem of nullable annotations on arrays and their elements: get the user to supply the desired annotations on the classpath, and also specify the desired null and non-null annotations. Then we apply the null/non-null annotation only if it is legal according to the @Target rules. In other words, if using ye olde JSR-305 annotations, arrays won't be marked nullable, but users who need better null analysis can choose Checker Framework or JetBrains annotations.

So to write out JSR-305 annotations:

-Xannotation-null=javax.annotation.Nullable
-Xannotation-nonnull=javax.annotation.Nonnull

Or for Checker Framework annotations:

-Xannotation-null=org.checkerframework.checker.nullness.qual.Nullable
-Xannotation-nonnull=org.checkerframework.checker.nullness.qual.NonNull

For JetBrains annotations:

-Xannotation-null=org.jetbrains.annotations.Nullable
-Xannotation-nonnull=org.jetbrains.annotations.NotNull

And maybe even Lombok's NonNull annotation which can insert runtime not-null assertions (if compiling with Lombok):

-Xannotation-nonnull=lombok.NonNull

I had to look them up, so I'll just put the Maven coordinates here (in Gradle-Kotlin form)

    implementation("com.google.code.findbugs:jsr305:3.0.2")
    implementation("org.checkerframework:checker-qual:3.7.1")
    implementation("org.jetbrains:annotations:20.1.0")
    implementation("org.projectlombok:lombok:1.18.16")

@Stephan202

I know it has been five years, but did you save any code from your prototyping work?

[Stephan202]

Closer to six years, haha :sweat_smile:.

I looked around on my hard drive, but found nothing, sorry. (Can't remember how far I got with this anyway; not sure there was any relevant code to be found in the first place.)

(On the topic of well-known nullability annotations, let me use this opportunity to raise awareness for JSpecify; the project is a work in progress, but you might be interested in tracking its progress.)

[lion7]

@seanf I know you question has been a while ago, but throwing it down here for anyone stumbling upon this via Google (or via other means).

I prototype a simple XJC plugin (not using this library) that annotates all getters with @NotNull or @Nullable respectively. You can find it here: https://gist.github.com/lion7/b1065b52fdf781f9f82d024c619d480c Note that it is currently only compatible with the latest Jakarta JAXB version and will not work with older versions.

[lion7]

Published it as a plugin to JitPack as well: https://jitpack.io/#lion7/nullability-xjc-plugin Sources: https://github.com/lion7/nullability-xjc-plugin