Skip to content

Commit

Permalink
Suggest more possible fixes for CheckReturnValue errors.
Browse files Browse the repository at this point in the history 
PiperOrigin-RevId: 478925901
  • Loading branch information
@cpovirk
cpovirk authored and Error Prone Team committed Oct 5, 2022
1 parent 266a16e commit a66e856
Show file tree
Hide file tree
Showing 6 changed files with 463 additions and 3 deletions.
99 changes: 99 additions & 0 deletions core/src/main/java/com/google/errorprone/bugpatterns/AbstractReturnValueIgnored.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,19 @@
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Suppliers.memoize;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.common.collect.Lists.reverse;
import static com.google.common.collect.Multimaps.toMultimap;
import static com.google.errorprone.fixes.SuggestedFix.delete;
import static com.google.errorprone.fixes.SuggestedFix.postfixWith;
import static com.google.errorprone.fixes.SuggestedFix.prefixWith;
import static com.google.errorprone.fixes.SuggestedFixes.qualifyStaticImport;
import static com.google.errorprone.matchers.Description.NO_MATCH;
import static com.google.errorprone.matchers.Matchers.allOf;
import static com.google.errorprone.matchers.Matchers.isThrowingFunctionalInterface;
import static com.google.errorprone.matchers.Matchers.not;
import static com.google.errorprone.matchers.Matchers.parentNode;
import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod;
import static com.google.errorprone.util.ASTHelpers.enclosingClass;
import static com.google.errorprone.util.ASTHelpers.getResultType;
import static com.google.errorprone.util.ASTHelpers.getRootAssignable;
Expand All @@ -36,10 +41,16 @@
import static com.google.errorprone.util.ASTHelpers.getUpperBound;
import static com.google.errorprone.util.ASTHelpers.isSubtype;
import static com.google.errorprone.util.ASTHelpers.isVoidType;
import static com.google.errorprone.util.ASTHelpers.matchingMethods;
import static com.google.errorprone.util.FindIdentifiers.findAllIdents;
import static com.sun.source.tree.Tree.Kind.EXPRESSION_STATEMENT;
import static com.sun.source.tree.Tree.Kind.MEMBER_SELECT;
import static com.sun.source.tree.Tree.Kind.METHOD_INVOCATION;
import static com.sun.source.tree.Tree.Kind.NEW_CLASS;
import static com.sun.tools.javac.parser.Tokens.TokenKind.RPAREN;
import static java.lang.String.format;
import static java.util.stream.IntStream.range;
import static java.util.stream.Stream.concat;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
Expand All @@ -63,6 +74,7 @@
import com.sun.source.tree.LambdaExpressionTree;
import com.sun.source.tree.MemberReferenceTree;
import com.sun.source.tree.MemberReferenceTree.ReferenceMode;
import com.sun.source.tree.MemberSelectTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.NewClassTree;
Expand All @@ -81,6 +93,7 @@
import java.util.Queue;
import java.util.Set;
import java.util.function.Supplier;
import java.util.stream.Stream;
import javax.lang.model.element.Name;
import javax.lang.model.type.TypeKind;

