Skip to content

Commit

Permalink
Add handling of FutureCombiner.callAsync
Browse files Browse the repository at this point in the history 
The initial bug pattern covered `Futures.transformAsync`. I recently noticed a similar issue with `FutureCombiner.callAsync`. Given that logic is pretty much identical and that `FutureCombiner` is a subclass of `Futures`, I have extended the existing bug pattern.

PiperOrigin-RevId: 735566050
  • Loading branch information
@java-team-github-bot
java-team-github-bot authored and Error Prone Team committed Mar 11, 2025
1 parent eac11e5 commit 82dbc53
Show file tree
Hide file tree
Showing 3 changed files with 187 additions and 37 deletions.
86 changes: 67 additions & 19 deletions core/src/main/java/com/google/errorprone/bugpatterns/FutureTransformAsync.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import com.sun.source.util.TreePath;
import com.sun.source.util.TreePathScanner;
import com.sun.tools.javac.code.Symbol;
import java.util.EnumSet;
import java.util.HashSet;
import java.util.Optional;

Expand All @@ -56,48 +57,95 @@
severity = WARNING)
public final class FutureTransformAsync extends BugChecker implements MethodInvocationTreeMatcher {

private static final ImmutableSet<String> CLASSES_WITH_STATIC_METHOD =
private enum Method {
TRANSFORM_ASYNC(TRANSFORM_ASYNC_MATCHER, "transform", false),
CALL_ASYNC(CALL_ASYNC_MATCHER, "call", true);

@SuppressWarnings("ImmutableEnumChecker")
private final Matcher<ExpressionTree> matcher;

private final String fixedName;
private final boolean canThrowCheckedException;

private Method(
Matcher<ExpressionTree> matcher, String fixedName, boolean canThrowCheckedException) {
this.matcher = matcher;
this.fixedName = fixedName;
this.canThrowCheckedException = canThrowCheckedException;
}
}

private static final ImmutableSet<String> CLASSES_WITH_TRANSFORM_ASYNC_STATIC_METHOD =
ImmutableSet.of(Futures.class.getName());

private static final ImmutableSet<String> CLASSES_WITH_INSTANCE_METHOD =
private static final ImmutableSet<String> CLASSES_WITH_TRANSFORM_ASYNC_INSTANCE_METHOD =
ImmutableSet.of(ClosingFuture.class.getName(), FluentFuture.class.getName());

private static final ImmutableSet<String> CLASSES_WITH_CALL_ASYNC_INSTANCE_METHOD =
ImmutableSet.of(Futures.FutureCombiner.class.getName());

private static final Matcher<ExpressionTree> TRANSFORM_ASYNC_MATCHER =
anyOf(
staticMethod().onClassAny(CLASSES_WITH_STATIC_METHOD).named("transformAsync"),
instanceMethod().onExactClassAny(CLASSES_WITH_INSTANCE_METHOD).named("transformAsync"));
staticMethod()
.onClassAny(CLASSES_WITH_TRANSFORM_ASYNC_STATIC_METHOD)
.named("transformAsync"),
instanceMethod()
.onExactClassAny(CLASSES_WITH_TRANSFORM_ASYNC_INSTANCE_METHOD)
.named("transformAsync"));

private static final Matcher<ExpressionTree> CALL_ASYNC_MATCHER =
instanceMethod().onExactClassAny(CLASSES_WITH_CALL_ASYNC_INSTANCE_METHOD).named("callAsync");

private static final Matcher<ExpressionTree> IMMEDIATE_FUTURE =
staticMethod()
.onClassAny(union(CLASSES_WITH_STATIC_METHOD, CLASSES_WITH_INSTANCE_METHOD))
.onClassAny(
union(
CLASSES_WITH_TRANSFORM_ASYNC_STATIC_METHOD,
CLASSES_WITH_TRANSFORM_ASYNC_INSTANCE_METHOD))
.named("immediateFuture");

