all: Enable ErrorProne during compilation

ErrorProne provides static analysis for common issues, including
misused variables GuardedBy locks.

This increases build time by 60% for parallel builds and 30% for
non-parallel, so I've provided a way to disable the check. It is on by
default though and will be run in our CI environments.

24 files changed, 64 insertions, 21 deletions

diff --git a/.travis.yml b/.travis.yml
index 30227741a..971833ddb 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -22,6 +22,7 @@ before_install:
   - mkdir -p $HOME/.gradle
   - echo "checkstyle.ignoreFailures=false" >> $HOME/.gradle/gradle.properties
   - echo "failOnWarnings=true" >> $HOME/.gradle/gradle.properties
+  - echo "errorProne=true" >> $HOME/.gradle/gradle.properties
 
 install:
   - ./gradlew assemble generateTestProto install
diff --git a/benchmarks/src/main/java/io/grpc/benchmarks/ByteBufInputStream.java b/benchmarks/src/main/java/io/grpc/benchmarks/ByteBufInputStream.java
index 91ba08bc5..b79191d30 100644
--- a/benchmarks/src/main/java/io/grpc/benchmarks/ByteBufInputStream.java
+++ b/benchmarks/src/main/java/io/grpc/benchmarks/ByteBufInputStream.java
@@ -41,6 +41,7 @@ import java.io.OutputStream;
 /**
  * A {@link Drainable} {@code InputStream} that reads an {@link ByteBuf}.
  */
