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

[cpovirk]

If NullAway does someday recognize @ParametricNullness, that should roughly undo Guava's recent nullness changes around generics

Sorry, I have to take that back—though I do think I have a workaround.

The problem is that we didn't just replace parameters' @Nullable annotations with @ParametricNullness; we also added @ParametricNullness to return types. So, if NullAway treats @ParametricNullness like @Nullable, it will not only undo the changes for parameters, but it will also make methods like AbstractFuture.get() look like they return @Nullable V:

  String go(AbstractFuture<Object> future) throws Exception {
    return future.get().toString();
  }

warning: [NullAway] dereferenced expression future.get() is @Nullable
    return future.get().toString();
                       ^

My tentative plan for a workaround is to change our return types to use a different annotation, one that will still be recognized by Kotlin but that will be ignored by NullAway. That should mean that uber/NullAway@6ab3dcd will do the right thing after we cut a new Guava release.

I'm hoping to get to that early next week.

[rwinograd]

A few questions about this.

First, AbstractFuture looks to have a set method that accepts a Nullable, and I thought the get API was driven by the Future interface which (I believe is actually Nullable). So is this flagging a real issue?

Second, my understanding is that there are going to be a few different cases here:

1. AbstractFuture (assuming it is actually Not Nullable)
2. Iterables.getFirst, which was annotated as Nullable in 30, and annotated with ParametericNullness in 31.

Is the new annotation going to only be used for the first case? What does ParametricNullness mean for the purpose of the two return types?

Third, what does this mean for version compatibility and does this matter? I believe that it would mean that if Nullaway increments from version 0.9.8 to 0.9.9, and guava goes from 31.1 to 31.2 that:

• 0.9.8, 31.1 will flag Iterables.getFirst
• 0.9.9, 31.1 will flag AbstractFuture
• 0.9.9, 31.2 will be correct
  I'd have to think about 0.9.8 & 31.2 compatibility.

[cpovirk]

Nowadays, AbstractFuture.set has a parameter type of @ParametricNullness V. As you say, it makes sense for that to match the return type of get.

Previously, the parameter type was indeed @Nullable V. Since we mostly weren't annotating return types back then, the parameter type and return type were inconsistent. That opened up another possible path to getting a NullPointerException. Our annotations at the time were incomplete.

The goal at this point is to annotate things completely. It's true that, among the many places where we could have used @Nullable/@ParametricNullness, we historically had been inconsistent, as your case (1) and case (2) show. But the goal at this point is to annotate without regard for the history, at least if we can do so without making things worse.

I want to think more about your questions about version compatibility. It might be that we're better off picking two new names for our annotations in the next Guava release. I fear that there are tradeoffs either way: The best option for NullAway users who've included us in AnnotatedPackages is going to be to avoid [31.0, 31.1]. But we'll see how we can best minimize the damage within that range.

(I'm leaving hanging your questions about the meaning of @ParametricNullness. I expect to document that better as part of the changes we make.)

[lazaroclapp]

So, reading this discussion, it sounds to me like:

• In the absence of proper handling of type parameter nullness, NullAway resolving @ParametricNullness to @Nullable is correct/sound, even for the AbstractFuture.get() case above. It truly can return null, since set(...) can receive null.
• One issue, though, is that Guava might more liberally annotate things as being parametrically-nullable now, since they will be "@Nullable only when the type parameter is @Nullable" and thus NullAway's lack of precision might make handling Guava as an annotated package way too noisy unless we get some basic generics support done quickly (which might or might not be a fully JSpecify compliant implementation).
• Changing returns to a different annotation gives us a knob we can tweak for sacrificing soundness for a less noisy user experience, but, at the same time, I get the feeling that orgs marking Guava as AnnotatedPackages might want us to default to being as sound as possible (i.e. treating these newly annotated APIs as having @Nullable returns and maybe in some cases using Guava-specific library models to forbid nullable consistently in a given class' API, e.g. make both AbstractFuture.get() and AbstractFuture.set(...) strictly non-null)

Not sure to what degree we expect (2) to be a big departure in terms of what has been previously annotated @Nullable in Guava...

Either way, it looks like releasing NullAway 0.9.9 with @ParametricNullness == @Nullable is still likely the best solution on our end.

p.s. Not sure how realistic "skip this Guava version" is as general advice, Guava tends to be a dependency that gets forced to one version or another by virtue of what the rest of your dependencies are built against.

[cpovirk]

Based on @lazaroclapp 's comment, including enhancements to arrive in NullAway 0.9.9, we'll try leaving Guava as it is for now: Putting Guava in AnnotatedPackages improves soundness at the cost of false positives (like the AbstractFuture example above); leaving Guava out does the reverse.

The good news is that we didn't add @ParametricNullness to a ton of return types, which will be the main source of false positives with NullAway 0.9.9. The biggest problems for some users will be the functional types (like Function and Supplier); other users will see trouble from generic utility methods (like Iterables.getOnlyElement and Comparators.max).

We'd like to hear how this goes for users once they're able to adopt the NullAway change. (I'm also trying to experiment with this a little myself.) The best way for us to minimize user pain will be for Guava and tools to adopt an annotation model that is more suited to generics, but in the meantime, we can monitor the effects of this intermediate step and consider alternatives.

I'm also merging some expanded docs for @ParametricNullness now. That won't cover generics in general, but the docs that @kevinb9n mentions will get into that once they arrive. I'll also make another update to the release notes for the Guava release that introduced these annotations. My @ParametricNullness commit is marked as closing this issue (since most of the followup will be non-Guava-specific docs around annotations, aside from the work tracked by #2960), but further comments are welcome, whether now (e.g., on the new docs) or after you've had a chance to upgrade NullAway to try its forthcoming behavior.