From 9a4b7226fbbc41575134a600efbf8e2ff3558b6f Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Thu, 4 Nov 2021 16:46:51 -0400 Subject: [PATCH] Replace out instances of `RateLimiter.create(rate, Duration.ZERO)` (#1958) --- .../errorprone/ZeroWarmupRateLimiter.java | 84 ++++++++++++++++++ .../errorprone/ZeroWarmupRateLimiterTest.java | 87 +++++++++++++++++++ changelog/@unreleased/pr-1958.v2.yml | 6 ++ .../BaselineErrorProneExtension.java | 1 + 4 files changed, 178 insertions(+) create mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ZeroWarmupRateLimiter.java create mode 100644 baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ZeroWarmupRateLimiterTest.java create mode 100644 changelog/@unreleased/pr-1958.v2.yml diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ZeroWarmupRateLimiter.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ZeroWarmupRateLimiter.java new file mode 100644 index 000000000..2476d9e3b --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ZeroWarmupRateLimiter.java @@ -0,0 +1,84 @@ +/* + * (c) Copyright 2021 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.common.util.concurrent.RateLimiter; +import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.LinkType; +import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.ChildMultiMatcher.MatchType; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.Matchers; +import com.google.errorprone.matchers.method.MethodMatchers; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Symbol.VarSymbol; +import java.time.Duration; + +@AutoService(BugChecker.class) +@BugPattern( + name = "ZeroWarmupRateLimiter", + link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", + linkType = LinkType.CUSTOM, + severity = SeverityLevel.ERROR, + summary = "RateLimiters with zero warmup duration do not rate limit. " + + "Tracked at https://github.com/google/guava/issues/2730") +public final class ZeroWarmupRateLimiter extends BugChecker implements BugChecker.MethodInvocationTreeMatcher { + + private static final Matcher MATCHER = Matchers.methodInvocation( + MethodMatchers.staticMethod() + .onClass(RateLimiter.class.getName()) + .named("create") + .withParameters("double", Duration.class.getName()), + MatchType.LAST, + ZeroWarmupRateLimiter::isDurationZero); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (MATCHER.matches(tree, state)) { + return buildDescription(tree) + .addFix(SuggestedFix.replace( + state.getEndPosition(tree.getArguments().get(0)), + state.getEndPosition(tree.getArguments().get(1)), + "")) + .build(); + } + return Description.NO_MATCH; + } + + /** + * Returns true if the input tree is {@link Duration#ZERO}. + * Note this only supports the constant reference for now, not the value+unit overload or {@code Duration.ofX(0)}. + */ + private static boolean isDurationZero(ExpressionTree expressionTree, VisitorState state) { + Symbol symbol = ASTHelpers.getSymbol(expressionTree); + if (symbol != null && symbol.isStatic() && symbol instanceof VarSymbol) { + VarSymbol varSymbol = (VarSymbol) symbol; + return varSymbol.name.contentEquals("ZERO") + && state.getTypes() + .isSameType(varSymbol.owner.type, state.getTypeFromString(Duration.class.getName())); + } + return false; + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ZeroWarmupRateLimiterTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ZeroWarmupRateLimiterTest.java new file mode 100644 index 000000000..e6bc5b0dc --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ZeroWarmupRateLimiterTest.java @@ -0,0 +1,87 @@ +/* + * (c) Copyright 2021 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 org.junit.jupiter.api.Test; + +class ZeroWarmupRateLimiterTest { + + @Test + public void should_remove_duration_zero() { + fix().addInputLines( + "Test.java", + "import com.google.common.util.concurrent.RateLimiter;", + "import java.time.Duration;", + "class Test {", + " void f() {", + " RateLimiter.create(10, Duration.ZERO);", + " }", + "}") + .addOutputLines( + "Test.java", + "import com.google.common.util.concurrent.RateLimiter;", + "import java.time.Duration;", + "class Test {", + " void f() {", + " RateLimiter.create(10);", + " }", + "}") + .doTest(); + } + + @Test + public void should_remove_duration_zero_static_import() { + fix().addInputLines( + "Test.java", + "import com.google.common.util.concurrent.RateLimiter;", + "import static java.time.Duration.ZERO;", + "class Test {", + " void f() {", + " RateLimiter.create(10, ZERO);", + " }", + "}") + .addOutputLines( + "Test.java", + "import com.google.common.util.concurrent.RateLimiter;", + "import static java.time.Duration.ZERO;", + "class Test {", + " void f() {", + " RateLimiter.create(10);", + " }", + "}") + .doTest(); + } + + @Test + public void should_not_modify_existing_uses() { + fix().addInputLines( + "Test.java", + "import com.google.common.util.concurrent.RateLimiter;", + "import java.time.Duration;", + "class Test {", + " void f() {", + " RateLimiter.create(10, Duration.ofMillis(100));", + " }", + "}") + .expectUnchanged() + .doTest(); + } + + private RefactoringValidator fix() { + return RefactoringValidator.of(ZeroWarmupRateLimiter.class, getClass()); + } +} diff --git a/changelog/@unreleased/pr-1958.v2.yml b/changelog/@unreleased/pr-1958.v2.yml new file mode 100644 index 000000000..7436eabe6 --- /dev/null +++ b/changelog/@unreleased/pr-1958.v2.yml @@ -0,0 +1,6 @@ +type: fix +fix: + description: Replace out instances of `RateLimiter.create(rate, Duration.ZERO)` + which do not rate limit at all. See https://github.com/google/guava/issues/2730 + links: + - https://github.com/palantir/gradle-baseline/pull/1958 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 48e49b8d5..9251cf36d 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 @@ -76,6 +76,7 @@ public class BaselineErrorProneExtension { // "ThrowSpecificity", "UnsafeGaugeRegistration", "VarUsage", + "ZeroWarmupRateLimiter", // Built-in checks "ArrayEquals",