Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Upgrade error_prone_annotations to 2.3.4 #6574

Merged
merged 30 commits into from Jan 3, 2020
Merged

Upgrade error_prone_annotations to 2.3.4 #6574

Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
0417f50
error_prone_annotation 2.3.4
suztomo Dec 27, 2019
c534c13
error_prone_annotations not from perfmark
suztomo Dec 27, 2019
f4e6c09
error prone core 2.3.3
suztomo Dec 27, 2019
484531b
errorprone core 2.3.4
suztomo Dec 27, 2019
e7f9c6a
Declaring errorprone to replace perfmark's dependency
suztomo Dec 27, 2019
d6b4454
Merge remote-tracking branch 'origin/master' into error_prone_annotat…
suztomo Dec 30, 2019
f7fa412
Merge remote-tracking branch 'origin/master' into error_prone_annotat…
suztomo Dec 30, 2019
ef4264e
IO_GRPC_GRPC_JAVA_ARTIFACTS
suztomo Dec 30, 2019
9e1c3a5
SuppressWarnings("UnnecessaryAnonymousClass")
suztomo Dec 30, 2019
51175e4
FormatMethod
suztomo Dec 30, 2019
f26fe6d
grpc-netty: @SuppressWarnings("UnnecessaryAnonymousClass")
suztomo Dec 30, 2019
61fb9f2
Removed unused argument
suztomo Dec 30, 2019
af940dc
grpc-interop-testing: SuppressWarnings("UnnecessaryAnonymousClass")
suztomo Dec 30, 2019
5e13810
SuppressWarnings("LiteProtoToString")
suztomo Dec 31, 2019
ab08958
SameNameButDifferent
suztomo Dec 31, 2019
6c98862
SuppressWarnings("UnnecessaryAnonymousClass") at build.gradle
suztomo Dec 31, 2019
5a1fa57
ManagedChannelImplTest for SameNameButDifferent
suztomo Dec 31, 2019
6c20c9d
Revert unnecessary change
suztomo Dec 31, 2019
211c2c2
ManagedChannelImplTest2 for SameNameButDifferent
suztomo Dec 31, 2019
7488e21
compileJmhJava with errorprone config
suztomo Dec 31, 2019
5bd6d01
OkHttpClientTransportTest for SameNameButDifferent
suztomo Dec 31, 2019
6fb9bac
SuppressWarning(PreferJavaTimeOverload)
suztomo Dec 31, 2019
fb4d7a6
SuppressWarnings("PreferJavaTimeOverload") at build.gradle
suztomo Dec 31, 2019
028bb6e
SuppressWarnings("GuardedBy")
suztomo Dec 31, 2019
a2f4a83
BinaryLogProviderTest for SameNameButDifferent
suztomo Dec 31, 2019
0a2a76b
prefer our version to perfmark's 2.3.3
suztomo Dec 31, 2019
749b67f
Fixed 'FormatMethod from an indirect dependency'
suztomo Jan 2, 2020
ef42e8b
SuppressWarnings("GuardedBy")
suztomo Jan 2, 2020
dbcdc4c
Merge remote-tracking branch 'origin/master' into error_prone_annotat…
suztomo Jan 3, 2020
10ec0cf
FakeNameResolverFactory2
suztomo Jan 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion android/build.gradle
View file
Open in desktop
Expand Up @@ -44,7 +44,7 @@ repositories {
}

