Nullness javadocs are unclear and break certain static analyzers

[rwinograd]

I'm looking at the changes in 31.0 for enhanced null-checking, and I'm trying to validate that my understanding of the change is correct and bring attention to some issues I am having with the approach.

As an example, let's look at one of the methods that changed from 30 to 31: Iterables.findFirst(). Comparing the signatures in javadoc we have:

• 31: public static <T extends @Nullable Object> T getFirst(Iterable<? extends T> iterable, T defaultValue)
• 30: public static <T> @Nullable T getFirst​(Iterable<? extends T> iterable, @Nullable T defaultValue)

If I understand correctly, nothing has changed in the contract, and in both versions the returned object can be null, the defaultValue can be null and the iterable cannot be null. The issues we are having are as follows:

1. This seems fairly hostile to some of the static analysis tools on the market as they have to change their implementation logic to inspect the Generic type of the method/class rather than looking at the parameter or method annotations. We are using Nullaway, and when upgrading to guava 31 previously working code is now broken. While the release notes indeed call this out it's unobvious that this should cause more false positives rather than just identifying more issues because more information is being shared: "By providing additional nullness information, this release may result in more errors or warnings from any nullness analyzers you might use." The issue isn't that we're receiving more nullness information, but that the contract for that how that information is being provided has changed. As an example this code now causes an issue: @Nullable String value = Iterables.getFirst(ImmutableList.<String>of(), null) because it appears to the static analyzer that second parameter of Iterable#getFirst is null hostile even though this is correct code.
2. This makes it difficult to read the javadoc for non-static methods like FutureCallback#onSuccess. I can no longer look at this method and understand the nullness convention because there is no information in the method that V is actually nullable. I have to look at the class signature itself. Compare this to the old javadoc. Even for static methods, it's unclear that the Nullable actually applies to instances of type T in the signature as the annotation is on the base type Object. Is this a common java convention that I just haven't seen before?
3. It's seems strange in light of the package annotation of ParametersAreNonnullByDefault the definition which calls out that "to indicate that the method parameters in that element are nonnull by default unless there is ... An explicit nullness annotation." It's unobvious that the nullness parameter on the generic type of the method or class count as an explicit nullness annotation (but I am curious whether this was the intent of the JSR authors).

There are a couple other questions I have (how does this convention apply when there is a non-null param and a nullable return, or vice versa) but the ones above are the most pressing.

[kevinb9n]

I appreciate this report and am sorry for the trouble.

I think we have the following situation:

• We are in the process of defining new and very precise semantics for the meanings of nullness annotations (jspecify.dev)
• NullAway (and several other tools) has been on board with the new direction, but doesn't support it fully yet

So I think the main question here is whether our change was premature; i.e. whether there's a reasonable way for us to execute this transition more smoothly for you.

Note that the old signature did have a bug: it would not accept an Iterable<@Nullable Foo> even though there's nothing about the spec or implementation that can't handle that case. So the new signature is actually providing more correct information, at least.

It is true that in the new world, type parameters are declared as either nullable-bounded (<T extends @Nullable Foo>) or non-null-bounded (<T extends Foo>). That seems to mean extra steps when understanding a method signature (that uses T). But you should almost always "get away with" a blanket assumption instead: "if there's any reason I would actually want to give type arguments of either nullness, then I probably can". And that's the case most of the time, too (Class and ImmutableList are counterexamples that pop to mind, and you can see why a nullable type argument wouldn't make sense).

There is a lot more information about the new semantics to be published very soon. I'm working on it right now. I'm happy to continue this conversation, too.

[lazaroclapp]