Expand Down Expand Up @@ -256,6 +269,64 @@ final ImmutableList<Fix> fixesAtCallSite(ExpressionTree invocationTree, VisitorS
* Luckily, they're not a ton harder to include than plain code comments would be.
*/
ImmutableMap.Builder<String, SuggestedFix> fixes = ImmutableMap.builder();
if (MOCKITO_VERIFY.matches(invocationTree, state)) {
ExpressionTree maybeCallToMock =
((MethodInvocationTree) invocationTree).getArguments().get(0);
if (maybeCallToMock.getKind() == METHOD_INVOCATION) {
ExpressionTree maybeMethodSelectOnMock =
((MethodInvocationTree) maybeCallToMock).getMethodSelect();
if (maybeMethodSelectOnMock.getKind() == MEMBER_SELECT) {
MemberSelectTree maybeSelectOnMock = (MemberSelectTree) maybeMethodSelectOnMock;
// For this suggestion, we want to move the closing parenthesis:
// verify(foo .bar())
// ^ v
// +------+
//
// The result is:
// verify(foo).bar()
//
// TODO(cpovirk): Suggest this only if `foo` looks like an actual mock object.
SuggestedFix.Builder fix = SuggestedFix.builder();
fix.postfixWith(maybeSelectOnMock.getExpression(), ")");
int closingParen =
reverse(state.getOffsetTokensForNode(invocationTree)).stream()
.filter(t -> t.kind() == RPAREN)
.findFirst()
.get()
.pos();
fix.replace(closingParen, closingParen + 1, "");
fixes.put(
format("Verify that %s was called", maybeSelectOnMock.getIdentifier()), fix.build());
}
}
}
if (resultType != null && resultType.getKind() == TypeKind.BOOLEAN) {
// Fix by calling either assertThat(...).isTrue() or verify(...).
if (state.errorProneOptions().isTestOnlyTarget()) {
SuggestedFix.Builder fix = SuggestedFix.builder();
fix.prefixWith(
invocationTree,
qualifyStaticImport("com.google.common.truth.Truth.assertThat", fix, state) + "(")
.postfixWith(invocationTree, ").isTrue()");
fixes.put("Assert that the result is true", fix.build());
} else {
SuggestedFix.Builder fix = SuggestedFix.builder();
fix.prefixWith(
invocationTree,
qualifyStaticImport("com.google.common.base.Verify.verify", fix, state) + "(")
.postfixWith(invocationTree, ")");
fixes.put("Insert a runtime check that the result is true", fix.build());
}
} else if (resultType != null
// By looking for any isTrue() method, we handle not just Truth but also AssertJ.
&& matchingMethods(
NAME_OF_IS_TRUE.get(state),
m -> m.getParameters().isEmpty(),
resultType,
state.getTypes())
.anyMatch(m -> true)) {
fixes.put("Assert that the result is true", postfixWith(invocationTree, ".isTrue()"));
}
if (identifierExpr != null
&& symbol != null
&& !symbol.name.contentEquals("this")
Expand All @@ -265,9 +336,31 @@ final ImmutableList<Fix> fixesAtCallSite(ExpressionTree invocationTree, VisitorS
"Assign result back to variable",
prefixWith(invocationTree, state.getSourceForNode(identifierExpr) + " = "));
}
/*
* TODO(cpovirk): Suggest returning the value from the enclosing method where possible... *if*
* we can find a good heuristic. We could consider "Is the return type a protobuf" and/or "Is
* this a constructor call or build() call?"
*/
if (parent.getKind() == EXPRESSION_STATEMENT
&& !constantExpressions.constantExpression(invocationTree, state).isPresent()) {
ImmutableSet<String> identifiersInScope =
findAllIdents(state).stream().map(v -> v.name.toString()).collect(toImmutableSet());
concat(Stream.of("unused"), range(2, 10).mapToObj(i -> "unused" + i))
// TODO(b/72928608): Handle even local variables declared *later* within this scope.
// TODO(b/250568455): Also check whether we have suggested this name before in this scope.
.filter(n -> !identifiersInScope.contains(n))
.findFirst()
.ifPresent(
n ->
fixes.put(
"Suppress error by assigning to a variable",
prefixWith(parent, format("var %s = ", n))));
}
if (parent.getKind() == EXPRESSION_STATEMENT) {
if (constantExpressions.constantExpression(invocationTree, state).isPresent()) {
fixes.put("Delete call", delete(parent));
} else {
fixes.put("Delete call and any side effects", delete(parent));
}
}
return fixes.buildOrThrow().entrySet().stream()
Expand Down Expand Up @@ -523,4 +616,10 @@ public Description matchReturn(ReturnTree tree, VisitorState state) {
}
return NO_MATCH;
}

private static final Matcher<ExpressionTree> MOCKITO_VERIFY =
staticMethod().onClass("org.mockito.Mockito").named("verify");

private static final com.google.errorprone.suppliers.Supplier<com.sun.tools.javac.util.Name>
NAME_OF_IS_TRUE = VisitorState.memoize(state -> state.getName("isTrue"));
}
44 changes: 43 additions & 1 deletion core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,14 @@
import static com.google.errorprone.bugpatterns.checkreturnvalue.ResultUsePolicy.OPTIONAL;
import static com.google.errorprone.bugpatterns.checkreturnvalue.Rules.globalDefault;
import static com.google.errorprone.bugpatterns.checkreturnvalue.Rules.mapAnnotationSimpleName;
import static com.google.errorprone.fixes.SuggestedFix.emptyFix;
import static com.google.errorprone.fixes.SuggestedFixes.qualifyType;
import static com.google.errorprone.util.ASTHelpers.getAnnotationsWithSimpleName;
import static com.google.errorprone.util.ASTHelpers.getSymbol;
import static com.google.errorprone.util.ASTHelpers.getType;
import static com.google.errorprone.util.ASTHelpers.hasDirectAnnotationWithSimpleName;
import static com.google.errorprone.util.ASTHelpers.isGeneratedConstructor;
import static com.sun.source.tree.Tree.Kind.METHOD;

