From 8fac01e7364b654662b9504bc94ee6d4104b5753 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Mon, 27 Jun 2022 01:55:27 -0700 Subject: [PATCH] Enable error prone's UnusedVariable check (#6217) * Enable error prone's UnusedVariable check * Spotless --- .../otel.errorprone-conventions.gradle.kts | 4 ---- .../support/AttributeBindingFactory.java | 8 +++---- .../src/test/java/test/GrailsTest.java | 22 ++++++++++--------- .../grpc/v1_6/AbstractGrpcTest.java | 1 - .../internal/JdbcConnectionUrlParser.java | 1 - .../JspCompilationContextInstrumentation.java | 1 + .../netty/v4_0/BootstrapInstrumentation.java | 1 + .../reactor/ContextPropagationOperator.java | 2 +- .../java/test/tapestry/components/Layout.java | 1 + .../tooling/bytebuddy/BadAdvice.java | 2 ++ .../javaagent/tooling/HelperInjector.java | 5 ++--- .../muzzle/DeclaredFieldTestClass.java | 1 + .../HelperReferenceWrapperTestClasses.java | 3 ++- muzzle/src/test/java/muzzle/TestClasses.java | 5 +++-- .../windows/WindowsTestContainerManager.java | 14 ++++-------- .../java/ResourceLevelInstrumentation.java | 1 + .../junit/http/AbstractHttpClientTest.java | 1 - 17 files changed, 35 insertions(+), 38 deletions(-) diff --git a/conventions/src/main/kotlin/otel.errorprone-conventions.gradle.kts b/conventions/src/main/kotlin/otel.errorprone-conventions.gradle.kts index bc35b308e2..e87e341bdd 100644 --- a/conventions/src/main/kotlin/otel.errorprone-conventions.gradle.kts +++ b/conventions/src/main/kotlin/otel.errorprone-conventions.gradle.kts @@ -86,10 +86,6 @@ tasks { disable("JdkObsolete") disable("JavaUtilDate") - // Storing into a variable in onEnter triggers this unfortunately. - // TODO(anuraaga): Only disable for auto instrumentation project. - disable("UnusedVariable") - // TODO(anuraaga): Remove this, we use this pattern in several tests and it will mean // some moving. disable("DefaultPackage") diff --git a/instrumentation-api-annotation-support/src/main/java/io/opentelemetry/instrumentation/api/annotation/support/AttributeBindingFactory.java b/instrumentation-api-annotation-support/src/main/java/io/opentelemetry/instrumentation/api/annotation/support/AttributeBindingFactory.java index d875f0d909..a959394ae5 100644 --- a/instrumentation-api-annotation-support/src/main/java/io/opentelemetry/instrumentation/api/annotation/support/AttributeBindingFactory.java +++ b/instrumentation-api-annotation-support/src/main/java/io/opentelemetry/instrumentation/api/annotation/support/AttributeBindingFactory.java @@ -134,11 +134,11 @@ class AttributeBindingFactory { } if (componentType == Integer.class) { AttributeKey> key = AttributeKey.longArrayKey(name); - return mappedListBinding(Integer.class, key, Integer::longValue); + return mappedListBinding(key, Integer::longValue); } if (componentType == Float.class) { AttributeKey> key = AttributeKey.doubleArrayKey(name); - return mappedListBinding(Float.class, key, Float::doubleValue); + return mappedListBinding(key, Float::doubleValue); } return defaultListBinding(name); @@ -308,7 +308,7 @@ class AttributeBindingFactory { } private static AttributeBinding mappedListBinding( - Class fromClass, AttributeKey> key, Function mapping) { + AttributeKey> key, Function mapping) { return (setter, arg) -> { @SuppressWarnings("unchecked") List list = (List) arg; @@ -331,7 +331,7 @@ class AttributeBindingFactory { private static AttributeBinding defaultListBinding(String name) { AttributeKey> key = AttributeKey.stringArrayKey(name); - return mappedListBinding(Object.class, key, Object::toString); + return mappedListBinding(key, Object::toString); } private static AttributeBinding defaultBinding(String name) { diff --git a/instrumentation/grails-3.0/javaagent/src/test/java/test/GrailsTest.java b/instrumentation/grails-3.0/javaagent/src/test/java/test/GrailsTest.java index fc1eec1486..e71bfe9528 100644 --- a/instrumentation/grails-3.0/javaagent/src/test/java/test/GrailsTest.java +++ b/instrumentation/grails-3.0/javaagent/src/test/java/test/GrailsTest.java @@ -22,7 +22,6 @@ import io.opentelemetry.instrumentation.testing.junit.http.HttpServerTestOptions import io.opentelemetry.instrumentation.testing.junit.http.ServerEndpoint; import io.opentelemetry.sdk.testing.assertj.SpanDataAssert; import io.opentelemetry.sdk.trace.data.StatusData; -import java.lang.reflect.Method; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -67,21 +66,24 @@ public class GrailsTest extends AbstractHttpServerTest properties = new HashMap<>(); properties.put("server.port", port); - properties.put(contextPathKey, contextPath); + properties.put(getContextPathKey(), contextPath); grailsApp.setDefaultProperties(properties); return grailsApp.run(); } + @SuppressWarnings("ReturnValueIgnored") + private static String getContextPathKey() { + // context path configuration property name changes between spring boot versions + try { + ServerProperties.class.getDeclaredMethod("getServlet"); + return "server.servlet.contextPath"; + } catch (NoSuchMethodException ignore) { + return "server.context-path"; + } + } + @SuppressWarnings("rawtypes") @Override public Collection classes() { diff --git a/instrumentation/grpc-1.6/testing/src/main/java/io/opentelemetry/instrumentation/grpc/v1_6/AbstractGrpcTest.java b/instrumentation/grpc-1.6/testing/src/main/java/io/opentelemetry/instrumentation/grpc/v1_6/AbstractGrpcTest.java index 9fbb17402a..55c9938c4e 100644 --- a/instrumentation/grpc-1.6/testing/src/main/java/io/opentelemetry/instrumentation/grpc/v1_6/AbstractGrpcTest.java +++ b/instrumentation/grpc-1.6/testing/src/main/java/io/opentelemetry/instrumentation/grpc/v1_6/AbstractGrpcTest.java @@ -1192,7 +1192,6 @@ public abstract class AbstractGrpcTest { GreeterGrpc.GreeterStub client = GreeterGrpc.newStub(channel); IllegalStateException thrown = new IllegalStateException("illegal"); - AtomicReference response = new AtomicReference<>(); AtomicReference error = new AtomicReference<>(); CountDownLatch latch = new CountDownLatch(1); testing() diff --git a/instrumentation/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/internal/JdbcConnectionUrlParser.java b/instrumentation/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/internal/JdbcConnectionUrlParser.java index 61bd3a561b..5d917a8f47 100644 --- a/instrumentation/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/internal/JdbcConnectionUrlParser.java +++ b/instrumentation/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/internal/JdbcConnectionUrlParser.java @@ -76,7 +76,6 @@ public enum JdbcConnectionUrlParser { @Override DbInfo.Builder doParse(String jdbcUrl, DbInfo.Builder builder) { String serverName = ""; - Integer port = null; int hostIndex = jdbcUrl.indexOf("jtds:sqlserver://"); if (hostIndex < 0) { diff --git a/instrumentation/jsp-2.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jsp/JspCompilationContextInstrumentation.java b/instrumentation/jsp-2.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jsp/JspCompilationContextInstrumentation.java index a64b999ca8..77b5fd23e7 100644 --- a/instrumentation/jsp-2.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jsp/JspCompilationContextInstrumentation.java +++ b/instrumentation/jsp-2.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jsp/JspCompilationContextInstrumentation.java @@ -34,6 +34,7 @@ public class JspCompilationContextInstrumentation implements TypeInstrumentation JspCompilationContextInstrumentation.class.getName() + "$JasperJspCompilationContext"); } + @SuppressWarnings("unused") public static class JasperJspCompilationContext { @Advice.OnMethodEnter(suppress = Throwable.class) diff --git a/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/BootstrapInstrumentation.java b/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/BootstrapInstrumentation.java index ba55bac5ca..a05e101356 100644 --- a/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/BootstrapInstrumentation.java +++ b/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/BootstrapInstrumentation.java @@ -37,6 +37,7 @@ public class BootstrapInstrumentation implements TypeInstrumentation { BootstrapInstrumentation.class.getName() + "$ConnectAdvice"); } + @SuppressWarnings("unused") public static class ConnectAdvice { @Advice.OnMethodEnter public static void startConnect( diff --git a/instrumentation/reactor/reactor-3.1/library/src/main/java/io/opentelemetry/instrumentation/reactor/ContextPropagationOperator.java b/instrumentation/reactor/reactor-3.1/library/src/main/java/io/opentelemetry/instrumentation/reactor/ContextPropagationOperator.java index 1c345b0ba1..7fe10e97e4 100644 --- a/instrumentation/reactor/reactor-3.1/library/src/main/java/io/opentelemetry/instrumentation/reactor/ContextPropagationOperator.java +++ b/instrumentation/reactor/reactor-3.1/library/src/main/java/io/opentelemetry/instrumentation/reactor/ContextPropagationOperator.java @@ -162,7 +162,7 @@ public final class ContextPropagationOperator { implements BiFunction, CoreSubscriber> { /** Holds reference to strategy to prevent it from being collected. */ - @SuppressWarnings("FieldCanBeLocal") + @SuppressWarnings({"FieldCanBeLocal", "UnusedVariable"}) private final ReactorAsyncOperationEndStrategy asyncOperationEndStrategy; public Lifter(ReactorAsyncOperationEndStrategy asyncOperationEndStrategy) { diff --git a/instrumentation/tapestry-5.4/javaagent/src/test/java/test/tapestry/components/Layout.java b/instrumentation/tapestry-5.4/javaagent/src/test/java/test/tapestry/components/Layout.java index 52be1368d5..7d83154da8 100644 --- a/instrumentation/tapestry-5.4/javaagent/src/test/java/test/tapestry/components/Layout.java +++ b/instrumentation/tapestry-5.4/javaagent/src/test/java/test/tapestry/components/Layout.java @@ -11,6 +11,7 @@ import org.apache.tapestry5.annotations.Parameter; import org.apache.tapestry5.annotations.Property; import org.apache.tapestry5.ioc.annotations.Inject; +@SuppressWarnings("unused") public class Layout { @Inject private ComponentResources resources; diff --git a/javaagent-tooling/src/testExceptionHandler/java/io/opentelemetry/javaagent/tooling/bytebuddy/BadAdvice.java b/javaagent-tooling/src/testExceptionHandler/java/io/opentelemetry/javaagent/tooling/bytebuddy/BadAdvice.java index 5f39973198..3bc0d08f3c 100644 --- a/javaagent-tooling/src/testExceptionHandler/java/io/opentelemetry/javaagent/tooling/bytebuddy/BadAdvice.java +++ b/javaagent-tooling/src/testExceptionHandler/java/io/opentelemetry/javaagent/tooling/bytebuddy/BadAdvice.java @@ -7,7 +7,9 @@ package io.opentelemetry.javaagent.tooling.bytebuddy; import net.bytebuddy.asm.Advice; +@SuppressWarnings("unused") public class BadAdvice { + @Advice.OnMethodExit(suppress = Throwable.class) public static void throwAnException(@Advice.Return(readOnly = false) boolean returnVal) { returnVal = true; diff --git a/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java b/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java index 1bacd51855..712450e05d 100644 --- a/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java +++ b/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java @@ -169,7 +169,7 @@ public class HelperInjector implements Transformer { ClassLoader classLoader, JavaModule module) { if (!helperClassNames.isEmpty()) { - injectHelperClasses(typeDescription, classLoader, module); + injectHelperClasses(typeDescription, classLoader); } if (classLoader != null && helpersSource != null && !helperResources.isEmpty()) { @@ -226,8 +226,7 @@ public class HelperInjector implements Transformer { }); } - private void injectHelperClasses( - TypeDescription typeDescription, ClassLoader classLoader, JavaModule module) { + private void injectHelperClasses(TypeDescription typeDescription, ClassLoader classLoader) { classLoader = maskNullClassLoader(classLoader); if (classLoader == BOOTSTRAP_CLASSLOADER_PLACEHOLDER && instrumentation == null) { logger.log( diff --git a/muzzle/src/test/java/io/opentelemetry/javaagent/tooling/muzzle/DeclaredFieldTestClass.java b/muzzle/src/test/java/io/opentelemetry/javaagent/tooling/muzzle/DeclaredFieldTestClass.java index 41c0f33ee0..b21b63d104 100644 --- a/muzzle/src/test/java/io/opentelemetry/javaagent/tooling/muzzle/DeclaredFieldTestClass.java +++ b/muzzle/src/test/java/io/opentelemetry/javaagent/tooling/muzzle/DeclaredFieldTestClass.java @@ -5,6 +5,7 @@ package io.opentelemetry.javaagent.tooling.muzzle; +@SuppressWarnings("unused") public class DeclaredFieldTestClass { public static class Advice { public void instrument() { diff --git a/muzzle/src/test/java/muzzle/HelperReferenceWrapperTestClasses.java b/muzzle/src/test/java/muzzle/HelperReferenceWrapperTestClasses.java index 672fa26460..eb10c585b9 100644 --- a/muzzle/src/test/java/muzzle/HelperReferenceWrapperTestClasses.java +++ b/muzzle/src/test/java/muzzle/HelperReferenceWrapperTestClasses.java @@ -5,7 +5,7 @@ package muzzle; -@SuppressWarnings({"UnusedMethod", "MethodCanBeStatic"}) +@SuppressWarnings("unused") public class HelperReferenceWrapperTestClasses { interface Interface1 { void foo(); @@ -21,6 +21,7 @@ public class HelperReferenceWrapperTestClasses { static void staticMethodsAreIgnored() {} + @SuppressWarnings("MethodCanBeStatic") private void privateMethodsToo() {} } } diff --git a/muzzle/src/test/java/muzzle/TestClasses.java b/muzzle/src/test/java/muzzle/TestClasses.java index b7dca652da..2d5765ae3d 100644 --- a/muzzle/src/test/java/muzzle/TestClasses.java +++ b/muzzle/src/test/java/muzzle/TestClasses.java @@ -10,9 +10,10 @@ import io.opentelemetry.instrumentation.OtherTestHelperClasses; import io.opentelemetry.instrumentation.TestHelperClasses.Helper; import net.bytebuddy.asm.Advice; -@SuppressWarnings("ClassNamedLikeTypeParameter") +@SuppressWarnings("unused") public class TestClasses { + @SuppressWarnings("ClassNamedLikeTypeParameter") public static class MethodBodyAdvice { @SuppressWarnings("ReturnValueIgnored") @Advice.OnMethodEnter @@ -46,7 +47,7 @@ public class TestClasses { return s; } - @SuppressWarnings({"UnusedMethod", "MethodCanBeStatic"}) + @SuppressWarnings("MethodCanBeStatic") private void privateStuff() {} protected void protectedMethod() {} diff --git a/smoke-tests/src/test/java/io/opentelemetry/smoketest/windows/WindowsTestContainerManager.java b/smoke-tests/src/test/java/io/opentelemetry/smoketest/windows/WindowsTestContainerManager.java index fd80727565..6e36705da2 100644 --- a/smoke-tests/src/test/java/io/opentelemetry/smoketest/windows/WindowsTestContainerManager.java +++ b/smoke-tests/src/test/java/io/opentelemetry/smoketest/windows/WindowsTestContainerManager.java @@ -244,7 +244,7 @@ public class WindowsTestContainerManager extends AbstractTestContainerManager { } } - private ContainerLogHandler consumeLogs(String containerId, Waiter waiter, Logger logger) { + private void registerLogListener(String containerId, Waiter waiter, Logger logger) { ContainerLogFrameConsumer consumer = new ContainerLogFrameConsumer(); waiter.configureLogger(consumer); @@ -257,7 +257,6 @@ public class WindowsTestContainerManager extends AbstractTestContainerManager { .exec(consumer); consumer.addListener(new Slf4jDockerLogLineListener(logger)); - return consumer; } private static int extractMappedPort(Container container, int internalPort) { @@ -295,11 +294,11 @@ public class WindowsTestContainerManager extends AbstractTestContainerManager { prepareAction.accept(containerId); client.startContainerCmd(containerId).exec(); - ContainerLogHandler logHandler = consumeLogs(containerId, waiter, logger); + registerLogListener(containerId, waiter, logger); InspectContainerResponse inspectResponse = inspect ? client.inspectContainerCmd(containerId).exec() : null; - Container container = new Container(imageName, containerId, logHandler, inspectResponse); + Container container = new Container(imageName, containerId, inspectResponse); waiter.waitFor(container); return container; @@ -318,17 +317,12 @@ public class WindowsTestContainerManager extends AbstractTestContainerManager { private static class Container { public final String imageName; public final String containerId; - public final ContainerLogHandler logConsumer; public final InspectContainerResponse inspectResponse; private Container( - String imageName, - String containerId, - ContainerLogHandler logConsumer, - InspectContainerResponse inspectResponse) { + String imageName, String containerId, InspectContainerResponse inspectResponse) { this.imageName = imageName; this.containerId = containerId; - this.logConsumer = logConsumer; this.inspectResponse = inspectResponse; } } diff --git a/testing-common/integration-tests/src/main/java/ResourceLevelInstrumentation.java b/testing-common/integration-tests/src/main/java/ResourceLevelInstrumentation.java index eebc6e56fc..e362a06128 100644 --- a/testing-common/integration-tests/src/main/java/ResourceLevelInstrumentation.java +++ b/testing-common/integration-tests/src/main/java/ResourceLevelInstrumentation.java @@ -23,6 +23,7 @@ public class ResourceLevelInstrumentation implements TypeInstrumentation { named("toString"), this.getClass().getName() + "$ToStringAdvice"); } + @SuppressWarnings("unused") public static class ToStringAdvice { @Advice.OnMethodExit(suppress = Throwable.class) static void toStringReplace(@Advice.Return(readOnly = false) String ret) { diff --git a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java index 301d05a0aa..2e493720e6 100644 --- a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java +++ b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java @@ -315,7 +315,6 @@ public abstract class AbstractHttpClientTest { testing.waitAndAssertTraces( trace -> { - List> traces = testing.traces(); trace.hasSpansSatisfyingExactly( span -> assertClientSpan(span, uri, method, 200).hasNoParent(), span -> assertServerSpan(span).hasParent(trace.getSpan(0)));