Add NullAway for more extensive static analysis

NullAway is an addition to ErrorProne to perform a more robust data flow analysis for possible NPEs. This inferrence requires richer usage of the defacto null checking annotations by defaulting to a non-null expectation. Due to limitations in the data flow, there are false positives which requires slightly more verbose code or suppression. When possible, the suppressions are isolated to individual statements to minimize their scope.
ben-manes · Jan 1, 2018 · 6226cf9 · 6226cf9
1 parent ace16b7
commit 6226cf9
Show file tree

Hide file tree

Showing 50 changed files with 435 additions and 374 deletions.
diff --git a/build.gradle b/build.gradle
@@ -83,8 +83,6 @@ subprojects {
     provided libraries.jsr305
     provided libraries.errorProneAnnotations
 
-    errorprone libraries.errorProneCore
-
     testCompile libraries.guava
     testCompile testLibraries.mockito
     testCompile testLibraries.hamcrest

diff --git a/...eine/src/javaPoet/java/com/github/benmanes/caffeine/cache/LocalCacheFactoryGenerator.java b/...eine/src/javaPoet/java/com/github/benmanes/caffeine/cache/LocalCacheFactoryGenerator.java
@@ -39,6 +39,7 @@
 import java.util.Set;
 import java.util.TreeMap;
 
+import javax.annotation.Nullable;
 import javax.lang.model.element.Modifier;
 
 import com.github.benmanes.caffeine.cache.local.AddConstructor;
@@ -122,7 +123,7 @@ private void addFactoryMethods() {
         .addModifiers(Modifier.STATIC)
         .addCode(LocalCacheSelectorCode.get())
         .addParameter(BUILDER_PARAM)
-        .addParameter(CACHE_LOADER_PARAM)
+        .addParameter(CACHE_LOADER_PARAM.toBuilder().addAnnotation(Nullable.class).build())
         .addParameter(boolean.class, "async")
         .addJavadoc("Returns a cache optimized for this configuration.\n")
         .build());

diff --git a/caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/local/AddSubtype.java b/caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/local/AddSubtype.java
@@ -40,7 +40,7 @@ protected boolean applies() {
   protected void execute() {
     context.cache.superclass(context.superClass)
         .addAnnotation(AnnotationSpec.builder(SuppressWarnings.class)
-            .addMember("value", "{$S, $S}", "unchecked", "MissingOverride")
+            .addMember("value", "{$S, $S, $S}", "unchecked", "MissingOverride", "NullAway")
             .build())
         .addJavadoc(getJavaDoc())
         .addTypeVariable(kTypeVar)

diff --git a/caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/node/AddSubtype.java b/caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/node/AddSubtype.java
@@ -43,8 +43,8 @@ protected boolean applies() {
   protected void execute() {
     context.nodeSubtype = TypeSpec.classBuilder(context.className)
         .addAnnotation(AnnotationSpec.builder(SuppressWarnings.class)
-            .addMember("value", "{$S, $S, $S}",
-                "unchecked", "PMD.UnusedFormalParameter", "MissingOverride")
+            .addMember("value", "{$S, $S, $S, $S}",
+                "unchecked", "PMD.UnusedFormalParameter", "MissingOverride", "NullAway")
             .build())
         .addJavadoc(getJavaDoc())
         .addTypeVariable(kTypeVar)

diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/SingleConsumerQueue.java b/caffeine/src/main/java/com/github/benmanes/caffeine/SingleConsumerQueue.java
@@ -76,6 +76,7 @@
  * @author ben.manes@gmail.com (Ben Manes)
  * @param <E> the type of elements held in this collection
  */
+@SuppressWarnings("NullAway")
 public final class SingleConsumerQueue<E> extends SCQHeader.HeadAndTailRef<E>
     implements Queue<E>, Serializable {
 
@@ -467,8 +468,8 @@ Object readResolve() {
   static class Node<E> {
     static final long NEXT_OFFSET = UnsafeAccess.objectFieldOffset(Node.class, "next");
 
-    E value;
-    volatile Node<E> next;
+    @Nullable E value;
+    @Nullable volatile Node<E> next;
 
     Node(@Nullable E value) {
       this.value = value;
@@ -536,7 +537,7 @@ abstract static class PadHead<E> extends AbstractQueue<E> {
 
   /** Enforces a memory layout to avoid false sharing by padding the head node. */
   abstract static class HeadRef<E> extends PadHead<E> {
-    Node<E> head;
+    @Nullable Node<E> head;
   }
 
   abstract static class PadHeadAndTail<E> extends HeadRef<E> {
@@ -548,7 +549,7 @@ abstract static class PadHeadAndTail<E> extends HeadRef<E> {
   abstract static class HeadAndTailRef<E> extends PadHeadAndTail<E> {
     static final long TAIL_OFFSET = UnsafeAccess.objectFieldOffset(HeadAndTailRef.class, "tail");
 
-    volatile Node<E> tail;
+    @Nullable volatile Node<E> tail;
 
     void lazySetTail(Node<E> next) {
       UnsafeAccess.UNSAFE.putOrderedObject(this, TAIL_OFFSET, next);

diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AbstractLinkedDeque.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AbstractLinkedDeque.java
@@ -19,6 +19,7 @@
 import java.util.Collection;
 import java.util.NoSuchElementException;
 
+import javax.annotation.Nullable;
 import javax.annotation.concurrent.NotThreadSafe;
 
 /**
@@ -42,14 +43,14 @@ abstract class AbstractLinkedDeque<E> extends AbstractCollection<E> implements L
    * Invariant: (first == null && last == null) ||
    *            (first.prev == null)
    */
-  E first;
+  @Nullable E first;
 
   /**
    * Pointer to last node.
    * Invariant: (first == null && last == null) ||
    *            (last.next == null)
    */
-  E last;
+  @Nullable E last;
 
   /**
    * Links the element to the front of the deque so that it becomes the first element.
@@ -86,6 +87,7 @@ void linkLast(final E e) {
   }
 
   /** Unlinks the non-null first element. */
+  @SuppressWarnings("NullAway")
   E unlinkFirst() {
     final E f = first;
     final E next = getNext(f);
@@ -101,6 +103,7 @@ E unlinkFirst() {
   }
 
   /** Unlinks the non-null last element. */
+  @SuppressWarnings("NullAway")
   E unlinkLast() {
     final E l = last;
     final E prev = getPrevious(l);
@@ -200,27 +203,29 @@ public void moveToBack(E e) {
   }
 
   @Override
-  public E peek() {
+  public @Nullable E peek() {
     return peekFirst();
   }
 
   @Override
-  public E peekFirst() {
+  public @Nullable E peekFirst() {
     return first;
   }
 
   @Override
-  public E peekLast() {
+  public @Nullable E peekLast() {
     return last;
   }
 
   @Override
+  @SuppressWarnings("NullAway")
   public E getFirst() {
     checkNotEmpty();
     return peekFirst();
   }
 
   @Override
+  @SuppressWarnings("NullAway")
   public E getLast() {
     checkNotEmpty();
     return peekLast();
@@ -274,17 +279,17 @@ public void addLast(E e) {
   }
 
   @Override
-  public E poll() {
+  public @Nullable E poll() {
     return pollFirst();
   }
 
   @Override
-  public E pollFirst() {
+  public @Nullable E pollFirst() {
     return isEmpty() ? null : unlinkFirst();
   }
 
   @Override
-  public E pollLast() {
+  public @Nullable E pollLast() {
     return isEmpty() ? null : unlinkLast();
   }
 
@@ -294,6 +299,7 @@ public E remove() {
   }
 
   @Override
+  @SuppressWarnings("NullAway")
   public E removeFirst() {
     checkNotEmpty();
     return pollFirst();
@@ -305,6 +311,7 @@ public boolean removeFirstOccurrence(Object o) {
   }
 
   @Override
+  @SuppressWarnings("NullAway")
   public E removeLast() {
     checkNotEmpty();
     return pollLast();
@@ -335,33 +342,35 @@ public E pop() {
   }
 
   @Override
+  @SuppressWarnings("NullAway")
   public PeekingIterator<E> iterator() {
     return new AbstractLinkedIterator(first) {
-      @Override E computeNext() {
+      @Override @Nullable E computeNext() {
         return getNext(cursor);
       }
     };
   }
 
   @Override
+  @SuppressWarnings("NullAway")
   public PeekingIterator<E> descendingIterator() {
     return new AbstractLinkedIterator(last) {
-      @Override E computeNext() {
+      @Override @Nullable E computeNext() {
         return getPrevious(cursor);
       }
     };
   }
 
   abstract class AbstractLinkedIterator implements PeekingIterator<E> {
-    E previous;
-    E cursor;
+    @Nullable E previous;
+    @Nullable E cursor;
 
     /**
      * Creates an iterator that can can traverse the deque.
      *
      * @param start the initial element to begin traversal from
      */
-    AbstractLinkedIterator(E start) {
+    AbstractLinkedIterator(@Nullable E start) {
       cursor = start;
     }
 
@@ -371,11 +380,12 @@ public boolean hasNext() {
     }
 
     @Override
-    public E peek() {
+    public @Nullable E peek() {
       return cursor;
     }
 
     @Override
+    @SuppressWarnings("NullAway")
     public E next() {
       if (!hasNext()) {
         throw new NoSuchElementException();
@@ -386,7 +396,7 @@ public E next() {
     }
 
     /** Retrieves the next element to traverse to or <tt>null</tt> if there are no more elements. */
-    abstract E computeNext();
+    abstract @Nullable E computeNext();
 
     @Override
     public void remove() {

diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AccessOrderDeque.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AccessOrderDeque.java
@@ -17,6 +17,7 @@
 
 import java.util.Deque;
 
+import javax.annotation.Nullable;
 import javax.annotation.concurrent.NotThreadSafe;
 
 import com.github.benmanes.caffeine.cache.AccessOrderDeque.AccessOrder;
@@ -58,22 +59,22 @@ boolean remove(E e) {
   }
 
   @Override
-  public E getPrevious(E e) {
+  public @Nullable E getPrevious(E e) {
     return e.getPreviousInAccessOrder();
   }
 
   @Override
-  public void setPrevious(E e, E prev) {
+  public void setPrevious(E e, @Nullable E prev) {
     e.setPreviousInAccessOrder(prev);
   }
 
   @Override
-  public E getNext(E e) {
+  public @Nullable E getNext(E e) {
     return e.getNextInAccessOrder();
   }
 
   @Override
-  public void setNext(E e, E next) {
+  public void setNext(E e, @Nullable E next) {
     e.setNextInAccessOrder(next);
   }
 
@@ -86,18 +87,18 @@ interface AccessOrder<T extends AccessOrder<T>> {
      * Retrieves the previous element or <tt>null</tt> if either the element is unlinked or the first
      * element on the deque.
      */
-    T getPreviousInAccessOrder();
+    @Nullable T getPreviousInAccessOrder();
 
     /** Sets the previous element or <tt>null</tt> if there is no link. */
-    void setPreviousInAccessOrder(T prev);
+    void setPreviousInAccessOrder(@Nullable T prev);
 
     /**
      * Retrieves the next element or <tt>null</tt> if either the element is unlinked or the last
      * element on the deque.
      */
-    T getNextInAccessOrder();
+    @Nullable T getNextInAccessOrder();
 
     /** Sets the next element or <tt>null</tt> if there is no link. */
-    void setNextInAccessOrder(T next);
+    void setNextInAccessOrder(@Nullable T next);
   }
 }
diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Async.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Async.java
@@ -23,7 +23,6 @@
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Executor;
 
-import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 
 /**
@@ -44,6 +43,7 @@ static boolean isReady(@Nullable CompletableFuture<?> future) {
   }
 
   /** Returns the current value or null if either not done or failed. */
+  @SuppressWarnings("NullAway")
   static @Nullable <V> V getIfReady(@Nullable CompletableFuture<V> future) {
     return isReady(future) ? future.join() : null;
   }
@@ -78,8 +78,11 @@ static final class AsyncRemovalListener<K, V>
 
     @Override
     @SuppressWarnings("FutureReturnValueIgnored")
-    public void onRemoval(K key, @Nonnull CompletableFuture<V> future, RemovalCause cause) {
-      future.thenAcceptAsync(value -> delegate.onRemoval(key, value, cause), executor);
+    public void onRemoval(@Nullable K key,
+        @Nullable CompletableFuture<V> future, RemovalCause cause) {
+      if (future != null) {
+        future.thenAcceptAsync(value -> delegate.onRemoval(key, value, cause), executor);
+      }
     }
 
     Object writeReplace() {

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
@@ -21,8 +21,8 @@
 import java.util.function.BiFunction;
 import java.util.function.Function;
 
-import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
 import javax.annotation.concurrent.ThreadSafe;
 
 /**
@@ -48,7 +48,7 @@ public interface AsyncLoadingCache<K, V> {
    *         or {@code null} if this map contains no mapping for the key
    * @throws NullPointerException if the specified key is null
    */
-  @CheckForNull
+  @Nullable
   CompletableFuture<V> getIfPresent(@Nonnull Object key);
 
   /**