dependencies {
errorprone 'com.google.errorprone:error_prone_core:2.3.3'
errorprone 'com.google.errorprone:error_prone_core:2.3.4'
errorproneJavac 'com.google.errorprone:javac:9+181-r4173-1'

implementation 'io.grpc:grpc-core:1.27.0-SNAPSHOT' // CURRENT_GRPC_VERSION
Expand Down
12 changes: 10 additions & 2 deletions build.gradle
View file
Open in desktop
Expand Up @@ -113,7 +113,7 @@ subprojects {
libraries = [
android_annotations: "com.google.android:annotations:4.1.1.4",
animalsniffer_annotations: "org.codehaus.mojo:animal-sniffer-annotations:1.18",
errorprone: "com.google.errorprone:error_prone_annotations:2.3.3",
errorprone: "com.google.errorprone:error_prone_annotations:2.3.4",
gson: "com.google.code.gson:gson:2.8.6",
guava: "com.google.guava:guava:${guavaVersion}",
hpack: 'com.twitter:hpack:0.10.1',
Expand Down Expand Up @@ -263,7 +263,7 @@ subprojects {

if (rootProject.properties.get('errorProne', true)) {
dependencies {
errorprone 'com.google.errorprone:error_prone_core:2.3.3'
errorprone 'com.google.errorprone:error_prone_core:2.3.4'
errorproneJavac 'com.google.errorprone:javac:9+181-r4173-1'

annotationProcessor 'com.google.guava:guava-beta-checker:1.0'
Expand All @@ -279,9 +279,17 @@ subprojects {
}
}

compileJava {
// This project targets Java 7 (no method references)
options.errorprone.check("UnnecessaryAnonymousClass", CheckSeverity.OFF)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suppressing UnnecessaryAnonymousClass as project-level suppression, because grpc-java targets Java 7 source.

// This project targets Java 7 (no time.Duration class)
options.errorprone.check("PreferJavaTimeOverload", CheckSeverity.OFF)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without PreferJavaTimeOverload suppression, the build for Java 11 fails:

> Task :grpc-context:compileJava FAILED
/Users/suztomo/grpc-java/context/src/main/java/io/grpc/Deadline.java:177: warning: [PreferJavaTimeOverload] Prefer using java.time-based APIs when available. Note that this checker does not and cannot guarantee that the overloads have equivalent semantics, but that is generally the case with overloaded methods.
    return unit.convert(deadlineNanos - nowNanos, TimeUnit.NANOSECONDS);
                       ^
    (see https://errorprone.info/bugpattern/PreferJavaTimeOverload)
  Did you mean 'return unit.convert(Duration.ofNanos(deadlineNanos - nowNanos));'?
error: warnings found and -Werror specified
1 error
1 warning

Setting this suppression at project level (rather than annotation) because whole grpc-java project targets Java 7, which does not have java.time.Duration.

}
compileTestJava {
// LinkedList doesn't hurt much in tests and has lots of usages
options.errorprone.check("JdkObsolete", CheckSeverity.OFF)
options.errorprone.check("UnnecessaryAnonymousClass", CheckSeverity.OFF)
options.errorprone.check("PreferJavaTimeOverload", CheckSeverity.OFF)
}
}

Expand Down
14 changes: 13 additions & 1 deletion core/build.gradle
View file
Open in desktop
Expand Up @@ -16,7 +16,10 @@ dependencies {
compile project(':grpc-api'),
libraries.gson,
libraries.android_annotations,
libraries.perfmark
libraries.errorprone // prefer our version to perfmark's 2.3.3
compile (libraries.perfmark) {
exclude group: 'com.google.errorprone', module: 'error_prone_annotations'
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gradle complained perfmark's errorprone annotation 2.3.3.

https://travis-ci.org/grpc/grpc-java/jobs/630091143?utm_medium=notification&utm_source=github_status

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. You did the appropriate thing here, although we normally put a comment saying we are using version X instead of version Y (so that in the future we can tell if the exclude is old and should be removed).

Our build enforces dependency convergence because I've not found a better approach to detecting requireUpperBoundDeps violations from within Gradle (because they aren't a problem in Gradle...).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated comment as "prefer our version to perfmark's 2.3.3".

compile (libraries.opencensus_api) {
// prefer our own versions instead of opencensus-api's dependency
exclude group: 'com.google.code.findbugs', module: 'jsr305'
Expand Down Expand Up @@ -56,3 +59,12 @@ animalsniffer {
sourceSets.test
]
}

import net.ltgt.gradle.errorprone.CheckSeverity

plugins.withId("java") {
compileJmhJava {
// This project targets Java 7 (no method references)
options.errorprone.check("UnnecessaryAnonymousClass", CheckSeverity.OFF)
}
}
Comment on lines +65 to +70
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grpc-core has special task compileJmhJava. Without this configuration, the build failed:

> Task :grpc-core:compileJmhJava FAILED
/Users/suztomo/grpc-java/core/src/jmh/java/io/grpc/internal/SerializingExecutorBenchmark.java:58: warning: [UnnecessaryAnonymousClass] Implementing a functional interface is unnecessary; prefer to implement the functional interface method directly and use a method reference instead.
  private final Runnable phaserRunnable = new Runnable() {
                         ^
    (see https://errorprone.info/bugpattern/UnnecessaryAnonymousClass)
  Did you mean 'private  void phaserRunnable() {'?
error: warnings found and -Werror specified
1 error
1 warning

213 changes: 127 additions & 86 deletions core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java
View file
Open in desktop

Large diffs are not rendered by default.

220 changes: 131 additions & 89 deletions core/src/test/java/io/grpc/internal/ManagedChannelImplTest2.java
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion cronet/build.gradle
View file
Open in desktop
Expand Up @@ -51,7 +51,7 @@ android {
}

dependencies {
errorprone 'com.google.errorprone:error_prone_core:2.3.3'
errorprone 'com.google.errorprone:error_prone_core:2.3.4'
errorproneJavac 'com.google.errorprone:javac:9+181-r4173-1'

implementation 'io.grpc:grpc-core:1.27.0-SNAPSHOT' // CURRENT_GRPC_VERSION
Expand Down
1 change: 1 addition & 0 deletions cronet/src/test/java/io/grpc/cronet/CronetClientStreamTest.java
View file
Open in desktop
Expand Up @@ -94,6 +94,7 @@ void setStream(CronetClientStream stream) {
}

@Override
@SuppressWarnings("GuardedBy")
public void run() {
assertTrue(stream != null);
stream.transportState().start(factory);
Expand Down
2 changes: 1 addition & 1 deletion examples/pom.xml
View file
Open in desktop
Expand Up @@ -65,7 +65,7 @@
<dependency>
<groupId>com.google.errorprone</groupId>
<artifactId>error_prone_annotations</artifactId>
<version>2.3.3</version> <!-- prefer to use 2.3.3 or later -->
<version>2.3.4</version> <!-- prefer to use 2.3.3 or later -->
</dependency>
<dependency>
<groupId>junit</groupId>
Expand Down
1 change: 1 addition & 0 deletions interop-testing/src/main/java/io/grpc/testing/integration/Util.java
View file
Open in desktop
Expand Up @@ -37,6 +37,7 @@ public class Util {
= Metadata.Key.of("x-grpc-test-echo-trailing-bin", Metadata.BINARY_BYTE_MARSHALLER);

/** Assert that two messages are equal, producing a useful message if not. */
@SuppressWarnings("LiteProtoToString")
public static void assertEquals(MessageLite expected, MessageLite actual) {
if (expected == null || actual == null) {
Assert.assertEquals(expected, actual);
Expand Down
1 change: 1 addition & 0 deletions okhttp/BUILD.bazel
View file
Open in desktop
Expand Up @@ -12,6 +12,7 @@ java_library(
"//api",
"//core:internal",
"@com_google_code_findbugs_jsr305//jar",
"@com_google_errorprone_error_prone_annotations//jar",
"@com_google_guava_guava//jar",
"@com_google_j2objc_j2objc_annotations//jar",
"@com_squareup_okhttp_okhttp//jar",
Expand Down
6 changes: 6 additions & 0 deletions okhttp/src/test/java/io/grpc/okhttp/OkHttpClientStreamTest.java
View file
Open in desktop
Expand Up @@ -127,6 +127,7 @@ public void closed(
}

@Test
@SuppressWarnings("GuardedBy")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this suppression, the build fails.
https://travis-ci.org/grpc/grpc-java/jobs/631198698?utm_medium=notification&utm_source=github_status

#6578 will take care of the root cause.

public void cancel_started() {
stream.start(new BaseClientStreamListener());
stream.transportState().start(1234);
Expand All @@ -145,6 +146,7 @@ public Void answer(InvocationOnMock invocation) throws Throwable {
}

@Test
@SuppressWarnings("GuardedBy")
public void start_alreadyCancelled() {
stream.start(new BaseClientStreamListener());
stream.cancel(Status.CANCELLED);
Expand All @@ -155,6 +157,7 @@ public void start_alreadyCancelled() {
}

@Test
@SuppressWarnings("GuardedBy")
public void start_userAgentRemoved() throws IOException {
Metadata metaData = new Metadata();
metaData.put(GrpcUtil.USER_AGENT_KEY, "misbehaving-application");
Expand All @@ -171,6 +174,7 @@ public void start_userAgentRemoved() throws IOException {
}

@Test
@SuppressWarnings("GuardedBy")
public void start_headerFieldOrder() throws IOException {
Metadata metaData = new Metadata();
metaData.put(GrpcUtil.USER_AGENT_KEY, "misbehaving-application");
Expand All @@ -194,6 +198,7 @@ public void start_headerFieldOrder() throws IOException {
}

@Test
@SuppressWarnings("GuardedBy")
public void start_headerPlaintext() throws IOException {
Metadata metaData = new Metadata();
metaData.put(GrpcUtil.USER_AGENT_KEY, "misbehaving-application");
Expand All @@ -218,6 +223,7 @@ public void start_headerPlaintext() throws IOException {
}

@Test
@SuppressWarnings("GuardedBy")
public void getUnaryRequest() throws IOException {
MethodDescriptor<?, ?> getMethod = MethodDescriptor.<Void, Void>newBuilder()
.setType(MethodDescriptor.MethodType.UNARY)
Expand Down
2 changes: 1 addition & 1 deletion okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java
View file
Open in desktop
Expand Up @@ -2127,7 +2127,7 @@ 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 {
public boolean nextFrame(FrameReader.Handler handler) throws IOException {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SameNameButDifferent.

Result result;
try {
result = nextResults.take();
Expand Down
5 changes: 4 additions & 1 deletion okhttp/third_party/okhttp/main/java/io/grpc/okhttp/internal/framed/Http2.java
View file
Open in desktop
Expand Up @@ -19,6 +19,7 @@

package io.grpc.okhttp.internal.framed;

import com.google.errorprone.annotations.FormatMethod;
import io.grpc.okhttp.internal.Protocol;
import java.io.IOException;
import java.util.List;
Expand Down Expand Up @@ -361,7 +362,7 @@ private void readWindowUpdate(
throws IOException {
if (length != 4) throw ioException("TYPE_WINDOW_UPDATE length !=4: %s", length);
long increment = (source.readInt() & 0x7fffffffL);
if (increment == 0) throw ioException("windowSizeIncrement was 0", increment);
if (increment == 0) throw ioException("windowSizeIncrement was 0");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That @FormatMethod annotation detected this unused argument.

> Task :grpc-okhttp:compileJava
/home/travis/build/grpc/grpc-java/okhttp/third_party/okhttp/main/java/io/grpc/okhttp/internal/framed/Http2.java:365: error: [FormatStringAnnotation] extra format arguments: used 0, provided 1
      if (increment == 0) throw ioException("windowSizeIncrement was 0", increment);
                                           ^
    (see https://errorprone.info/bugpattern/FormatStringAnnotation)
1 error
> Task :grpc-okhttp:compileJava FAILED

handler.windowUpdate(streamId, increment);
}

Expand Down Expand Up @@ -586,10 +587,12 @@ void frameHeader(int streamId, int length, byte type, byte flags) throws IOExcep
}
}

@FormatMethod
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build failed by https://errorprone.info/bugpattern/AnnotateFormatMethod

private static IllegalArgumentException illegalArgument(String message, Object... args) {
throw new IllegalArgumentException(format(message, args));
}

@FormatMethod
private static IOException ioException(String message, Object... args) throws IOException {
throw new IOException(format(message, args));
}
Expand Down
6 changes: 3 additions & 3 deletions repositories.bzl
View file
Open in desktop
Expand Up @@ -17,7 +17,7 @@ IO_GRPC_GRPC_JAVA_ARTIFACTS = [
"com.google.auth:google-auth-library-oauth2-http:0.19.0",
"com.google.code.findbugs:jsr305:3.0.2",
"com.google.code.gson:gson:jar:2.8.6",
"com.google.errorprone:error_prone_annotations:2.3.3",
"com.google.errorprone:error_prone_annotations:2.3.4",
"com.google.guava:failureaccess:1.0.1",
"com.google.guava:guava:28.1-android",
"com.google.j2objc:j2objc-annotations:1.3",
Expand Down Expand Up @@ -226,9 +226,9 @@ def com_google_code_gson_gson():
def com_google_errorprone_error_prone_annotations():
jvm_maven_import_external(
name = "com_google_errorprone_error_prone_annotations",
artifact = "com.google.errorprone:error_prone_annotations:2.3.3",
artifact = "com.google.errorprone:error_prone_annotations:2.3.4",
server_urls = ["https://repo.maven.apache.org/maven2/"],
artifact_sha256 = "ec59f1b702d9afc09e8c3929f5c42777dec623a6ea2731ac694332c7d7680f5a",
artifact_sha256 = "baf7d6ea97ce606c53e11b6854ba5f2ce7ef5c24dddf0afa18d1260bd25b002c",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

suztomo@suxtomo24:~/grpc-java$ shasum -a 256 ~/Downloads/error_prone_annotations-2.3.4.jar 
baf7d6ea97ce606c53e11b6854ba5f2ce7ef5c24dddf0afa18d1260bd25b002c  /usr/local/google/home/suztomo/Downloads/error_prone_annotations-2.3.4.jar

licenses = ["notice"], # Apache 2.0
)

Expand Down
5 changes: 2 additions & 3 deletions services/src/main/java/io/grpc/services/BinlogHelper.java
View file
Open in desktop
Expand Up @@ -46,7 +46,6 @@
import io.grpc.MethodDescriptor;
import io.grpc.MethodDescriptor.Marshaller;
import io.grpc.ServerCall;
import io.grpc.ServerCall.Listener;
import io.grpc.ServerCallHandler;
import io.grpc.ServerInterceptor;
import io.grpc.Status;
Expand Down Expand Up @@ -408,7 +407,7 @@ public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall(

return new SimpleForwardingClientCall<ReqT, RespT>(next.newCall(method, callOptions)) {
@Override
public void start(final Listener<RespT> responseListener, Metadata headers) {
public void start(final ClientCall.Listener<RespT> responseListener, Metadata headers) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SameNameButDifferent.

final Duration timeout = deadline == null ? null
: Durations.fromNanos(deadline.timeRemaining(TimeUnit.NANOSECONDS));
writer.logClientHeader(
Expand Down Expand Up @@ -500,7 +499,7 @@ public void cancel(String message, Throwable cause) {
public ServerInterceptor getServerInterceptor(final long callId) {
return new ServerInterceptor() {
@Override
public <ReqT, RespT> Listener<ReqT> interceptCall(
public <ReqT, RespT> ServerCall.Listener<ReqT> interceptCall(
final ServerCall<ReqT, RespT> call,
Metadata headers,
ServerCallHandler<ReqT, RespT> next) {
Expand Down
7 changes: 3 additions & 4 deletions services/src/test/java/io/grpc/services/BinaryLogProviderTest.java
View file
Open in desktop
Expand Up @@ -36,7 +36,6 @@
import io.grpc.MethodDescriptor.Marshaller;
import io.grpc.MethodDescriptor.MethodType;
import io.grpc.ServerCall;
import io.grpc.ServerCall.Listener;
import io.grpc.ServerCallHandler;
import io.grpc.ServerInterceptor;
import io.grpc.ServerMethodDefinition;
Expand Down Expand Up @@ -127,7 +126,7 @@ public <RequestT, ResponseT> ClientCall<RequestT, ResponseT> newCall(
MethodDescriptor<RequestT, ResponseT> methodDescriptor, CallOptions callOptions) {
return new NoopClientCall<RequestT, ResponseT>() {
@Override
public void start(Listener<ResponseT> responseListener, Metadata headers) {
public void start(ClientCall.Listener<ResponseT> responseListener, Metadata headers) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SameNameButDifferent

listener.set(responseListener);
}

Expand Down Expand Up @@ -211,7 +210,7 @@ public void wrapMethodDefinition_methodDescriptor() throws Exception {
method,
new ServerCallHandler<String, Integer>() {
@Override
public Listener<String> startCall(
public ServerCall.Listener<String> startCall(
ServerCall<String, Integer> call, Metadata headers) {
throw new UnsupportedOperationException();
}
Expand Down Expand Up @@ -310,7 +309,7 @@ public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall(
assertSame(BinaryLogProvider.BYTEARRAY_MARSHALLER, method.getResponseMarshaller());
return new SimpleForwardingClientCall<ReqT, RespT>(next.newCall(method, callOptions)) {
@Override
public void start(Listener<RespT> responseListener, Metadata headers) {
public void start(ClientCall.Listener<RespT> responseListener, Metadata headers) {
super.start(
new SimpleForwardingClientCallListener<RespT>(responseListener) {
@Override
Expand Down