AnnotatedPackages w/ com.google.common not working due to Preconditions.checkNotNull

I'm using Nullaway 0.9.0 using the Bazel build system. We have a small library that is using google guava and are trying to treat the Guava library as if it is correctly annotated with Nullable annotations. Our problem is that when we add com.google.common to our list of annotatedPackages, the build starts failing because it says that Preconditions.checkNotNull rejects a Nullable field.

The reason we're trying to do this in our real application is because we have some fields where a null value indicates a coding mistake and cannot easily restructure the code to avoid this pattern. Sample code:

BUILD.bazel

java_library(
    name = "mypackage",
    srcs = glob(
        ["*.java"],
    ),
    javacopts = [
        "-XepOpt:NullAway:AnnotatedPackages=com.mypackage,com.google.common",
    ],
    plugins = [
        "//third_party:nullaway",
    ],
    visibility = [
    ],
    deps = [
        "@third_party_jvm//3rdparty/jvm/com/google/code/findbugs:jsr305",
        "@third_party_jvm//3rdparty/jvm/com/google/guava",
    ],
)

com.mypackage.SomeClass

package com.mypackage;

import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import javax.annotation.Nullable;

public class SomeClass {

  public ImmutableList<String> someMethod(@Nullable String nullable) {
    return ImmutableList.of(Preconditions.checkNotNull(nullable));
  }
}

Expected:

My understanding is that this code should pass because Preconditions.checkNotNull is marked as a special method that accepts a nullable field, but ensures that the returned value is no-null because it fails if it is null. This is from reading the javadoc of the LibraryModels#failIfNullParameters interface (Note that description is actually on the nonNullParameters method). The code indicates that Preconditions is marked as FAIL_IF_NULL_PARAMETERS. This appears to be wired in as the default, and I believe that the documentation indicates that this should always take precedence:

Also, specific method annotations can always be overridden by explicit Library Models, which take precedence over both the optimistic defaults and any annotations in the code, whether marked as annotated or unannotated.

Actual behaviour:

