Skip to content

Commit fca0c24

Browse files
cpovirkError Prone Team
authored and
Error Prone Team
committed
More prefactoring to set us up to provide more suggested fixes.
This CL does provide descriptions for the existing fixes (which possibly no one will ever see), and it does make it theoretically possible for the checker to generate two fix options (though I somewhat doubt that both conditions can hold for any existing APIs). But its purpose is just to get some of the reshuffling out of the way in advance of "real" changes, such as by: - looking for a fix in the case of constructor calls (though probably neither of the currently existing fixes can apply) - preparing to generate multiple fixes PiperOrigin-RevId: 477830403
1 parent 1bbda1e commit fca0c24

File tree

2 files changed

+57
-27
lines changed

2 files changed

+57
-27
lines changed

core/src/main/java/com/google/errorprone/bugpatterns/AbstractReturnValueIgnored.java

Lines changed: 50 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,11 @@
1616

1717
package com.google.errorprone.bugpatterns;
1818

19+
import static com.google.common.base.Preconditions.checkArgument;
1920
import static com.google.common.base.Suppliers.memoize;
21+
import static com.google.common.collect.ImmutableList.toImmutableList;
2022
import static com.google.common.collect.Multimaps.toMultimap;
2123
import static com.google.errorprone.fixes.SuggestedFix.delete;
22-
import static com.google.errorprone.fixes.SuggestedFix.emptyFix;
2324
import static com.google.errorprone.fixes.SuggestedFix.prefixWith;
2425
import static com.google.errorprone.matchers.Description.NO_MATCH;
2526
import static com.google.errorprone.matchers.Matchers.allOf;
@@ -35,8 +36,12 @@
3536
import static com.google.errorprone.util.ASTHelpers.getUpperBound;
3637
import static com.google.errorprone.util.ASTHelpers.isSubtype;
3738
import static com.google.errorprone.util.ASTHelpers.isVoidType;
39+
import static com.sun.source.tree.Tree.Kind.EXPRESSION_STATEMENT;
40+
import static com.sun.source.tree.Tree.Kind.METHOD_INVOCATION;
41+
import static com.sun.source.tree.Tree.Kind.NEW_CLASS;
3842
import static java.lang.String.format;
3943