NullAway maintainer here. This definitely is already in our radar (cross-ref: uber/NullAway#628, cc: @msridhar)

It appears that in all cases where @Nullable is being removed, a new @ParametricNullness annotation is being added. The documentation isn't super clear on the semantics, but it seems to be:

1. a no-op for tools that support full nullability of generics (i.e. the JSpecify spec),
2. a hint for tools that do not (Kotlin? Not sure here), that the field can, under some condition based on type arguments, be @Nullable.

Are we getting this right?

Also, if we are, is this something we can rely on for Guava 31+ (at least for the foreseeable future)?

I think the best path forward for NullAway is to, in the short term, force @ParametricNullness to be an alias for @Nullable. We will likely build in this translation into the next release of NullAway, and are aiming to get it out this week or the next. If you need to be unblocked here immediately, I recommend passing this class to NullAway's configuration as a custom nullability annotation.

In the long term, this will be moot once we support full JSpecify semantics. In fact, we will then have to remove the alias to avoid the lost of precision involved in always considering getFirst(Iterable<? extends T> iterable, T defaultValue) nullable - even for a non-null T. As will any codebase that configures it as a custom nullable annotation. That said, full JSpecify support is something we really want to get to, but at the same it isn't something we are resourced to do in a super short timeframe.

[cpovirk]

Thanks, Richard, and sorry for the trouble.

One question to help me understand the scope of the problem: Are you setting the AnnotatedPackages (sometimes set under the differently cased name "annotatedPackages") flag to cover com.google.common? I'm finding that I need to do that in order to reproduce the false positives. That's not to take away from the trouble our Guava change is causing: It's sad that it changes Guava from working well in the AnnotatedPackages flag to not working well there. If the problem is even more widespread than that, then that's even worse.

[rwinograd]

Thanks all for the quick responses. Greatly appreciated.

A few things to call out.

It appears that in all cases where @nullable is being removed, a new @ParametricNullness annotation is being added.

I actually avoided mentioning this because AFAIK this was intended to be a package-private implementation detail of gauva. I actually think at least some of the documentation problems could be avoided by making it public (and having a single consistent annotation with more documentation) and thus also making it clear on the parameter/method that the nullness properties are indeed defined by the generic type.

I think this also may have some nice fail-safe behaviour whereas people can decide what this means until we support it more fully. One thing to call out here is that the @ParametricNullness annotation seems to have a @NonNull(when=UNKNOWN) meta-annotation. I'm curious if it would make more sense to make it @Nullable(when=UNKNOWN) to make "the best path forward for NullAway is to, in the short term, force @ParametricNullness to be an alias for @nullable." a reasonable option.

Are you setting the AnnotatedPackages (sometimes set under the differently cased name "annotatedPackages") flag to cover com.google.common

I can confirm that we are indeed doing this in common build logic. Agree that this does make the problem less widespread, but still sad. Guava is also big enough that I think we'd be sad about turning this off for the entirety of the 31 version, but curious to hear your thoughts.

There is a lot more information about the new semantics to be published very soon. I'm working on it right now. I'm happy to continue this conversation, too.

Super excited to hear about this. I think this item that you called out above is most critical: "I think the main question here is whether our change was premature; i.e. whether there's a reasonable way for us to execute this transition more smoothly for you." I'm 100% excited about having better annotations here, I'm just hoping that we can make sure the transition to that (hopefully near) future is smooth.

[lazaroclapp]

One question to help me understand the scope of the problem: Are you setting the AnnotatedPackages (sometimes set under the differently cased name "annotatedPackages") flag to cover com.google.common?

For what it's worth, this is not an uncommon configuration. I think that, if not currently, at various points we have also relied internally on the fact that Guava is annotated for nullness (not that it isn't anymore, but the change in how it is annotated will definitely confuse various tools, including NullAway).

I actually avoided mentioning this because AFAIK this was intended to be a package-private implementation detail of gauva.

Ah, I actually didn't realize the annotation was package private! I think NullAway should be able to see it either way, but it certainly makes me even more concerned to depend heavily on it. Is there any way we could get this upgraded to at least a transitional annotation that tools that don't currently support nullable annotations on type parameters can rely upon?

[cpovirk]

RE: @ParametricNullness: The docs are indeed bad :( Of course, the ultimate goal is to continue with the work that will make them unnecessary. And—stop me if you've heard this one before—the plan was for that work to be done a lot earlier than it's going to be, so we figured we could get by with the docs we have now during the transition....

As for the meaning, you've got it, including that the main consumer is Kotlin. (See the only marginally improved docs from the latest release.) It's defined to undo the effect of @ParametersAreNonnullByDefault, and there's a good argument that it should be @Documented for that reason. (The downside of @Documented is that "@ParametricNullness" sounds very scary, especially considering that it's just a fancy name for how generics work in, e.g., Kotlin.) We'd probably prefer not to make it public because we've learned that we would never be able to remove it afterward :( We could maybe justify doing so, but first I'd say:

The package-private @ParametricNullness annotations (sigh, there is one per package) can stick around indefinitely if they continue to be helpful. Once Kotlin users no longer need them, we should remove the part of their declarations that makes them affect Kotlin. In fact, we've already done that internally. But it is likely to be useful to keep the usages around for other tools after that, and we should remember to check on NullAway as part of that. To that end, I've updated a couple internal bugs about that cleanup to link back here as a reminder.

I was going to say that there's agreement between us and NullAway that that makes -XepOpt:NullAway:CustomNullableAnnotations=com.google.common.base.ParametricNullness,...,com.google.common.util.concurrent.ParametricNullness a good option to unblock you and a good thing for NullAway to do by default someday, but we'll see where the discussion about package-private annotations goes :)

If NullAway does someday recognize @ParametricNullness, that should roughly undo Guava's recent nullness changes around generics, leaving you pretty much where you were before except with better annotations in other locations (especially on return types). Hopefully that will make the combination of changes a net win for NullAway users who've included us in AnnotatedPackages. (I did not realize how common this was, so thanks to both of you for the information. I wish I'd thought to ask ahead of time.) Hopefully that eventually includes making them a net win for you, @rwinograd, despite the trouble it's been causing so far.

[lazaroclapp]

If NullAway does someday today recognize @ParametricNullness [...]

😉

In all fairness, that's on master branch right now. We are aiming to cut a release before the end of the week if we can, though.

[rwinograd]

Hopefully that eventually includes making them a net win for you, @rwinograd, despite the trouble it's been causing so far.

Appreciate all the help and support on this issue everyone. Looking forward to see what happens with the jspecify.dev stuff