Skip to content

Commit

Permalink
PatternMatchingInstanceof: handle inverted ifs.
Browse files Browse the repository at this point in the history 
Some sample new findings: unknown commit (some of them look pre-existing)

PiperOrigin-RevId: 716275126
  • Loading branch information
@graememorgan
graememorgan authored and Error Prone Team committed Jan 16, 2025
1 parent 249c359 commit 7431230
Show file tree
Hide file tree
Showing 2 changed files with 220 additions and 71 deletions.
178 changes: 107 additions & 71 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 @@ -24,17 +24,19 @@
import static com.google.errorprone.util.ASTHelpers.getType;
import static com.google.errorprone.util.SourceVersion.supportsPatternMatchingInstanceof;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker.InstanceOfTreeMatcher;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.util.Reachability;
import com.sun.source.tree.AssignmentTree;
import com.sun.source.tree.BlockTree;
import com.sun.source.tree.IfTree;
import com.sun.source.tree.InstanceOfTree;
import com.sun.source.tree.ParenthesizedTree;
import com.sun.source.tree.StatementTree;
import com.sun.source.tree.Tree;
import com.sun.source.tree.TypeCastTree;
import com.sun.source.tree.VariableTree;
Expand All @@ -60,50 +62,49 @@ public Description matchInstanceOf(InstanceOfTree instanceOfTree, VisitorState s
if (!supportsPatternMatchingInstanceof(state.context)) {
return NO_MATCH;
}
var enclosingIf = findEnclosingIf(instanceOfTree, state);
if (enclosingIf != null) {
// 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) {
if (isReassigned(varSymbol, enclosingIf.getThenStatement())) {
return NO_MATCH;
}
Type targetType = getType(instanceOfTree.getType());
var allCasts =
new HashSet<>(
findAllCasts(varSymbol, enclosingIf.getThenStatement(), targetType, state));
String name = null;
SuggestedFix.Builder fix = SuggestedFix.builder();
var impliedStatements = findImpliedStatements(instanceOfTree, state);
if (impliedStatements.isEmpty()) {
return NO_MATCH;
}
// 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) {
if (isReassigned(varSymbol, impliedStatements)) {
return NO_MATCH;
}
Type targetType = getType(instanceOfTree.getType());
var allCasts = new HashSet<>(findAllCasts(varSymbol, impliedStatements, 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 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,
fix.postfixWith(instanceOfTree, " " + name)
.merge(
allCasts.stream()
.map(c -> SuggestedFix.replace(c.getLeaf(), fn))
.collect(mergeFixes()))
.build());
}
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,
fix.postfixWith(instanceOfTree, " " + name)
.merge(
allCasts.stream()
.map(c -> SuggestedFix.replace(c.getLeaf(), fn))
.collect(mergeFixes()))
.build());
}
}
// TODO(ghm): Handle things other than just ifs. It'd be great to refactor `foo instanceof Bar
Expand Down Expand Up @@ -140,54 +141,89 @@ private static String generateVariableName(Type targetType, VisitorState state)
return camelCased;
}

/**
* 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) {
/** Finds trees which are implied by the {@code instanceOfTree}. */
private static ImmutableList<Tree> findImpliedStatements(
InstanceOfTree tree, VisitorState state) {
Tree last = tree;
for (Tree parent : state.getPath().getParentPath()) {
boolean negated = false;
var impliedStatements = ImmutableList.<Tree>builder();
for (TreePath parentPath = state.getPath().getParentPath();
parentPath != null;
parentPath = parentPath.getParentPath()) {
Tree parent = parentPath.getLeaf();
switch (parent.getKind()) {
case CONDITIONAL_AND, PARENTHESIZED -> {}
case CONDITIONAL_AND -> {
if (negated) {
return impliedStatements.build();
}
}
case CONDITIONAL_OR -> {
if (!negated) {
return impliedStatements.build();
}
}
case PARENTHESIZED -> {}
case LOGICAL_COMPLEMENT -> {
negated = !negated;
}
case IF -> {
if (((IfTree) parent).getCondition() == last) {
return (IfTree) parent;
var ifTree = (IfTree) parent;
if (!(((IfTree) parent).getCondition() == last)) {
return impliedStatements.build();
}
if (negated) {
if (ifTree.getElseStatement() != null) {
impliedStatements.add(ifTree.getElseStatement());
}
if (!Reachability.canCompleteNormally(ifTree.getThenStatement())) {
var pparent = parentPath.getParentPath().getLeaf();
if (pparent instanceof BlockTree blockTree) {
var index = blockTree.getStatements().indexOf(ifTree);
impliedStatements.addAll(
blockTree.getStatements().subList(index + 1, blockTree.getStatements().size()));
}
}
} else {
return null;
impliedStatements.add(ifTree.getThenStatement());
}
return impliedStatements.build();
}
default -> {
return null;
return impliedStatements.build();
}
}
last = parent;
}
return null;
return impliedStatements.build();
}

/** Finds all casts of {@code symbol} which are cast to {@code targetType} within {@code tree}. */
private static ImmutableSet<TreePath> findAllCasts(
VarSymbol symbol, StatementTree tree, Type targetType, VisitorState state) {
VarSymbol symbol, Iterable<Tree> trees, Type targetType, VisitorState state) {
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
? getCurrentPath().getParentPath()
: getCurrentPath());
var scanner =
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
? getCurrentPath().getParentPath()
: getCurrentPath());
}
}
return super.visitTypeCast(node, null);
}
}
return super.visitTypeCast(node, null);
}
}.scan(new TreePath(new TreePath(state.getPath().getCompilationUnit()), tree), null);
};
for (Tree tree : trees) {
scanner.scan(new TreePath(new TreePath(state.getPath().getCompilationUnit()), tree), null);
}
return usages.build();
}

private static boolean isReassigned(VarSymbol symbol, Tree tree) {
private static boolean isReassigned(VarSymbol symbol, Iterable<Tree> trees) {
AtomicBoolean isReassigned = new AtomicBoolean(false);
new TreeScanner<Void, Void>() {
@Override
Expand All @@ -198,7 +234,7 @@ public Void visitAssignment(AssignmentTree assignmentTree, Void unused) {
}
return super.visitAssignment(assignmentTree, null);
}
}.scan(tree, null);
}.scan(trees, null);
return isReassigned.get();
}
}
113 changes: 113 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 @@ -57,6 +57,119 @@ void test(Object o) {
.doTest();
}

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

@Test
public void negatedIf_withOrs() {
helper
.addInputLines(
"Test.java",
"""
class Test {
void test(Object o) {
if (!(o instanceof Test) || o.hashCode() == 0) {
return;
}
Test test = (Test) o;
test(test);
}
}
""")
.addOutputLines(
"Test.java",
"""
class Test {
void test(Object o) {
if (!(o instanceof Test test) || o.hashCode() == 0) {
return;
}
test(test);
}
}
""")
.doTest();
}

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

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

@Test
public void notDefinitelyChecked_noFinding() {
helper
Expand Down

0 comments on commit 7431230

Please sign in to comment.