Discrepancy in expected Nullness checks when upgrading from v0.8.0 to v0.9.+

[holtherndon-stripe]

• 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.

• Please include the library version number, including the minor and patch version, in the issue text.

Current Production Version of NullAway: v0.8.0
Attempting to move to NullAway: v0.9.9
Tests performed on NullAway: v0.9.0 and v0.9.9 (with same result)
Errorprone Version: 2.4.0
Bazel Version: 5.0.0

We are currently trying to migrate to v0.9.9 so we can upgrade our Guava version (see: #628), however, when we attempted to upgrade, we got the follow error message from Bazel:

mypackage/MyClass.java: warning: [NullAway] passing @Nullable parameter 'id' where @NonNull is required
      builder.setId(id);

On the code:

    // Truncated for security reasons
    } else if (!Strings.isNullOrEmpty(id)) {
      builder.setId(id);
    }

After digging for a while, I was able to pin the issue down to us including com.google.common in our AnnotatedPackages flag - after removing it, it worked. However, that removed NullAway's ability to check against Guava's annotations. I wasn't able to find where in NullAway this discrepancy was occurring, but I was able to put together a set of tests that have contradicting results.

Thanks in advance! 😄

Reproducing code samples in test format:

Example 1: This example fails, however, we are checking a's Nullness Strings.isNullOrEmpty - this works as expected in v0.8.0, but fails in v0.9.+. This is the issue we ran into in CI while trying to upgrade.

  @Test
  public void shouldPass_nestedCallWithIsNullOrEmptyAndAddedAnnotatedPackages() {
    // This test fails tho it is properly guarded
    makeTestHelperWithArgs(
            Arrays.asList(
                "-d",
                temporaryFolder.getRoot().getAbsolutePath(),
                "-XepOpt:NullAway:AnnotatedPackages=com.uber,com.google.common"))
        .addSourceLines(
            "Test.java",
            "package com.uber;",
            "import javax.annotation.Nullable;",
            "import com.google.common.base.Preconditions;",
            "import com.google.common.base.Strings;",
            "class Test {",
            "  private void foo(@Nullable String a) {",
            "    if (!Strings.isNullOrEmpty(a)) {",
            "      a.toString();",
            "    }",
            "  }",
            "}")
        .doTest();
  }

Example 2: This example passes, however, we've removed com.google.common as a properly annotated package.

  @Test
  public void shouldPass_nestedCallWithIsNullOrEmptyAndNoAddedAnnotatedPackages() {
    // This test passes, but we're no longer checking Guava
    makeTestHelperWithArgs(
            Arrays.asList(
                "-d",
                temporaryFolder.getRoot().getAbsolutePath(),
                "-XepOpt:NullAway:AnnotatedPackages=com.uber"))
        .addSourceLines(
            "Test.java",
            "package com.uber;",
            "import javax.annotation.Nullable;",
            "import com.google.common.base.Preconditions;",
            "import com.google.common.base.Strings;",
            "class Test {",
            "  private void foo(@Nullable String a) {",
            "    if (!Strings.isNullOrEmpty(a)) {",
            "      a.toString();",
            "    }",
            "  }",
            "}")
        .doTest();
  }

The following two examples are synthetic examples to show the discrepancy.

Example 3: This example passes, but should fail. Without com.google.common as a properly annotated package, we pass @Nullable String a to BigDecimalMath.roundToDouble which is in a class marked with @ElementTypesAreNonnullByDefault.

  @Test
  public void shouldFail_callGuavaWithConflictingAnnotations() {
    // This test passes, even tho `roundToDouble` expects NonNull
    makeTestHelperWithArgs(
            Arrays.asList(
                "-d",
                temporaryFolder.getRoot().getAbsolutePath(),
                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
                // Adding AcknowledgeRestrictiveAnnotations to make sure
                "-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true"))
        .addSourceLines(
            "Test.java",
            "package com.uber;",
            "import javax.annotation.Nullable;",
            "import com.google.common.math.BigDecimalMath;",
            "import java.math.BigDecimal;",
            "import java.math.RoundingMode;",
            "class Test {",
            "  private void foo(@Nullable BigDecimal a) {",
            "    BigDecimalMath.roundToDouble(a, RoundingMode.UP);",
            "  }",
            "}")
        .doTest();
  }

Example 4: This example fails, with com.google.common as a properly annotated package - this behaves as we expect in both v0.8.0 and v0.9.+, and this is the result we would expect. However, this now contradicts the result in Example 1.

  @Test
  public void shouldFail_callGuavaWithConflictingAnnotationsAndAddedAnnotatedPackages() {
    // This test performs as we would expect, fails since `a` is marked nullable and
    //  BigDecimalMath expects NonNull by default
    makeTestHelperWithArgs(
            Arrays.asList(
                "-d",
                temporaryFolder.getRoot().getAbsolutePath(),
                "-XepOpt:NullAway:AnnotatedPackages=com.uber,com.google.common"))
        .addSourceLines(
            "Test.java",
            "package com.uber;",
            "import javax.annotation.Nullable;",
            "import com.google.common.math.BigDecimalMath;",
            "import java.math.BigDecimal;",
            "import java.math.RoundingMode;",
            "class Test {",
            "  private void foo(@Nullable BigDecimal a) {",
            "    BigDecimalMath.roundToDouble(a, RoundingMode.UP);",
            "  }",
            "}")
        .doTest();
  }

[msridhar]

Thanks for the report! I assume all these tests are with Guava 31.x?

Regarding examples 3 and 4, these work as expected since NullAway does not support @ElementTypesAreNonnullByDefault. I feel like this issue is separate from that of Examples 1 and 2, so maybe it should be filed separately?

For Example 1, can you try passing the newly added config flag AcknowledgeLibraryModelsOfAnnotatedCode like this:

NullAway/guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java

Line 45 in a8051cd

"-XepOpt:NullAway:AcknowledgeLibraryModelsOfAnnotatedCode=true"));

I think that may fix the problem; let us know. We are planning to default this flag to true in the next release as it seems like the config most users would want.

[rwinograd]

Possible to update the Configuration documentation here?

[msridhar]

Possible to update the Configuration documentation here?

Sure, it's updated:

https://github.com/uber/NullAway/wiki/Configuration#acknowledge-library-models-of-annotated-code

Sorry for not doing it before the release.

[rwinograd]

Sorry for not doing it before the release.

No worries - we really appreciate the feature! Also I think this is related and likely can be closed now.

[msridhar]

Thanks a lot for linking #453! There is some history here that I had forgotten. We should think more carefully about how exactly this new flag should interact with inheritance and write some tests around that. Once we figure that out we can close the issues. /cc @lazaroclapp

[lazaroclapp]

Thanks a lot for linking #453!

Agreed! (And sorry to see we kinda dropped the ball on that issue!)

I actually ran into this due to our test suite (not that issue specifically, but rather #445) when working to remove AcknowledgeLibraryModelsOfAnnotatedCode and set it to true by default. I will try to have a PR for that up today, but let me re-read #453 too and make sure I am not missing something from that discussion!

p.s. If we do remove AcknowledgeLibraryModelsOfAnnotatedCode, NullAway might complain about that option not existing in a future release...

[msridhar]

@holtherndon-stripe @rwinograd FYI, in just-released NullAway 0.9.10, the AcknowledgeLibraryModelsOfAnnotatedCode option is removed, as that behavior is now always on. Also we fixed some tricky issues around interactions between method overriding and library models. If you see any unexpected behaviors from the latest release, please let us know.

@holtherndon-stripe are we ok to close out this issue now?