From 41d97e69ce09088a533015da70ad7c53ffd38884 Mon Sep 17 00:00:00 2001 From: Chris Povirk Date: Tue, 3 Dec 2024 12:00:27 -0500 Subject: [PATCH 1/6] Avoid giving the result of loads a non-null type. https://github.com/ben-manes/caffeine/commit/2542ca8ca22fe4c30b62ee569ee71436fc44a37e changed types like the return type of `Cache.get` to be non-null instead of unspecified. As discussed in https://github.com/ben-manes/caffeine/issues/594, the types really "should" be nullable, but that is inconvenient for most users. When Caffeine was using Checker Framework annotations, it could use `@PolyNull` to give Checker Framework users the best of both worlds and give users of other tools (notably Kotlin) a convenient (if not fully strict) experience, since those tools viewed the types as having unspecified nullness (in Kotlin terms, "platform types"). Caffeine could still use `@PolyNull` today if you want to restore the Checker Framework dependency. (And maybe someday JSpecify will include its own version, as discussed in https://github.com/jspecify/jspecify/issues/79.) But the important thing is that Caffeine _not_ affirmatively make the type be non-null. And that's what `@NullMarked` does. That would cause trouble for a number of Kotlin users in Google's codebase. So, to undo that, we can use `@NullUnmarked` (recognized by Kotlin as of 2.0.20) on the methods where we want to keep types unspecified. Then we can optionally use `@NonNull` annotations on the _other_ types in the API in order to still make those types non-null. Kotlin actually _still_ doesn't quite get the types right, thanks to https://youtrack.jetbrains.com/issue/KT-73658. But this PR at least annotates correctly (I hope :)), which may help other tools even today and which should help Kotlin eventually. If enough people run into the Kotlin bug in practice, then we could consider backing out even more nullness information: If we were to remove `@NullMarked` from `Cache`+`AsyncCache` and then potentially sprinkle in some `@NonNull` annotations to compensate, then we could still preserve at least some of the new information in those classes. And of course other classes can remain `@NullMarked`. --- .../benmanes/caffeine/cache/AsyncCache.java | 45 ++++++++++++------- .../github/benmanes/caffeine/cache/Cache.java | 19 ++++---- 2 files changed, 39 insertions(+), 25 deletions(-) diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncCache.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncCache.java index a7e5d37af0..5e1dc29a8a 100644 --- a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncCache.java +++ b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncCache.java @@ -23,7 +23,9 @@ import java.util.function.BiFunction; import java.util.function.Function; +import org.jspecify.annotations.NonNull; import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.NullUnmarked; import org.jspecify.annotations.Nullable; /** @@ -47,7 +49,7 @@ public interface AsyncCache { * * @param key the key whose associated value is to be returned * @return the future value to which the specified key is mapped, or {@code null} if this cache - * does not contain a mapping for the key + * does not contain a mapping for the key * @throws NullPointerException if the specified key is null */ @Nullable @@ -71,7 +73,9 @@ public interface AsyncCache { * @return the current (existing or computed) future value associated with the specified key * @throws NullPointerException if the specified key or mappingFunction is null */ - CompletableFuture get(K key, Function mappingFunction); + @NullUnmarked + @NonNull CompletableFuture get( + @NonNull K key, @NonNull Function mappingFunction); /** * Returns the future associated with the {@code key} in this cache, obtaining that value from @@ -89,15 +93,22 @@ public interface AsyncCache { * * @param key the key with which the specified value is to be associated * @param mappingFunction the function to asynchronously compute a value, optionally using the - * given executor + * given executor * @return the current (existing or computed) future value associated with the specified key - * @throws NullPointerException if the specified key or mappingFunction is null, or if the - * future returned by the mappingFunction is null - * @throws RuntimeException or Error if the mappingFunction does when constructing the future, - * in which case the mapping is left unestablished + * @throws NullPointerException if the specified key or mappingFunction is null, or if the future + * returned by the mappingFunction is null + * @throws RuntimeException or Error if the mappingFunction does when constructing the future, in + * which case the mapping is left unestablished */ - CompletableFuture get(K key, BiFunction> mappingFunction); + @NullUnmarked + @NonNull CompletableFuture get( + @NonNull K key, + @NonNull + BiFunction< + ? super @NonNull K, + ? super @NonNull Executor, + ? extends @NonNull CompletableFuture> + mappingFunction); /** * Returns the future of a map of the values associated with the {@code keys}, creating or @@ -118,11 +129,11 @@ CompletableFuture get(K key, BiFunction> getAll(Iterable keys, Function, ? extends Map> mappingFunction); @@ -146,13 +157,13 @@ CompletableFuture> getAll(Iterable keys, * * @param keys the keys whose associated values are to be returned * @param mappingFunction the function to asynchronously compute the values, optionally using the - * given executor + * given executor * @return a future containing an unmodifiable mapping of keys to values for the specified keys in - * this cache + * this cache * @throws NullPointerException if the specified collection is null or contains a null element, or - * if the future returned by the mappingFunction is null + * if the future returned by the mappingFunction is null * @throws RuntimeException or Error if the mappingFunction does so, in which case the mapping is - * left unestablished + * left unestablished */ CompletableFuture> getAll(Iterable keys, BiFunction, ? super Executor, @@ -170,7 +181,7 @@ CompletableFuture> getAll(Iterable keys, * @param valueFuture the value to be associated with the specified key * @throws NullPointerException if the specified key or value is null */ - void put(K key, CompletableFuture valueFuture); + void put(K key, CompletableFuture valueFuture); /** * Returns a view of the entries stored in this cache as a thread-safe map. Modifications made to diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Cache.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Cache.java index cddcd68179..4459fd0a5c 100644 --- a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Cache.java +++ b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Cache.java @@ -20,7 +20,9 @@ import java.util.concurrent.ConcurrentMap; import java.util.function.Function; +import org.jspecify.annotations.NonNull; import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.NullUnmarked; import org.jspecify.annotations.Nullable; import com.github.benmanes.caffeine.cache.stats.CacheStats; @@ -46,7 +48,7 @@ public interface Cache { * * @param key the key whose associated value is to be returned * @return the value to which the specified key is mapped, or {@code null} if this cache does not - * contain a mapping for the key + * contain a mapping for the key * @throws NullPointerException if the specified key is null */ @Nullable @@ -70,14 +72,15 @@ public interface Cache { * @param key the key with which the specified value is to be associated * @param mappingFunction the function to compute a value * @return the current (existing or computed) value associated with the specified key, or null if - * the computed value is null + * the computed value is null * @throws NullPointerException if the specified key or mappingFunction is null * @throws IllegalStateException if the computation detectably attempts a recursive update to this - * cache that would otherwise never complete + * cache that would otherwise never complete * @throws RuntimeException or Error if the mappingFunction does so, in which case the mapping is - * left unestablished + * left unestablished */ - V get(K key, Function mappingFunction); + @NullUnmarked + V get(@NonNull K key, Function mappingFunction); /** * Returns a map of the values associated with the {@code keys} in this cache. The returned map @@ -114,9 +117,9 @@ public interface Cache { * @param mappingFunction the function to compute the values * @return an unmodifiable mapping of keys to values for the specified keys in this cache * @throws NullPointerException if the specified collection is null or contains a null element, or - * if the map returned by the mappingFunction is null + * if the map returned by the mappingFunction is null * @throws RuntimeException or Error if the mappingFunction does so, in which case the mapping is - * left unestablished + * left unestablished */ Map getAll(Iterable keys, Function, ? extends Map> mappingFunction); @@ -143,7 +146,7 @@ Map getAll(Iterable keys, * * @param map the mappings to be stored in this cache * @throws NullPointerException if the specified map is null or the specified map contains null - * keys or values + * keys or values */ void putAll(Map map); From e9e5013ccc3bc6f619836a36e95c94f964a74de7 Mon Sep 17 00:00:00 2001 From: Chris Povirk Date: Tue, 3 Dec 2024 13:01:01 -0500 Subject: [PATCH 2/6] Make function output types nullable instead of unspecified. --- .../github/benmanes/caffeine/cache/AsyncCache.java | 12 ++++++------ .../com/github/benmanes/caffeine/cache/Cache.java | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncCache.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncCache.java index 5e1dc29a8a..94406763e0 100644 --- a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncCache.java +++ b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncCache.java @@ -75,7 +75,7 @@ public interface AsyncCache { */ @NullUnmarked @NonNull CompletableFuture get( - @NonNull K key, @NonNull Function mappingFunction); + @NonNull K key, @NonNull Function mappingFunction); /** * Returns the future associated with the {@code key} in this cache, obtaining that value from @@ -95,10 +95,10 @@ public interface AsyncCache { * @param mappingFunction the function to asynchronously compute a value, optionally using the * given executor * @return the current (existing or computed) future value associated with the specified key - * @throws NullPointerException if the specified key or mappingFunction is null, or if the future - * returned by the mappingFunction is null - * @throws RuntimeException or Error if the mappingFunction does when constructing the future, in - * which case the mapping is left unestablished + * @throws NullPointerException if the specified key or mappingFunction is null, or if the + * future returned by the mappingFunction is null + * @throws RuntimeException or Error if the mappingFunction does when constructing the future, + * in which case the mapping is left unestablished */ @NullUnmarked @NonNull CompletableFuture get( @@ -107,7 +107,7 @@ public interface AsyncCache { BiFunction< ? super @NonNull K, ? super @NonNull Executor, - ? extends @NonNull CompletableFuture> + ? extends @NonNull CompletableFuture> mappingFunction); /** diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Cache.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Cache.java index 4459fd0a5c..47dbd6f9ec 100644 --- a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Cache.java +++ b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Cache.java @@ -80,7 +80,7 @@ public interface Cache { * left unestablished */ @NullUnmarked - V get(@NonNull K key, Function mappingFunction); + V get(@NonNull K key, Function mappingFunction); /** * Returns a map of the values associated with the {@code keys} in this cache. The returned map From 7b93d046e67436d6a2bbbd6dc9c572517c70c0e6 Mon Sep 17 00:00:00 2001 From: Chris Povirk Date: Tue, 3 Dec 2024 13:03:53 -0500 Subject: [PATCH 3/6] Leave the value type of the `CompletableFuture` in `asMap` unspecified. --- .../java/com/github/benmanes/caffeine/cache/AsyncCache.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncCache.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncCache.java index 94406763e0..bc5fd7b4d0 100644 --- a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncCache.java +++ b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncCache.java @@ -198,7 +198,8 @@ CompletableFuture> getAll(Iterable keys, * * @return a thread-safe view of this cache supporting all of the optional {@link Map} operations */ - ConcurrentMap> asMap(); + @NullUnmarked + @NonNull ConcurrentMap<@NonNull K, @NonNull CompletableFuture> asMap(); /** * Returns a view of the entries stored in this cache as a synchronous {@link Cache}. A mapping is From 1b767fb744d5a058dbebc1b7b0e7bc16363ee3d4 Mon Sep 17 00:00:00 2001 From: Chris Povirk Date: Tue, 3 Dec 2024 14:12:04 -0500 Subject: [PATCH 4/6] Allow async loads to return `null`. This makes them match what `CacheLoader` already allows for synchronous loading and reloading via `@Nullable V load(K key)` and `@Nullable V reload(K key, V oldValue)`. --- .../com/github/benmanes/caffeine/cache/AsyncCacheLoader.java | 5 +++-- .../java/com/github/benmanes/caffeine/cache/CacheLoader.java | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncCacheLoader.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncCacheLoader.java index 745922807a..8abff8d7e1 100644 --- a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncCacheLoader.java +++ b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncCacheLoader.java @@ -25,6 +25,7 @@ import java.util.function.Function; import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.Nullable; /** * Computes or retrieves values asynchronously based on a key, for use in populating a @@ -62,7 +63,7 @@ public interface AsyncCacheLoader { * treated like any other {@code Exception} in all respects except that, when it is * caught, the thread's interrupt status is set */ - CompletableFuture asyncLoad(K key, Executor executor) throws Exception; + CompletableFuture asyncLoad(K key, Executor executor) throws Exception; /** * Asynchronously computes or retrieves the values corresponding to {@code keys}. This method is @@ -114,7 +115,7 @@ public interface AsyncCacheLoader { * treated like any other {@code Exception} in all respects except that, when it is * caught, the thread's interrupt status is set */ - default CompletableFuture asyncReload( + default CompletableFuture asyncReload( K key, V oldValue, Executor executor) throws Exception { return asyncLoad(key, executor); } diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/CacheLoader.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/CacheLoader.java index 217c81a868..3d8b9ca642 100644 --- a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/CacheLoader.java +++ b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/CacheLoader.java @@ -101,7 +101,7 @@ public interface CacheLoader extends AsyncCacheLoader { * @return the future value associated with {@code key} */ @Override - default CompletableFuture asyncLoad(K key, Executor executor) throws Exception { + default CompletableFuture asyncLoad(K key, Executor executor) throws Exception { requireNonNull(key); requireNonNull(executor); return CompletableFuture.supplyAsync(() -> { @@ -193,7 +193,7 @@ default CompletableFuture asyncLoad(K key, Executor executor) throw * {@code null} if the mapping is to be removed */ @Override - default CompletableFuture asyncReload( + default CompletableFuture asyncReload( K key, V oldValue, Executor executor) throws Exception { requireNonNull(key); requireNonNull(executor); From bc74eb6a07046edffa323bdc75327572a2de586e Mon Sep 17 00:00:00 2001 From: Chris Povirk Date: Tue, 3 Dec 2024 16:29:27 -0500 Subject: [PATCH 5/6] Undo accidental Javadoc formatting. --- .../benmanes/caffeine/cache/AsyncCache.java | 18 +++++++++--------- .../github/benmanes/caffeine/cache/Cache.java | 14 +++++++------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncCache.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncCache.java index bc5fd7b4d0..90ae3d7c4e 100644 --- a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncCache.java +++ b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncCache.java @@ -49,7 +49,7 @@ public interface AsyncCache { * * @param key the key whose associated value is to be returned * @return the future value to which the specified key is mapped, or {@code null} if this cache - * does not contain a mapping for the key + * does not contain a mapping for the key * @throws NullPointerException if the specified key is null */ @Nullable @@ -93,7 +93,7 @@ public interface AsyncCache { * * @param key the key with which the specified value is to be associated * @param mappingFunction the function to asynchronously compute a value, optionally using the - * given executor + * given executor * @return the current (existing or computed) future value associated with the specified key * @throws NullPointerException if the specified key or mappingFunction is null, or if the * future returned by the mappingFunction is null @@ -129,11 +129,11 @@ public interface AsyncCache { * @param keys the keys whose associated values are to be returned * @param mappingFunction the function to asynchronously compute the values * @return a future containing an unmodifiable mapping of keys to values for the specified keys in - * this cache + * this cache * @throws NullPointerException if the specified collection is null or contains a null element, or - * if the future returned by the mappingFunction is null + * if the future returned by the mappingFunction is null * @throws RuntimeException or Error if the mappingFunction does so, in which case the mapping is - * left unestablished + * left unestablished */ CompletableFuture> getAll(Iterable keys, Function, ? extends Map> mappingFunction); @@ -157,13 +157,13 @@ CompletableFuture> getAll(Iterable keys, * * @param keys the keys whose associated values are to be returned * @param mappingFunction the function to asynchronously compute the values, optionally using the - * given executor + * given executor * @return a future containing an unmodifiable mapping of keys to values for the specified keys in - * this cache + * this cache * @throws NullPointerException if the specified collection is null or contains a null element, or - * if the future returned by the mappingFunction is null + * if the future returned by the mappingFunction is null * @throws RuntimeException or Error if the mappingFunction does so, in which case the mapping is - * left unestablished + * left unestablished */ CompletableFuture> getAll(Iterable keys, BiFunction, ? super Executor, diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Cache.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Cache.java index 47dbd6f9ec..8f825ed90a 100644 --- a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Cache.java +++ b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Cache.java @@ -48,7 +48,7 @@ public interface Cache { * * @param key the key whose associated value is to be returned * @return the value to which the specified key is mapped, or {@code null} if this cache does not - * contain a mapping for the key + * contain a mapping for the key * @throws NullPointerException if the specified key is null */ @Nullable @@ -72,12 +72,12 @@ public interface Cache { * @param key the key with which the specified value is to be associated * @param mappingFunction the function to compute a value * @return the current (existing or computed) value associated with the specified key, or null if - * the computed value is null + * the computed value is null * @throws NullPointerException if the specified key or mappingFunction is null * @throws IllegalStateException if the computation detectably attempts a recursive update to this - * cache that would otherwise never complete + * cache that would otherwise never complete * @throws RuntimeException or Error if the mappingFunction does so, in which case the mapping is - * left unestablished + * left unestablished */ @NullUnmarked V get(@NonNull K key, Function mappingFunction); @@ -117,9 +117,9 @@ public interface Cache { * @param mappingFunction the function to compute the values * @return an unmodifiable mapping of keys to values for the specified keys in this cache * @throws NullPointerException if the specified collection is null or contains a null element, or - * if the map returned by the mappingFunction is null + * if the map returned by the mappingFunction is null * @throws RuntimeException or Error if the mappingFunction does so, in which case the mapping is - * left unestablished + * left unestablished */ Map getAll(Iterable keys, Function, ? extends Map> mappingFunction); @@ -146,7 +146,7 @@ Map getAll(Iterable keys, * * @param map the mappings to be stored in this cache * @throws NullPointerException if the specified map is null or the specified map contains null - * keys or values + * keys or values */ void putAll(Map map); From d9235d3a7d0c604cd62823ec4ae34e487af2ff69 Mon Sep 17 00:00:00 2001 From: Chris Povirk Date: Wed, 4 Dec 2024 10:55:56 -0500 Subject: [PATCH 6/6] Unmark `*LoadingCache` result types. --- .../github/benmanes/caffeine/cache/AsyncLoadingCache.java | 5 ++++- .../com/github/benmanes/caffeine/cache/LoadingCache.java | 8 ++++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncLoadingCache.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncLoadingCache.java index 8b60c3168d..f714203e92 100644 --- a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncLoadingCache.java +++ b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncLoadingCache.java @@ -18,7 +18,9 @@ import java.util.Map; import java.util.concurrent.CompletableFuture; +import org.jspecify.annotations.NonNull; import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.NullUnmarked; /** * A semi-persistent mapping from keys to values. Values are automatically loaded by the cache @@ -50,7 +52,8 @@ public interface AsyncLoadingCache extends AsyncCache { * @throws RuntimeException or Error if the {@link AsyncCacheLoader} does when constructing the * future, in which case the mapping is left unestablished */ - CompletableFuture get(K key); + @NullUnmarked + @NonNull CompletableFuture get(@NonNull K key); /** * Returns the future of a map of the values associated with {@code keys}, creating or retrieving diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LoadingCache.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LoadingCache.java index c06aeb2116..f354e84fb9 100644 --- a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LoadingCache.java +++ b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LoadingCache.java @@ -19,7 +19,9 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; +import org.jspecify.annotations.NonNull; import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.NullUnmarked; import com.google.errorprone.annotations.CanIgnoreReturnValue; @@ -62,7 +64,8 @@ public interface LoadingCache extends Cache { * @throws RuntimeException or Error if the {@link CacheLoader} does so, in which case the mapping * is left unestablished */ - V get(K key); + @NullUnmarked + V get(@NonNull K key); /** * Returns a map of the values associated with the {@code keys}, creating or retrieving those @@ -110,7 +113,8 @@ public interface LoadingCache extends Cache { * @throws NullPointerException if the specified key is null */ @CanIgnoreReturnValue - CompletableFuture refresh(K key); + @NullUnmarked + @NonNull CompletableFuture refresh(@NonNull K key); /** * Loads a new value for each {@code key}, asynchronously. While the new value is loading the