44+
import com.google.common.collect.ImmutableList;
4045
import com.google.common.collect.ImmutableListMultimap;
4146
import com.google.common.collect.ImmutableMap;
4247
import com.google.common.collect.ImmutableSet;
@@ -50,10 +55,10 @@
5055
import com.google.errorprone.bugpatterns.BugChecker.ReturnTreeMatcher;
5156
import com.google.errorprone.bugpatterns.threadsafety.ConstantExpressions;
5257
import com.google.errorprone.fixes.Fix;
58+
import com.google.errorprone.fixes.SuggestedFix;
5359
import com.google.errorprone.matchers.Description;
5460
import com.google.errorprone.matchers.Matcher;
5561
import com.google.errorprone.matchers.UnusedReturnValueMatcher;
56-
import com.sun.source.tree.ExpressionStatementTree;
5762
import com.sun.source.tree.ExpressionTree;
5863
import com.sun.source.tree.LambdaExpressionTree;
5964
import com.sun.source.tree.MemberReferenceTree;
@@ -208,39 +213,67 @@ protected boolean allowInExceptionThrowers() {
208213
protected Description describeReturnValueIgnored(
209214
MethodInvocationTree methodInvocationTree, VisitorState state) {
210215
return buildDescription(methodInvocationTree)
211-
.addFix(makeFix(methodInvocationTree, state))
216+
.addAllFixes(fixesAtCallSite(methodInvocationTree, state))
212217
.setMessage(getMessage(getSymbol(methodInvocationTree).getSimpleName()))
213218
.build();
214219
}
215220

216-
final Fix makeFix(MethodInvocationTree methodInvocationTree, VisitorState state) {
217-
Type returnType = getType(methodInvocationTree);
221+
final ImmutableList<Fix> fixesAtCallSite(ExpressionTree invocationTree, VisitorState state) {
222+
checkArgument(
223+
invocationTree.getKind() == METHOD_INVOCATION || invocationTree.getKind() == NEW_CLASS,
224+
"unexpected kind: %s",
225+
invocationTree.getKind());
226+
227+
Tree parent = state.getPath().getParentPath().getLeaf();
228+
229+
Type resultType = getType(invocationTree);
218230
// Find the root of the field access chain, i.e. a.intern().trim() ==> a.
219231
/*
220232
* TODO(cpovirk): Enhance getRootAssignable to return array accesses (e.g., `x[y]`)? If we do,
221233
* then we'll also need to accept `symbol == null` (which is fine, since all we need the symbol
222234
* for is to check against `this`, and `x[y]` is not `this`.)
223235
*/
224-
ExpressionTree identifierExpr = getRootAssignable(methodInvocationTree);
236+
ExpressionTree identifierExpr =
237+
invocationTree.getKind() == METHOD_INVOCATION
238+
? getRootAssignable((MethodInvocationTree) invocationTree)
239+
: null; // null root assignable for constructor calls (as well as some method calls)
225240
Symbol symbol = getSymbol(identifierExpr);
226241
Type identifierType = getType(identifierExpr);
227242

243+
/*
244+
* A map from short description to fix instance (even though every short description ultimately
245+
* will become _part of_ a fix instance later).
246+
*
247+
* As always, the order of suggested fixes can matter. In practice, it probably matters mostly
248+
* just to the checker's own tests. But it also affects the order in which the fixes are printed
249+
* during compile errors, and it affects which fix is chosen for automatically generated fix CLs
250+
* (though those should be rare inside Google: b/244334502#comment13).
251+
*
252+
* Note that, when possible, we have separate code that suggests adding @CanIgnoreReturnValue in
253+
* preference to all the fixes below.
254+
*
255+
* The _names_ of the fixes probably don't actually matter inside Google: b/204435834#comment4.
256+
* Luckily, they're not a ton harder to include than plain code comments would be.
257+
*/
258+
ImmutableMap.Builder<String, SuggestedFix> fixes = ImmutableMap.builder();
228259
if (identifierExpr != null
229260
&& symbol != null
230261
&& !symbol.name.contentEquals("this")
231-
&& returnType != null
232-
&& state.getTypes().isAssignable(returnType, identifierType)) {
233-
// Fix by assigning the result of the call to the root receiver reference.
234-
return prefixWith(methodInvocationTree, state.getSourceForNode(identifierExpr) + " = ");
235-
} else {
236-
// Unclear what the programmer intended. Delete since we don't know what else to do.
237-
Tree parent = state.getPath().getParentPath().getLeaf();
238-
if (parent instanceof ExpressionStatementTree
239-
&& constantExpressions.constantExpression(methodInvocationTree, state).isPresent()) {
240-
return delete(parent);
262+
&& resultType != null
263+
&& state.getTypes().isAssignable(resultType, identifierType)) {
264+
fixes.put(
265+
"Assign result back to variable",
266+
prefixWith(invocationTree, state.getSourceForNode(identifierExpr) + " = "));
267+
}
268+
if (parent.getKind() == EXPRESSION_STATEMENT) {
269+
if (constantExpressions.constantExpression(invocationTree, state).isPresent()) {
270+
fixes.put("Delete call", delete(parent));
241271
}
242272
}
243-
return emptyFix();
273+
return fixes.buildOrThrow().entrySet().stream()
274+
.map(
275+
e -> SuggestedFix.builder().merge(e.getValue()).setShortDescription(e.getKey()).build())
276+
.collect(toImmutableList());
244277
}
245278

246279
/**

core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@
3030
import static com.google.errorprone.bugpatterns.checkreturnvalue.ResultUsePolicy.OPTIONAL;
3131
import static com.google.errorprone.bugpatterns.checkreturnvalue.Rules.globalDefault;
3232
import static com.google.errorprone.bugpatterns.checkreturnvalue.Rules.mapAnnotationSimpleName;
33-
import static com.google.errorprone.fixes.SuggestedFix.emptyFix;
3433
import static com.google.errorprone.util.ASTHelpers.getSymbol;
3534
import static com.google.errorprone.util.ASTHelpers.getType;
3635
import static com.google.errorprone.util.ASTHelpers.hasDirectAnnotationWithSimpleName;
@@ -45,7 +44,6 @@
4544
import com.google.errorprone.bugpatterns.checkreturnvalue.PackagesRule;
4645
import com.google.errorprone.bugpatterns.checkreturnvalue.ResultUsePolicy;
4746
import com.google.errorprone.bugpatterns.checkreturnvalue.ResultUsePolicyEvaluator;
48-
import com.google.errorprone.fixes.Fix;
4947
import com.google.errorprone.matchers.Description;
5048
import com.google.errorprone.matchers.Matcher;
5149
import com.google.errorprone.util.ASTHelpers;
@@ -57,7 +55,6 @@
5755
import com.sun.source.tree.MethodInvocationTree;
5856
import com.sun.source.tree.MethodTree;
5957
import com.sun.source.tree.NewClassTree;
60-
import com.sun.source.tree.Tree;
6158
import com.sun.tools.javac.code.Symbol;
6259
import com.sun.tools.javac.code.Symbol.MethodSymbol;
6360
import com.sun.tools.javac.code.Type;
@@ -253,11 +250,10 @@ && hasDirectAnnotationWithSimpleName(ASTHelpers.getSymbol(tree), CAN_IGNORE_RETU
253250
}
254251

255252
private Description describeInvocationResultIgnored(
256-
Tree tree,
253+
ExpressionTree invocationTree,
257254
String shortCall,
258255
String shortCallWithoutNew,
259256
MethodSymbol symbol,
260-
Fix fix,
261257
VisitorState state) {
262258
String message =
263259
String.format(
@@ -269,16 +265,18 @@ private Description describeInvocationResultIgnored(
269265
+ " then annotate it with `@CanIgnoreReturnValue`.\n"
270266
+ "%s",
271267
shortCall, shortCallWithoutNew, apiTrailer(symbol, state));
272-
return buildDescription(tree).addFix(fix).setMessage(message).build();
268+
return buildDescription(invocationTree)
269+
.addAllFixes(fixesAtCallSite(invocationTree, state))
270+
.setMessage(message)
271+
.build();
273272
}
274273

275274
@Override
276275
protected Description describeReturnValueIgnored(MethodInvocationTree tree, VisitorState state) {
277276
MethodSymbol symbol = getSymbol(tree);
278277
String shortCall = symbol.name + (tree.getArguments().isEmpty() ? "()" : "(...)");
279278
String shortCallWithoutNew = shortCall;
280-
return describeInvocationResultIgnored(
281-
tree, shortCall, shortCallWithoutNew, symbol, makeFix(tree, state), state);
279+
return describeInvocationResultIgnored(tree, shortCall, shortCallWithoutNew, symbol, state);
282280
}
283281

284282
@Override
@@ -288,8 +286,7 @@ protected Description describeReturnValueIgnored(NewClassTree tree, VisitorState
288286
state.getSourceForNode(tree.getIdentifier())
289287
+ (tree.getArguments().isEmpty() ? "()" : "(...)");
290288
String shortCall = "new " + shortCallWithoutNew;
291-
return describeInvocationResultIgnored(
292-
tree, shortCall, shortCallWithoutNew, symbol, emptyFix(), state);
289+
return describeInvocationResultIgnored(tree, shortCall, shortCallWithoutNew, symbol, state);
293290
}
294291

295292
@Override

0 commit comments

Comments
 (0)