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.

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

Replace out instances of RateLimiter.create(rate, Duration.ZERO) #1958

Merged
merged 2 commits into from
Nov 4, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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<ExpressionTree> 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;
}
}
Original file line number Diff line number Diff line change
@@ -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;",
Copy link
Contributor

@kevin-yu-0602 kevin-yu-0602 Nov 4, 2021

Choose a reason for hiding this comment

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

No action required. Wondering if it is common/feasible to do import removal in cases like these. Or is there no way of guaranteeing the import isn't being used elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great question: We could search the CompilationUnit for uses of any given import, but the fun part is that we don't have to :-) We always follow up automated refactors with our code formatter, that way the results become pretty, and unused imports are removed automatically!

"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();
}

Copy link
Contributor

@kevin-yu-0602 kevin-yu-0602 Nov 4, 2021

Choose a reason for hiding this comment

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

nit: Could benefit from a sanity check that tests double arguments, i.e: RateLimiter.create(10, 100, TimeUnit.SECONDS);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figure we can add that when we fix the constant-zero case for the value-and-unit overload -- at the moment the check doesn't look at other overloads.

private RefactoringValidator fix() {
return RefactoringValidator.of(ZeroWarmupRateLimiter.class, getClass());
}
}
6 changes: 6 additions & 0 deletions changelog/@unreleased/pr-1958.v2.yml
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ public class BaselineErrorProneExtension {
// "ThrowSpecificity",
"UnsafeGaugeRegistration",
"VarUsage",
"ZeroWarmupRateLimiter",

// Built-in checks
"ArrayEquals",
Expand Down