[0.10.6] @Nullable ignored and warns as non-null field

[ben-manes]

This is easily resolved by adding the suppression, but it appears to be a regression unless I am doing something wrong?

> Task :guava:compileJava
/Users/ben/projects/caffeine/guava/src/main/java/com/github/benmanes/caffeine/guava/CaffeinatedGuavaCache.java:156:
warning: [NullAway] @NonNull field CaffeinatedGuavaCache$AsMapView.entrySet not initialized
    @Nullable EntrySetView entrySet;
                           ^
    (see http://t.uber.com/nullaway )
  Did you mean '@SuppressWarnings("NullAway.Init") @Nullable EntrySetView entrySet;'?
/Users/ben/projects/caffeine/guava/src/main/java/com/github/benmanes/caffeine/guava/CaffeinatedGuavaCache.java:157:
warning: [NullAway] @NonNull field CaffeinatedGuavaCache$AsMapView.keySet not initialized
    @Nullable KeySetView keySet;
                         ^
    (see http://t.uber.com/nullaway )
  Did you mean '@SuppressWarnings("NullAway.Init") @Nullable KeySetView keySet;'?
/Users/ben/projects/caffeine/guava/src/main/java/com/github/benmanes/caffeine/guava/CaffeinatedGuavaCache.java:158:
warning: [NullAway] @NonNull field CaffeinatedGuavaCache$AsMapView.values not initialized
    @Nullable ValuesView values;
                         ^
    (see http://t.uber.com/nullaway )
  Did you mean '@SuppressWarnings("NullAway.Init") @Nullable ValuesView values;'?
3 warnings

[lazaroclapp]

Um, that's really weird. We are definitely not seeing that behavior in general (i.e. for all @Nullable fields) or it would be literally impossible to get NullAway to work on any non-trivial codebase. But if the code in question is here, then I also don't immediately see what could be going on. Do you have a minimal repro? Is the fact that we have a final inner class part of the issue? (There was a change in NullAway 0.10.6 about final fields, but not classes, I think...)

p.s. Trying to get a repro going right now, but so far, the following produces no warnings:

package com.uber;
import java.util.HashMap;
import java.util.Map;
import org.checkerframework.checker.nullness.qual.Nullable;

class Test<K, V> {
  final class Inner extends HashMap<K,V> {
     @Nullable Object f;
  }
}

[ben-manes]

Sorry, I don't have a repro but I pushed the commit to a dev branch if you want to run the build.

./gradlew guava:compileJava

[lazaroclapp]

lazaro@lazaro--java:~/caffeine$ git checkout -b v3.dev origin/v3.dev
branch 'v3.dev' set up to track 'origin/v3.dev' by rebasing.
Switched to a new branch 'v3.dev'
lazaro@lazaro--java:~/caffeine$ ./gradlew guava:compileJava
Downloading https://services.gradle.org/distributions/gradle-8.0-rc-1-bin.zip
[...]

BUILD SUCCESSFUL in 29s
10 actionable tasks: 1 executed, 9 from cache
Configuration cache entry stored.

Well, something really weird is going on here... does your gradle build set a specific java version? If not, what do you get out of java -version?

cc: @msridhar

[ben-manes]

Yes, the build uses Java 11 via a toolchain. Maybe due to the remote build cache it was not run locally? you can try

./gradlew guava:clean guava:compileJava --console plain --no-build-cache

[lazaroclapp]

Update: That did the trick.

Also, got a repro now:

  @Test
  public void repro705() {
    defaultCompilationHelper
            .addSourceLines("Test.java",
                    "package com.uber;",
                    "import com.google.common.collect.ForwardingConcurrentMap;",
                    "import java.util.Map;",
                    "import java.util.Map.Entry;",
                    "import java.util.Set;",
                    "import java.util.concurrent.ConcurrentMap;",
                    "import java.util.concurrent.ConcurrentHashMap;",
                    "import org.checkerframework.checker.nullness.qual.Nullable;",
                    "class Test<K, V> {",
                    "  @Nullable Foo f;",
                    "  final class Foo { }",
                    "}")
            .doTest();
  }

And the first commit in NullAway it fails on (after #702). Looking into the cause.

[lazaroclapp]

Got a candidate fix: #706

Issue is in how we detect the locations of type use annotations (i.e. how we figure out we are looking at @Nullable List<T> vs List<@Nullable T> (we don't yet support reasoning about the later case, but we are getting the foundations set there). We were missing some handling of the case where the annotation is on an inner type (e.g. Foo.@Nullable Bar). This is what was happening implicitly in this case.

For the error to manifest, it needs to involve a field of an inner type where the outer type is implicit, and a type use version of @Nullable, like org.checkerframework.checker.nullness.qual.Nullable. In the end, final was a red herring.

That suggests one, admittedly annoying, alternative workaround:

+  @SuppressWarnings("NullableOnContainingClass")
   final class AsMapView extends ForwardingConcurrentMap<K, V> {
-    @Nullable EntrySetView entrySet;
-    @Nullable KeySetView keySet;
-    @Nullable ValuesView values;
+    @Nullable CaffeinatedGuavaCache<K,V>.EntrySetView entrySet;
+    @Nullable CaffeinatedGuavaCache<K,V>.KeySetView keySet;
+    @Nullable CaffeinatedGuavaCache<K,V>.ValuesView values;

(Alternatively, suppressing NullAway.Init or remaining on 0.10.5 for a couple days are good options too!)

Realistically, we probably won't be able to cut NullAway 0.10.7 until after the new year, but once we do, it should include the fix above :)

[ben-manes]

thanks for the quick fix! I'll remove the suppression whenever the next release lands.

[lazaroclapp]

Update: NullAway 0.10.7 is now out with the #706 as the only change. I just tested updating the dep in the repro given above and it all builds successfully, except for some EP warnings about AvoidObjectArrays which seem unrelated.

This is a temporary fix, pending some more principled handling of type use annotations that matches JSpecify semantics (see #708), but it should resolve this issue.