Incomplete nullness specification (@NullMarked)

[joschi]

The changes in commit 2542ca8 introduce JSpecify and specifically the @NullMarked annotation in Caffeine:

Within null-marked code, as a general rule, a type usage is considered non-null (to exclude null as a value) unless explicitly annotated as Nullable.

Unfortunately the nullness specifications don't seem to be backward-compatible or some of the Javadoc is now outdated.

Example

While CacheLoader has the @NullMarked annotation, CacheLoader#load(K) doesn't have any additional @Nullable annotation meaning that it cannot return null.

But the Javadoc for this method says:

@return the value associated with {@code key} or {@code null} if not found

Assuming there shouldn't be a breaking change in the API in a minor version, the method signature should be changed to match its documentation:

-  V load(K key) throws Exception;
+  @Nullable V load(K key) throws Exception;

There are also other methods affected, but this is the one I ran into problems with. 😓

With tools such as NullAway already supporting and verifying the JSpecify annotations, this can lead to some issues with downstream projects such as Dropwizard.

Error:  /home/runner/work/dropwizard/dropwizard/dropwizard-hibernate/src/main/java/io/dropwizard/hibernate/UnitOfWorkAwareProxyFactory.java:[64,39] [NullAway] method returns @Nullable, but superclass method com.github.benmanes.caffeine.cache.CacheLoader.load(K) returns @NonNull

dropwizard/dropwizard#9792

[cpovirk]

Sorry about that. I had contributed the nullness annotations, but I didn't follow through on updating the Javadoc before the release was published.

The intent is that, if you want your CacheLoader to be able to return null, you'll now declare it as a CacheLoader<..., @Nullable Class<?>>. Basically, you're setting V to @Nullable Class<?>, so every usage of V (like in the return type of load) turns into a usage of @Nullable Class<?>. (Then you would likewise declare the LoadingCache itself to have a value type of @Nullable Class<?>. That would express to tools that calls to get can return null, though Caffeine will continue to not cache that result for future calls.)

Now, support for generics varies across tools. NullAway in particular has been doing some work in this area, both as part of their ongoing work on JSpecify and in response to requests from others, including Spring recently. If you find that it can't handle the scenario here, the NullAway maintainers would be interested to hear about it.

(There is a definite chicken-and-egg problem with libraries that want to use advanced nullness annotations and tools that want to support them. We're getting at least a bit of a positive feedback loop in place through (among other things) the work that the Kotlin folks have done, but it's still early days.)

[cpovirk]

I confirmed that the change I had in mind does not help with the NullAway error:

--- a/dropwizard-hibernate/src/main/java/io/dropwizard/hibernate/UnitOfWorkAwareProxyFactory.java
+++ b/dropwizard-hibernate/src/main/java/io/dropwizard/hibernate/UnitOfWorkAwareProxyFactory.java
@@ -34,7 +34,7 @@ import java.util.stream.Collectors;
 public class UnitOfWorkAwareProxyFactory {
 
     private final Map<String, SessionFactory> sessionFactories;
-    private final LoadingCache<Class<?>, Class<?>> proxyCache;
+    private final LoadingCache<Class<?>, @Nullable Class<?>> proxyCache;
 
     public UnitOfWorkAwareProxyFactory(String name, SessionFactory sessionFactory) {
         this(Caffeine.newBuilder(), name, sessionFactory);
@@ -58,8 +58,8 @@ public class UnitOfWorkAwareProxyFactory {
         proxyCache = buildCache(proxyCacheBuilder);
     }
 
-    private LoadingCache<Class<?>, Class<?>> buildCache(Caffeine<Object, Object> proxyCacheBuilder) {
-        return proxyCacheBuilder.build(new CacheLoader<Class<?>, Class<?>>() {
+    private LoadingCache<Class<?>, @Nullable Class<?>> buildCache(Caffeine<Object, Object> proxyCacheBuilder) {
+        return proxyCacheBuilder.build(new CacheLoader<Class<?>, @Nullable Class<?>>() {
             @Override
             public @Nullable Class<?> load(@NonNull Class<?> key) {
                 return createProxyClass(key);

[cpovirk]

Oh, but I should try that with -XepOpt:NullAway:JSpecifyMode=true.

...OK, I did that, and I still got the error. This seems similar to uber/NullAway#1075 (comment).

Now, JSpecifyMode (in combination with my change) did get me two new errors

[ERROR] dropwizard-hibernate/src/main/java/io/dropwizard/hibernate/UnitOfWorkAwareProxyFactory.java:[109,28] [NullAway] dereferenced expression proxied is @Nullable
[ERROR] dropwizard-hibernate/src/main/java/io/dropwizard/hibernate/UnitOfWorkAwareProxyFactory.java:[110,28] [NullAway] dereferenced expression proxied is @Nullable

That's here. And that does seem legitimate, given the background that the cache loader can return null.

...although... can it return null? The loader calls getLoaded(), and it's not clear to me that that method can return null. Not that it's clear to me that it can't, either :) But I wonder if your load method just has @Nullable on its return type just because the supermethod used to have it. So maybe we can just remove it....

[cpovirk]

Once I remove @Nullable from load, I'm able to get mvn install -DskipTests[*] to succeed, with or without -XepOpt:NullAway:JSpecifyMode=true. (You might still consider setting -XepOpt:NullAway:JSpecifyMode=true to get somewhat enhanced checking, which should improve in the future. Then, if you start seeing errors from it, you can decide whether it's a net win.) I might send you a PR eventually if you don't get to it first.

(None of this takes away from the fact that this upgrade is more painful as a result of the annotations and that I should get together better docs.)

[*] I passed -DskipTests because I would have needed to figure out some platform-specific fun to get all the tests to pass:

[ERROR]   Http2WithConscryptTest.testHttp1WithCustomCipher » UnsatisfiedLink /tmp/libconscrypt_openjdk_jni-linux-x86_6417375822108010000.so: libstdc++.so.6: cannot open shared object file: No such file or directory
[ERROR]   Http2WithConscryptTest.testHttp2WithCustomCipher » NoClassDefFound Could not initialize class io.dropwizard.http2.Http2WithConscryptTest

[ben-manes]

Can you try that change @joschi?

[joschi]

Thanks for looking into this issue so quickly!

I opted to use the PR you've opened (dropwizard/dropwizard#9806) and remove the @Nullable annotation because the return value cannot be null (or shouldn't in any sane configuration 😅).

[smaheshwar-pltr]

We're experiencing extensive backwards compatibility issues as a result of 2542ca8. Even refactoring to use @Nullable V as suggested in this thread:

private static final class DummyLoader implements CacheLoader<String, @Nullable Integer> {

    private DummyLoader() {}

    @Override
    public @Nullable Integer load(@NotNull String key) {
        return null;
    }
}

yields

warning: [NullAway] method returns @Nullable, but superclass method com.github.benmanes.caffeine.cache.CacheLoader.load(K) returns @NonNull
        public @Nullable Integer load(@NotNull String key) {
                                 ^

but maybe I need to set some NullAway flags.

Was this release meant to be this breaking?

[ben-manes]

I think you’d need to raise this with NullAway (@msridhar) as an analysis issue.

[msridhar]

Yes, please raise an issue on the NullAway repo and we will take a look.