Skip to content

Commit 4171fd7

Browse files
graememorganError Prone Team
graememorgan
authored and
Error Prone Team
committed
FindIdentifiers: find binding variables declared by enclosing or earlier if statements, as well as ternaries, and enclosing binary trees.
PiperOrigin-RevId: 748725027
1 parent d78f515 commit 4171fd7

File tree

2 files changed

+279
-1
lines changed

2 files changed

+279
-1
lines changed

‎check_api/src/main/java/com/google/errorprone/util/FindIdentifiers.java‎

Lines changed: 104 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,29 +20,39 @@
2020
import static com.google.common.collect.ImmutableSet.toImmutableSet;
2121
import static com.google.errorprone.util.ASTHelpers.isConsideredFinal;
2222
import static com.google.errorprone.util.ASTHelpers.isStatic;
23+
import static com.google.errorprone.util.Reachability.canCompleteNormally;
2324

2425
import com.google.common.collect.ImmutableList;
2526
import com.google.common.collect.ImmutableSet;
2627
import com.google.common.collect.Sets;
2728
import com.google.errorprone.VisitorState;
29+
import com.sun.source.tree.BinaryTree;
30+
import com.sun.source.tree.BindingPatternTree;
2831
import com.sun.source.tree.BlockTree;
2932
import com.sun.source.tree.CatchTree;
3033
import com.sun.source.tree.ClassTree;
3134
import com.sun.source.tree.CompilationUnitTree;
35+
import com.sun.source.tree.ConditionalExpressionTree;
3236
import com.sun.source.tree.EnhancedForLoopTree;
37+
import com.sun.source.tree.ExpressionTree;
3338
import com.sun.source.tree.ForLoopTree;
3439
import com.sun.source.tree.IdentifierTree;
40+
import com.sun.source.tree.IfTree;
3541
import com.sun.source.tree.ImportTree;
42+
import com.sun.source.tree.InstanceOfTree;
3643
import com.sun.source.tree.LambdaExpressionTree;
3744
import com.sun.source.tree.MemberSelectTree;
3845
import com.sun.source.tree.MethodInvocationTree;
3946
import com.sun.source.tree.MethodTree;
4047
import com.sun.source.tree.NewClassTree;
48+
import com.sun.source.tree.ParenthesizedTree;
4149
import com.sun.source.tree.StatementTree;
4250
import com.sun.source.tree.Tree;
4351
import com.sun.source.tree.Tree.Kind;
4452
import com.sun.source.tree.TryTree;
53+
import com.sun.source.tree.UnaryTree;
4554
import com.sun.source.tree.VariableTree;
55+
import com.sun.source.util.SimpleTreeVisitor;
4656
import com.sun.source.util.TreePath;
4757
import com.sun.source.util.TreeScanner;
4858
import com.sun.tools.javac.code.Kinds.KindSelector;
@@ -157,7 +167,35 @@ private static Symbol findIdent(
157167
*/
158168
public static ImmutableSet<VarSymbol> findAllIdents(VisitorState state) {
159169
ImmutableSet.Builder<VarSymbol> result = new ImmutableSet.Builder<>();
170+
171+
// If we're in a binary tree, scan up separately to find anything to the left that implies us.
160172
Tree prev = state.getPath().getLeaf();
173+
loop:
174+
for (Tree curr : state.getPath().getParentPath()) {
175+
switch (curr.getKind()) {
176+
case CONDITIONAL_AND -> {
177+
BinaryTree binaryTree = (BinaryTree) curr;
178+
if (prev == binaryTree.getRightOperand()) {
179+
findBindingVariables(binaryTree.getLeftOperand(), result, /* startNegated= */ false);
180+
}
181+
}
182+
case CONDITIONAL_OR -> {
183+
BinaryTree binaryTree = (BinaryTree) curr;
184+
if (prev == binaryTree.getRightOperand()) {
185+
findBindingVariables(binaryTree.getLeftOperand(), result, /* startNegated= */ true);
186+
}
187+
}
188+
default -> {
189+
if (!(curr instanceof ExpressionTree)) {
190+
break loop;
191+
}
192+
}
193+
}
194+
195+
prev = curr;
196+
}
197+
198+
prev = state.getPath().getLeaf();
161199
for (Tree curr : state.getPath().getParentPath()) {
162200
switch (curr.getKind()) {
163201
case BLOCK -> {
@@ -166,6 +204,9 @@ public static ImmutableSet<VarSymbol> findAllIdents(VisitorState state) {
166204
break;
167205
}
168206
addIfVariable(stmt, result);
207+
if (stmt instanceof IfTree ifTree && !canCompleteNormally(ifTree.getThenStatement())) {
208+
findBindingVariables(ifTree.getCondition(), result, /* startNegated= */ true);
209+
}
169210
}
170211
}
171212
case LAMBDA_EXPRESSION -> {
@@ -231,6 +272,26 @@ public static ImmutableSet<VarSymbol> findAllIdents(VisitorState state) {
231272
addAllIfVariable(tryTree.getResources(), result);
232273
}
233274
}
275+
case IF -> {
276+
var ifTree = (IfTree) curr;
277+
if (prev == ifTree.getThenStatement()) {
278+
findBindingVariables(ifTree.getCondition(), result, /* startNegated= */ false);
279+
}
280+
if (prev == ifTree.getElseStatement()) {
281+
findBindingVariables(ifTree.getCondition(), result, /* startNegated= */ true);
282+
}
283+
}
284+
case CONDITIONAL_EXPRESSION -> {
285+
ConditionalExpressionTree conditionalExpressionTree = (ConditionalExpressionTree) curr;
286+
if (prev == conditionalExpressionTree.getTrueExpression()) {
287+
findBindingVariables(
288+
conditionalExpressionTree.getCondition(), result, /* startNegated= */ false);
289+
}
290+
if (prev == conditionalExpressionTree.getFalseExpression()) {
291+
findBindingVariables(
292+
conditionalExpressionTree.getCondition(), result, /* startNegated= */ true);
293+
}
294+
}
234295
case COMPILATION_UNIT -> {
235296
for (ImportTree importTree : ((CompilationUnitTree) curr).getImports()) {
236297
if (importTree.isStatic()
@@ -266,6 +327,48 @@ public static ImmutableSet<VarSymbol> findAllIdents(VisitorState state) {
266327
.collect(toImmutableSet());
267328
}
268329

330+
private static void findBindingVariables(
331+
Tree tree, ImmutableSet.Builder<VarSymbol> result, boolean startNegated) {
332+
new SimpleTreeVisitor<Void, Void>() {
333+
boolean negated = startNegated;
334+
335+
@Override
336+
public Void visitInstanceOf(InstanceOfTree node, Void unused) {
337+
if (!negated && node.getPattern() instanceof BindingPatternTree bpt) {
338+
addIfVariable(bpt.getVariable(), result);
339+
}
340+
return null;
341+
}
342+
343+
@Override
344+
public Void visitParenthesized(ParenthesizedTree node, Void unused) {
345+
return visit(node.getExpression(), null);
346+
}
347+
348+
@Override
349+
public Void visitUnary(UnaryTree node, Void unused) {
350+
if (node.getKind().equals(Kind.LOGICAL_COMPLEMENT)) {
351+
negated = !negated;
352+
return visit(node.getExpression(), null);
353+
}
354+
return null;
355+
}
356+
357+
@Override
358+
public Void visitBinary(BinaryTree node, Void unused) {
359+
if (node.getKind().equals(Kind.CONDITIONAL_AND) && !negated) {
360+
visit(node.getLeftOperand(), null);
361+
visit(node.getRightOperand(), null);
362+
}
363+
if (node.getKind().equals(Kind.CONDITIONAL_OR) && negated) {
364+
visit(node.getLeftOperand(), null);
365+
visit(node.getRightOperand(), null);
366+
}
367+
return null;
368+
}
369+
}.visit(tree, null);
370+
}
371+
269372
/**
270373
* Finds all variable declarations which are unused at this point in the AST (i.e. they might be
271374
* used further on).
@@ -419,7 +522,7 @@ private static boolean isVisible(VarSymbol var, TreePath path) {
419522
// in the enclosing class or a superclass).
420523
return modifiers.contains(Modifier.PUBLIC) || modifiers.contains(Modifier.PROTECTED);
421524
}
422-
case PARAMETER, LOCAL_VARIABLE -> {
525+
case PARAMETER, LOCAL_VARIABLE, BINDING_VARIABLE -> {
423526
// If we are in an anonymous inner class, lambda, or local class, any local variable or
424527
// method parameter we access that is defined outside the anonymous class/lambda must be
425528
// final or effectively final (JLS 8.1.3).

‎check_api/src/test/java/com/google/errorprone/util/FindIdentifiersTest.java‎

Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -829,6 +829,181 @@ void doIt() {
829829
.doTest();
830830
}
831831

832+
@Test
833+
public void findAllIdents_bindingVariables() {
834+
CompilationTestHelper.newInstance(PrintIdents.class, getClass())
835+
.addSourceLines(
836+
"pkg/MyInterface.java",
837+
"""
838+
package pkg;
839+
840+
public interface MyInterface {
841+
static void test(Object o) {
842+
if (o instanceof MyInterface mi && o instanceof MyInterface mi2) {
843+
// BUG: Diagnostic contains: [mi, mi2, o]
844+
String.format("");
845+
}
846+
}
847+
}
848+
""")
849+
.doTest();
850+
}
851+
852+
@Test
853+
public void findAllIdents_bindingVariablesNeitherVisible() {
854+
CompilationTestHelper.newInstance(PrintIdents.class, getClass())
855+
.addSourceLines(
856+
"pkg/MyInterface.java",
857+
"""
858+
package pkg;
859+
860+
public interface MyInterface {
861+
static void test(Object o) {
862+
if (o instanceof MyInterface mi || o instanceof MyInterface mi2) {
863+
// BUG: Diagnostic contains: [o]
864+
String.format("");
865+
}
866+
}
867+
}
868+
""")
869+
.doTest();
870+
}
871+
872+
@Test
873+
public void findAllIdents_bindingVariablesNegated() {
874+
CompilationTestHelper.newInstance(PrintIdents.class, getClass())
875+
.addSourceLines(
876+
"pkg/MyInterface.java",
877+
"""
878+
package pkg;
879+
880+
public interface MyInterface {
881+
static void test(Object o) {
882+
if (!(o instanceof MyInterface mi)) {
883+
return;
884+
}
885+
// BUG: Diagnostic contains: [mi, o]
886+
String.format("");
887+
}
888+
}
889+
""")
890+
.doTest();
891+
}
892+
893+
@Test
894+
public void findAllIdents_bindingVariablesNegatedWithExplicitElse() {
895+
CompilationTestHelper.newInstance(PrintIdents.class, getClass())
896+
.addSourceLines(
897+
"pkg/MyInterface.java",
898+
"""
899+
package pkg;
900+
901+
public interface MyInterface {
902+
static void test(Object o) {
903+
if (!(o instanceof MyInterface mi)) {
904+
// BUG: Diagnostic contains: [o]
905+
String.format("");
906+
} else {
907+
// BUG: Diagnostic contains: [mi, o]
908+
String.format("");
909+
}
910+
}
911+
}
912+
""")
913+
.doTest();
914+
}
915+
916+
@Test
917+
public void findAllIdents_bindingVariablesNegatedButMayFallThrough() {
918+
CompilationTestHelper.newInstance(PrintIdents.class, getClass())
919+
.addSourceLines(
920+
"pkg/MyInterface.java",
921+
"""
922+
package pkg;
923+
924+
public interface MyInterface {
925+
static void test(Object o) {
926+
if (!(o instanceof MyInterface mi)) {
927+
}
928+
// BUG: Diagnostic contains: [o]
929+
String.format("");
930+
}
931+
}
932+
""")
933+
.doTest();
934+
}
935+
936+
@Test
937+
public void findAllIdents_bindingVariableWithTernary() {
938+
CompilationTestHelper.newInstance(PrintIdents.class, getClass())
939+
.addSourceLines(
940+
"pkg/MyInterface.java",
941+
"""
942+
package pkg;
943+
944+
public interface MyInterface {
945+
static boolean test(Object o) {
946+
return o instanceof MyInterface mi
947+
?
948+
// BUG: Diagnostic contains: [mi, o]
949+
String.format("").isEmpty()
950+
:
951+
// BUG: Diagnostic contains: [o]
952+
String.format("").isEmpty();
953+
}
954+
}
955+
""")
956+
.doTest();
957+
}
958+
959+
@Test
960+
public void findAllIdents_bindingVariableWithComplexConditions() {
961+
CompilationTestHelper.newInstance(PrintIdents.class, getClass())
962+
.addSourceLines(
963+
"pkg/MyInterface.java",
964+
"""
965+
package pkg;
966+
967+
public interface MyInterface {
968+
static boolean test(Object o) {
969+
// BUG: Diagnostic contains: [s, o]
970+
return o instanceof String s && String.format(s).isEmpty();
971+
}
972+
973+
static boolean test2(Object o) {
974+
// BUG: Diagnostic contains: [o]
975+
return o instanceof String s || String.format("").isEmpty();
976+
}
977+
978+
static boolean test3(Object o) {
979+
// BUG: Diagnostic contains: [s, o]
980+
return !(o instanceof String s) || String.format(s).isEmpty();
981+
}
982+
983+
static boolean test4(Object o) {
984+
// BUG: Diagnostic contains: [s, o]
985+
return o instanceof String s && true && String.format(s).isEmpty();
986+
}
987+
988+
static boolean test5(Object o) {
989+
// BUG: Diagnostic contains: [s, o]
990+
return !(o instanceof String s && true) || (true && String.format(s).isEmpty());
991+
}
992+
993+
static boolean test6(Object o) {
994+
// BUG: Diagnostic contains: [o]
995+
return !(o instanceof String s && true) && (true && String.format("").isEmpty());
996+
}
997+
998+
static boolean test7(Object o) {
999+
// BUG: Diagnostic contains: [o]
1000+
return o instanceof String s && true || true && String.format("").isEmpty();
1001+
}
1002+
}
1003+
""")
1004+
.doTest();
1005+
}
1006+
8321007
/** A {@link BugChecker} that prints all identifiers in scope at a method declaration. */
8331008
@BugPattern(
8341009
severity = SeverityLevel.ERROR,

0 commit comments

Comments
 (0)