Skip to content

Commit

Permalink
Avoid giving the result of loads a non-null type.
Browse files Browse the repository at this point in the history
2542ca8
changed types like the return type of `Cache.get` to be non-null instead
of unspecified.

As discussed in #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
jspecify/jspecify#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`.
  • Loading branch information
cpovirk committed Dec 3, 2024
1 parent 2542ca8 commit 41d97e6
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 25 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand All @@ -47,7 +49,7 @@ public interface AsyncCache<K, V> {
*
* @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
Expand All @@ -71,7 +73,9 @@ public interface AsyncCache<K, V> {
* @return the current (existing or computed) future value associated with the specified key
* @throws NullPointerException if the specified key or mappingFunction is null
*/
CompletableFuture<V> get(K key, Function<? super K, ? extends V> mappingFunction);
@NullUnmarked
@NonNull CompletableFuture<V> get(
@NonNull K key, @NonNull Function<? super @NonNull K, ? extends V> mappingFunction);

/**
* Returns the future associated with the {@code key} in this cache, obtaining that value from
Expand All @@ -89,15 +93,22 @@ public interface AsyncCache<K, V> {
*
* @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<V> get(K key, BiFunction<? super K, ? super Executor,
? extends CompletableFuture<? extends V>> mappingFunction);
@NullUnmarked
@NonNull CompletableFuture<V> get(
@NonNull K key,
@NonNull
BiFunction<
? super @NonNull K,
? super @NonNull Executor,
? extends @NonNull CompletableFuture<? extends V>>
mappingFunction);

/**
* Returns the future of a map of the values associated with the {@code keys}, creating or
Expand All @@ -118,11 +129,11 @@ CompletableFuture<V> get(K key, BiFunction<? super K, ? super Executor,
* @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<Map<K, V>> getAll(Iterable<? extends K> keys,
Function<? super Set<? extends K>, ? extends Map<? extends K, ? extends V>> mappingFunction);
Expand All @@ -146,13 +157,13 @@ CompletableFuture<Map<K, V>> getAll(Iterable<? extends K> 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<Map<K, V>> getAll(Iterable<? extends K> keys,
BiFunction<? super Set<? extends K>, ? super Executor,
Expand All @@ -170,7 +181,7 @@ CompletableFuture<Map<K, V>> getAll(Iterable<? extends K> 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<? extends V> valueFuture);
void put(K key, CompletableFuture<? extends @Nullable V> valueFuture);

/**
* Returns a view of the entries stored in this cache as a thread-safe map. Modifications made to
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -46,7 +48,7 @@ public interface Cache<K, V> {
*
* @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
Expand All @@ -70,14 +72,15 @@ public interface Cache<K, V> {
* @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<? super K, ? extends V> mappingFunction);
@NullUnmarked
V get(@NonNull K key, Function<? super @NonNull K, ? extends V> mappingFunction);

/**
* Returns a map of the values associated with the {@code keys} in this cache. The returned map
Expand Down Expand Up @@ -114,9 +117,9 @@ public interface Cache<K, V> {
* @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<K, V> getAll(Iterable<? extends K> keys,
Function<? super Set<? extends K>, ? extends Map<? extends K, ? extends V>> mappingFunction);
Expand All @@ -143,7 +146,7 @@ Map<K, V> getAll(Iterable<? extends K> 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<? extends K, ? extends V> map);

Expand Down

0 comments on commit 41d97e6

Please sign in to comment.