private static final Matcher<ExpressionTree> IMMEDIATE_VOID_FUTURE =
staticMethod()
.onClassAny(union(CLASSES_WITH_STATIC_METHOD, CLASSES_WITH_INSTANCE_METHOD))
.onClassAny(
union(
CLASSES_WITH_TRANSFORM_ASYNC_STATIC_METHOD,
CLASSES_WITH_TRANSFORM_ASYNC_INSTANCE_METHOD))
.named("immediateVoidFuture");

@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
if (!TRANSFORM_ASYNC_MATCHER.matches(tree, state)) {
return NO_MATCH;
}
return getMethod(tree, state)
.map(method -> matchMethodInvocation(method, tree, state))
.orElse(NO_MATCH);
}

// Find the lambda expression. The transformAsync() methods might have different number of
// arguments, but they all have a single lambda. Also discard the lambdas that throw checked
// exceptions, since they cannot be supported by transform().
private Description matchMethodInvocation(
Method method, MethodInvocationTree tree, VisitorState state) {
// Find the lambda expression. The transformAsync() / callAsync() methods might have different
// number of arguments, but they all have a single lambda. In case of transformAsync(), discard
// the lambdas that throw checked exceptions, since they cannot be supported by transform().
Optional<LambdaExpressionTree> lambda =
tree.getArguments().stream()
.filter(LambdaExpressionTree.class::isInstance)
.map(arg -> (LambdaExpressionTree) arg)
.filter(lambdaTree -> !throwsCheckedException(lambdaTree, state))
.filter(
lambdaTree ->
method.canThrowCheckedException || !throwsCheckedException(lambdaTree, state))
.findFirst();

return lambda.map(lambdaTree -> handleTransformAsync(tree, lambdaTree, state)).orElse(NO_MATCH);
return lambda
.map(lambdaTree -> handleTransformAsync(method, tree, lambdaTree, state))
.orElse(NO_MATCH);
}

private static Optional<Method> getMethod(MethodInvocationTree tree, VisitorState state) {
return EnumSet.allOf(Method.class).stream()
.filter(method -> method.matcher.matches(tree, state))
.findFirst();
}

