Merge pull request spotify#132 from pettermahlen/add-errorprone

Add ErrorProne

build.gradle

@@ -20,6 +20,7 @@ plugins {
     id 'com.github.sherter.google-java-format' version '0.8'
     id 'com.github.hierynomus.license' version '0.15.0'
     id 'io.codearte.nexus-staging' version '0.21.2'
+    id 'net.ltgt.errorprone' version '1.2.1'
 }
 
 repositories {
@@ -52,15 +53,24 @@ ext {
             'hamcrestLibrary'  : '1.3',
             'mockito'          : '1.10.19',
             'androidxLifecycle' : '2.2.0',
-            'androidXCoreTesting' : '2.1.0'
+            'androidXCoreTesting' : '2.1.0',
+            'errorProne'          : '2.4.0',
+            'errorProneJavac'     : '9+181-r4173-1',
     ]
 }
 
 subprojects {
     ext.VERSION_NAME = properties.version
+    apply plugin: 'net.ltgt.errorprone'
 
     repositories {
         jcenter()
+        mavenCentral()
+    }
+
+    dependencies {
+        errorprone("com.google.errorprone:error_prone_core:${versions.errorProne}")
+        errorproneJavac("com.google.errorprone:javac:${versions.errorProneJavac}")
     }
 
     group = GROUP
@@ -83,6 +93,15 @@ subprojects {
         }
         // ensure that builds fail if code is not formatted properly
         proj.tasks.check.dependsOn(rootProject.tasks.verifyFormat)
+
+        tasks.withType(JavaCompile).configureEach {
+            options.errorprone.disableWarningsInGeneratedCode = true
+            options.errorprone.disable("AutoValueImmutableFields")
+        }
+
+        tasks.withType(JavaCompile) {
+            options.compilerArgs << "-Werror"
+        }
     }
 }
 

mobius-android/src/main/java/com/spotify/mobius/android/LiveQueue.java

