-
Notifications
You must be signed in to change notification settings - Fork 135
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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;", | ||
"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(); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()); | ||
} | ||
} |
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!