private Description handleTransformAsync(
MethodInvocationTree tree, LambdaExpressionTree lambda, VisitorState state) {
Method method, MethodInvocationTree tree, LambdaExpressionTree lambda, VisitorState state) {
HashSet<ExpressionTree> returnExpressions = new HashSet<>();
if (lambda.getBody() instanceof ExpressionTree) {
returnExpressions.add((ExpressionTree) lambda.getBody());
Expand Down Expand Up @@ -136,7 +184,7 @@ public Void visitReturn(ReturnTree tree, Void unused) {

if (areAllImmediateFutures) {
SuggestedFix.Builder fix = SuggestedFix.builder();
suggestFixTransformAsyncToTransform(tree, state, fix);
suggestFixTransformAsyncToTransform(method, tree, state, fix);
for (ExpressionTree expression : returnExpressions) {
suggestFixRemoveImmediateFuture((MethodInvocationTree) expression, state, fix);
}
Expand All @@ -153,20 +201,20 @@ private static boolean throwsCheckedException(LambdaExpressionTree lambda, Visit
}

/**
* Suggests fix to replace transformAsync with transform.
* Suggests fix to replace transformAsync/callAsync with transform/call.
*
* <p>If the transformAsync is imported as a static method, it takes care of adding the equivalent
* import static for transform.
*/
private static void suggestFixTransformAsyncToTransform(
MethodInvocationTree tree, VisitorState state, SuggestedFix.Builder fix) {
Method method, MethodInvocationTree tree, VisitorState state, SuggestedFix.Builder fix) {
ExpressionTree methodSelect = tree.getMethodSelect();
if (state.getSourceForNode(methodSelect).equals("transformAsync")) {
Symbol symbol = getSymbol(methodSelect);
String className = enclosingClass(symbol).getQualifiedName().toString();
fix.addStaticImport(className + "." + "transform");
}
fix.merge(SuggestedFixes.renameMethodInvocation(tree, "transform", state));
fix.merge(SuggestedFixes.renameMethodInvocation(tree, method.fixedName, state));
}

/** Suggests fix to remove the immediateFuture or immediateVoidFuture call. */
Expand Down
132 changes: 117 additions & 15 deletions core/src/test/java/com/google/errorprone/bugpatterns/FutureTransformAsyncTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -577,22 +577,22 @@ public void transformAsync_fluentFuture() {
.addInputLines(
"in/Test.java",
"""
import com.google.common.util.concurrent.FluentFuture;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import java.util.concurrent.Executor;
import com.google.common.util.concurrent.FluentFuture;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import java.util.concurrent.Executor;
class Test {
private Executor executor;
class Test {
private Executor executor;
ListenableFuture<String> test() {
ListenableFuture<String> future =
FluentFuture.from(Futures.immediateFuture(5))
.transformAsync(value -> Futures.immediateFuture("value: " + value), executor);
return future;
}
}
""")
ListenableFuture<String> test() {
ListenableFuture<String> future =
FluentFuture.from(Futures.immediateFuture(5))
.transformAsync(value -> Futures.immediateFuture("v: " + value), executor);
return future;
}
}
""")
.addOutputLines(
"out/Test.java",
"""
Expand All @@ -607,7 +607,109 @@ class Test {
ListenableFuture<String> test() {
ListenableFuture<String> future =
FluentFuture.from(Futures.immediateFuture(5))
.transform(value -> "value: " + value, executor);
.transform(value -> "v: " + value, executor);
return future;
}
}
""")
.doTest();
}

@Test
public void futureCombiner_callAsync() {
refactoringHelper
.addInputLines(
"in/Test.java",
"""
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import java.util.concurrent.Executor;
class Test {
private Executor executor;
ListenableFuture<String> test() {
ListenableFuture<Integer> future1 = Futures.immediateFuture(5);
ListenableFuture<Integer> future2 = Futures.immediateFuture(10);
ListenableFuture<String> future =
Futures.whenAllSucceed(future1, future2)
.callAsync(() -> Futures.immediateFuture("All values succeeded"), executor);
return future;
}
}
""")
.addOutputLines(
"out/Test.java",
"""
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import java.util.concurrent.Executor;
class Test {
private Executor executor;
ListenableFuture<String> test() {
ListenableFuture<Integer> future1 = Futures.immediateFuture(5);
ListenableFuture<Integer> future2 = Futures.immediateFuture(10);
ListenableFuture<String> future =
Futures.whenAllSucceed(future1, future2)
.call(() -> "All values succeeded", executor);
return future;
}
}
""")
.doTest();
}

@Test
public void futureCombiner_callAsyncWithCheckedException() {
refactoringHelper
.addInputLines(
"in/Test.java",
"""
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import java.util.concurrent.Executor;
class Test {
private Executor executor;
ListenableFuture<String> test() {
ListenableFuture<Integer> future1 = Futures.immediateFuture(5);
ListenableFuture<Integer> future2 = Futures.immediateFuture(10);
ListenableFuture<String> future =
Futures.whenAllSucceed(future1, future2)
.callAsync(
() -> {
int total = Futures.getDone(future1) + Futures.getDone(future2);
return Futures.immediateFuture("Sum = " + total);
},
executor);
return future;
}
}
""")
.addOutputLines(
"out/Test.java",
"""
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import java.util.concurrent.Executor;
class Test {
private Executor executor;
ListenableFuture<String> test() {
ListenableFuture<Integer> future1 = Futures.immediateFuture(5);
ListenableFuture<Integer> future2 = Futures.immediateFuture(10);
ListenableFuture<String> future =
Futures.whenAllSucceed(future1, future2)
.call(
() -> {
int total = Futures.getDone(future1) + Futures.getDone(future2);
return "Sum = " + total;
},
executor);
return future;
}
}
Expand Down
6 changes: 3 additions & 3 deletions docs/bugpattern/FutureTransformAsync.md
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
The usage of `transformAsync` is not necessary when all the return values of the
transformation function are immediate futures. In this case, the usage of
`transform` is preferred.
The usage of `transformAsync` and `callAsync` is not necessary when all the
return values of the transformation function are immediate futures. In this
case, the usage of `transform` and `call` is preferred.

Note that `transform` cannot be used if the body of the transformation function
throws checked exceptions.

0 comments on commit 82dbc53

Please sign in to comment.