Skip to content

Commit

Permalink
Remove unnecessary nullness annotations (fixes #337)
Browse files Browse the repository at this point in the history
  • Loading branch information
@ben-manes
ben-manes committed Jan 17, 2021
1 parent 9937263 commit fb4eb0b
Show file tree
Hide file tree
Showing 42 changed files with 200 additions and 311 deletions.
7 changes: 3 additions & 4 deletions caffeine/src/jmh/java/com/github/benmanes/caffeine/cache/BasicCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
*/
package com.github.benmanes.caffeine.cache;

import org.checkerframework.checker.nullness.qual.NonNull;
import org.checkerframework.checker.nullness.qual.Nullable;

/**
Expand All @@ -26,13 +25,13 @@
public interface BasicCache<K, V> {

This comment has been minimized.

Sign in to view

Copy link
@vlsi

vlsi Jan 17, 2021

Contributor

Do you mean BasicCache should always be instantiated with a non-nullable K?
If that is the case, then please use BasicCache<K extends Object, V> which literally means K is a non-nullable Object.

By default, BasicCache<K, V> means it allows clients to use the interface as both implements BasicCache<@Nullable String, ..> and BasicCache<@NonNull String, ...>.

If you use BasicCache<K extends Object, V> (or BasicCache<K extends @NonNull Object, V> which is the same thing), then clients won't be able to implement the interface like implements BasicCache<@Nullable String, ...> which is probably nice.

By the way, BasicCache<K extends @NonNull Object, V> defeats org.jboss:jandex annotation parser (see smallrye/jandex#99 ), so the less annotations the better.
Just in case, jandex is used in Hibernate, so the net bytecode parsing result might be hard to understand.

This comment has been minimized.

Sign in to view

Copy link
@ben-manes

ben-manes Jan 18, 2021

Author Owner

This is where it gets confusing. Why can't the package-info.java declare that be default K is not null, and opt-in when needed. Won't I need to now annotate everywhere like Cache and LoadingCache this behavior?

In this case BasicCache is an adapter for JMH tests. I assume you wrote this once and the pattern needs to be replicated all over?

Is K extends Object different from @NonNull K? If so, why?

I think we can remove jandex as Quarkus asked for it and then decided not to use it for Caffeine.

This comment has been minimized.

Sign in to view

Copy link
@vlsi

vlsi Jan 18, 2021

Contributor

I think we can remove jandex as Quarkus asked for it and then decided not to use it for Caffeine

That is easier to clarify.
Suppose someone uses both Caffeine and Hibernate in the same project. I guess that is possible even in case the libraries do not interact directly.

However, Hibernate uses jandex for its own annotation scanning. If jandex would try to parse Caffeine classes (well, it would try to search for JPA annotations there :) ), then janxex can fail since:
a) There are Javac issues which make javac to produce invalid bytecode
b) There are jandex issues which cause jandex failures even in the case bytecode is valid

Of course, jandex issues are slightly easier to heal, however, I'm inclined to keep jandex processing in the build pipeline, and I discard the index file (e.g. to avoid duplicate files in case someone would shade the library).
In other words, I use jandex as an extra verifier to tell if the produced bytecode looks like a valid one :)

Here's the change I made for pgjdbc to workaround jandex issues (well, generic uses are very limited there, so the workaround changes are small): pgjdbc/pgjdbc#2010

This comment has been minimized.

Sign in to view

Copy link
@vlsi

vlsi Jan 18, 2021

Contributor

Is K extends Object different from @NonNull K? If so, why?

I assume you ask for the difference between interface Cache<@NonNull K> vs interface Cache<K extends Object>

The reference documentation is here: https://checkerframework.org/manual/#generics-defaults

Frankly speaking, I have no clear words here, however, for nullness checker interface Cache<@NonNull K> and interface Cache<K> are equivalent declarations.

There are just three usable declarations for null-nonnull:

interface Cache<K extends @NonNull Object>. It means all implementations of Cache must use non-nullable K. In other words, the code like MyCache implements Cache<@Nullable String> would fail to compile.

interface Cache<@Nullable K>. It means: all implementations of Cache must use nullable type for K.
This is helpful if you want to have default K getKeyOrNull() { ... return null; } method in the interface. Note how the result type is generic, and the method returns null sometimes. So you want to ensure that all instantiation of K are nullable. That is what interface Cache<@Nullable K> gives you.

interface Cache<K>. It means implementations can be either nullable or non-nullable.

to be replicated all over?

The pattern <K extends Object> should indeed be replicated whenever you declare type variable which must be instantiated with a non-null type.
However, that factors out nullability declaration to a single place. In other words, it is better to declare type variable declaration right in the single place where the variable is declared (e.g. in interface Cache<....>) rather than annotate all parameters which use this type variable.

For instance:

<K> K baseline(K key) { return key; }

ok: Utils.<String>baseline(...)
ok: Utils.<@Nullable String>baseline(...)

<K extends Objects> K noNulls(K key) { return key; }

ok: Utils.<String>baseline(...)
fails: Utils.<@Nullable String>baseline(...)

<@Nullable K> K noNulls(K key) { return key; }

fails: Utils.<String>baseline(...)
ok: Utils.<@Nullable String>baseline(...)

Does that make sense?

/** Returns the value stored in the cache, or null if not present. */
@Nullable V get(@NonNull K key);
@Nullable V get(K key);

/** Stores the value into the cache, replacing an existing mapping if present. */
void put(@NonNull K key, @NonNull V value);
void put(K key, V value);

/** Removes the entry from the cache, if present. */
void remove(@NonNull K key);
void remove(K key);

/** Invalidates all entries from the cache. */
void clear();
Expand Down
28 changes: 10 additions & 18 deletions caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import java.util.function.BiFunction;
import java.util.function.Function;

import org.checkerframework.checker.nullness.qual.NonNull;
import org.checkerframework.checker.nullness.qual.Nullable;

/**
Expand All @@ -50,7 +49,7 @@ public interface AsyncCache<K, V> {
* @throws NullPointerException if the specified key is null
*/
@Nullable
CompletableFuture<V> getIfPresent(@NonNull K key);
CompletableFuture<V> getIfPresent(K key);

/**
* Returns the future associated with {@code key} in this cache, obtaining that value from
Expand All @@ -70,9 +69,7 @@ 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
*/
@NonNull
CompletableFuture<V> get(@NonNull K key,
@NonNull Function<? super K, ? extends V> mappingFunction);
CompletableFuture<V> get(K key, Function<? super K, ? extends V> mappingFunction);

/**
* Returns the future associated with {@code key} in this cache, obtaining that value from
Expand All @@ -95,9 +92,8 @@ CompletableFuture<V> get(@NonNull K key,
* @throws RuntimeException or Error if the mappingFunction does when constructing the future,
* in which case the mapping is left unestablished
*/
@NonNull
CompletableFuture<V> get(@NonNull K key,
@NonNull BiFunction<? super K, Executor, CompletableFuture<V>> mappingFunction);
CompletableFuture<V> get(K key,
BiFunction<? super K, Executor, CompletableFuture<V>> mappingFunction);

This comment has been minimized.

Sign in to view

Copy link
@vlsi

vlsi Jan 17, 2021

Contributor

Did you mean BiFunction<? super K, ? super Executor, ? extends CompletableFuture<? extends V>>?

This comment has been minimized.

Sign in to view

Copy link
@ben-manes

ben-manes Jan 18, 2021

Author Owner

do I need to do this on every generic usage?

This comment has been minimized.

Sign in to view

Copy link
@vlsi

vlsi Jan 18, 2021

Contributor

That is a different story, however, ? extends CompletableFuture<? extends V>> would allow users to use method reference (e.g. UserClass::userMethod) even in the case when userMethod returns something that extends CompletableFuture.

Suppose user writes code like

AsyncCache<String, Number> cache =  ...
cache.get("hello", BigIntegerCalculator::computeValue);

class BigIntegerCalculator {
  BigInteger computeValue(String key, Executor executor) {...}
}

I probably need to double-check, however, it looks like every parameter which is Function-like thing should prefer Function<? super K, ? extends V> kind of signature rather than Function<K, V> to allow users to pass method references and/or implementations that happen to return a subclass.

This comment has been minimized.

Sign in to view

Copy link
@ben-manes

ben-manes Jan 18, 2021

Author Owner

Yeah, I agree with that. I was thinking about it but wasn't sure if that was what you meant given the context here. I agree it would be a good idea to be more open to subtypes in these generics.

This comment has been minimized.

Sign in to view

Copy link
@ben-manes

ben-manes Jan 18, 2021

Author Owner

For a bulk case like,

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

Should that be Map<? extends, ? extends V> in the mappingFunction?

This comment has been minimized.

Sign in to view

Copy link
@ben-manes

ben-manes Jan 18, 2021

Author Owner

oh, you answered that below, nm!

/**
* Returns the future of a map of the values associated with {@code keys}, creating or retrieving
Expand All @@ -122,9 +118,8 @@ CompletableFuture<V> get(@NonNull K key,
* @throws RuntimeException or Error if the mappingFunction does so, in which case the mapping is
* left unestablished
*/
@NonNull
CompletableFuture<Map<K, V>> getAll(@NonNull Iterable<? extends @NonNull K> keys,
@NonNull Function<Set<? extends @NonNull K>, @NonNull Map<K, V>> mappingFunction);
CompletableFuture<Map<K, V>> getAll(Iterable<? extends K> keys,
Function<Set<? extends K>, Map<K, V>> mappingFunction);

This comment has been minimized.

Sign in to view

Copy link
@vlsi

vlsi Jan 17, 2021

Contributor

Did you mean Function<? super Set<? extends K>, ? extends Map<? extends K, ? extends V>>

/**
* Returns the future of a map of the values associated with {@code keys}, creating or retrieving
Expand All @@ -149,9 +144,8 @@ CompletableFuture<Map<K, V>> getAll(@NonNull Iterable<? extends @NonNull K> keys
* @throws RuntimeException or Error if the mappingFunction does so, in which case the mapping is
* left unestablished
*/
@NonNull
CompletableFuture<Map<K, V>> getAll(@NonNull Iterable<? extends @NonNull K> keys,
@NonNull BiFunction<Set<? extends @NonNull K>, Executor, CompletableFuture<Map<K, V>>> mappingFunction);
CompletableFuture<Map<K, V>> getAll(Iterable<? extends K> keys,
BiFunction<Set<? extends K>, Executor, CompletableFuture<Map<K, V>>> mappingFunction);

/**
* Associates {@code value} with {@code key} in this cache. If the cache previously contained a
Expand All @@ -165,7 +159,7 @@ CompletableFuture<Map<K, V>> getAll(@NonNull Iterable<? extends @NonNull K> keys
* @param valueFuture value to be associated with the specified key
* @throws NullPointerException if the specified key or value is null
*/
void put(@NonNull K key, @NonNull CompletableFuture<V> valueFuture);
void put(K key, CompletableFuture<V> valueFuture);

/**
* Returns a view of the entries stored in this cache as a thread-safe map. Modifications made to
Expand All @@ -182,8 +176,7 @@ CompletableFuture<Map<K, V>> getAll(@NonNull Iterable<? extends @NonNull K> keys
*
* @return a thread-safe view of this cache supporting all of the optional {@link Map} operations
*/
@NonNull
ConcurrentMap<@NonNull K, @NonNull CompletableFuture<V>> asMap();
ConcurrentMap<K, CompletableFuture<V>> asMap();

/**
* Returns a view of the entries stored in this cache as a synchronous {@link Cache}. A mapping is
Expand All @@ -193,6 +186,5 @@ CompletableFuture<Map<K, V>> getAll(@NonNull Iterable<? extends @NonNull K> keys
*
* @return a thread-safe synchronous view of this cache
*/
@NonNull
Cache<K, V> synchronous();
}
16 changes: 4 additions & 12 deletions caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncCacheLoader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@
import java.util.function.BiFunction;
import java.util.function.Function;