+@SuppressWarnings("InputStreamSlowMultibyteRead") // doesn't matter if slow. It'll throw
 public class ByteBufInputStream extends InputStream
     implements Drainable, KnownLength {
 
diff --git a/benchmarks/src/main/java/io/grpc/benchmarks/SocketAddressValidator.java b/benchmarks/src/main/java/io/grpc/benchmarks/SocketAddressValidator.java
index b21b73058..73e1b849c 100644
--- a/benchmarks/src/main/java/io/grpc/benchmarks/SocketAddressValidator.java
+++ b/benchmarks/src/main/java/io/grpc/benchmarks/SocketAddressValidator.java
@@ -31,12 +31,14 @@
 
 package io.grpc.benchmarks;
 
+import com.google.errorprone.annotations.Immutable;
 import java.net.InetSocketAddress;
 import java.net.SocketAddress;
 
 /**
  * Verifies whether or not the given {@link SocketAddress} is valid.
  */
+@Immutable
 public interface SocketAddressValidator {
   /**
    * Verifier for {@link InetSocketAddress}es.
diff --git a/benchmarks/src/test/java/io/grpc/benchmarks/driver/LoadClientTest.java b/benchmarks/src/test/java/io/grpc/benchmarks/driver/LoadClientTest.java
index cb1649764..afa5cc470 100644
--- a/benchmarks/src/test/java/io/grpc/benchmarks/driver/LoadClientTest.java
+++ b/benchmarks/src/test/java/io/grpc/benchmarks/driver/LoadClientTest.java
@@ -77,9 +77,9 @@ public class LoadClientTest {
 
     Stats.ClientStats stats = loadClient.getStats();
 
-    assertEquals(1.0, stats.getLatencies().getMinSeen());
-    assertEquals(1000.0, stats.getLatencies().getMaxSeen());
-    assertEquals(10.0, stats.getLatencies().getCount());
+    assertEquals(1.0, stats.getLatencies().getMinSeen(), 0.0);
+    assertEquals(1000.0, stats.getLatencies().getMaxSeen(), 0.0);
+    assertEquals(10.0, stats.getLatencies().getCount(), 0.0);
 
     double base = 0;
     double logBase = 1;
diff --git a/build.gradle b/build.gradle
index def381d74..a3c63b9e7 100644
--- a/build.gradle
+++ b/build.gradle
@@ -2,10 +2,14 @@ buildscript {
   repositories {
     mavenCentral()
     mavenLocal()
+    maven {
+      url "https://plugins.gradle.org/m2/"
+    }
   }
   dependencies {
     classpath 'com.google.gradle:osdetector-gradle-plugin:1.4.0'
-    classpath "ru.vyarus:gradle-animalsniffer-plugin:1.2.0"
+    classpath 'ru.vyarus:gradle-animalsniffer-plugin:1.2.0'
+    classpath 'net.ltgt.gradle:gradle-errorprone-plugin:0.0.9'
   }
 }
 
@@ -20,6 +24,9 @@ subprojects {
     apply plugin: "com.google.osdetector"
     // The plugin only has an effect if a signature is specified
     apply plugin: "ru.vyarus.animalsniffer"
+    if (!rootProject.hasProperty('errorProne') || rootProject.errorProne.toBoolean()) {
+      apply plugin: "net.ltgt.errorprone"
+    }
 
     group = "io.grpc"
     version = "1.2.0-SNAPSHOT" // CURRENT_GRPC_VERSION
@@ -33,7 +40,7 @@ subprojects {
     }
 
     [compileJava, compileTestJava].each() {
-        it.options.compilerArgs += ["-Xlint:all", "-Xlint:-options"]
+        it.options.compilerArgs += ["-Xlint:all", "-Xlint:-options", "-Xlint:-path"]
         it.options.encoding = "UTF-8"
         if (rootProject.hasProperty('failOnWarnings') && rootProject.failOnWarnings.toBoolean()) {
             it.options.compilerArgs += ["-Werror"]
@@ -136,7 +143,9 @@ subprojects {
 
           [compileJava, compileTestJava].each() {
             // Protobuf-generated code produces some warnings.
-            it.options.compilerArgs += ["-Xlint:-cast"]
+            // https://github.com/google/protobuf/issues/2718
+            it.options.compilerArgs += ["-Xlint:-cast", "-Xep:MissingOverride:OFF",
+                "-Xep:ReferenceEquality:OFF", "-Xep:FunctionalInterfaceClash:OFF"]
           }
         }
 
@@ -198,6 +207,10 @@ subprojects {
 
         // Configuration for modules that use Netty tcnative (for OpenSSL).
         tcnative libraries.netty_tcnative
+
+        // The ErrorProne plugin defaults to the latest, which would break our
+        // build if error prone releases a new version with a new check
+        errorprone 'com.google.errorprone:error_prone_core:2.0.15'
     }
 
     signing {
diff --git a/compiler/build.gradle b/compiler/build.gradle
index e52aaa230..a8a8423d0 100644
--- a/compiler/build.gradle
+++ b/compiler/build.gradle
@@ -142,12 +142,14 @@ sourceSets {
 }
 
 compileTestJava {
-  options.compilerArgs += ["-Xlint:-cast"]
+  options.compilerArgs += ["-Xlint:-cast", "-Xep:MissingOverride:OFF",
+      "-Xep:ReferenceEquality:OFF", "-Xep:FunctionalInterfaceClash:OFF"]
 }
 
 compileTestLiteJava {
   // Protobuf-generated Lite produces quite a few warnings.
-  options.compilerArgs += ["-Xlint:-rawtypes", "-Xlint:-unchecked"]
+  options.compilerArgs += ["-Xlint:-rawtypes", "-Xlint:-unchecked",
+      "-Xep:MissingOverride:OFF", "-Xep:ReferenceEquality:OFF"]
 }
 
 compileTestNanoJava {
diff --git a/core/src/main/java/io/grpc/Status.java b/core/src/main/java/io/grpc/Status.java
index 3f7dcd23f..8617a6203 100644
--- a/core/src/main/java/io/grpc/Status.java
+++ b/core/src/main/java/io/grpc/Status.java
@@ -214,6 +214,7 @@ public final class Status {
     UNAUTHENTICATED(16);
 
     private final int value;
+    @SuppressWarnings("ImmutableEnumChecker") // we make sure the byte[] can't be modified
     private final byte[] valueAscii;
 
     private Code(int value) {
diff --git a/core/src/main/java/io/grpc/internal/GrpcUtil.java b/core/src/main/java/io/grpc/internal/GrpcUtil.java
index c078f17ca..c532fe2c6 100644
--- a/core/src/main/java/io/grpc/internal/GrpcUtil.java
+++ b/core/src/main/java/io/grpc/internal/GrpcUtil.java
@@ -238,6 +238,9 @@ public final class GrpcUtil {
     }
 
     private final int code;
+    // Status is not guaranteed to be deeply immutable. Don't care though, since that's only true
+    // when there are exceptions in the Status, which is not true here.
+    @SuppressWarnings("ImmutableEnumChecker")
     private final Status status;
 
     Http2Error(int code, Status status) {
diff --git a/core/src/main/java/io/grpc/internal/ServerCallImpl.java b/core/src/main/java/io/grpc/internal/ServerCallImpl.java
index 2318738a1..fad2b700c 100644
--- a/core/src/main/java/io/grpc/internal/ServerCallImpl.java
+++ b/core/src/main/java/io/grpc/internal/ServerCallImpl.java
@@ -220,6 +220,7 @@ final class ServerCallImpl<ReqT, RespT> extends ServerCall<ReqT, RespT> {
       this.statsTraceCtx = checkNotNull(statsTraceCtx, "statsTraceCtx");
     }
 
+    @SuppressWarnings("Finally") // The code avoids suppressing the exception thrown from try
     @Override
     public void messageRead(final InputStream message) {
       Throwable t = null;
diff --git a/core/src/test/java/io/grpc/ConnectivityStateInfoTest.java b/core/src/test/java/io/grpc/ConnectivityStateInfoTest.java
index 868cf4c6f..cdc0cd8b3 100644
--- a/core/src/test/java/io/grpc/ConnectivityStateInfoTest.java
+++ b/core/src/test/java/io/grpc/ConnectivityStateInfoTest.java
@@ -88,6 +88,7 @@ public class ConnectivityStateInfoTest {
     assertNotEquals(info4, info6);
 
     assertFalse(info1.equals(null));
-    assertFalse(info1.equals(this));
+    // Extra cast to avoid ErrorProne EqualsIncompatibleType failure
+    assertFalse(((Object) info1).equals(this));
   }
 }
diff --git a/interop-testing/src/main/java/io/grpc/testing/integration/TestServiceImpl.java b/interop-testing/src/main/java/io/grpc/testing/integration/TestServiceImpl.java
index f23da93b1..dad7d127d 100644
--- a/interop-testing/src/main/java/io/grpc/testing/integration/TestServiceImpl.java
+++ b/interop-testing/src/main/java/io/grpc/testing/integration/TestServiceImpl.java
@@ -449,6 +449,7 @@ public class TestServiceImpl extends TestServiceGrpc.TestServiceImplBase {
   /**
    * Creates a buffer with data read from a file.
    */
+  @SuppressWarnings("Finally") // Not concerned about suppression; expected to be exceedingly rare
   private ByteString createBufferFromFile(String fileClassPath) {
     ByteString buffer = ByteString.EMPTY;
     InputStream inputStream = getClass().getResourceAsStream(fileClassPath);
diff --git a/interop-testing/src/test/java/io/grpc/testing/integration/ProxyTest.java b/interop-testing/src/test/java/io/grpc/testing/integration/ProxyTest.java
index f0a8a008d..20e4fe0ee 100644
--- a/interop-testing/src/test/java/io/grpc/testing/integration/ProxyTest.java
+++ b/interop-testing/src/test/java/io/grpc/testing/integration/ProxyTest.java
@@ -202,7 +202,7 @@ public class ProxyTest {
   }
 
   // server with echo and streaming modes
-  private class Server implements Runnable {
+  private static class Server implements Runnable {
     private ServerSocket server;
     private Socket rcv;
     private boolean shutDown;
diff --git a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java
index f1871eb86..c0a25f1fb 100644
--- a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java
+++ b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java
@@ -84,7 +84,8 @@ public class NettyChannelBuilderTest {
     thrown.expect(IllegalArgumentException.class);
     thrown.expectMessage("Invalid host or port");
 
-    NettyChannelBuilder.forAddress(new InetSocketAddress("invalid_authority", 1234));
+    Object unused =
+        NettyChannelBuilder.forAddress(new InetSocketAddress("invalid_authority", 1234));
   }
 
   @Test
diff --git a/netty/src/test/java/io/grpc/netty/NettyStreamTestBase.java b/netty/src/test/java/io/grpc/netty/NettyStreamTestBase.java
index d35b90859..603916ce5 100644
--- a/netty/src/test/java/io/grpc/netty/NettyStreamTestBase.java
+++ b/netty/src/test/java/io/grpc/netty/NettyStreamTestBase.java
@@ -31,6 +31,7 @@
 
 package io.grpc.netty;
 
+import static com.google.common.base.Charsets.US_ASCII;
 import static io.grpc.netty.NettyTestUtil.messageFrame;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -199,7 +200,7 @@ public abstract class NettyStreamTestBase<T extends Stream> {
   }
 
   protected byte[] smallMessage() {
-    return MESSAGE.getBytes();
+    return MESSAGE.getBytes(US_ASCII);
   }
 
   protected byte[] largeMessage() {
diff --git a/netty/src/test/java/io/grpc/netty/ProtocolNegotiatorsTest.java b/netty/src/test/java/io/grpc/netty/ProtocolNegotiatorsTest.java
index 6d41f3810..5650f0e33 100644
--- a/netty/src/test/java/io/grpc/netty/ProtocolNegotiatorsTest.java
+++ b/netty/src/test/java/io/grpc/netty/ProtocolNegotiatorsTest.java
@@ -113,7 +113,7 @@ public class ProtocolNegotiatorsTest {
     thrown.expect(NullPointerException.class);
     thrown.expectMessage("ssl");
 
-    ProtocolNegotiators.serverTls(null);
+    Object unused = ProtocolNegotiators.serverTls(null);
   }
 
   @Test
@@ -264,7 +264,7 @@ public class ProtocolNegotiatorsTest {
   public void tls_failsOnNullSslContext() {
     thrown.expect(NullPointerException.class);
 
-    ProtocolNegotiators.tls(null, "authority");
+    Object unused = ProtocolNegotiators.tls(null, "authority");
   }
 
   @Test
@@ -299,13 +299,14 @@ public class ProtocolNegotiatorsTest {
   @Test
   public void httpProxy_nullAddressNpe() throws Exception {
     thrown.expect(NullPointerException.class);
-    ProtocolNegotiators.httpProxy(null, "user", "pass", ProtocolNegotiators.plaintext());
+    Object unused =
+        ProtocolNegotiators.httpProxy(null, "user", "pass", ProtocolNegotiators.plaintext());
   }
 
   @Test
   public void httpProxy_nullNegotiatorNpe() throws Exception {
     thrown.expect(NullPointerException.class);
-    ProtocolNegotiators.httpProxy(
+    Object unused = ProtocolNegotiators.httpProxy(
         InetSocketAddress.createUnresolved("localhost", 80), "user", "pass", null);
   }
 
diff --git a/okhttp/src/main/java/io/grpc/okhttp/OutboundFlowController.java b/okhttp/src/main/java/io/grpc/okhttp/OutboundFlowController.java
index a6f0aae02..a11ae0ed7 100644
--- a/okhttp/src/main/java/io/grpc/okhttp/OutboundFlowController.java
+++ b/okhttp/src/main/java/io/grpc/okhttp/OutboundFlowController.java
@@ -217,7 +217,7 @@ class OutboundFlowController {
   /**
    * Simple status that keeps track of the number of writes performed.
    */
-  private final class WriteStatus {
+  private static final class WriteStatus {
     int numWrites;
 
     void incrementNumWrites() {
diff --git a/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java b/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java
index 923802239..70939b12d 100644
--- a/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java
+++ b/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java
@@ -1605,6 +1605,8 @@ public class OkHttpClientTransportTest {
       }
     }
 
+    // The wait is safe; nextFrame is called in a loop and can have spurious wakeups
+    @SuppressWarnings("WaitNotInLoop")
     @Override
     public boolean nextFrame(Handler handler) throws IOException {
       Result result;
@@ -1692,6 +1694,7 @@ public class OkHttpClientTransportTest {
       }
     }
 
+    @SuppressWarnings("Finally") // We don't care about suppressed exceptions in the test
     static String getContent(InputStream message) {
       BufferedReader br = new BufferedReader(new InputStreamReader(message, UTF_8));
       try {
diff --git a/okhttp/third_party/okhttp/java/io/grpc/okhttp/internal/DistinguishedNameParser.java b/okhttp/third_party/okhttp/java/io/grpc/okhttp/internal/DistinguishedNameParser.java
index dd6deb297..166b09794 100644
--- a/okhttp/third_party/okhttp/java/io/grpc/okhttp/internal/DistinguishedNameParser.java
+++ b/okhttp/third_party/okhttp/java/io/grpc/okhttp/internal/DistinguishedNameParser.java
@@ -137,6 +137,7 @@ final class DistinguishedNameParser {
   }
 
   // gets hex string attribute value: "#" hexstring
+  @SuppressWarnings("NarrowingCompoundAssignment")
   private String hexAV() {
     if (pos + 4 >= length) {
       // encoded byte array  must be not less then 4 c
diff --git a/okhttp/third_party/okhttp/java/io/grpc/okhttp/internal/Platform.java b/okhttp/third_party/okhttp/java/io/grpc/okhttp/internal/Platform.java
index e1d9c2623..e755ae747 100644
--- a/okhttp/third_party/okhttp/java/io/grpc/okhttp/internal/Platform.java
+++ b/okhttp/third_party/okhttp/java/io/grpc/okhttp/internal/Platform.java
@@ -191,7 +191,8 @@ public class Platform {
   private static Provider getAppEngineProvider() {
     try {
       // Forcibly load conscrypt as it is unlikely to be an installed provider on AppEngine
-      return (Provider) Class.forName("org.conscrypt.OpenSSLProvider").newInstance();
+      return (Provider) Class.forName("org.conscrypt.OpenSSLProvider")
+          .getConstructor().newInstance();
     } catch (Throwable t) {
       throw new RuntimeException("Unable to load conscrypt security provider", t);
     }
diff --git a/okhttp/third_party/okhttp/java/io/grpc/okhttp/internal/framed/Huffman.java b/okhttp/third_party/okhttp/java/io/grpc/okhttp/internal/framed/Huffman.java
index 11370ddbb..5eb4a88c2 100644
--- a/okhttp/third_party/okhttp/java/io/grpc/okhttp/internal/framed/Huffman.java
+++ b/okhttp/third_party/okhttp/java/io/grpc/okhttp/internal/framed/Huffman.java
@@ -171,6 +171,7 @@ class Huffman {
     }
   }
 
+  @SuppressWarnings("NarrowingCompoundAssignment")
   private void addCode(int sym, int code, byte len) {
     Node terminal = new Node(sym, len);
 
diff --git a/protobuf-lite/build.gradle b/protobuf-lite/build.gradle
index 9d55933ee..84a8390f4 100644
--- a/protobuf-lite/build.gradle
+++ b/protobuf-lite/build.gradle
@@ -24,7 +24,8 @@ dependencies {
 
 compileTestJava {
   // Protobuf-generated Lite produces quite a few warnings.
-  options.compilerArgs += ["-Xlint:-rawtypes", "-Xlint:-unchecked", "-Xlint:-fallthrough"]
+  options.compilerArgs += ["-Xlint:-rawtypes", "-Xlint:-unchecked", "-Xlint:-fallthrough",
+        "-Xep:MissingOverride:OFF", "-Xep:ReferenceEquality:OFF"]
 }
 
 protobuf {
diff --git a/testing/src/main/java/io/grpc/testing/DeadlineSubject.java b/testing/src/main/java/io/grpc/testing/DeadlineSubject.java
index 61c668f9e..8bc7f7e2a 100644
--- a/testing/src/main/java/io/grpc/testing/DeadlineSubject.java
+++ b/testing/src/main/java/io/grpc/testing/DeadlineSubject.java
@@ -92,7 +92,7 @@ public final class DeadlineSubject extends ComparableSubject<DeadlineSubject, De
    * A partially specified proposition about an approximate relationship to a {@code deadline}
    * subject using a tolerance.
    */
-  public abstract class TolerantDeadlineComparison {
+  public abstract static class TolerantDeadlineComparison {
 
     private TolerantDeadlineComparison() {}
 
diff --git a/thrift/build.gradle b/thrift/build.gradle
index 33e509ea9..5ad249326 100644
--- a/thrift/build.gradle
+++ b/thrift/build.gradle
@@ -22,8 +22,14 @@ project.sourceSets {
   }
 }
 
+compileTestJava {
+  // Thrift-generated code produces some warnings.
+  options.compilerArgs += ["-Xep:MissingOverride:OFF",
+    "-Xep:NonOverridingEquals:OFF", "-Xep:TypeParameterUnusedInFormals:OFF"]
+}
+
 idea {
     module {
         sourceDirs += file("${projectDir}/src/generated/test/java");
     }
-}
\ No newline at end of file
+}
diff --git a/thrift/src/main/java/io/grpc/thrift/ThriftInputStream.java b/thrift/src/main/java/io/grpc/thrift/ThriftInputStream.java
index bf2bf6708..8af49f265 100644
--- a/thrift/src/main/java/io/grpc/thrift/ThriftInputStream.java
+++ b/thrift/src/main/java/io/grpc/thrift/ThriftInputStream.java
@@ -45,6 +45,7 @@ import org.apache.thrift.TException;
 import org.apache.thrift.TSerializer;
 
 /** InputStream for Thrift. */
+@SuppressWarnings("InputStreamSlowMultibyteRead") // TODO(ejona): would be good to fix
 final class ThriftInputStream extends InputStream implements Drainable, KnownLength {
 
   /**