@NonNull method type param for a @Nullable class type param confusion

[ben-manes]

I think that I am getting contradictory warnings for this complex scenario. I am trying to annotate a test adapter where a Guava cache implements Caffeine's interfaces so that parameterized tests can assert against both implementations. This allows Guava's to be a golden reference for regression testing, compatibility, and intentional deviations.

The Caffeine cache was annotated where a nullable value is allowed for Map.compute style behaviors, even though null cannot be stored in the cache.

@NullMarked
public interface Cache<K, V extends @Nullable Object> {

  @Nullable
  V getIfPresent(K key);

  V get(K key, Function<? super K, ? extends V> mappingFunction);

  Map<K, @NonNull V> getAll(
      Iterable<? extends K> keys,
      Function<? super Set<? extends K>, ? extends Map<? extends K, ? extends @NonNull V>>
          mappingFunction);
}

As the adapter may return null, I tried adding the annotations

static class GuavaCache<K, V> implements Cache<K, @Nullable V>, Serializable {

  @Override
  public Map<K, @NonNull V> getAll(Iterable<? extends K> keys, Function<? super Set<? extends K>,
      ? extends Map<? extends K, ? extends V>> mappingFunction) { ... }

but this emits the warning

/Users/ben/projects/caffeine/caffeine/src/test/java/com/github/benmanes/caffeine/cache/testing/GuavaCacheFromContext.java:203: warning: [NullAway] Method returns Map<K, @org.jspecify.annotations.NonNull V>, but overridden method returns Map<K, @org.jspecify.annotations.Nullable V>, which has mismatched type parameter nullability
    public Map<K, @NonNull V> getAll(Iterable<? extends K> keys, Function<? super Set<? extends K>,
                              ^
    (see http://t.uber.com/nullaway )
  Did you mean '@SuppressWarnings("NullAway") @Override'

If I specify that as @Nullable then it is happy and warns at the implementation's return logic. However, I don't believe this is correct given the interface makes it explicitly non-null. I can suppress these or use @NullUnmarked on the implementation. I'm unsure if this is a declaration or analysis mistake (cc @cpovirk).

[msridhar]

This may be the same underlying issue as #1091. When we are substituting type variables, we don't have the logic to preserve the annotations in the type we are substituting into. For this case, we are substituting @Nullable V for V inside @NonNull V and getting @Nullable V as the result, which is wrong I think. I'll keep this case in mind when working on #1091.

[ben-manes]

Good news. I was able to fully enable the null analysis on the project (main, tests, benchmarks), with only a few of these undesirable suppressions needed. That exercise found a couple small mistakes in the test logic thanks to the stricter null checks required.

[msridhar]

Fantastic! And thanks for all the reports! We'll work on getting them fixed soon.

[ben-manes]

This is a bit unrelated and not worth opening a new issue for. I was spot checking that jitpack.io worked after I added sigstore artifact signing, but it fails first with a nullaway error that I cannot reproduce. The warning makes sense, but I am unsure why it does not happen elsewhere. Jitpack is good for temporary hot patches and their build environment can be quirky, so I keep it working best-effort if others need to patch the library. I think this is a legitimate warning so I am unsure why it was not reproducible.

https://jitpack.io/com/github/ben-manes/caffeine/-ddd12106c6-1/build.log

I tried to force both github actions and jitpack to use the same version of zulu, disabled caching, etc. but they give different results.

• https://jitpack.io/com/github/ben-manes/caffeine/v3.dev-ba6dc63492-1/build.log
• https://github.com/ben-manes/caffeine/actions/runs/12494912309/job/34865080985

Here's a buildscan.

[msridhar]

Hrm, that's weird. My first guess would be somehow this is related to #1005. JSpecify mode does not yet support reading all relevant annotations from bytecodes, so there may be weird issues unless the version of javac used is JDK 23+. But I don't really understand why this issue would only get reported on jitpack; maybe I'm way off on the root cause. This example also seems to involve wildcards so those could be involved somehow. It's concerning that the jitpack error looks valid, but given we have hardly tested any support for wildcards, that could just be NullAway getting lucky.

[ben-manes]

I couldn't figure out how to reproduce it, but that warning seemed valid to me and so I fixed it as part of making the jitpack build pass again.

[msridhar]

@ben-manes do you know if you still get warnings from NullAway for the original complex scenario from this report?

[ben-manes]

Removing the class suppression and I don't see this warning anymore. The ones that it lists appear reasonable and worth me cleaning up or narrowing down. Closing as done, thanks!

[msridhar]

Thanks for letting us know!