From 25b61e3dba4ddc5acd8c1ea557b862b6f15b1e8d Mon Sep 17 00:00:00 2001 From: ghm Date: Thu, 21 Nov 2024 15:08:51 -0800 Subject: [PATCH] Worry about generics in PatternMatchingInstanceof. PiperOrigin-RevId: 698929262 --- .../google/errorprone/fixes/SuggestedFix.java | 6 + .../PatternMatchingInstanceof.java | 143 ++++++++++++------ .../PatternMatchingInstanceofTest.java | 38 ++++- 3 files changed, 135 insertions(+), 52 deletions(-) diff --git a/check_api/src/main/java/com/google/errorprone/fixes/SuggestedFix.java b/check_api/src/main/java/com/google/errorprone/fixes/SuggestedFix.java index feabf10f910..46b53643c7e 100644 --- a/check_api/src/main/java/com/google/errorprone/fixes/SuggestedFix.java +++ b/check_api/src/main/java/com/google/errorprone/fixes/SuggestedFix.java @@ -35,6 +35,7 @@ import java.util.LinkedHashSet; import java.util.List; import java.util.Set; +import java.util.stream.Collector; import org.jspecify.annotations.Nullable; /** @@ -168,6 +169,11 @@ public static SuggestedFix merge(SuggestedFix first, SuggestedFix second, Sugges return builder.build(); } + public static Collector mergeFixes() { + return Collector.of( + SuggestedFix::builder, Builder::merge, Builder::merge, SuggestedFix.Builder::build); + } + public static Builder builder() { return new Builder(); } 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 d461743fe39..f58f3ea7bbb 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceof.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceof.java @@ -17,6 +17,7 @@ package com.google.errorprone.bugpatterns; 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; @@ -25,96 +26,144 @@ import com.google.common.collect.ImmutableSet; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; -import com.google.errorprone.bugpatterns.BugChecker.IfTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.InstanceOfTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; -import com.sun.source.tree.BinaryTree; import com.sun.source.tree.BlockTree; -import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.IfTree; import com.sun.source.tree.InstanceOfTree; import com.sun.source.tree.ParenthesizedTree; -import com.sun.source.tree.Tree.Kind; +import com.sun.source.tree.StatementTree; +import com.sun.source.tree.Tree; import com.sun.source.tree.TypeCastTree; import com.sun.source.tree.VariableTree; -import com.sun.source.util.SimpleTreeVisitor; +import com.sun.source.util.TreePath; +import com.sun.source.util.TreePathScanner; import com.sun.tools.javac.code.Symbol.VarSymbol; +import com.sun.tools.javac.code.Type; +import org.jspecify.annotations.Nullable; /** A BugPattern; see the summary. */ @BugPattern( severity = WARNING, summary = "This code can be simplified to use a pattern-matching instanceof.") -public final class PatternMatchingInstanceof extends BugChecker implements IfTreeMatcher { +public final class PatternMatchingInstanceof extends BugChecker implements InstanceOfTreeMatcher { + @Override - public Description matchIf(IfTree tree, VisitorState state) { + public Description matchInstanceOf(InstanceOfTree instanceOfTree, VisitorState state) { if (!supportsPatternMatchingInstanceof(state.context)) { return NO_MATCH; } - ImmutableSet instanceofChecks = scanForInstanceOf(tree.getCondition()); - if (instanceofChecks.isEmpty()) { - return NO_MATCH; + var enclosingIf = findEnclosingIf(instanceOfTree, state); + if (enclosingIf != null) { + var replaceExistingVariable = checkForImmediateVariable(instanceOfTree, state, enclosingIf); + if (replaceExistingVariable != NO_MATCH) { + return replaceExistingVariable; + } + if (getSymbol(instanceOfTree.getExpression()) instanceof VarSymbol varSymbol) { + Type targetType = getType(instanceOfTree.getType()); + var allCasts = findAllCasts(varSymbol, enclosingIf.getThenStatement(), targetType, state); + if (!allCasts.isEmpty()) { + // 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. + var name = lowerFirstLetter(targetType.tsym.getSimpleName().toString()); + return describeMatch( + instanceOfTree, + SuggestedFix.builder() + .postfixWith(instanceOfTree, " " + name) + .merge( + allCasts.stream() + .map(c -> SuggestedFix.replace(c, name)) + .collect(mergeFixes())) + .build()); + } + } } - var body = tree.getThenStatement(); - if (!(body instanceof BlockTree)) { + // TODO(ghm): Handle things other than just ifs. It'd be great to refactor `foo instanceof Bar + // && ((Bar) foo).baz()`. + return NO_MATCH; + } + + private Description checkForImmediateVariable( + InstanceOfTree instanceOfTree, VisitorState state, IfTree enclosingIf) { + var body = enclosingIf.getThenStatement(); + if (!(body instanceof BlockTree block)) { return NO_MATCH; } - var block = (BlockTree) body; if (block.getStatements().isEmpty()) { return NO_MATCH; } var firstStatement = block.getStatements().get(0); - if (!(firstStatement instanceof VariableTree)) { + if (!(firstStatement instanceof VariableTree variableTree)) { return NO_MATCH; } - var variableTree = (VariableTree) firstStatement; - if (!(variableTree.getInitializer() instanceof TypeCastTree)) { + if (!(variableTree.getInitializer() instanceof TypeCastTree typeCast)) { return NO_MATCH; } - var typeCast = (TypeCastTree) variableTree.getInitializer(); - var matchingInstanceof = - instanceofChecks.stream() - .filter( - i -> - state.getTypes().isSameType(getType(i.getType()), getType(typeCast.getType())) - && getSymbol(i.getExpression()) instanceof VarSymbol - && getSymbol(i.getExpression()).equals(getSymbol(typeCast.getExpression()))) - .findFirst() - .orElse(null); - if (matchingInstanceof == null) { + // TODO(ghm): Relax the requirement of this being an exact same symbol. Handle expressions? + if (!(state + .getTypes() + .isSubtype(getType(instanceOfTree.getType()), getType(variableTree.getType())) + && getSymbol(instanceOfTree.getExpression()) instanceof VarSymbol varSymbol + && varSymbol.equals(getSymbol(typeCast.getExpression())))) { return NO_MATCH; } return describeMatch( firstStatement, SuggestedFix.builder() .delete(variableTree) - .postfixWith(matchingInstanceof, " " + variableTree.getName().toString()) + .postfixWith(instanceOfTree, " " + variableTree.getName().toString()) .build()); } - private ImmutableSet scanForInstanceOf(ExpressionTree condition) { - ImmutableSet.Builder instanceOfs = ImmutableSet.builder(); - new SimpleTreeVisitor() { - @Override - public Void visitParenthesized(ParenthesizedTree tree, Void unused) { - return visit(tree.getExpression(), null); - } - - @Override - public Void visitBinary(BinaryTree tree, Void unused) { - if (tree.getKind() != Kind.CONDITIONAL_AND) { + /** + * Finds the enclosing IfTree if the provided {@code instanceof} is guaranteed to imply the then + * branch. + */ + // TODO(ghm): handle _inverted_ ifs. + private static @Nullable IfTree findEnclosingIf(InstanceOfTree tree, VisitorState state) { + Tree last = tree; + for (Tree parent : state.getPath().getParentPath()) { + switch (parent.getKind()) { + case CONDITIONAL_AND, PARENTHESIZED -> {} + case IF -> { + if (((IfTree) parent).getCondition() == last) { + return (IfTree) parent; + } else { + return null; + } + } + default -> { return null; } - visit(tree.getLeftOperand(), null); - visit(tree.getRightOperand(), null); - return null; } + last = parent; + } + return null; + } + /** Finds all casts of {@code symbol} which are cast to {@code targetType} within {@code tree}. */ + private static ImmutableSet findAllCasts( + VarSymbol symbol, StatementTree tree, Type targetType, VisitorState state) { + ImmutableSet.Builder usages = ImmutableSet.builder(); + new TreePathScanner() { @Override - public Void visitInstanceOf(InstanceOfTree tree, Void unused) { - instanceOfs.add(tree); - return null; + public Void visitTypeCast(TypeCastTree node, Void unused) { + if (getSymbol(node.getExpression()) instanceof VarSymbol v) { + if (v.equals(symbol) && state.getTypes().isSubtype(targetType, getType(node.getType()))) { + usages.add( + getCurrentPath().getParentPath().getLeaf() instanceof ParenthesizedTree p + ? p + : node); + } + } + return super.visitTypeCast(node, null); } - }.visit(condition, null); - return instanceOfs.build(); + }.scan(new TreePath(new TreePath(state.getPath().getCompilationUnit()), tree), null); + return usages.build(); + } + + private static String lowerFirstLetter(String description) { + return Character.toLowerCase(description.charAt(0)) + description.substring(1); } } 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 321ad4ad836..8065a4cb357 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceofTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceofTest.java @@ -186,9 +186,38 @@ void test(Object x, String k) { .doTest(); } + @Test + public void notImmediatelyAssignedToVariable() { + helper + .addInputLines( + "Test.java", + """ + class Test { + void test(Object o) { + if (o instanceof Test) { + test((Test) o); + test(((Test) o).hashCode()); + } + } + } + """) + .addOutputLines( + "Test.java", + """ + class Test { + void test(Object o) { + if (o instanceof Test test) { + test(test); + test(test.hashCode()); + } + } + } + """) + .doTest(); + } + @Test public void genericWithUpperBoundedWildcard() { - assume().that(Runtime.version().feature()).isAtLeast(21); helper .addInputLines( "Test.java", @@ -199,13 +228,11 @@ class Test { void test(Object object) { if (object instanceof List) { @SuppressWarnings("unchecked") - List list = (List) object; - System.err.println(list.get(0)); + List xs = (List) object; } } } """) - // TODO: b/380054832 - this shouldn't get re-written .addOutputLines( "Test.java", """ @@ -214,7 +241,8 @@ void test(Object object) { class Test { void test(Object object) { if (object instanceof List list) { - System.err.println(list.get(0)); + @SuppressWarnings("unchecked") + List xs = list; } } }