Guava v31.0.+ introducing @ParametricNullness breaks our builds

[holtherndon-stripe]

Nullaway Version: 0.8.0
Guava Version: v31.0
Build System: Bazel version 4.2.2

• If you think you found a bug, please include a code sample that reproduces the problem. A test case that reproduces the issue is preferred. A stack trace alone is ok but may not contain enough context for us to address the issue.

In the latest version of Google Guava, ParametricNullness is introduced. In v30.0 FutureCallback, for example, onSuccess is marked with @Nullable, however in v31.0 FutureCallback Guava changes this to @ParametricNullness. Since this annotation is unrecognized by Nullaway, it causes our builds to fail with the error:

pkg/NullawayParametricNullness.java:11: warning: [NullAway] passing @Nullable parameter 'result' where @NonNull is required
        futureCallback.onSuccess(result);
                                 ^
    (see http://t.uber.com/nullaway )
error: warnings found and -Werror specified

Source:

package pkg;

import com.google.common.util.concurrent.FutureCallback;
import javax.annotation.Nullable;

public class NullawayParametricNullness {
  public static <T> FutureCallback<T> wrapCallback(FutureCallback<T> futureCallback) {
    return new FutureCallback<T>() {
      @Override
      public void onSuccess(@Nullable T result) {
        futureCallback.onSuccess(result);
      }

      @Override
      public void onFailure(Throwable throwable) {
        futureCallback.onFailure(throwable);
      }
    };
  }
}

[msridhar]

Thanks for the report! My first thought as a quick fix is to just treat @ParametricNullness as if it were the same as @Nullable. That should certainly fix this issue. But I will try to dig and understand a bit better.

[lazaroclapp]

@holtherndon-stripe : Potential work around and our own proposed plan forward in this comment upstream.

tl;dr: -XepOpt:NullAway:CustomNullableAnnotations=com.google.common.util.concurrent.ParametricNullness should get you unblocked and we plan to hard-code support in the next NullAway version. It's not a perfect map for the semantics of @ParametricNullness (and I am still confirming that we got those right), but it might be the best we can do until we have full support for nullability of generics (type parameters).

Edit: Apparently there are many different @ParametricNullness annotations in Guava. Might be easier to wait for a new release of NullAway which handles them all (currently have a PR up for this: #629 )

[rwinograd]

Might be easier to wait for a new release of NullAway which handles them all (currently have a PR up for this: #629 )

OOC, roughly when would this release be?

[lazaroclapp]

Might be easier to wait for a new release of NullAway which handles them all (currently have a PR up for this: #629 )

OOC, roughly when would this release be?

Aiming for the end of this week or early next week. However, if this is blocking people, I can also try and aim for tonight/tomorrow. There is some internal testing we'd like to do on an unrelated feature (unlikely to be enabled outside of Uber right now), but worst case it just means cutting two releases one soon after the other.

[rwinograd]

I don't believe it's blocking. We wound up suppressing a large number of warnings for now, and those can be undone when we merge in the next version of NullAway.

It is somewhat time sensitive though, as I would like to avoid people too eagerly suppressing warnings and having to find them later.

[lazaroclapp]

It is somewhat time sensitive though, as I would like to avoid people too eagerly suppressing warnings and having to find them later.

Given this, we are releasing NullAway 0.9.9 with the fix today, please let us know if you run into any issues!

Note two things:

• In general, said fix involves a trade-off where false positives are possible for Guava APIs (as opposed to false negatives). We are explicitly treating Foo<T> as having @Nullable T if that type is permitted for any instance of Foo.
• In theory, this is no different than the level of support we had for nullability in Guava before 31.0, but the fact that they have more precise tracking of nullability depending on type parameters, means Guava has added @ParametricNullness to places were @Nullable wasn't present before (see this comment) and might do so more liberally in the future.

Short term, this is the best option we have for handling 31.0 in a manner that is compatible with how we handled previous versions of Guava.

Long term, if both Guava and NullAway implement JSpecify semantics (or even some more rudimentary compatible generics / type parameter support to begin with), this will lead to more precise tracking of nullness information through Guava APIs (e.g. FutureCallback<@Nullable String> will allow null, and FutureCallback<String> will not, and NullAway should be able to track which is which). This is definitely in our roadmap, just not something we can implement in a short timeframe!

We are exploring some medium-term solutions too. On the user side, right now, a medium effort workaround exists for any new false positives: 0.9.9's treatment of @ParametricNullness can be augmented with custom library models to force specific classes within Guava to default to disallowing null, even if Guava itself says it's up to the type parameter.

(Edit: Already ran into one case where library models were needed in our own codebase for Guava classes using parametric nullness, see #632)

Will close this specific issue after a while or if we hear that NullAway 0.9.9 generally solves the issue for the time being :)

[msridhar]

@holtherndon-stripe @rwinograd can this issue be closed now?

[holtherndon-stripe]

Yep! This solved our issue, I'll go ahead and close it!