diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceof.java b/core/src/main/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceof.java index e5810018dff..fd46161dbe7 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceof.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceof.java @@ -21,9 +21,11 @@ import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import static com.google.errorprone.fixes.SuggestedFix.mergeFixes; import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.util.ASTHelpers.getSymbol; import static com.google.errorprone.util.ASTHelpers.getType; import static com.google.errorprone.util.SourceVersion.supportsPatternMatchingInstanceof; import static com.google.errorprone.util.TargetType.targetType; +import static java.lang.Boolean.TRUE; import static java.util.Collections.nCopies; import static java.util.stream.Collectors.joining; @@ -38,8 +40,10 @@ import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; import com.google.errorprone.util.Reachability; +import com.sun.source.tree.AssignmentTree; import com.sun.source.tree.BinaryTree; import com.sun.source.tree.BlockTree; +import com.sun.source.tree.CompoundAssignmentTree; import com.sun.source.tree.ConditionalExpressionTree; import com.sun.source.tree.IfTree; import com.sun.source.tree.InstanceOfTree; @@ -49,12 +53,17 @@ import com.sun.source.tree.Tree; import com.sun.source.tree.Tree.Kind; import com.sun.source.tree.TypeCastTree; +import com.sun.source.tree.UnaryTree; import com.sun.source.tree.VariableTree; import com.sun.source.util.TreePath; import com.sun.source.util.TreePathScanner; +import com.sun.source.util.TreeScanner; +import com.sun.tools.javac.code.Symbol.VarSymbol; import com.sun.tools.javac.code.Type; import com.sun.tools.javac.code.TypeTag; import java.util.HashSet; +import java.util.List; +import java.util.Set; import java.util.stream.Stream; import javax.inject.Inject; import javax.lang.model.SourceVersion; @@ -90,10 +99,8 @@ public Description matchInstanceOf(InstanceOfTree instanceOfTree, VisitorState s return NO_MATCH; } Type targetType = getType(instanceOfTree.getType()); - var allCasts = new HashSet<>(findAllCasts(constant, impliedStatements, targetType, state)); - String name = null; - SuggestedFix.Builder fix = SuggestedFix.builder(); + var allCasts = findAllCasts(constant, impliedStatements, targetType, state); int typeArgCount = getType(instanceOfTree.getType()).tsym.getTypeParameters().size(); if (typeArgCount != 0 && allCasts.stream() @@ -102,24 +109,43 @@ public Description matchInstanceOf(InstanceOfTree instanceOfTree, VisitorState s return NO_MATCH; } - // If we find a variable tree which exists only to be assigned the cast result, use that as - // the name and delete it. - // NOTE: an even nicer approach would be to delete all such VariableTrees, and rename all - // the names to one. That would require another scan, though. + // Find if we can delete at least one variable, and collect casts to replace. + // We prefer to delete a variable that holds the cast result and reuse its name, + // but we can only do so if it is not reassigned (since pattern variables are final). + Set castsToReplace = new HashSet<>(); + boolean hasCastsInExpressions = false; + VariableTree variableToDelete = null; + String name = null; + SuggestedFix.Builder fix = SuggestedFix.builder(); for (TreePath cast : allCasts) { VariableTree variableTree = isVariableAssignedFromCast(cast, instanceOfTree, state); if (variableTree != null) { - allCasts.remove(cast); - fix.delete(variableTree); - name = variableTree.getName().toString(); - break; + if (!isReassigned(variableTree, impliedStatements) && variableToDelete == null) { + // Use this variable's name and delete its declaration. + variableToDelete = variableTree; + name = variableTree.getName().toString(); + } else { + // The variable is reassigned (so we can't delete it or reuse its name as the pattern + // variable), or we already selected another variable to delete. + castsToReplace.add(cast); + } + } else { + // The cast is used in an expression, not just a simple variable assignment. + castsToReplace.add(cast); + hasCastsInExpressions = true; } } - if (!allCasts.isEmpty() || !fix.isEmpty()) { + // We refactor if we can delete a local variable and replace it with a pattern variable, + // OR if there are cast expressions that should be replaced with a pattern variable + // (even if some other variables assigned from the casts are reassigned elsewhere). + boolean shouldRefactor = variableToDelete != null || hasCastsInExpressions; + + if (shouldRefactor) { + if (variableToDelete != null) { + fix.delete(variableToDelete); + } if (name == null) { - // This is a gamble as to an appropriate name. We could make sure it doesn't clash with - // anything in scope, but that's effort. name = generateVariableName(targetType, state); } if (typeArgCount != 0 && !(instanceOfTree.getType() instanceof ParameterizedTypeTree)) { @@ -132,7 +158,7 @@ public Description matchInstanceOf(InstanceOfTree instanceOfTree, VisitorState s instanceOfTree, fix.postfixWith(instanceOfTree, " " + name) .merge( - allCasts.stream() + castsToReplace.stream() .map(c -> SuggestedFix.replace(c.getLeaf(), fn)) .collect(mergeFixes())) .build()); @@ -289,4 +315,43 @@ private static boolean requiresParentheses(TreePath path) { default -> true; }; } + + /** Returns true if the given variable is reassigned anywhere in the given trees. */ + private static boolean isReassigned(VariableTree variableTree, List trees) { + VarSymbol varSymbol = getSymbol(variableTree); + if (varSymbol == null) { + return false; + } + var scanner = + new TreeScanner() { + @Override + public Boolean visitAssignment(AssignmentTree node, Void unused) { + return varSymbol.equals(getSymbol(node.getVariable())) + || super.visitAssignment(node, null); + } + + @Override + public Boolean visitCompoundAssignment(CompoundAssignmentTree node, Void unused) { + return varSymbol.equals(getSymbol(node.getVariable())) + || super.visitCompoundAssignment(node, null); + } + + @Override + public Boolean visitUnary(UnaryTree node, Void unused) { + return (switch (node.getKind()) { + case POSTFIX_INCREMENT, POSTFIX_DECREMENT, PREFIX_INCREMENT, PREFIX_DECREMENT -> + varSymbol.equals(getSymbol(node.getExpression())); + default -> false; + }) + || super.visitUnary(node, null); + } + + @Override + public Boolean reduce(Boolean left, Boolean right) { + // be careful because the parameters can be null! + return TRUE.equals(left) || TRUE.equals(right); + } + }; + return scanner.scan(trees, null); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceofTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceofTest.java index ae9f4058fe6..9f00cbbf847 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceofTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceofTest.java @@ -1048,11 +1048,8 @@ void test(Object e) { } @Test - public void castVariableReassigned_brokenRefactoring() { - // TODO: b/395603588 - This refactoring is broken because 's' is reassigned, - // but pattern variables are implicitly final. It should be expectUnchanged(). + public void castVariableReassigned_noFinding() { helper - .allowBreakingChanges() .addInputLines( "Test.java", """ @@ -1066,19 +1063,47 @@ void test(Object o) { } } """) - .addOutputLines( + .expectUnchanged() + .doTest(); + } + + @Test + public void castVariableIncremented_noFinding() { + helper + .addInputLines( "Test.java", """ class Test { void test(Object o) { - if (o instanceof String s) { + if (o instanceof Integer) { + Integer i = (Integer) o; + i++; + System.out.println(i); + } + } + } + """) + .expectUnchanged() + .doTest(); + } - s = "new value"; - System.out.println(s); + @Test + public void castVariableCompoundAssigned_noFinding() { + helper + .addInputLines( + "Test.java", + """ + class Test { + void test(Object o) { + if (o instanceof Integer) { + Integer i = (Integer) o; + i += 1; + System.out.println(i); } } } """) + .expectUnchanged() .doTest(); }