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.

.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

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 {
 

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.

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;

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 {

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 {

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) {

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) {

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;

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));
   }
 }

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);

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;

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

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() {

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);
   }
 

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() {

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 {

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

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);
     }

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);
 

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 {

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() {}
 

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");
     }
-}
+}

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 {
 
   /**