import com.google.common.collect.ImmutableMap;
import com.google.errorprone.BugPattern;
Expand All @@ -44,6 +49,8 @@
import com.google.errorprone.bugpatterns.checkreturnvalue.PackagesRule;
import com.google.errorprone.bugpatterns.checkreturnvalue.ResultUsePolicy;
import com.google.errorprone.bugpatterns.checkreturnvalue.ResultUsePolicyEvaluator;
import com.google.errorprone.fixes.Fix;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.util.ASTHelpers;
Expand All @@ -55,12 +62,16 @@
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.NewClassTree;
import com.sun.source.util.TreePath;
import com.sun.source.util.Trees;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Symbol.MethodSymbol;
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.code.Type.MethodType;
import com.sun.tools.javac.processing.JavacProcessingEnvironment;
import java.util.Optional;
import javax.lang.model.element.ElementKind;
import org.checkerframework.checker.nullness.qual.Nullable;

/**
* @author [email protected] (Eddie Aftandilian)
Expand Down Expand Up @@ -266,6 +277,7 @@ private Description describeInvocationResultIgnored(
+ "%s",
shortCall, shortCallWithoutNew, apiTrailer(symbol, state));
return buildDescription(invocationTree)
.addFix(fixAtDeclarationSite(symbol, state))
.addAllFixes(fixesAtCallSite(invocationTree, state))
.setMessage(message)
.build();
Expand Down Expand Up @@ -336,7 +348,10 @@ protected Description describeReturnValueIgnored(MemberReferenceTree tree, Visit
parensAndMaybeEllipsis,
shortCallWithoutNew,
apiTrailer(symbol, state));
return buildDescription(tree).setMessage(message).build();
return buildDescription(tree)
.setMessage(message)
.addFix(fixAtDeclarationSite(symbol, state))
.build();
}

private String apiTrailer(MethodSymbol symbol, VisitorState state) {
Expand All @@ -360,4 +375,31 @@ enum MessageTrailerStyle {
NONE,
API_ERASED_SIGNATURE,
}

/** Returns a fix that adds {@code @CanIgnoreReturnValue} to the given symbol, if possible. */
private static Fix fixAtDeclarationSite(MethodSymbol symbol, VisitorState state) {
MethodTree method = findDeclaration(symbol, state);
if (method == null || isGeneratedConstructor(method)) {
return emptyFix();
}
SuggestedFix.Builder fix = SuggestedFix.builder();
fix.prefixWith(
method, "@" + qualifyType(state, fix, CanIgnoreReturnValue.class.getName()) + " ");
getAnnotationsWithSimpleName(method.getModifiers().getAnnotations(), CHECK_RETURN_VALUE)
.forEach(fix::delete);
fix.setShortDescription("Annotate the method with @CanIgnoreReturnValue");
return fix.build();
}

private static @Nullable MethodTree findDeclaration(Symbol symbol, VisitorState state) {
JavacProcessingEnvironment javacEnv = JavacProcessingEnvironment.instance(state.context);
TreePath declPath = Trees.instance(javacEnv).getPath(symbol);
// Skip fields declared in other compilation units since we can't make a fix for them here.
if (declPath != null
&& declPath.getCompilationUnit() == state.getPath().getCompilationUnit()
&& (declPath.getLeaf().getKind() == METHOD)) {
return (MethodTree) declPath.getLeaf();
}
return null;
}
}
1 change: 1 addition & 0 deletions core/src/main/java/com/google/errorprone/bugpatterns/ConstantPatternCompile.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,7 @@ private static boolean isArgStaticAndConstant(ExpressionTree arg) {
return (argSymbol.flags() & Flags.STATIC) != 0;
}

// TODO(b/250568455): Make this more widely available.
private static final class NameUniquifier {
final Multiset<String> assignmentCounts = HashMultiset.create();

Expand Down
Loading

0 comments on commit a66e856

Please sign in to comment.