From Building src/main/java/com/mypackage/libmypackage.jar (1 source file):
src/main/java/com/mypackage/SomeClass.java:10: warning: [NullAway] passing @Nullable parameter 'nullable' where @NonNull is required
    return ImmutableList.of(Preconditions.checkNotNull(nullable));
                                                       ^
    (see http://t.uber.com/nullaway )

Our workaround right now would be to include guava in the AnnotatedPackages classpath but treat Preconditions as an unannotated class

    javacopts = [
        "-XepOpt:NullAway:AnnotatedPackages=com.mypackage,com.google.common",
        "-XepOpt:NullAway:UnannotatedClasses=com.google.common.base.Preconditions",
    ],

This approach the build works as expected, however, from the documentation this doesn't appear to be the expected integration path. Is this a bug where the override behaviour is not working or am I misreading the documentation?

We ran through some extra tests just to see what would happen.

Test case 1: Do not include guava in the AnnotatedPackages classpath

java_library(
    name = "mypackage",
    srcs = glob(
        ["*.java"],
    ),
    javacopts = [
        "-XepOpt:NullAway:AnnotatedPackages=com.mypackage",
    ],
    plugins = [
        "//third_party:nullaway",
    ],
    visibility = [
    ],
    deps = [
        "@third_party_jvm//3rdparty/jvm/com/google/code/findbugs:jsr305",
        "@third_party_jvm//3rdparty/jvm/com/google/guava",
    ],
)

package com.mypackage;

import com.google.common.collect.ImmutableList;
import javax.annotation.Nullable;

public class SomeClass {

  public ImmutableList<String> someMethod(@Nullable String nullable) {
    return ImmutableList.of(nullable);
  }
}

As expected, errorprone did not error out and NullAway because it will not treat ImmutableList's argument as a nullable parameter.

Test case 2: Include guava in the AnnotatedPackages classpath but don't use Preconditions

java_library(
    name = "mypackage",
    srcs = glob(
        ["*.java"],
    ),
    javacopts = [
        "-XepOpt:NullAway:AnnotatedPackages=com.mypackage,com.google.base",
    ],
    plugins = [
        "//third_party:nullaway",
    ],
    visibility = [
    ],
    deps = [
        "@third_party_jvm//3rdparty/jvm/com/google/code/findbugs:jsr305",
        "@third_party_jvm//3rdparty/jvm/com/google/guava",
    ],
)

public class SomeClass {

  public ImmutableList<String> someMethod(@Nullable String nullable) {
    return ImmutableList.of(nullable);
  }
}

As expected, errorprone failed with the following message.

INFO: From Building src/main/java/com/mypackage/libmypackage.jar (1 source file):
src/main/java/com/mypackage/SomeClass.java:9: warning: [NullAway] passing @Nullable parameter 'nullable' where @NonNull is required
    return ImmutableList.of(nullable);
                            ^
    (see http://t.uber.com/nullaway )

Calling out that I also believe due to issue 47 that this should be supported.

I believe com.google.common.base.Preconditions.checkNotNull is already supported in open source NullAway too. @MatteoJoliveau If you can use that method, then that's the fastest workaround right now. If this does not work, please let me know.

At a quick glance, this looks like a bug to me. @lazaroclapp any chance you have time to take a look?

This is an unfortunate consequence of #446 . Basically, since we are not allowing library models to affect annotated code anymore (due to the "inherited annotations" issue), after NullAway 0.9.0, marking com.google.common as annotated means "this is annotated according to NullAway, so skip third-party library models".

This is not exactly a bug, in the sense that, if Guava was truly first party here, the issue could be resolved by simply annotating:

  @CanIgnoreReturnValue
  public static <T extends @NonNull Object> T checkNotNull(@Nullable T reference) {
    if (reference == null) {
      throw new NullPointerException();
    }
    return reference;
  }

Optionally with an @Contract("null -> fail") annotation.

That said, the above is not terribly useful is Guava happens to be a binary library that is null annotated, and "mostly" NullAway compatible in how it's null annotated, but not 100% conformant with what NullAway expects and thus needing a few library models.

Note: I am not sure Guava really matches the above sentence, and we don't treat Guava as annotated at Uber (we do run with -XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true, which will pick some annotations from it, though). But, the general point is such a library is certainly possible.

The way I see it, there are three solutions:

Do nothing. AnnotatedPackages means "fully annotated for NullAway, in the source/bytecode" and thus library models are always ignored for packages listed there. That's probably the most elegant/principled solution, but needlessly restricts a use case that user might care about.
Distinguish between first-party and annotated. We have been treating those concepts as equivalent so far, but they aren't. If NullAway has a way to configure "this are the packages for which we have source" and "these are NullAway annotated third-party jars", then it becomes obvious that third-party models apply to the second, but not the first.
Special case inheritance in library models. So, models directly mentioning method m1(...) apply to m1 always, whether annotated or unannotated (i.e. first-party or not), but they only apply to m2 which overrides m1 if m2 is unannotated/third-party. This will do the right thing in most cases, but it's super hacky conceptually.

One thing that might drive our decision here, @rwinograd : is this a configuration you already had running correctly for NullAway 0.8.0? (before the change in #446) Or are you just trying to make Guava an annotated package for the first time?

I am somewhat surprised that the configuration would work, even if Guava has good nullability annotations, since the annotation convention is currently not the same followed by NullAway. There is work to standardize this stuff, but neither project is there yet.

The unexpected behaviour also exists in NullAway 0.8.0 and we are trying to make guava an annotated package for the first time.

If you're asking my opinion, 2 is the option I'd prefer as a user. I would be happy to make a distinction between "this are the target packages that I'm analyzing" and "these are the library packages that are annotated", which seems somewhat equivalent to your model above. Of course, that has all sorts of potentially weird impacts. For my typical situation, I'd be analyzing something in com.mypackage.x, and assuming (in addition to com.google.common) that everything in com.mypackage.* is correct - perhaps a simple hierarchy is sufficient here where the "analyzing package" takes precedence over the "annotated package".

I am somewhat surprised that the configuration would work, even if Guava has good nullability annotations, since the annotation convention is currently not the same followed by NullAway.

Our package is obviously much more complex than the toy application I linked above. When we tried to add com.google.common, NullAway isn't producing any errors other than the Precondition cases. If that is giving us a false sense of security I'd love more details here so I can set my expectations accordingly.

It is a bit more complicated than just saying "this are the target packages that I'm analyzing", because - at least for us, in our monorepo - there is a difference between "this is not the current target I am building, but I can easily go and change the source if I want to" vs "this is a compiled, versioned, third-party jar that I am pretty sure is NullAway annotated but I don't control". But, yeah, on a basic level, we can create a different category for "not mine, but annotated".

For practical reasons, this will probably end up being -XepOpt:NullAway:ThirdPartyAnnotatedPackages or some such, since changing the meaning of AnnotatedPackages would be a pretty major breaking change.

Our package is obviously much more complex than the toy application I linked above. When we tried to add com.google.common, NullAway isn't producing any errors other than the Precondition cases. If that is giving us a false sense of security I'd love more details here so I can set my expectations accordingly.

That's interesting. My worry here wouldn't be that you get a false sense of security in the sense of missing potential NPEs. Anything marked as annotated is treated more strictly than the optimistic defaults for unannotated code. My worry here is that, because Guava is not exactly NullAway-annotated (even if it has nullability annotations), you'll be getting a number of false positives (like the one you are seeing in this issue but for things that don't have a library model) out of it.

Actually, just to check how much of the issue is the effect of #446 , could you try building both your toy example and your full codebase with NullAway 0.8.0? (Alternatively, just build a version of NullAway master that reverts the changes in that PR)

"this is not the current target I am building, but I can easily go and change the source if I want to" vs "this is a compiled, versioned, third-party jar that I am pretty sure is NullAway annotated but I don't control"

We have a mono repo also, and I'm not sure I see that distinction. From my perspective while I'm building com.mypackage.x and it depends on com.mypackage.y, there is no difference between com.mypackage.y and com.google.common - I want it to flag the place in com.mypackage.x that incorrectly passes nullable fields into those packages. The solution may be different, because in the case of issues calling com.mypackage.y I may be able to change the dependency, but (genuinely curious) does that change the analysis?

Good to know about the issue - I'll definitely take the noisy false positive with the idea that I should add some SuppressWarnings or file bugs against Guava versus a false sense of security. Can you go into more detail about this: "Guava is not exactly NullAway-annotated (even if it has nullability annotations)"?

~I originally thought this wasn't working, but I just retried it and it seems to be working now. Going to look into this a little bit more, because I actually tried this originally on 0.8 and then tested/filed the bug on 0.9.~

Test case 1: Do not include guava in the AnnotatedPackages classpath

    javacopts = [
        "-XepOpt:NullAway:AnnotatedPackages=com.mypackage",
    ],
    plugins = [
        "//third_party:nullaway",
    ],

public class SomeClass {

  public ImmutableList<String> someMethod(@Nullable String nullable) {
    return ImmutableList.of(Preconditions.checkNotNull(nullable));
  }
}

Result: Successfully compiled

Test case 2: Include guava in the AnnotatedPackages classpath but don't use Preconditions

    javacopts = [
        "-XepOpt:NullAway:AnnotatedPackages=com.mypackage,com.google.common",
    ],

public class SomeClass {

  public ImmutableList<String> someMethod(@Nullable String nullable) {
    return ImmutableList.of(nullable);
  }
}

Result:

src/main/java/com/mypackage/SomeClass.java:9: warning: [NullAway] passing @Nullable parameter 'nullable' where @NonNull is required
    return ImmutableList.of(nullable);
                            ^
    (see http://t.uber.com/nullaway )

Test case 3: Include guava in the AnnotatedPackages classpath but strip out Preconditions

    javacopts = [
        "-XepOpt:NullAway:AnnotatedPackages=com.mypackage,com.google.common",
        "-XepOpt:NullAway:UnannotatedClasses=com.google.common.base.Preconditions",
    ],
    plugins = [
        "//third_party:nullaway",
    ],

public class SomeClass {

  public ImmutableList<String> someMethod(@Nullable String nullable) {
    return ImmutableList.of(Preconditions.checkNotNull(nullable));
  }
}

Result: Success

Test case 4: Include guava in the AnnotatedPackages classpath

    javacopts = [
        "-XepOpt:NullAway:AnnotatedPackages=com.mypackage,com.google.common",
    ],
    plugins = [
        "//third_party:nullaway",
    ],

package com.mypackage;

import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import javax.annotation.Nullable;

public class SomeClass {

  public ImmutableList<String> someMethod(@Nullable String nullable) {
    return ImmutableList.of(Preconditions.checkNotNull(nullable));
  }
}

Result: ~Working~

src/main/java/com/mypackage/SomeClass.java:10: warning: [NullAway] passing @Nullable parameter 'nullable' where @NonNull is required
    return ImmutableList.of(Preconditions.checkNotNull(nullable));
                                                       ^
    (see http://t.uber.com/nullaway )

@rwinograd can you comment on whether this issue is "fixed" as of NullAway 0.9.10?

uber / NullAway