Skip to content

Commit

Permalink
Unify the "only one variable" and "replace all the casts" approaches …
Browse files Browse the repository at this point in the history 
…in PatternMatchingInstanceof.

This allows us to eliminate non-immediate variable trees.

PiperOrigin-RevId: 716169009
  • Loading branch information
@graememorgan
graememorgan authored and Error Prone Team committed Jan 16, 2025
1 parent 3b9cbce commit cabcfd7
Show file tree
Hide file tree
Showing 2 changed files with 82 additions and 49 deletions.
100 changes: 51 additions & 49 deletions core/src/main/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceof.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
import com.google.errorprone.bugpatterns.BugChecker.InstanceOfTreeMatcher;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description;
import com.sun.source.tree.BlockTree;
import com.sun.source.tree.IfTree;
import com.sun.source.tree.InstanceOfTree;
import com.sun.source.tree.ParenthesizedTree;
Expand All @@ -43,6 +42,7 @@
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 javax.lang.model.SourceVersion;
import org.jspecify.annotations.Nullable;

Expand All @@ -59,25 +59,42 @@ public Description matchInstanceOf(InstanceOfTree instanceOfTree, VisitorState s
}
var enclosingIf = findEnclosingIf(instanceOfTree, state);
if (enclosingIf != null) {
var replaceExistingVariable = checkForImmediateVariable(instanceOfTree, state, enclosingIf);
if (replaceExistingVariable != NO_MATCH) {
return replaceExistingVariable;
}
// TODO(ghm): Relax the requirement of this being an identical VarSymbol: it would be nice to
// support expressions, though we'd then need to worry about their purity.
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.
String name = generateVariableName(targetType, state);
var allCasts =
new HashSet<>(
findAllCasts(varSymbol, enclosingIf.getThenStatement(), targetType, state));
String name = null;
SuggestedFix.Builder fix = SuggestedFix.builder();

// 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.
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 (!allCasts.isEmpty() || !fix.isEmpty()) {
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);
}
String fn = name;
return describeMatch(
instanceOfTree,
SuggestedFix.builder()
.postfixWith(instanceOfTree, " " + name)
fix.postfixWith(instanceOfTree, " " + name)
.merge(
allCasts.stream()
.map(c -> SuggestedFix.replace(c, name))
.map(c -> SuggestedFix.replace(c.getLeaf(), fn))
.collect(mergeFixes()))
.build());
}
Expand All @@ -88,6 +105,23 @@ public Description matchInstanceOf(InstanceOfTree instanceOfTree, VisitorState s
return NO_MATCH;
}

private static @Nullable VariableTree isVariableAssignedFromCast(
TreePath treePath, InstanceOfTree instanceOfTree, VisitorState state) {
var parent = treePath.getParentPath().getLeaf();
if (!(parent instanceof VariableTree variableTree)) {
return null;
}
if (!variableTree.getInitializer().equals(treePath.getLeaf())) {
return null;
}
if (!state
.getTypes()
.isSubtype(getType(instanceOfTree.getType()), getType(variableTree.getType()))) {
return null;
}
return variableTree;
}

private static String generateVariableName(Type targetType, VisitorState state) {
Type unboxed = state.getTypes().unboxedType(targetType);
String simpleName = targetType.tsym.getSimpleName().toString();
Expand All @@ -100,38 +134,6 @@ private static String generateVariableName(Type targetType, VisitorState state)
return camelCased;
}

private Description checkForImmediateVariable(
InstanceOfTree instanceOfTree, VisitorState state, IfTree enclosingIf) {
var body = enclosingIf.getThenStatement();
if (!(body instanceof BlockTree block)) {
return NO_MATCH;
}
if (block.getStatements().isEmpty()) {
return NO_MATCH;
}
var firstStatement = block.getStatements().get(0);
if (!(firstStatement instanceof VariableTree variableTree)) {
return NO_MATCH;
}
if (!(variableTree.getInitializer() instanceof TypeCastTree typeCast)) {
return NO_MATCH;
}
// 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(instanceOfTree, " " + variableTree.getName().toString())
.build());
}

/**
* Finds the enclosing IfTree if the provided {@code instanceof} is guaranteed to imply the then
* branch.
Expand Down Expand Up @@ -159,18 +161,18 @@ && getSymbol(instanceOfTree.getExpression()) instanceof VarSymbol varSymbol
}

/** Finds all casts of {@code symbol} which are cast to {@code targetType} within {@code tree}. */
private static ImmutableSet<Tree> findAllCasts(
private static ImmutableSet<TreePath> findAllCasts(
VarSymbol symbol, StatementTree tree, Type targetType, VisitorState state) {
ImmutableSet.Builder<Tree> usages = ImmutableSet.builder();
var usages = ImmutableSet.<TreePath>builder();
new TreePathScanner<Void, Void>() {
@Override
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);
? getCurrentPath().getParentPath()
: getCurrentPath());
}
}
return super.visitTypeCast(node, null);
Expand Down
31 changes: 31 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceofTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -358,4 +358,35 @@ void test(Object o) {
.expectUnchanged()
.doTest();
}

@Test
public void newVariableNotInstantlyAssigned_pleasantFix() {
helper
.addInputLines(
"Test.java",
"""
class Test {
void test(Object o) {
if (o instanceof Test) {
test((Test) o);
Test test = (Test) o;
test(test);
}
}
}
""")
.addOutputLines(
"Test.java",
"""
class Test {
void test(Object o) {
if (o instanceof Test test) {
test(test);
test(test);
}
}
}
""")
.doTest();
}
}

0 comments on commit cabcfd7

Please sign in to comment.