Support JSR305 meta annotations TypeQualifierNickname and TypeQualifierDefault

[keithl-stripe]

I'm filing this as a follow-up to #628. (Note that fixing this will allow undoing the temporary workaround in #629)

If you read the bug at #628 (and https://github.com/google/guava/issues/6126), @ParametricNullness is not be doing anything novel or weird – it is just using javax.annotation.meta annotations which were specified as part of JSR 305 fifteen years ago in 2007. Unfortunately, unlike IntelliJ and TCF, NullAway does not support these.

The ideal fix for #628 would be for NullAway to support TypeQualifierNickname and TypeQualifierDefault. This would obviate the need for special cases to handle these Guava annotations.

I would guess (not knowing much about NullAway architecture) it should not be a huge effort to implement this (though it may be hard to not degrade performance). Roughly:

• Every time NullAway understands @Nullable (e.g. variable declarations, JSR308 annotated types):
• For each other annotation present,
  • if that annotation class itself is marked as @TypeQualifierNickname, interpret that class's annotations as being on the original AST element
• For each parent AST node, check if it is marked with @TypeQualifierDefault, and interpret that node's annotations as being on the original AST element

[msridhar]

Thanks for the suggestion! But I'm not sure I completely follow. Look at a @ParametricNullness annotation I see:

@javax.annotation.meta.TypeQualifierNickname
@javax.annotation.Nonnull(when = javax.annotation.meta.When.UNKNOWN)

But, our solution in #629 ended up treating @ParametricNullness as if it were @Nullable, not @Nonnull. So I don't think the support for @TypeQualifierNickname would replace the implemented solution. Did I miss something?

[lazaroclapp]

There was some discussion on https://github.com/google/guava/issues/6126 about changing the meta-annotation for @ParametricNullness from @javax.annotation.Nonnull to @javax.annotation.Nullable. But, until/unless that is done, supporting the general mechanism doesn't actually help us parse that annotation (we would still need to special case it, and would have to pay the overhead of checking every annotation for meta annotations). Arguably, NullAway's original behavior was correct with respect to the meta-annotations, since @NonNull is our default.

If there are unrelated use cases that would benefit of us supporting @TypeQualifierNickname/@TypeQualifierDefault, that's a worthwhile discussion, but I feel JSpecify support is a higher priority for us than JSR 305 support, and there is definitely a performance/simplicity dividend that we get from NullAway not supporting meta annotations and being opinionated about nonnull being the default everywhere.

That said, it is certainly doable, if there is a need beyond just Guava's @ParametricNullness.

Now that we support a slower mode for code with mixed-checking for @NullMarked, we could support @TypeQualifierDefault specifically for @Nullable/@NonNull. It would require switching to that slower checking whenever we see @TypeQualifierDefault for the first time on a compilation unit, though.

No idea how to support @TypeQualifierNickname without penalty: probably caching, for every new annotation we see, whether it is nullness related due to meta annotations or not? No idea what will be faster, a map lookup or a check for the presence of @TypeQualifierNickname.

[keithkml]

Thanks for the thoughtful responses!

I guess we need to agree on what this annotation means:

@javax.annotation.Nonnull(when = javax.annotation.meta.When.UNKNOWN)

To me, that means it may be null, because, while there are situations where it's nonnull, it's "unknown" what these situations are. Is that how we're supposed to read the when parameter?

[cpovirk]

I'm not sure how where there's good official documentation for the jsr305 annotations. My understanding is that UNKNOWN means something like: "Static analyzers, don't try to report any nullness problems here: Act like normal Java, where it's fine to put null in but also fine to dereference the value you take out."

If the goal is instead to tell tools to issue warnings appropriate to a value that might be null, then I think the right value is MAYBE.

Here's at least some evidence that others have interpreted it that way:

• https://kotlinlang.org/docs/java-interop.html#jsr-305-support
• https://youtrack.jetbrains.com/issue/IDEA-286129/Spotbugss-Nullable-annotation-causes-Cannot-annotate-with-both-Nullable-and-Nullable-warning#focus=Comments-27-5704200.0-0
• (And this is also historically how Guava used the jsr305 @Nullable, informed by how Findbugs (predecessor to Spotbugs) worked.)

[lazaroclapp]

That's interesting. I think from the NullAway standpoint, I wasn't even thinking of the subtleties of UNKNOWN vs MAYBE, but rather that there is two ways to interpret @javax.annotation.Nonnull(when = [...]) given NullAway's opinionated defaults for annotated code. Namely:

"In annotated code, NullAway considers all references to be non-null by default, unless explicitly annotated as @Nullable"

There are two ways to interpret @NonNull(when = [...]) in that light:

1. It adds the @Nonnull annotation under some circumstance, but we don't care, because the only annotation NullAway cares about in "annotated" code is @Nullable or it's aliases.
2. It modifies the "considers all references to be non-null by default" part of our defaults, conceptually changing an implicit @Nonnull into an explicit but conditional @Nonnull.

Either way, that's only with respect to NullAway's internal mental model. If we decide that actually implementing JSR305 is something we care about, rather than basically hijacking its annotations for our purposes, then I think going by the interpretations @cpovirk shared makes the most sense.

Personally, I am not against more faithful JSR305 support if it doesn't compromise NullAway's performance on code that doesn't use these @TypeQualifierNickname/@TypeQualifierDefault annotations. That said, in terms of our own resourcing and prioritization, I am not sure how much of a priority this is vs e.g. JSpecify support (having any type parameter nullability support, even in a relatively hacky version, would both render our existing workarounds obsolete and make the correct of handling of @ ParametricNullness be a no-op, no matter what meta-annotations it carries)

[keithl-stripe]

Thanks for explaining your rationale Lázaro & Chris!

Fortunately, I believe the spec and documentation for JSR305 answers all of our questions!

In JSR305, @Nullable is actually defined simply as a @TypeQualifierNickname for @Nonnull(when = UNKNOWN).

From the source code:

@TypeQualifierNickname
@Nonnull(when=UNKNOWN)
public @interface Nullable {}

It has no semantics of its own; in fact, @Nullable (or @UnknownNullness as it was originally called) is not even mentioned in the (brief) written specification for JSR 305.

So it seems safe and correct to treat @Nonnull(when = UNKNOWN) as exactly equivalent to @Nullable.

[cpovirk]

Oh, wow, thanks for those jsr305 slides!

Treating them equivalently would definitely be the technically correct thing to do. My understanding (which might or might not be what you're advocating for) is that the technically correct way to resolve that difference would be to change how the jsr305 @Nullable is currently handled, making it different from how NullAway handles other @Nullable annotations and how it handles @CheckForNull. I think this is what an IntelliJ issue comment was getting at:

In the latest release, we started to recognize some meta-tags like @Nonnull(when = When.UNKNOWN) and this feature accidentally restored the semantics of @Nullable as 'unknown nullability'.

As the slides you found say, @Nullable technically means "unknown nullability." But tools have found that they have to special case it to mean "known to be possibly null" in order to match users' expectations. And so, aside from Findbugs/Spotbugs, I don't think any tool (NullAway included) treats the jsr305 @Nullable "correctly." (This has caused Guava users pain over the years, since we historically tried to follow the technical contract.)

I assume that changing NullAway's handling of jsr305 @Nullable is off the table. So I'm not sure how much help that ends up being for choosing its handling of @ParametricNullness and @TypeQualifierNickname: Is it better to be consistent with the technically incorrect existing behavior or not? As with @Nullable, I suspect that we're best off making that decision based not necessarily on the jsr305 docs as much as on how they've historically been used and interpreted.