@@ -34,14 +34,14 @@
 public interface LiveQueue<T> {
 
   /**
-   * @return <code>true</code> if the current observer is in a Resumed state<br>
-   *     <code>false</code> if the current observer is not Resumed, or there is no current observer
+   * Returns <code>true</code> if the current observer is in a Resumed state<br>
+   * <code>false</code> if the current observer is not Resumed, or there is no current observer
    */
   boolean hasActiveObserver();
 
   /**
-   * @return <code>true</code> if there is an observer of this <code>LiveQueue</code><br>
-   *     <code>false</code> if there is no current observer assigned
+   * Returns <code>true</code> if there is an observer of this <code>LiveQueue</code><br>
+   * <code>false</code> if there is no current observer assigned
    */
   boolean hasObserver();
 

mobius-android/src/main/java/com/spotify/mobius/android/MobiusLoopViewModel.java

@@ -25,7 +25,6 @@
 import com.spotify.mobius.First;
 import com.spotify.mobius.Init;
 import com.spotify.mobius.MobiusLoop;
-import com.spotify.mobius.MobiusLoop.Factory;
 import com.spotify.mobius.android.runners.MainThreadWorkRunner;
 import com.spotify.mobius.functions.Consumer;
 import com.spotify.mobius.functions.Function;
@@ -71,13 +70,14 @@ public class MobiusLoopViewModel<M, E, F, V> extends ViewModel {
   private final AtomicBoolean loopActive = new AtomicBoolean(true);
 
   protected MobiusLoopViewModel(
-      @Nonnull Function<Consumer<V>, Factory<M, E, F>> loopFactoryProvider,
+      @Nonnull Function<Consumer<V>, MobiusLoop.Factory<M, E, F>> loopFactoryProvider,
       @Nonnull M modelToStartFrom,
       @Nonnull Init<M, F> init,
       @Nonnull WorkRunner mainLoopWorkRunner,
       int maxEffectQueueSize) {
     viewEffectQueue = new MutableLiveQueue<>(mainLoopWorkRunner, maxEffectQueueSize);
-    final Factory<M, E, F> loopFactory = loopFactoryProvider.apply(this::acceptViewEffect);
+    final MobiusLoop.Factory<M, E, F> loopFactory =
+        loopFactoryProvider.apply(this::acceptViewEffect);
     final First<M, F> first = init.init(modelToStartFrom);
     loop = loopFactory.startFrom(first.model(), first.effects());
     startModel = first.model();
@@ -97,7 +97,7 @@ protected MobiusLoopViewModel(
    * @param <V> the view effect type
    */
   public static <M, E, F, V> MobiusLoopViewModel<M, E, F, V> create(
-      @Nonnull Function<Consumer<V>, Factory<M, E, F>> loopFactoryProvider,
+      @Nonnull Function<Consumer<V>, MobiusLoop.Factory<M, E, F>> loopFactoryProvider,
       @Nonnull M modelToStartFrom,
       @Nonnull Init<M, F> init) {
     return create(loopFactoryProvider, modelToStartFrom, init, 100);
@@ -117,7 +117,7 @@ public static <M, E, F, V> MobiusLoopViewModel<M, E, F, V> create(
    * @param <V> the view effect type
    */
   public static <M, E, F, V> MobiusLoopViewModel<M, E, F, V> create(
-      @Nonnull Function<Consumer<V>, Factory<M, E, F>> loopFactoryProvider,
+      @Nonnull Function<Consumer<V>, MobiusLoop.Factory<M, E, F>> loopFactoryProvider,
       @Nonnull M modelToStartFrom,
       @Nonnull Init<M, F> init,
       int maxEffectsToQueue) {

mobius-android/src/main/java/com/spotify/mobius/android/MutableLiveQueue.java

@@ -141,9 +141,15 @@ private void onLifecycleChanged(Lifecycle.Event event) {
           clearObserver();
         }
         break;
+      default:
+        // ignore other events
     }
   }
 
+  // errorprone recommends using ArrayDeque instead of LinkedList here, but ArrayDeque doesn't
+  // implement equals, so it's not very useful for testing, and performance isn't going to be an
+  // issue here
+  @SuppressWarnings("JdkObsolete")
   private void sendQueuedEffects() {
     final Queue<T> queueToSend = new LinkedList<>();
     synchronized (lock) {

mobius-android/src/test/java/com/spotify/mobius/android/MobiusLoopViewModelTest.java

@@ -37,9 +37,9 @@
 import com.spotify.mobius.android.MobiusLoopViewModelTestUtilClasses.ViewEffectSendingEffectHandler;
 import com.spotify.mobius.functions.Consumer;
 import com.spotify.mobius.runners.ImmediateWorkRunner;
+import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashSet;
-import java.util.LinkedList;
 import java.util.List;
 import java.util.Set;
 import org.junit.Before;
@@ -49,7 +49,7 @@
 public class MobiusLoopViewModelTest {
 
   @Rule public InstantTaskExecutorRule rule = new InstantTaskExecutorRule();
-  private List<TestEvent> recordedEvents = new LinkedList<>();
+  private List<TestEvent> recordedEvents = new ArrayList<>();
   private final Update<TestModel, TestEvent, TestEffect> updateFunction =
       (model, event) -> {
         recordedEvents.add(event);
@@ -69,7 +69,7 @@ public class MobiusLoopViewModelTest {
   @Before
   public void setUp() {
     fakeLifecycle = new FakeLifecycleOwner();
-    recordedEvents = new LinkedList<>();
+    recordedEvents = new ArrayList<>();
     testViewEffectHandler = null;
     recordingModelObserver = new RecordingObserver<>();
     recordingForegroundViewEffectObserver = new RecordingObserver<>();

mobius-android/src/test/java/com/spotify/mobius/android/MutableLiveQueueTest.java

@@ -217,6 +217,10 @@ public void shouldThrowIllegalStateExceptionIfQueueFull() throws Exception {
         .hasMessageContaining(String.valueOf(QUEUE_CAPACITY));
   }
 
+  // errorprone recommends using ArrayDeque instead of LinkedList here, but ArrayDeque doesn't
+  // implement equals, so it's not very useful for testing, and performance isn't going to be an
+  // issue here or in the production code.
+  @SuppressWarnings("JdkObsolete")
   private Queue<String> queueOf(String... args) {
     return new LinkedList<>(Arrays.asList(args));
   }

mobius-android/src/test/java/com/spotify/mobius/android/RecordingObserver.java

@@ -19,13 +19,12 @@
  */
 package com.spotify.mobius.android;
 
-import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsInAnyOrder;
-import static org.junit.Assert.assertThat;
 
 import androidx.lifecycle.Observer;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.List;
 
 public class RecordingObserver<V> implements Observer<V> {
@@ -41,18 +40,6 @@ public void onChanged(V value) {
     }
   }
 
-  public boolean waitForChange(long timeoutMs) {
-    synchronized (lock) {
-      try {
-        lock.wait(timeoutMs);
-        return true;
-
-      } catch (InterruptedException e) {
-        return false;
-      }
-    }
-  }
-
   public int valueCount() {
     synchronized (lock) {
       return values.size();
@@ -62,7 +49,7 @@ public int valueCount() {
   @SafeVarargs
   public final void assertValues(V... expectedValues) {
     synchronized (lock) {
-      assertThat(values, equalTo(Arrays.asList(expectedValues)));
+      assertThat(values, contains(expectedValues));
     }
   }
 

mobius-core/src/main/java/com/spotify/mobius/Connection.java

@@ -36,6 +36,7 @@ public interface Connection<I> extends Disposable, Consumer<I> {
    *
    * @param value the value that should be sent to the connection
    */
+  @Override
   void accept(I value);
 
   /**
@@ -44,5 +45,6 @@ public interface Connection<I> extends Disposable, Consumer<I> {
    * <p>The connection will no longer be valid after dispose has been called. No further values will
    * be accepted, and any repeated calls to dispose should be ignored.
    */
+  @Override
   void dispose();
 }

mobius-core/src/main/java/com/spotify/mobius/ConnectionException.java

@@ -44,7 +44,7 @@ public ConnectionException(Object value, Throwable throwable) {
   @Override
   public boolean equals(Object o) {
     if (this == o) return true;
-    if (o == null || getClass() != o.getClass()) return false;
+    if (!(o instanceof ConnectionException)) return false;
 
     ConnectionException that = (ConnectionException) o;
 

mobius-core/src/main/java/com/spotify/mobius/First.java

@@ -28,11 +28,11 @@
 @AutoValue
 public abstract class First<M, F> {
 
-  /** @return the initial model to use */
+  /** the initial model to use */
   @Nonnull
   public abstract M model();
 
-  /** @return the possibly empty set of effects to initially dispatch */
+  /** the possibly empty set of effects to initially dispatch */
   @Nonnull
   public abstract Set<F> effects();
 

mobius-core/src/main/java/com/spotify/mobius/Mobius.java

@@ -21,7 +21,6 @@
 
 import static com.spotify.mobius.internal_util.Preconditions.checkNotNull;
 
-import com.spotify.mobius.functions.Consumer;
 import com.spotify.mobius.functions.Producer;
 import com.spotify.mobius.internal_util.ImmutableUtil;
 import com.spotify.mobius.runners.WorkRunner;
@@ -40,21 +39,14 @@ private Mobius() {
   }
 
   private static final Connectable<?, ?> NOOP_EVENT_SOURCE =
-      new Connectable<Object, Object>() {
-
-        @Nonnull
-        @Override
-        public Connection<Object> connect(Consumer<Object> output)
-            throws ConnectionLimitExceededException {
-          return new Connection<Object>() {
+      output ->
+          new Connection<Object>() {
             @Override
             public void accept(Object value) {}
 
             @Override
             public void dispose() {}
           };
-        }
-      };
 
   private static final MobiusLoop.Logger<?, ?, ?> NOOP_LOGGER =
       new MobiusLoop.Logger<Object, Object, Object>() {

mobius-core/src/main/java/com/spotify/mobius/MobiusLoop.java

@@ -248,60 +248,61 @@ public synchronized void dispose() {
   public interface Builder<M, E, F> extends Factory<M, E, F> {
 
     /**
+     * Returns a new {@link Builder} with the supplied {@link Init}, and the same values as the
+     * current one for the other fields.
+     *
      * @deprecated Pass your initial effects to {@link #startFrom(Object, Set)} instead.
-     * @return a new {@link Builder} with the supplied {@link Init}, and the same values as the
-     *     current one for the other fields.
      */
     @Nonnull
     @Deprecated
     Builder<M, E, F> init(Init<M, F> init);
 
     /**
-     * @return a new {@link Builder} with the supplied {@link EventSource}, and the same values as
-     *     the current one for the other fields. NOTE: Invoking this method will replace the current
-     *     {@link EventSource} with the supplied one. If you want to pass multiple event sources,
-     *     please use {@link #eventSources(EventSource, EventSource[])}.
+     * Returns a new {@link Builder} with the supplied {@link EventSource}, and the same values as
+     * the current one for the other fields. NOTE: Invoking this method will replace the current
+     * {@link EventSource} with the supplied one. If you want to pass multiple event sources, please
+     * use {@link #eventSources(EventSource, EventSource[])}.
      */
     @Nonnull
     Builder<M, E, F> eventSource(EventSource<E> eventSource);
 
     /**
-     * @return a new {@link Builder} with an {@link EventSource} that merges the supplied event
-     *     sources, and the same values as the current one for the other fields.
+     * Returns a new {@link Builder} with an {@link EventSource} that merges the supplied event
+     * sources, and the same values as the current one for the other fields.
      */
     @Nonnull
     Builder<M, E, F> eventSources(EventSource<E> eventSource, EventSource<E>... eventSources);
 
     /**
-     * @return a new {@link Builder} with the supplied {@link Connectable<M,E>}, and the same values
-     *     as the current one for the other fields. NOTE: Invoking this method will replace the
-     *     current event source with the supplied one. If a loop has a {@link Connectable<M,E>} as
-     *     its event source, it will connect to it and will invoke the {@link Connection<M>} accept
-     *     method every time the model changes. This allows us to conditionally subscribe to
-     *     different sources based on the current state. If you provide a regular {@link
-     *     EventSource<E>}, it will be wrapped in a {@link Connectable} and that implementation will
-     *     subscribe to that event source only once when the loop is initialized.
+     * Returns a new {@link Builder} with the supplied {@link Connectable<M,E>}, and the same values
+     * as the current one for the other fields. NOTE: Invoking this method will replace the current
+     * event source with the supplied one. If a loop has a {@link Connectable<M,E>} as its event
+     * source, it will connect to it and will invoke the {@link Connection<M>} accept method every
+     * time the model changes. This allows us to conditionally subscribe to different sources based
+     * on the current state. If you provide a regular {@link EventSource<E>}, it will be wrapped in
+     * a {@link Connectable} and that implementation will subscribe to that event source only once
+     * when the loop is initialized.
      */
     @Nonnull
     Builder<M, E, F> eventSource(Connectable<M, E> eventSource);
 
     /**
-     * @return a new {@link Builder} with the supplied logger, and the same values as the current
-     *     one for the other fields.
+     * Returns a new {@link Builder} with the supplied logger, and the same values as the current
+     * one for the other fields.
      */
     @Nonnull
     Builder<M, E, F> logger(Logger<M, E, F> logger);
 
     /**
-     * @return a new {@link Builder} with the supplied event runner, and the same values as the
-     *     current one for the other fields.
+     * Returns a new {@link Builder} with the supplied event runner, and the same values as the
+     * current one for the other fields.
      */
     @Nonnull
     Builder<M, E, F> eventRunner(Producer<WorkRunner> eventRunner);
 
     /**
-     * @return a new {@link Builder} with the supplied effect runner, and the same values as the
-     *     current one for the other fields.
+     * Returns a new {@link Builder} with the supplied effect runner, and the same values as the
+     * current one for the other fields.
      */
     @Nonnull
     Builder<M, E, F> effectRunner(Producer<WorkRunner> effectRunner);

mobius-core/src/main/java/com/spotify/mobius/MobiusLoopController.java

@@ -94,6 +94,7 @@ public synchronized M getModel() {
     return currentState.onGetModel();
   }
 
+  @Override
   public void postUpdateView(final M model) {
     mainThreadRunner.post(
         new Runnable() {

mobius-core/src/main/java/com/spotify/mobius/runners/ExecutorServiceWorkRunner.java

@@ -37,6 +37,8 @@ public ExecutorServiceWorkRunner(ExecutorService service) {
     this.service = service;
   }
 
+  // we handle exceptions by wrapping the submitted runnables in something that will report them
+  @SuppressWarnings("FutureReturnValueIgnored")
   @Override
   public void post(Runnable runnable) {
     service.submit(runnable);