diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/FutureTransformAsync.java b/core/src/main/java/com/google/errorprone/bugpatterns/FutureTransformAsync.java new file mode 100644 index 00000000000..7da7325dcca --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/FutureTransformAsync.java @@ -0,0 +1,191 @@ +/* + * Copyright 2024 The Error Prone Authors. + * + * 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.google.errorprone.bugpatterns; + +import static com.google.common.collect.Sets.union; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.matchers.Matchers.anyOf; +import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; +import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod; +import static com.google.errorprone.util.ASTHelpers.enclosingClass; +import static com.google.errorprone.util.ASTHelpers.getSymbol; +import static com.google.errorprone.util.ASTHelpers.getThrownExceptions; +import static com.google.errorprone.util.ASTHelpers.isCheckedExceptionType; + +import com.google.common.collect.ImmutableSet; +import com.google.common.util.concurrent.ClosingFuture; +import com.google.common.util.concurrent.FluentFuture; +import com.google.common.util.concurrent.Futures; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +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.sun.source.tree.BlockTree; +import com.sun.source.tree.ClassTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.LambdaExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.ReturnTree; +import com.sun.source.util.TreePath; +import com.sun.source.util.TreePathScanner; +import com.sun.tools.javac.code.Symbol; +import java.util.HashSet; +import java.util.Optional; + +/** See summary for details. */ +@BugPattern( + summary = "Use transform instead of transformAsync when all returns are an immediate future.", + severity = WARNING) +public final class FutureTransformAsync extends BugChecker implements MethodInvocationTreeMatcher { + + private static final ImmutableSet CLASSES_WITH_STATIC_METHOD = + ImmutableSet.of(Futures.class.getName()); + + private static final ImmutableSet CLASSES_WITH_INSTANCE_METHOD = + ImmutableSet.of(ClosingFuture.class.getName(), FluentFuture.class.getName()); + + private static final Matcher TRANSFORM_ASYNC_MATCHER = + anyOf( + staticMethod().onClassAny(CLASSES_WITH_STATIC_METHOD).named("transformAsync"), + instanceMethod().onExactClassAny(CLASSES_WITH_INSTANCE_METHOD).named("transformAsync")); + + private static final Matcher IMMEDIATE_FUTURE = + staticMethod() + .onClassAny(union(CLASSES_WITH_STATIC_METHOD, CLASSES_WITH_INSTANCE_METHOD)) + .named("immediateFuture"); + + private static final Matcher IMMEDIATE_VOID_FUTURE = + staticMethod() + .onClassAny(union(CLASSES_WITH_STATIC_METHOD, CLASSES_WITH_INSTANCE_METHOD)) + .named("immediateVoidFuture"); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (!TRANSFORM_ASYNC_MATCHER.matches(tree, state)) { + return 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(). + Optional lambda = + tree.getArguments().stream() + .filter(LambdaExpressionTree.class::isInstance) + .map(arg -> (LambdaExpressionTree) arg) + .filter(lambdaTree -> !throwsCheckedException(lambdaTree, state)) + .findFirst(); + + return lambda.map(lambdaTree -> handleTransformAsync(tree, lambdaTree, state)).orElse(NO_MATCH); + } + + private Description handleTransformAsync( + MethodInvocationTree tree, LambdaExpressionTree lambda, VisitorState state) { + HashSet returnExpressions = new HashSet<>(); + if (lambda.getBody() instanceof ExpressionTree) { + returnExpressions.add((ExpressionTree) lambda.getBody()); + } else if (lambda.getBody() instanceof BlockTree) { + new TreePathScanner() { + @Override + public Void visitLambdaExpression(LambdaExpressionTree node, Void unused) { + // don't descend into lambdas (to handle nested lambdas) + return null; + } + + @Override + public Void visitClass(ClassTree node, Void unused) { + // don't descend into classes (to handle nested classes) + return null; + } + + @Override + public Void visitReturn(ReturnTree tree, Void unused) { + returnExpressions.add(tree.getExpression()); + // Don't descend deeper into returns, since we already checked the body of this return. + return null; + } + }.scan(TreePath.getPath(state.getPath().getCompilationUnit(), lambda.getBody()), null); + } else { + return NO_MATCH; + } + + boolean areAllImmediateFutures = + returnExpressions.stream() + .allMatch( + expression -> + expression instanceof MethodInvocationTree + && (IMMEDIATE_FUTURE.matches(expression, state) + || IMMEDIATE_VOID_FUTURE.matches(expression, state))); + + if (areAllImmediateFutures) { + SuggestedFix.Builder fix = SuggestedFix.builder(); + suggestFixTransformAsyncToTransform(tree, state, fix); + for (ExpressionTree expression : returnExpressions) { + suggestFixRemoveImmediateFuture((MethodInvocationTree) expression, state, fix); + } + state.reportMatch(describeMatch(tree, fix.build())); + } + + return NO_MATCH; + } + + /** Returns true if the lambda throws a checked exception. */ + private static boolean throwsCheckedException(LambdaExpressionTree lambda, VisitorState state) { + return getThrownExceptions(lambda.getBody(), state).stream() + .anyMatch(type -> isCheckedExceptionType(type, state)); + } + + /** + * Suggests fix to replace transformAsync with transform. + * + *

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) { + 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)); + } + + /** Suggests fix to remove the immediateFuture or immediateVoidFuture call. */ + private static void suggestFixRemoveImmediateFuture( + MethodInvocationTree tree, VisitorState state, SuggestedFix.Builder fix) { + String typeArgument = ""; + String argument = ""; + if (IMMEDIATE_FUTURE.matches(tree, state)) { + var typeArguments = tree.getTypeArguments(); + if (typeArguments.size() == 1) { + typeArgument = state.getSourceForNode(typeArguments.get(0)); + } + argument = state.getSourceForNode(tree.getArguments().get(0)); + } else if (IMMEDIATE_VOID_FUTURE.matches(tree, state)) { + typeArgument = "Void"; + argument = "null"; + } + String fixString = + typeArgument.isEmpty() ? argument : String.format("(%s) %s", typeArgument, argument); + fix.replace(tree, fixString); + } +} diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index 682043064f7..7b4c5834598 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -156,6 +156,7 @@ import com.google.errorprone.bugpatterns.FunctionalInterfaceClash; import com.google.errorprone.bugpatterns.FunctionalInterfaceMethodChanged; import com.google.errorprone.bugpatterns.FutureReturnValueIgnored; +import com.google.errorprone.bugpatterns.FutureTransformAsync; import com.google.errorprone.bugpatterns.FuturesGetCheckedIllegalExceptionType; import com.google.errorprone.bugpatterns.FuzzyEqualsShouldNotBeUsedInEqualsMethod; import com.google.errorprone.bugpatterns.GetClassOnAnnotation; @@ -929,6 +930,7 @@ public static ScannerSupplier warningChecks() { FragmentInjection.class, FragmentNotInstantiable.class, FutureReturnValueIgnored.class, + FutureTransformAsync.class, GetClassOnEnum.class, GuiceNestedCombine.class, HidingField.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/FutureTransformAsyncTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/FutureTransformAsyncTest.java new file mode 100644 index 00000000000..3558e1d4e20 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/FutureTransformAsyncTest.java @@ -0,0 +1,592 @@ +/* + * Copyright 2024 The Error Prone Authors. + * + * 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.google.errorprone.bugpatterns; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Unit tests for {@link FutureTransformAsync}. */ +@RunWith(JUnit4.class) +public class FutureTransformAsyncTest { + private final CompilationTestHelper compilationHelper = + CompilationTestHelper.newInstance(FutureTransformAsync.class, getClass()); + + private final BugCheckerRefactoringTestHelper refactoringHelper = + BugCheckerRefactoringTestHelper.newInstance(FutureTransformAsync.class, getClass()); + + @Test + public void transformAsync_expressionLambda() { + 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 test() { + ListenableFuture future = + Futures.transformAsync( + Futures.immediateFuture(5), + value -> Futures.immediateFuture("value: " + value), + 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 test() { + ListenableFuture future = + Futures.transform( + Futures.immediateFuture(5), + value -> "value: " + value, + executor); + return future; + } + } + """) + .doTest(); + } + + @Test + public void transformAsync_statementLambda() { + 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 test() { + ListenableFuture future = + Futures.transformAsync( + Futures.immediateFuture(5), + value -> { + if (value > 5) { + return Futures.immediateFuture("large"); + } else if (value < 5) { + return Futures.immediateFuture("small"); + } + return Futures.immediateFuture("value: " + value); + }, + 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 test() { + ListenableFuture future = + Futures.transform( + Futures.immediateFuture(5), + value -> { + if (value > 5) { + return "large"; + } else if (value < 5) { + return "small"; + } + return "value: " + value; + }, + executor); + return future; + } + } + """) + .doTest(); + } + + @Test + public void transformAsync_notAllImmediateFutures() { + compilationHelper + .addSourceLines( + "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 foo(String s) { + return Futures.immediateFuture(s); + } + ListenableFuture test() { + ListenableFuture future = + Futures.transformAsync( + Futures.immediateFuture(5), + value -> { + if (value > 0) { + return foo("large"); + } + return Futures.immediateFuture("value: " + value); + }, + executor); + return future; + } + } + """) + .doTest(); + } + + @Test + public void transformAsync_statementLambda_throwsCheckedException() { + compilationHelper + .addSourceLines( + "in/Test.java", + """ + import com.google.common.util.concurrent.Futures; + import com.google.common.util.concurrent.ListenableFuture; + import java.io.FileNotFoundException; + import java.util.concurrent.Executor; + class Test { + private Executor executor; + ListenableFuture test() { + ListenableFuture future = + Futures.transformAsync( + Futures.immediateFuture(5), + value -> { + if (value > 0) { + throw new FileNotFoundException("large"); + } + return Futures.immediateFuture("value: " + value); + }, + executor); + return future; + } + } + """) + .doTest(); + } + + @Test + public void transformAsync_statementLambda_methodThrowsCheckedException() { + compilationHelper + .addSourceLines( + "in/Test.java", + """ + import com.google.common.util.concurrent.Futures; + import com.google.common.util.concurrent.ListenableFuture; + import java.io.FileNotFoundException; + import java.util.concurrent.Executor; + class Test { + private Executor executor; + private void throwIfLarge(int unused) throws FileNotFoundException { + } + ListenableFuture test() { + ListenableFuture future = + Futures.transformAsync( + Futures.immediateFuture(5), + value -> { + throwIfLarge(value); + return Futures.immediateFuture("value: " + value); + }, + executor); + return future; + } + } + """) + .doTest(); + } + + @Test + public void transformAsync_expressionLambda_methodThrowsCheckedException() { + compilationHelper + .addSourceLines( + "in/Test.java", + """ + import com.google.common.util.concurrent.Futures; + import com.google.common.util.concurrent.ListenableFuture; + import java.io.FileNotFoundException; + import java.util.concurrent.Executor; + class Test { + private Executor executor; + private String throwIfLarge(int value) throws FileNotFoundException { + return "value: " + value; + } + ListenableFuture test() { + ListenableFuture future = + Futures.transformAsync( + Futures.immediateFuture(5), + value -> Futures.immediateFuture(throwIfLarge(value)), + executor); + return future; + } + } + """) + .doTest(); + } + + @Test + public void transformAsync_uncheckedException() { + 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 test() { + ListenableFuture future = + Futures.transformAsync( + Futures.immediateFuture(5), + value -> { + if (value > 0) { + throw new IllegalStateException("large"); + } + return Futures.immediateFuture("value: " + value); + }, + 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 test() { + ListenableFuture future = + Futures.transform( + Futures.immediateFuture(5), + value -> { + if (value > 0) { + throw new IllegalStateException("large"); + } + return "value: " + value; + }, + executor); + return future; + } + } + """) + .doTest(); + } + + @Test + public void transformAsync_returnTransformAsyncResult() { + 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 test() { + return Futures.transformAsync( + Futures.immediateFuture(5), + value -> Futures.immediateFuture("value: " + value), + executor); + } + } + """) + .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 test() { + return Futures.transform( + Futures.immediateFuture(5), + value -> "value: " + value, + executor); + } + } + """) + .doTest(); + } + + @Test + public void transformAsync_staticImports() { + refactoringHelper + .addInputLines( + "in/Test.java", + """ + import static com.google.common.util.concurrent.Futures.immediateFuture; + import static com.google.common.util.concurrent.Futures.transformAsync; + 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 foo(String s) { + return immediateFuture(s); + } + ListenableFuture test() { + ListenableFuture future = + transformAsync( + foo("x"), + value -> immediateFuture("value: " + value), + executor); + return future; + } + } + """) + .addOutputLines( + "out/Test.java", + """ + import static com.google.common.util.concurrent.Futures.immediateFuture; + import static com.google.common.util.concurrent.Futures.transform; + import static com.google.common.util.concurrent.Futures.transformAsync; + 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 foo(String s) { + return immediateFuture(s); + } + ListenableFuture test() { + ListenableFuture future = + transform( + foo("x"), + value -> "value: " + value, + executor); + return future; + } + } + """) + .doTest(); + } + + @Test + public void transformAsync_immediateVoidFuture() { + 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 foo(String s) { + return Futures.immediateFuture(s); + } + ListenableFuture test() { + ListenableFuture future = + Futures.transformAsync( + foo("x"), + value -> Futures.immediateVoidFuture(), + 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 foo(String s) { + return Futures.immediateFuture(s); + } + ListenableFuture test() { + ListenableFuture future = + Futures.transform( + foo("x"), + value -> (Void) null, + executor); + return future; + } + } + """) + .doTest(); + } + + @Test + public void transformAsync_withTypeArgument() { + 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 test() { + ListenableFuture future = + Futures.transformAsync( + Futures.immediateFuture("x"), + value -> Futures.immediateFuture(null), + 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 test() { + ListenableFuture future = + Futures.transform( + Futures.immediateFuture("x"), + value -> (Void) null, + executor); + return future; + } + } + """) + .doTest(); + } + + @Test + public void transformAsync_nestedLambdas() { + 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; + interface TestInterface { + ListenableFuture apply(String value); + } + void foo(TestInterface unused) { + return; + } + ListenableFuture test() { + ListenableFuture future = + Futures.transformAsync( + Futures.immediateFuture("x"), + unused -> { + foo(x -> { + return Futures.immediateVoidFuture(); + }); + return Futures.immediateVoidFuture(); + }, + 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; + interface TestInterface { + ListenableFuture apply(String value); + } + void foo(TestInterface unused) { + return; + } + ListenableFuture test() { + ListenableFuture future = + Futures.transform( + Futures.immediateFuture("x"), + unused -> { + foo(x -> { + return Futures.immediateVoidFuture(); + }); + return (Void) null; + }, + executor); + return future; + } + } + """) + .doTest(); + } + + @Test + public void transformAsync_fluentFuture() { + refactoringHelper + .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; + class Test { + private Executor executor; + ListenableFuture test() { + ListenableFuture future = + FluentFuture.from(Futures.immediateFuture(5)) + .transformAsync( + value -> Futures.immediateFuture("value: " + value), + executor); + return future; + } + } + """) + .addOutputLines( + "out/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; + class Test { + private Executor executor; + ListenableFuture test() { + ListenableFuture future = + FluentFuture.from(Futures.immediateFuture(5)) + .transform( + value -> "value: " + value, + executor); + return future; + } + } + """) + .doTest(); + } +} diff --git a/docs/bugpattern/FutureTransformAsync.md b/docs/bugpattern/FutureTransformAsync.md new file mode 100644 index 00000000000..f87812c1d7b --- /dev/null +++ b/docs/bugpattern/FutureTransformAsync.md @@ -0,0 +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. + +Note that `transform` cannot be used if the body of the transformation function +throws checked exceptions.