From 7f673be1489a3fa541f19b034b83091b916d2e01 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Fri, 13 Dec 2019 18:05:46 -0500 Subject: [PATCH 1/6] Gauges should be registered with `registerWithReplacement` Using TaggedMetricRegistry.gauge is equivalent to map.putIfAbsent, and can result in subtle resource leaks. Prefer replacing existing gauge values using `registerWithReplacement`. This check doesn't apply unless a new enough version of Tritium is available on the compilation classpath. --- README.md | 1 + baseline-error-prone/build.gradle | 1 + .../errorprone/UnsafeGaugeRegistration.java | 91 +++++++++++++++++++ .../UnsafeGaugeRegistrationTest.java | 63 +++++++++++++ .../BaselineErrorProneExtension.java | 1 + versions.lock | 18 ++-- versions.props | 1 + 7 files changed, 168 insertions(+), 8 deletions(-) create mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/UnsafeGaugeRegistration.java create mode 100644 baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/UnsafeGaugeRegistrationTest.java diff --git a/README.md b/README.md index a2e221589..b654505cc 100644 --- a/README.md +++ b/README.md @@ -185,6 +185,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c - `AssertjPrimitiveComparison`: Prefer using AssertJ fluent comparisons over logic in an assertThat statement. - `ExceptionSpecificity`: Prefer more specific catch types than Exception and Throwable. - `ThrowSpecificity`: Prefer to declare more specific `throws` types than Exception and Throwable. +- `UnsafeGaugeRegistration`: Use TaggedMetricRegistry.registerWithReplacement over TaggedMetricRegistry.gauge. ### Programmatic Application diff --git a/baseline-error-prone/build.gradle b/baseline-error-prone/build.gradle index f04bda0c1..75e8cb4fe 100644 --- a/baseline-error-prone/build.gradle +++ b/baseline-error-prone/build.gradle @@ -20,6 +20,7 @@ dependencies { testCompile 'commons-lang:commons-lang' testCompile 'org.assertj:assertj-core' testCompile 'org.jooq:jooq' + testCompile 'com.palantir.tritium:tritium-registry' testImplementation 'org.junit.jupiter:junit-jupiter' testRuntimeOnly 'org.junit.jupiter:junit-jupiter-migrationsupport' diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/UnsafeGaugeRegistration.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/UnsafeGaugeRegistration.java new file mode 100644 index 000000000..eed2adcb2 --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/UnsafeGaugeRegistration.java @@ -0,0 +1,91 @@ +/* + * (c) Copyright 2018 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.baseline.errorprone; + +import com.google.auto.service.AutoService; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.AbstractReturnValueIgnored; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.fixes.SuggestedFixes; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.method.MethodMatchers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.tools.javac.code.Symbol; + +@AutoService(BugChecker.class) +@BugPattern( + name = "UnsafeGaugeRegistration", + link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", + linkType = BugPattern.LinkType.CUSTOM, + providesFix = BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION, + severity = BugPattern.SeverityLevel.WARNING, + summary = "Using TaggedMetricRegistry.gauge is equivalent to map.putIfAbsent, and can result in subtle " + + "resource leaks. Prefer replacing existing gauge values.\n" + // This check may begin to fail after a version upgrade, where fixes aren't automatically applied + + "This can be fixed automatically using " + + "./gradlew compileJava compileTestJava -PerrorProneApply=UnsafeGaugeRegistration") +public final class UnsafeGaugeRegistration extends AbstractReturnValueIgnored { + + private static final String TAGGED_REGISTRY = "com.palantir.tritium.metrics.registry.TaggedMetricRegistry"; + private static final String REGISTER_WITH_REPLACEMENT = "registerWithReplacement"; + private static final Matcher MATCHER = MethodMatchers.instanceMethod() + .onDescendantOf(TAGGED_REGISTRY) + .named("gauge") + .withParameters("com.palantir.tritium.metrics.registry.MetricName", "com.codahale.metrics.Gauge"); + + @Override + public Matcher specializedMatcher() { + return MATCHER; + } + + // Override matchMethodInvocation from AbstractReturnValueIgnored to apply our suggested fix. + @Override + public Description matchMethodInvocation(MethodInvocationTree methodInvocationTree, VisitorState state) { + Description description = super.matchMethodInvocation(methodInvocationTree, state); + if (Description.NO_MATCH.equals(description) + || !hasRegisterWithReplacement(state) + || TestCheckUtils.isTestCode(state)) { + return Description.NO_MATCH; + } + return buildDescription(methodInvocationTree) + .addFix(SuggestedFixes.renameMethodInvocation(methodInvocationTree, REGISTER_WITH_REPLACEMENT, state)) + .build(); + } + + /** + * TaggedMetricRegistry.registerWithReplacement was added in Tritium 0.16.1, avoid flagging older versions. + */ + private static boolean hasRegisterWithReplacement(VisitorState state) { + Symbol symbol = state.getSymbolFromString(TAGGED_REGISTRY); + if (!(symbol instanceof Symbol.ClassSymbol)) { + return false; + } + Symbol.ClassSymbol classSymbol = (Symbol.ClassSymbol) symbol; + for (Symbol enclosed : classSymbol.getEnclosedElements()) { + if (enclosed instanceof Symbol.MethodSymbol) { + Symbol.MethodSymbol enclosedMethod = (Symbol.MethodSymbol) enclosed; + if (enclosedMethod.name.contentEquals(REGISTER_WITH_REPLACEMENT)) { + return true; + } + } + } + return false; + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/UnsafeGaugeRegistrationTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/UnsafeGaugeRegistrationTest.java new file mode 100644 index 000000000..355517f7d --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/UnsafeGaugeRegistrationTest.java @@ -0,0 +1,63 @@ +/* + * (c) Copyright 2018 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.baseline.errorprone; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +class UnsafeGaugeRegistrationTest { + + @Test + void testFix() { + RefactoringValidator.of(new UnsafeGaugeRegistration(), getClass()) + .addInputLines( + "Test.java", + "import com.palantir.tritium.metrics.registry.MetricName;", + "import com.palantir.tritium.metrics.registry.TaggedMetricRegistry;", + "class Test {", + " void f(TaggedMetricRegistry registry, MetricName name) {", + " registry.gauge(name, () -> 1);", + " }", + "}") + .addOutputLines( + "Test.java", + "import com.palantir.tritium.metrics.registry.MetricName;", + "import com.palantir.tritium.metrics.registry.TaggedMetricRegistry;", + "class Test {", + " void f(TaggedMetricRegistry registry, MetricName name) {", + " registry.registerWithReplacement(name, () -> 1);", + " }", + "}" + ) + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + void testNegative() { + CompilationTestHelper.newInstance(UnsafeGaugeRegistration.class, getClass()).addSourceLines( + "Test.java", + "import com.codahale.metrics.Gauge;", + "import com.palantir.tritium.metrics.registry.MetricName;", + "import com.palantir.tritium.metrics.registry.TaggedMetricRegistry;", + "class Test {", + " Gauge f(TaggedMetricRegistry registry, MetricName name) {", + " return registry.gauge(name, () -> 1);", + " }", + "}").doTest(); + } +} diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java index 5091f1055..38f566b3c 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java @@ -48,6 +48,7 @@ public class BaselineErrorProneExtension { "ThrowError", // TODO(ckozak): re-enable pending scala check //"ThrowSpecificity", + "UnsafeGaugeRegistration", // Built-in checks "ArrayEquals", diff --git a/versions.lock b/versions.lock index 73691b1bc..ca3004c06 100644 --- a/versions.lock +++ b/versions.lock @@ -13,10 +13,10 @@ com.google.auto.service:auto-service:1.0-rc4 (1 constraints: db05b241) com.google.auto.value:auto-value:1.5.3 (1 constraints: 1c121afb) com.google.auto.value:auto-value-annotations:1.6.3 (1 constraints: 620a25b9) com.google.code.findbugs:jFormatString:3.0.0 (1 constraints: f01022b8) -com.google.code.findbugs:jsr305:3.0.2 (4 constraints: 3b429cde) +com.google.code.findbugs:jsr305:3.0.2 (5 constraints: c9524c6b) com.google.code.gson:gson:2.8.2 (1 constraints: b61a7239) com.google.errorprone:error_prone_annotation:2.3.4 (3 constraints: 3738b160) -com.google.errorprone:error_prone_annotations:2.3.4 (8 constraints: b372b138) +com.google.errorprone:error_prone_annotations:2.3.4 (9 constraints: 4883eb7c) com.google.errorprone:error_prone_check_api:2.3.4 (2 constraints: 542522be) com.google.errorprone:error_prone_core:2.3.4 (3 constraints: 6825b509) com.google.errorprone:error_prone_refaster:2.3.4 (1 constraints: 0b050236) @@ -26,9 +26,9 @@ com.google.errorprone:javac:9+181-r4173-1 (4 constraints: 3352a10d) com.google.errorprone:javac-shaded:9-dev-r4023-3 (1 constraints: 151671fc) com.google.googlejavaformat:google-java-format:1.4 (1 constraints: fd138d38) com.google.guava:failureaccess:1.0.1 (1 constraints: 140ae1b4) -com.google.guava:guava:27.1-jre (14 constraints: ecf4ebdf) +com.google.guava:guava:28.0-jre (15 constraints: ce06bcb2) com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava (1 constraints: bd17c918) -com.google.j2objc:j2objc-annotations:1.1 (1 constraints: b609eba0) +com.google.j2objc:j2objc-annotations:1.3 (1 constraints: b809eda0) com.google.jimfs:jimfs:1.1 (1 constraints: fa138a38) com.google.protobuf:protobuf-java:3.4.0 (1 constraints: f4102eb8) com.google.testing.compile:compile-testing:0.18 (1 constraints: 3114b84c) @@ -67,7 +67,7 @@ org.bouncycastle:bcpg-jdk15on:1.61 (1 constraints: 1a0e4342) org.bouncycastle:bcpkix-jdk15on:1.61 (1 constraints: 1a0e4342) org.bouncycastle:bcprov-jdk15on:1.61 (3 constraints: e327dc7a) org.checkerframework:checker-compat-qual:2.5.5 (1 constraints: 640a29b9) -org.checkerframework:checker-qual:3.0.0 (4 constraints: 0735fc6c) +org.checkerframework:checker-qual:3.0.0 (4 constraints: 09359e6d) org.checkerframework:dataflow:3.0.0 (3 constraints: 2538eb5c) org.checkerframework:javacutil:3.0.0 (1 constraints: 400d2d1d) org.codehaus.groovy:groovy:2.4.7 (1 constraints: 990d932a) @@ -102,7 +102,7 @@ org.scala-lang.modules:scala-parser-combinators_2.11:1.0.6 (1 constraints: 960d3 org.scala-lang.modules:scala-xml_2.11:1.0.6 (1 constraints: 960d3e43) org.scalariform:scalariform_2.11:0.2.0 (1 constraints: d50cb424) org.scalastyle:scalastyle_2.11:1.0.0 (1 constraints: 8117cd64) -org.slf4j:slf4j-api:1.7.25 (3 constraints: 972185d3) +org.slf4j:slf4j-api:1.7.25 (4 constraints: e2303624) [Test dependencies] cglib:cglib-nodep:3.2.2 (1 constraints: 490ded24) @@ -111,11 +111,13 @@ com.fasterxml.jackson.core:jackson-core:2.10.1 (1 constraints: 84122d21) com.fasterxml.jackson.core:jackson-databind:2.10.1 (1 constraints: e30d9737) com.github.stefanbirkner:system-rules:1.19.0 (1 constraints: 3d05443b) com.netflix.nebula:nebula-test:7.6.0 (1 constraints: 0f052036) -com.palantir.safe-logging:preconditions:1.12.2 (2 constraints: 10136143) -com.palantir.safe-logging:safe-logging:1.12.2 (2 constraints: 6916290c) +com.palantir.safe-logging:preconditions:1.12.2 (3 constraints: cf239c79) +com.palantir.safe-logging:safe-logging:1.12.2 (3 constraints: 2827bbdf) com.palantir.tokens:auth-tokens:3.6.1 (1 constraints: 0c050d36) +com.palantir.tritium:tritium-registry:0.16.2 (1 constraints: 3b05373b) commons-io:commons-io:2.5 (1 constraints: eb0c8d0a) commons-lang:commons-lang:2.6 (1 constraints: ac04232c) +io.dropwizard.metrics:metrics-core:3.2.6 (1 constraints: 95108ba5) javax.xml.bind:jaxb-api:2.3.0 (1 constraints: bf069459) junit:junit-dep:4.11 (1 constraints: ba1063b3) net.lingala.zip4j:zip4j:1.3.2 (1 constraints: 0805fb35) diff --git a/versions.props b/versions.props index 8877f0ce2..dba99127b 100644 --- a/versions.props +++ b/versions.props @@ -26,6 +26,7 @@ org.junit.jupiter:* = 5.5.2 org.mockito:* = 3.2.0 com.fasterxml.jackson.*:* = 2.10.1 com.palantir.tokens:auth-tokens = 3.6.1 +com.palantir.tritium:tritium-registry = 0.16.2 # dependency-upgrader:OFF # Updating to 0.8 would raise our minimum compatible version to 5.2 From a35150e41254bb6cd5ce9c97e1fcbe15f791d451 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Fri, 13 Dec 2019 23:05:46 +0000 Subject: [PATCH 2/6] Add generated changelog entries --- changelog/@unreleased/pr-1123.v2.yml | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 changelog/@unreleased/pr-1123.v2.yml diff --git a/changelog/@unreleased/pr-1123.v2.yml b/changelog/@unreleased/pr-1123.v2.yml new file mode 100644 index 000000000..de32f89b2 --- /dev/null +++ b/changelog/@unreleased/pr-1123.v2.yml @@ -0,0 +1,11 @@ +type: improvement +improvement: + description: |- + Using TaggedMetricRegistry.gauge is equivalent to map.putIfAbsent, and + can result in subtle resource leaks. Prefer replacing existing gauge + values using `registerWithReplacement`. + + This check doesn't apply unless a new enough version of Tritium + is available on the compilation classpath. + links: + - https://github.com/palantir/gradle-baseline/pull/1123 From ec64263505e10083c5a80d7e3d38242add73911b Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 16 Dec 2019 10:42:06 -0500 Subject: [PATCH 3/6] Test for a known bug, add a refaster rule --- .../UnsafeGaugeRegistrationTest.java | 28 ++++++++++ baseline-refaster-rules/build.gradle | 4 ++ .../refaster/TritiumReplaceGauge.java | 47 +++++++++++++++++ .../refaster/TritiumReplaceGaugeTest.java | 52 +++++++++++++++++++ versions.lock | 9 ++-- versions.props | 1 + 6 files changed, 137 insertions(+), 4 deletions(-) create mode 100644 baseline-refaster-rules/src/main/java/com/palantir/baseline/refaster/TritiumReplaceGauge.java create mode 100644 baseline-refaster-rules/src/test/java/com/palantir/baseline/refaster/TritiumReplaceGaugeTest.java diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/UnsafeGaugeRegistrationTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/UnsafeGaugeRegistrationTest.java index 355517f7d..69705da3d 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/UnsafeGaugeRegistrationTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/UnsafeGaugeRegistrationTest.java @@ -47,6 +47,34 @@ void testFix() { .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } + @Test + void testKnownBug() { + RefactoringValidator.of(new UnsafeGaugeRegistration(), getClass()) + .addInputLines( + "Test.java", + "import com.codahale.metrics.Gauge;", + "import com.palantir.tritium.metrics.registry.MetricName;", + "import com.palantir.tritium.metrics.registry.TaggedMetricRegistry;", + "class Test {", + " void f(TaggedMetricRegistry registry, MetricName name, Gauge gauge) {", + " registry.gauge(name, gauge);", + " }", + "}") + .addOutputLines( + "Test.java", + "import com.codahale.metrics.Gauge;", + "import com.palantir.tritium.metrics.registry.MetricName;", + "import com.palantir.tritium.metrics.registry.TaggedMetricRegistry;", + "class Test {", + " void f(TaggedMetricRegistry registry, MetricName name, Gauge gauge) {", + // This isn't right. Filed https://github.com/google/error-prone/issues/1451 + " registry.gauge(name, registerWithReplacement);", + " }", + "}" + ) + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + @Test void testNegative() { CompilationTestHelper.newInstance(UnsafeGaugeRegistration.class, getClass()).addSourceLines( diff --git a/baseline-refaster-rules/build.gradle b/baseline-refaster-rules/build.gradle index 5b1e6ee3e..e6ac2f4e5 100644 --- a/baseline-refaster-rules/build.gradle +++ b/baseline-refaster-rules/build.gradle @@ -7,9 +7,13 @@ dependencies { implementation 'com.google.errorprone:error_prone_refaster' implementation 'org.assertj:assertj-core' implementation 'org.mockito:mockito-core' + implementation 'com.palantir.tritium:tritium-registry' testCompile 'junit:junit' testCompile project(':baseline-refaster-testing') + testCompile 'org.immutables:value::annotations' + + compileOnly 'org.immutables:value::annotations' } // Do not apply refaster rules diff --git a/baseline-refaster-rules/src/main/java/com/palantir/baseline/refaster/TritiumReplaceGauge.java b/baseline-refaster-rules/src/main/java/com/palantir/baseline/refaster/TritiumReplaceGauge.java new file mode 100644 index 000000000..f2d204d15 --- /dev/null +++ b/baseline-refaster-rules/src/main/java/com/palantir/baseline/refaster/TritiumReplaceGauge.java @@ -0,0 +1,47 @@ +/* + * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.baseline.refaster; + +import com.codahale.metrics.Gauge; +import com.google.errorprone.refaster.annotation.AfterTemplate; +import com.google.errorprone.refaster.annotation.BeforeTemplate; +import com.palantir.tritium.metrics.registry.MetricName; +import com.palantir.tritium.metrics.registry.TaggedMetricRegistry; + +/** + * Remove unnecessary 'remove' invocation prior to a 'registerWithReplacement'. + * This refaster rule pairs with the 'UnsafeGaugeRegistration' error-prone rule + * to replace 'registry.remove(name); registry.gauge(name, value);' with a + * single 'registerWithReplacement' call. + * This refaster rule intentionally doesn't check the 'gauge' method in order to + * avoid creating changes that don't compile when older versions of Tritium are + * present. + */ +public final class TritiumReplaceGauge { + + @BeforeTemplate + void before(TaggedMetricRegistry registry, MetricName name, Gauge gauge) { + registry.remove(name); + registry.registerWithReplacement(name, gauge); + } + + @AfterTemplate + void after(TaggedMetricRegistry registry, MetricName name, Gauge gauge) { + registry.registerWithReplacement(name, gauge); + } + +} diff --git a/baseline-refaster-rules/src/test/java/com/palantir/baseline/refaster/TritiumReplaceGaugeTest.java b/baseline-refaster-rules/src/test/java/com/palantir/baseline/refaster/TritiumReplaceGaugeTest.java new file mode 100644 index 000000000..431bd5d39 --- /dev/null +++ b/baseline-refaster-rules/src/test/java/com/palantir/baseline/refaster/TritiumReplaceGaugeTest.java @@ -0,0 +1,52 @@ +/* + * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.baseline.refaster; + +import org.junit.Test; + +public class TritiumReplaceGaugeTest { + + @Test + public void test() { + RefasterTestHelper + .forRefactoring(TritiumReplaceGauge.class) + .withInputLines( + "Test", + "import com.codahale.metrics.Gauge;", + "import com.palantir.tritium.metrics.registry.DefaultTaggedMetricRegistry;", + "import com.palantir.tritium.metrics.registry.MetricName;", + "public class Test {", + " void f(DefaultTaggedMetricRegistry registry) {", + " MetricName name = MetricName.builder().safeName(\"foo\").build();", + " registry.remove(name);", + " registry.registerWithReplacement(name, () -> 1);", + " }", + "}") + .hasOutputLines( + "import com.codahale.metrics.Gauge;", + "import com.palantir.tritium.metrics.registry.DefaultTaggedMetricRegistry;", + "import com.palantir.tritium.metrics.registry.MetricName;", + "public class Test {", + " void f(DefaultTaggedMetricRegistry registry) {", + " MetricName name = MetricName.builder().safeName(\"foo\").build();", + " registry.registerWithReplacement(name, () -> 1);", + " ", + " }", + "}"); + } + +} diff --git a/versions.lock b/versions.lock index ca3004c06..2f42ff379 100644 --- a/versions.lock +++ b/versions.lock @@ -43,8 +43,12 @@ com.netflix.nebula:nebula-gradle-interop:1.0.11 (1 constraints: 9214098d) com.palantir.configurationresolver:gradle-configuration-resolver-plugin:0.4.0 (1 constraints: 0605f735) com.palantir.javaformat:gradle-palantir-java-format:0.3.9 (1 constraints: 0e05fd35) com.palantir.javaformat:palantir-java-format-spi:0.3.9 (1 constraints: 7b156abe) +com.palantir.safe-logging:preconditions:1.12.2 (3 constraints: cf239c79) +com.palantir.safe-logging:safe-logging:1.12.2 (3 constraints: 2827bbdf) +com.palantir.tritium:tritium-registry:0.16.2 (1 constraints: 3b05373b) com.typesafe:config:1.2.0 (1 constraints: d60cb924) gradle.plugin.org.jetbrains.gradle.plugin.idea-ext:gradle-idea-ext:0.7 (1 constraints: 1815b193) +io.dropwizard.metrics:metrics-core:3.2.6 (1 constraints: 95108ba5) javax.inject:javax.inject:1 (1 constraints: 9d0e8743) junit:junit:4.13-beta-1 (8 constraints: ad737933) net.bytebuddy:byte-buddy:1.10.3 (1 constraints: 3f0b35de) @@ -83,6 +87,7 @@ org.github.ngbinh.scalastyle:gradle-scalastyle-plugin_2.11:1.0.1 (1 constraints: org.hamcrest:hamcrest:2.2 (1 constraints: 720b95d5) org.hamcrest:hamcrest-core:2.2 (4 constraints: 2b2b319e) org.hamcrest:hamcrest-library:1.3 (1 constraints: fc138c38) +org.immutables:value:2.8.3 (1 constraints: 0f051036) org.inferred:freebuilder:1.14.6 (1 constraints: 3e053b3b) org.jetbrains:annotations:13.0 (1 constraints: df0e795c) org.jetbrains.kotlin:kotlin-stdlib:1.3.50 (2 constraints: be218de0) @@ -111,13 +116,9 @@ com.fasterxml.jackson.core:jackson-core:2.10.1 (1 constraints: 84122d21) com.fasterxml.jackson.core:jackson-databind:2.10.1 (1 constraints: e30d9737) com.github.stefanbirkner:system-rules:1.19.0 (1 constraints: 3d05443b) com.netflix.nebula:nebula-test:7.6.0 (1 constraints: 0f052036) -com.palantir.safe-logging:preconditions:1.12.2 (3 constraints: cf239c79) -com.palantir.safe-logging:safe-logging:1.12.2 (3 constraints: 2827bbdf) com.palantir.tokens:auth-tokens:3.6.1 (1 constraints: 0c050d36) -com.palantir.tritium:tritium-registry:0.16.2 (1 constraints: 3b05373b) commons-io:commons-io:2.5 (1 constraints: eb0c8d0a) commons-lang:commons-lang:2.6 (1 constraints: ac04232c) -io.dropwizard.metrics:metrics-core:3.2.6 (1 constraints: 95108ba5) javax.xml.bind:jaxb-api:2.3.0 (1 constraints: bf069459) junit:junit-dep:4.11 (1 constraints: ba1063b3) net.lingala.zip4j:zip4j:1.3.2 (1 constraints: 0805fb35) diff --git a/versions.props b/versions.props index dba99127b..f3730ea3b 100644 --- a/versions.props +++ b/versions.props @@ -12,6 +12,7 @@ org.github.ngbinh.scalastyle:gradle-scalastyle-plugin_2.11 = 1.0.1 org.inferred:freebuilder = 1.14.6 org.jooq:jooq = 3.12.3 org.slf4j:slf4j-api = 1.7.25 +org.immutables:* = 2.8.3 # test deps com.netflix.nebula:nebula-test = 7.6.0 From f71d4542b3d77cf9650d18914e480883b5495109 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 16 Dec 2019 10:52:11 -0500 Subject: [PATCH 4/6] workaround incorrect upstream `renameMethodInvocation` --- .../ExecutorSubmitRunnableFutureIgnored.java | 3 +- .../errorprone/MoreSuggestedFixes.java | 40 +++++++++++++++++++ .../errorprone/ReadReturnValueIgnored.java | 3 +- .../baseline/errorprone/ReverseDnsLookup.java | 5 +-- .../baseline/errorprone/Slf4jLevelCheck.java | 3 +- .../errorprone/UnsafeGaugeRegistration.java | 4 +- .../UnsafeGaugeRegistrationTest.java | 4 +- 7 files changed, 49 insertions(+), 13 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ExecutorSubmitRunnableFutureIgnored.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ExecutorSubmitRunnableFutureIgnored.java index efb01ecfc..2c2f3017d 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ExecutorSubmitRunnableFutureIgnored.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ExecutorSubmitRunnableFutureIgnored.java @@ -21,7 +21,6 @@ import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.AbstractReturnValueIgnored; import com.google.errorprone.bugpatterns.BugChecker; -import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.matchers.method.MethodMatchers; @@ -60,7 +59,7 @@ public Description matchMethodInvocation(MethodInvocationTree methodInvocationTr return description; } return buildDescription(methodInvocationTree) - .addFix(SuggestedFixes.renameMethodInvocation(methodInvocationTree, "execute", state)) + .addFix(MoreSuggestedFixes.renameMethodInvocation(methodInvocationTree, "execute", state)) .build(); } } diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/MoreSuggestedFixes.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/MoreSuggestedFixes.java index 0e21910f0..ac9e3ffe4 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/MoreSuggestedFixes.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/MoreSuggestedFixes.java @@ -16,17 +16,22 @@ package com.palantir.baseline.errorprone; +import com.google.common.collect.Lists; import com.google.errorprone.VisitorState; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.fixes.SuggestedFixes; +import com.google.errorprone.util.ErrorProneToken; import com.sun.source.tree.IdentifierTree; import com.sun.source.tree.MemberSelectTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.Tree; import com.sun.tools.javac.code.Type; +import com.sun.tools.javac.parser.Tokens; import com.sun.tools.javac.tree.JCTree; +import java.util.List; import java.util.stream.Collectors; import javax.annotation.Nullable; +import javax.lang.model.element.Name; /** * Additional utility functionality for {@link SuggestedFix} objects. @@ -67,6 +72,41 @@ static SuggestedFix renameInvocationRetainingTypeArguments( return fix.replace(startPos, endPos, extra + newMethodName).build(); } + /** + * Replaces the name of the method being invoked in {@code tree} with {@code replacement}. + * This implementation is forked from upstream error-prone to work around a bug that results in + * parameters being renamed in some scenarios: https://github.com/google/error-prone/issues/1451. + */ + public static SuggestedFix renameMethodInvocation( + MethodInvocationTree tree, + String replacement, + VisitorState state) { + Tree methodSelect = tree.getMethodSelect(); + Name identifier; + int startPos; + if (methodSelect instanceof MemberSelectTree) { + identifier = ((MemberSelectTree) methodSelect).getIdentifier(); + startPos = state.getEndPosition(((MemberSelectTree) methodSelect).getExpression()); + } else if (methodSelect instanceof IdentifierTree) { + identifier = ((IdentifierTree) methodSelect).getName(); + startPos = ((JCTree) tree).getStartPosition(); + } else { + throw new IllegalStateException("Malformed tree:\n" + state.getSourceForNode(tree)); + } + List tokens = state.getOffsetTokens(startPos, state.getEndPosition(tree)); + int depth = 0; + for (ErrorProneToken token : Lists.reverse(tokens)) { + if (depth == 0 && token.kind() == Tokens.TokenKind.IDENTIFIER && token.name().equals(identifier)) { + return SuggestedFix.replace(token.pos(), token.endPos(), replacement); + } else if (token.kind() == Tokens.TokenKind.RPAREN) { + depth++; + } else if (token.kind() == Tokens.TokenKind.LPAREN) { + depth--; + } + } + throw new IllegalStateException("Malformed tree:\n" + state.getSourceForNode(tree)); + } + /** * Identical to {@link SuggestedFixes#qualifyType(VisitorState, SuggestedFix.Builder, String)} unless the * compiling JVM is not supported by error-prone (JDK13) in which case a fallback is attempted. diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ReadReturnValueIgnored.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ReadReturnValueIgnored.java index 412bb9d00..b1db373f0 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ReadReturnValueIgnored.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ReadReturnValueIgnored.java @@ -25,7 +25,6 @@ import com.google.errorprone.bugpatterns.AbstractReturnValueIgnored; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.fixes.SuggestedFix; -import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.matchers.Matchers; @@ -121,7 +120,7 @@ public Description describeReturnValueIgnored(MethodInvocationTree methodInvocat } if (RAF_BUFFER_READ_MATCHER.matches(methodInvocationTree, state)) { return buildDescription(methodInvocationTree) - .addFix(SuggestedFixes.renameMethodInvocation(methodInvocationTree, "readFully", state)) + .addFix(MoreSuggestedFixes.renameMethodInvocation(methodInvocationTree, "readFully", state)) .build(); } if (READER_SKIP_MATCHER.matches(methodInvocationTree, state)) { diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ReverseDnsLookup.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ReverseDnsLookup.java index 83f0bff66..3892a7d62 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ReverseDnsLookup.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ReverseDnsLookup.java @@ -20,7 +20,6 @@ import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; -import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.matchers.method.MethodMatchers; @@ -56,7 +55,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState // Suggested fix exists to provide context when compilation fails, it shouldn't be used // as a drop in replacement because the unresolved string may not be sufficient in some // cases, particularly involving auditing. - .addFix(SuggestedFixes.renameMethodInvocation(tree, "getHostString", state)) + .addFix(MoreSuggestedFixes.renameMethodInvocation(tree, "getHostString", state)) .build(); } if (INET_ADDRESS_MATCHER.matches(tree, state)) { @@ -64,7 +63,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState // Suggested fix exists to provide context when compilation fails, it shouldn't be used // as a drop in replacement because the unresolved string may not be sufficient in some // cases, particularly involving auditing. - .addFix(SuggestedFixes.renameMethodInvocation(tree, "getHostAddress", state)) + .addFix(MoreSuggestedFixes.renameMethodInvocation(tree, "getHostAddress", state)) .build(); } return Description.NO_MATCH; diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/Slf4jLevelCheck.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/Slf4jLevelCheck.java index d3669c54d..3eb973b94 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/Slf4jLevelCheck.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/Slf4jLevelCheck.java @@ -23,7 +23,6 @@ import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.IfTreeMatcher; -import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.matchers.method.MethodMatchers; @@ -86,7 +85,7 @@ public Description matchIf(IfTree tree, VisitorState state) { return Description.NO_MATCH; } return buildDescription(tree) - .addFix(SuggestedFixes.renameMethodInvocation( + .addFix(MoreSuggestedFixes.renameMethodInvocation( levelCheckInvocation, mostSevere.levelCheckMethodName(), state)) .build(); } diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/UnsafeGaugeRegistration.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/UnsafeGaugeRegistration.java index eed2adcb2..f4eb12d1d 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/UnsafeGaugeRegistration.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/UnsafeGaugeRegistration.java @@ -21,7 +21,6 @@ import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.AbstractReturnValueIgnored; import com.google.errorprone.bugpatterns.BugChecker; -import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.matchers.method.MethodMatchers; @@ -65,7 +64,8 @@ public Description matchMethodInvocation(MethodInvocationTree methodInvocationTr return Description.NO_MATCH; } return buildDescription(methodInvocationTree) - .addFix(SuggestedFixes.renameMethodInvocation(methodInvocationTree, REGISTER_WITH_REPLACEMENT, state)) + .addFix(MoreSuggestedFixes.renameMethodInvocation( + methodInvocationTree, REGISTER_WITH_REPLACEMENT, state)) .build(); } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/UnsafeGaugeRegistrationTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/UnsafeGaugeRegistrationTest.java index 69705da3d..861a44a7a 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/UnsafeGaugeRegistrationTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/UnsafeGaugeRegistrationTest.java @@ -67,8 +67,8 @@ void testKnownBug() { "import com.palantir.tritium.metrics.registry.TaggedMetricRegistry;", "class Test {", " void f(TaggedMetricRegistry registry, MetricName name, Gauge gauge) {", - // This isn't right. Filed https://github.com/google/error-prone/issues/1451 - " registry.gauge(name, registerWithReplacement);", + // Tests our workaround for https://github.com/google/error-prone/issues/1451 + " registry.registerWithReplacement(name, gauge);", " }", "}" ) From c6584a463e04a82b6bd9a24382fde01ac766db1d Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 16 Dec 2019 10:59:23 -0500 Subject: [PATCH 5/6] refaster java 11... --- .../palantir/baseline/refaster/TritiumReplaceGaugeTest.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/baseline-refaster-rules/src/test/java/com/palantir/baseline/refaster/TritiumReplaceGaugeTest.java b/baseline-refaster-rules/src/test/java/com/palantir/baseline/refaster/TritiumReplaceGaugeTest.java index 431bd5d39..78c9140ac 100644 --- a/baseline-refaster-rules/src/test/java/com/palantir/baseline/refaster/TritiumReplaceGaugeTest.java +++ b/baseline-refaster-rules/src/test/java/com/palantir/baseline/refaster/TritiumReplaceGaugeTest.java @@ -18,10 +18,15 @@ import org.junit.Test; +import static org.assertj.core.api.Assumptions.assumeThat; + public class TritiumReplaceGaugeTest { @Test public void test() { + assumeThat(System.getProperty("java.specification.version")) + .describedAs("Refaster does not fully support java 11") + .isEqualTo("1.8"); RefasterTestHelper .forRefactoring(TritiumReplaceGauge.class) .withInputLines( From eb62e2314ae4dd65a95c2875b2f8e8e311b85ab5 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 16 Dec 2019 11:08:08 -0500 Subject: [PATCH 6/6] style --- .../palantir/baseline/refaster/TritiumReplaceGaugeTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/baseline-refaster-rules/src/test/java/com/palantir/baseline/refaster/TritiumReplaceGaugeTest.java b/baseline-refaster-rules/src/test/java/com/palantir/baseline/refaster/TritiumReplaceGaugeTest.java index 78c9140ac..d57f21cbb 100644 --- a/baseline-refaster-rules/src/test/java/com/palantir/baseline/refaster/TritiumReplaceGaugeTest.java +++ b/baseline-refaster-rules/src/test/java/com/palantir/baseline/refaster/TritiumReplaceGaugeTest.java @@ -16,10 +16,10 @@ package com.palantir.baseline.refaster; -import org.junit.Test; - import static org.assertj.core.api.Assumptions.assumeThat; +import org.junit.Test; + public class TritiumReplaceGaugeTest { @Test