From 675080b2080da738cb6b02d65f61e7f876f9ebc2 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Fri, 24 Feb 2017 14:53:23 -0800 Subject: [PATCH] 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 | 1 + .../grpc/benchmarks/ByteBufInputStream.java | 1 + .../benchmarks/SocketAddressValidator.java | 2 ++ .../benchmarks/driver/LoadClientTest.java | 6 +++--- build.gradle | 19 ++++++++++++++++--- compiler/build.gradle | 6 ++++-- core/src/main/java/io/grpc/Status.java | 1 + .../main/java/io/grpc/internal/GrpcUtil.java | 3 +++ .../java/io/grpc/internal/ServerCallImpl.java | 1 + .../io/grpc/ConnectivityStateInfoTest.java | 3 ++- .../testing/integration/TestServiceImpl.java | 1 + .../grpc/testing/integration/ProxyTest.java | 2 +- .../grpc/netty/NettyChannelBuilderTest.java | 3 ++- .../io/grpc/netty/NettyStreamTestBase.java | 3 ++- .../grpc/netty/ProtocolNegotiatorsTest.java | 9 +++++---- .../grpc/okhttp/OutboundFlowController.java | 2 +- .../okhttp/OkHttpClientTransportTest.java | 3 +++ .../internal/DistinguishedNameParser.java | 1 + .../io/grpc/okhttp/internal/Platform.java | 3 ++- .../grpc/okhttp/internal/framed/Huffman.java | 1 + protobuf-lite/build.gradle | 3 ++- .../java/io/grpc/testing/DeadlineSubject.java | 2 +- thrift/build.gradle | 8 +++++++- .../io/grpc/thrift/ThriftInputStream.java | 1 + 24 files changed, 64 insertions(+), 21 deletions(-) diff --git a/.travis.yml b/.travis.yml index 30227741a5a..971833ddb47 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 91ba08bc576..b79191d304a 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 @@ /** * 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 b21b7305800..73e1b849cd2 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 cb164976477..afa5cc4703d 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 void testHistogramToStatsConversion() throws Exception { 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 def381d746e..a3c63b9e795 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 e52aaa23042..a8a8423d0ea 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 3f7dcd23f4d..8617a620396 100644 --- a/core/src/main/java/io/grpc/Status.java +++ b/core/src/main/java/io/grpc/Status.java @@ -214,6 +214,7 @@ public enum Code { 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 c078f17ca17..c532fe2c6d6 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 @@ private static Http2Error[] buildHttp2CodeMap() { } 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 2318738a1c6..fad2b700cd7 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 @@ public ServerStreamListenerImpl( 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 868cf4c6f70..cdc0cd8b348 100644 --- a/core/src/test/java/io/grpc/ConnectivityStateInfoTest.java +++ b/core/src/test/java/io/grpc/ConnectivityStateInfoTest.java @@ -88,6 +88,7 @@ public void equality() { 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 f23da93b1c7..dad7d127d91 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 @@ private StreamingOutputCallResponse toResponse() { /** * 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 f0a8a008d99..20e4fe0ee40 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 void run() { } // 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 f1871eb8677..c0a25f1fbd0 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 void failInvalidAuthority() { 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 d35b908593c..603916ce588 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 void shouldNotBeReadyForDataAfterWritingLargeMessage() throws IOException } 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 6d41f38108c..5650f0e3389 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 void tlsHandler_failsOnNullEngine() throws Exception { thrown.expect(NullPointerException.class); thrown.expectMessage("ssl"); - ProtocolNegotiators.serverTls(null); + Object unused = ProtocolNegotiators.serverTls(null); } @Test @@ -264,7 +264,7 @@ public boolean isLoggable(LogRecord record) { public void tls_failsOnNullSslContext() { thrown.expect(NullPointerException.class); - ProtocolNegotiators.tls(null, "authority"); + Object unused = ProtocolNegotiators.tls(null, "authority"); } @Test @@ -299,13 +299,14 @@ public void tls_invalidHost() throws SSLException { @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 a6f0aae0207..a11ae0ed75b 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 @@ private void writeStreams() { /** * 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 92380223958..70939b12ded 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 @@ void assertClosed() { } } + // 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 @@ void waitUntilStreamClosed() throws InterruptedException { } } + @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 dd6deb297ea..166b0979434 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 @@ private String quotedAV() { } // 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 e1d9c26236f..e755ae7477c 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 @@ private static Platform findPlatform() { 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 11370ddbb8f..5eb4a88c257 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 @@ private void buildTree() { } } + @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 9d55933ee5c..84a8390f440 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 61c668f9eab..8bc7f7e2a9f 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 void of(Deadline expected) { * 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 33e509ea91d..5ad24932606 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 bf2bf6708ea..8af49f2654f 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.TSerializer; /** InputStream for Thrift. */ +@SuppressWarnings("InputStreamSlowMultibyteRead") // TODO(ejona): would be good to fix final class ThriftInputStream extends InputStream implements Drainable, KnownLength { /**