import org.checkerframework.checker.nullness.qual.NonNull;

/**
* Computes or retrieves values asynchronously, based on a key, for use in populating a
* {@link AsyncLoadingCache}.
Expand Down Expand Up @@ -57,8 +55,7 @@ public interface AsyncCacheLoader<K, V> {
* treated like any other {@code Exception} in all respects except that, when it is
* caught, the thread's interrupt status is set
*/
@NonNull
CompletableFuture<V> asyncLoad(@NonNull K key, @NonNull Executor executor) throws Exception;
CompletableFuture<V> asyncLoad(K key, Executor executor) throws Exception;

/**
* Asynchronously computes or retrieves the values corresponding to {@code keys}. This method is
Expand All @@ -82,9 +79,8 @@ public interface AsyncCacheLoader<K, V> {
* treated like any other {@code Exception} in all respects except that, when it is
* caught, the thread's interrupt status is set
*/
@NonNull
default CompletableFuture<Map<@NonNull K, @NonNull V>> asyncLoadAll(
@NonNull Set<? extends @NonNull K> keys, @NonNull Executor executor) throws Exception {
default CompletableFuture<Map<K, V>> asyncLoadAll(
Set<? extends K> keys, Executor executor) throws Exception {
throw new UnsupportedOperationException();
}

Expand All @@ -106,9 +102,7 @@ public interface AsyncCacheLoader<K, V> {
* treated like any other {@code Exception} in all respects except that, when it is
* caught, the thread's interrupt status is set
*/
@NonNull
default CompletableFuture<V> asyncReload(
@NonNull K key, @NonNull V oldValue, @NonNull Executor executor) throws Exception {
default CompletableFuture<V> asyncReload(K key, V oldValue, Executor executor) throws Exception {
return asyncLoad(key, executor);
}

Expand All @@ -128,7 +122,6 @@ default CompletableFuture<V> asyncReload(
* @return an asynchronous cache loader that delegates to the supplied {@code mappingFunction}
* @throws NullPointerException if the mappingFunction is null
*/
@NonNull
static <K, V> AsyncCacheLoader<K, V> bulk(Function<Set<? extends K>, Map<K, V>> mappingFunction) {

This comment has been minimized.

Sign in to view

Copy link
@vlsi

vlsi Jan 17, 2021

Contributor

Is nullable K allowed here? If not, please use K extends Object
return CacheLoader.bulk(mappingFunction);
}
Expand All @@ -149,7 +142,6 @@ static <K, V> AsyncCacheLoader<K, V> bulk(Function<Set<? extends K>, Map<K, V>>
* @return an asynchronous cache loader that delegates to the supplied {@code mappingFunction}
* @throws NullPointerException if the mappingFunction is null
*/
@NonNull
static <K, V> AsyncCacheLoader<K, V> bulk(
BiFunction<Set<? extends K>, Executor, CompletableFuture<Map<K, V>>> mappingFunction) {
requireNonNull(mappingFunction);
Expand Down
9 changes: 2 additions & 7 deletions caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncLoadingCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
import java.util.Map;
import java.util.concurrent.CompletableFuture;

import org.checkerframework.checker.nullness.qual.NonNull;

/**
* A semi-persistent mapping from keys to values. Values are automatically loaded by the cache
* asynchronously, and are stored in the cache until either evicted or manually invalidated.
Expand Down Expand Up @@ -49,8 +47,7 @@ public interface AsyncLoadingCache<K, V> extends AsyncCache<K, V> {
* @throws RuntimeException or Error if the {@link AsyncCacheLoader} does when constructing the
* future, in which case the mapping is left unestablished
*/
@NonNull
CompletableFuture<V> get(@NonNull K key);
CompletableFuture<V> get(K key);

/**
* Returns the future of a map of the values associated with {@code keys}, creating or retrieving
Expand Down Expand Up @@ -78,8 +75,7 @@ public interface AsyncLoadingCache<K, V> extends AsyncCache<K, V> {
* {@link AsyncCacheLoader#asyncLoadAll} returns {@code null}, or fails when constructing
* the future, in which case the mapping is left unestablished
*/
@NonNull
CompletableFuture<Map<K, V>> getAll(@NonNull Iterable<? extends @NonNull K> keys);
CompletableFuture<Map<K, V>> getAll(Iterable<? extends K> keys);

/**
* Returns a view of the entries stored in this cache as a synchronous {@link LoadingCache}. A
Expand All @@ -89,7 +85,6 @@ public interface AsyncLoadingCache<K, V> extends AsyncCache<K, V> {
*
* @return a thread-safe synchronous view of this cache
*/
@NonNull
@Override
LoadingCache<K, V> synchronous();
}
7 changes: 3 additions & 4 deletions caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@
import java.util.function.Predicate;
import java.util.function.Supplier;

import org.checkerframework.checker.nullness.qual.NonNull;
import org.checkerframework.checker.nullness.qual.Nullable;

import com.github.benmanes.caffeine.cache.Async.AsyncExpiry;
Expand Down Expand Up @@ -752,13 +751,13 @@ void evictFromMain(int candidates) {
K victimKey = victim.getKey();
K candidateKey = candidate.getKey();
if (victimKey == null) {
@NonNull Node<K, V> evict = victim;
Node<K, V> evict = victim;
victim = victim.getNextInAccessOrder();
evictEntry(evict, RemovalCause.COLLECTED, 0L);
continue;
} else if (candidateKey == null) {
candidates--;
@NonNull Node<K, V> evict = candidate;
Node<K, V> evict = candidate;
candidate = candidate.getPreviousInAccessOrder();
evictEntry(evict, RemovalCause.COLLECTED, 0L);
continue;
Expand Down Expand Up @@ -3529,7 +3528,7 @@ final class BoundedEviction implements Eviction<K, V> {
@Override public boolean isWeighted() {
return isWeighted;
}
@Override public OptionalInt weightOf(@NonNull K key) {
@Override public OptionalInt weightOf(K key) {
requireNonNull(key);
if (!isWeighted) {
return OptionalInt.empty();
Expand Down
6 changes: 2 additions & 4 deletions caffeine/src/main/java/com/github/benmanes/caffeine/cache/Buffer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@

import java.util.function.Consumer;

import org.checkerframework.checker.nullness.qual.NonNull;

/**
* A multiple-producer / single-consumer buffer that rejects new elements if it is full or
* fails spuriously due to contention. Unlike a queue and stack, a buffer does not guarantee an
Expand Down Expand Up @@ -50,15 +48,15 @@ static <E> Buffer<E> disabled() {
* @param e the element to add
* @return {@code 1} if the buffer is full, {@code -1} if the CAS failed, or {@code 0} if added
*/
int offer(@NonNull E e);
int offer(E e);

/**
* Drains the buffer, sending each element to the consumer for processing. The caller must ensure
* that a consumer has exclusive read access to the buffer.
*
* @param consumer the action to perform on each element
*/
void drainTo(@NonNull Consumer<E> consumer);
void drainTo(Consumer<E> consumer);

/**
* Returns the number of elements residing in the buffer.
Expand Down
29 changes: 12 additions & 17 deletions caffeine/src/main/java/com/github/benmanes/caffeine/cache/Cache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@
import java.util.function.Function;

import org.checkerframework.checker.index.qual.NonNegative;
import org.checkerframework.checker.nullness.qual.NonNull;
import org.checkerframework.checker.nullness.qual.Nullable;
import org.checkerframework.checker.nullness.qual.PolyNull;

import com.github.benmanes.caffeine.cache.stats.CacheStats;

Expand Down Expand Up @@ -50,7 +50,7 @@ public interface Cache<K, V> {
* @throws NullPointerException if the specified key is null
*/
@Nullable
V getIfPresent(@NonNull K key);
V getIfPresent(K key);

/**
* Returns the value associated with the {@code key} in this cache, obtaining that value from the
Expand All @@ -77,8 +77,8 @@ public interface Cache<K, V> {
* @throws RuntimeException or Error if the mappingFunction does so, in which case the mapping is
* left unestablished
*/
@Nullable
V get(@NonNull K key, @NonNull Function<? super K, ? extends V> mappingFunction);
@PolyNull
V get(K key, Function<? super K, ? extends @PolyNull V> mappingFunction);

/**
* Returns a map of the values associated with the {@code keys} in this cache. The returned map
Expand All @@ -91,8 +91,7 @@ public interface Cache<K, V> {
* @return the unmodifiable mapping of keys to values for the specified keys found in this cache
* @throws NullPointerException if the specified collection is null or contains a null element
*/
@NonNull
Map<@NonNull K, @NonNull V> getAllPresent(@NonNull Iterable<? extends K> keys);
Map<K, V> getAllPresent(Iterable<? extends K> keys);

/**
* Returns a map of the values associated with the {@code keys}, creating or retrieving those
Expand All @@ -118,9 +117,8 @@ public interface Cache<K, V> {
* @throws RuntimeException or Error if the mappingFunction does so, in which case the mapping is
* left unestablished
*/
@NonNull
Map<K, V> getAll(@NonNull Iterable<? extends @NonNull K> keys,
@NonNull Function<Set<? extends @NonNull K>, @NonNull Map<K, V>> mappingFunction);
Map<K, V> getAll(Iterable<? extends K> keys,
Function<Set<? extends K>, Map<K, V>> mappingFunction);

/**
* Associates the {@code value} with the {@code key} in this cache. If the cache previously
Expand All @@ -134,7 +132,7 @@ Map<K, V> getAll(@NonNull Iterable<? extends @NonNull K> keys,
* @param value value to be associated with the specified key
* @throws NullPointerException if the specified key or value is null
*/
void put(@NonNull K key, @NonNull V value);
void put(K key, V value);

/**
* Copies all of the mappings from the specified map to the cache. The effect of this call is
Expand All @@ -146,7 +144,7 @@ Map<K, V> getAll(@NonNull Iterable<? extends @NonNull K> keys,
* @throws NullPointerException if the specified map is null or the specified map contains null
* keys or values
*/
void putAll(@NonNull Map<? extends @NonNull K,? extends @NonNull V> map);
void putAll(Map<? extends K,? extends V> map);

/**
* Discards any cached value for the {@code key}. The behavior of this operation is undefined for
Expand All @@ -155,7 +153,7 @@ Map<K, V> getAll(@NonNull Iterable<? extends @NonNull K> keys,
* @param key the key whose mapping is to be removed from the cache
* @throws NullPointerException if the specified key is null
*/
void invalidate(@NonNull K key);
void invalidate(K key);

/**
* Discards any cached values for the {@code keys}. The behavior of this operation is undefined
Expand All @@ -164,7 +162,7 @@ Map<K, V> getAll(@NonNull Iterable<? extends @NonNull K> keys,
* @param keys the keys whose associated values are to be removed
* @throws NullPointerException if the specified collection is null or contains a null element
*/
void invalidateAll(@NonNull Iterable<? extends @NonNull K> keys);
void invalidateAll(Iterable<? extends K> keys);

/**
* Discards all entries in the cache. The behavior of this operation is undefined for an entry
Expand Down Expand Up @@ -192,7 +190,6 @@ Map<K, V> getAll(@NonNull Iterable<? extends @NonNull K> keys,
*
* @return the current snapshot of the statistics of this cache
*/
@NonNull
CacheStats stats();

/**
Expand All @@ -210,8 +207,7 @@ Map<K, V> getAll(@NonNull Iterable<? extends @NonNull K> keys,
*
* @return a thread-safe view of this cache supporting all of the optional {@link Map} operations
*/
@NonNull
ConcurrentMap<@NonNull K, @NonNull V> asMap();
ConcurrentMap<K, V> asMap();

/**
* Performs any pending maintenance operations needed by the cache. Exactly which activities are
Expand All @@ -226,6 +222,5 @@ Map<K, V> getAll(@NonNull Iterable<? extends @NonNull K> keys,
*
* @return access to inspect and perform advanced operations based on the cache's characteristics
*/
@NonNull
Policy<K, V> policy();
}
Loading

8 comments on commit fb4eb0b

@ben-manes
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @vlsi! I didn't get to this today, but hoping to tomorrow as a day off. 🙂

@vlsi
Copy link
Contributor

@vlsi vlsi commented on fb4eb0b Jan 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, @NonNull removal looks good.

The's a common pattern with type variables. In other words, you might want to replace ClassName<K> with ClassName<K extends Object> to specify that K must not be nullable.

Just in case, the following would mean both K and K1 are never nullable, while both V and V1 permit nullable types:

  public static <K extends Object, V, K1 extends K, V1 extends V> Cache<K1, V1> build(Caffeine<K, V> builder) {

--

I would recommend (checker framework recommend that as well) placing @Nullable, @PolyNull, @NonNegative closer to a type. At the end of the day, it is type annotation rather than method annotation.
For instance:

  @NonNegative
  public int frequency(@NonNull E e) {

=>

  public @NonNegative int frequency(@NonNull E e) {

@ben-manes
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not very comfortable with the CheckerFramework's nullness rules yet, so I guess this will take a few more iterations. I'm concerned that it will be wrong and cause problems downstream for those using it?

Now that CheckerFramework and ErrorProne are compatible, we could run both. Doing this locally shows various warnings that deserve a dedicated pass in a different commit.

@vlsi
Copy link
Contributor

@vlsi vlsi commented on fb4eb0b Jan 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned that it will be wrong and cause problems downstream for those using it?

Kotlin does not understand CheckerFramework's nullness annotations (it treats the code as if it was not annotated), so you are covered there :)
The misuse of jsr305 nullables might cause Kotlin's compilation errors.

If someone is impacted by improper or incomplete nullness annotations (I assume they are verifying nullness), they can use astub feature of the checker framework where they can augment the annotation of a third-party code.
So it does not hurt much to have incomplete or improper annotations.

IDEA does not support checkerframework's rules regarding generics, @PolyNull, @MonotonicNonNull and other complicated cases, so it might produce false positives. However, in my experience, even then nullness annotations help a lot. IDEA understands DefaultQualifier.

For instance, in Calcite we have a few stubs: https://github.com/apache/calcite/tree/master/src/main/config/checkerframework
A funny story is sometimes we use stubs to remove excessive @Nullable annotations (e.g. https://github.com/apache/calcite/blob/f277a2468805999a446e5bcd0ef70aa1e9550562/src/main/config/checkerframework/guava/Function.astub)

@ben-manes
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, thanks a lot. After I go through a pass would you be open to doing a review and sending in a PR of changes?

@vlsi
Copy link
Contributor

@vlsi vlsi commented on fb4eb0b Jan 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After I go through a pass would you be open to doing a review and sending in a PR of changes?

Sure.

@ben-manes
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I pushed to the v3 branch. The improved wildcard generics are nice, but I likely still have goofs wrt checker.

@ben-manes
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi, I am also starting a v3-checker branch where the checker framework plugin is enabled and will go through handing the warnings. That will be merged back into v3, but will take a while to resolve them all (so unlikely to be done soon).

Please sign in to comment.