Skip to content

Commit 1b4a745

Browse files
markhbradyError Prone Team
markhbrady
authored and
Error Prone Team
committed
[IfChainToSwitch] bugfix: when a guard is present and true, must be treated as an unguarded pattern
PiperOrigin-RevId: 875380431
1 parent f572c50 commit 1b4a745

File tree

4 files changed

+295
-60
lines changed

4 files changed

+295
-60
lines changed

‎core/src/main/java/com/google/errorprone/bugpatterns/IfChainToSwitch.java‎

Lines changed: 87 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -21,14 +21,17 @@
2121
import static com.google.common.collect.ImmutableSet.toImmutableSet;
2222
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
2323
import static com.google.errorprone.bugpatterns.SwitchUtils.COMPILE_TIME_CONSTANT_MATCHER;
24+
import static com.google.errorprone.bugpatterns.SwitchUtils.getReferencedLocalVariablesInTree;
2425
import static com.google.errorprone.bugpatterns.SwitchUtils.isEnumValue;
2526
import static com.google.errorprone.bugpatterns.SwitchUtils.renderComments;
2627
import static com.google.errorprone.matchers.Description.NO_MATCH;
2728
import static com.google.errorprone.util.ASTHelpers.constValue;
2829
import static com.google.errorprone.util.ASTHelpers.getStartPosition;
2930
import static com.google.errorprone.util.ASTHelpers.getType;
31+
import static com.google.errorprone.util.ASTHelpers.isConsideredFinal;
3032
import static com.google.errorprone.util.ASTHelpers.isSubtype;
3133
import static com.google.errorprone.util.ASTHelpers.sameVariable;
34+
import static com.google.errorprone.util.ASTHelpers.stripParentheses;
3235
import static com.sun.source.tree.Tree.Kind.EXPRESSION_STATEMENT;
3336
import static com.sun.source.tree.Tree.Kind.THROW;
3437
import static java.lang.Math.max;
@@ -59,6 +62,7 @@
5962
import com.sun.source.tree.ExpressionTree;
6063
import com.sun.source.tree.IfTree;
6164
import com.sun.source.tree.InstanceOfTree;
65+
import com.sun.source.tree.LiteralTree;
6266
import com.sun.source.tree.StatementTree;
6367
import com.sun.source.tree.Tree;
6468
import com.sun.source.tree.Tree.Kind;
@@ -415,6 +419,28 @@ private static boolean switchOnNullWouldImplicitlyThrow(
415419
&& cases.stream().noneMatch(CaseIr::hasCaseNull);
416420
}
417421

422+
/**
423+
* Determines whether the given case IR has an "unguarded" pattern. As defined in the JLS,
424+
* "unguarded" means that either there is no guard or the guard is the boolean literal `true`.
425+
*/
426+
private static boolean isUnguarded(CaseIr caseIr) {
427+
// Not a pattern
428+
if (caseIr.instanceOfOptional().isEmpty()) {
429+
return false;
430+
}
431+
432+
if (caseIr.guardOptional().isEmpty()) {
433+
return true;
434+
}
435+
436+
// Guard is present and is `true`
437+
ExpressionTree guard = stripParentheses(caseIr.guardOptional().get());
438+
return isBooleanLiteral(guard)
439+
&& guard instanceof LiteralTree literalTree
440+
&& literalTree.getValue() instanceof Boolean b
441+
&& b;
442+
}
443+
418444
/**
419445
* Analyzes the supplied case IRs for a switch statement for issues related default/unconditional
420446
* cases. If deemed necessary, this method injects a `default` and/or `case null` into the
@@ -449,7 +475,7 @@ private Optional<List<CaseIr>> maybeFixDefaultNullAndUnconditional(
449475
.filter(
450476
caseIr ->
451477
caseIr.instanceOfOptional().isPresent()
452-
&& caseIr.guardOptional().isEmpty()
478+
&& isUnguarded(caseIr)
453479
&& isSubtype(
454480
getType(subject),
455481
getType(caseIr.instanceOfOptional().get().type()),
@@ -673,12 +699,13 @@ && switchOnNullWouldImplicitlyThrow(
673699
}
674700

675701
/**
676-
* Analyzes the supplied case IRs for duplicate constants (either primitives or enum values). If
702+
* Analyzes the supplied case IRs for duplicate constants (primitives, enum values, or `null`). If
677703
* any duplicates are found, returns {@code Optional.empty()}.
678704
*/
679705
private static Optional<List<CaseIr>> maybeDetectDuplicateConstants(List<CaseIr> cases) {
680706

681707
Set<Object> seenConstants = new HashSet<>();
708+
boolean seenNull = false;
682709

683710
for (CaseIr caseIr : cases) {
684711
if (caseIr.expressionsOptional().isPresent()) {
@@ -700,6 +727,13 @@ private static Optional<List<CaseIr>> maybeDetectDuplicateConstants(List<CaseIr>
700727
}
701728
seenConstants.add(sym);
702729
}
730+
731+
if (isNull(expression)) {
732+
if (seenNull) {
733+
return Optional.empty();
734+
}
735+
seenNull = true;
736+
}
703737
}
704738
}
705739
}
@@ -958,40 +992,54 @@ private Optional<ExpressionTree> validatePredicateForSubject(
958992

959993
return Optional.empty();
960994
}
961-
} else if (predicateIsConditionalAnd) {
995+
} else if (predicateIsConditionalAnd && !mustBeInstanceOf) {
962996
// Maybe the predicate is something like `a instanceof Foo && predicate`. If so, recurse on
963997
// the left side, and attach the right side of the conditional and as a guard to the
964998
// resulting case.
965-
if (!mustBeInstanceOf && binaryTree.getKind().equals(Kind.CONDITIONAL_AND)) {
966-
int currentCasesSize = cases.size();
967-
var rv =
968-
validatePredicateForSubject(
969-
binaryTree.getLeftOperand(),
970-
subject,
971-
state,
972-
/* mustBeInstanceOf= */ true,
973-
cases,
974-
elseOptional,
975-
arrowRhsOptional,
976-
handledEnumValues,
977-
ifTreeRange,
978-
/* caseStartPosition= */ caseStartPosition);
979-
if (rv.isPresent()) {
980-
CaseIr oldLastCase = cases.get(currentCasesSize);
981-
// Update last case to attach the guard
982-
cases.set(
983-
currentCasesSize,
984-
new CaseIr(
985-
/* hasCaseNull= */ oldLastCase.hasCaseNull(),
986-
/* hasDefault= */ oldLastCase.hasDefault(),
987-
/* instanceOfOptional= */ oldLastCase.instanceOfOptional(),
988-
/* guardOptional= */ Optional.of(binaryTree.getRightOperand()),
989-
/* expressionsOptional= */ oldLastCase.expressionsOptional(),
990-
/* arrowRhsOptional= */ oldLastCase.arrowRhsOptional(),
991-
/* caseSourceCodeRange= */ oldLastCase.caseSourceCodeRange()));
992-
return rv;
999+
int currentCasesSize = cases.size();
1000+
var rv =
1001+
validatePredicateForSubject(
1002+
binaryTree.getLeftOperand(),
1003+
subject,
1004+
state,
1005+
/* mustBeInstanceOf= */ true,
1006+
cases,
1007+
elseOptional,
1008+
arrowRhsOptional,
1009+
handledEnumValues,
1010+
ifTreeRange,
1011+
/* caseStartPosition= */ caseStartPosition);
1012+
if (rv.isPresent()) {
1013+
CaseIr oldLastCase = cases.get(currentCasesSize);
1014+
ExpressionTree rightOperandNoParentheses =
1015+
ASTHelpers.stripParentheses(binaryTree.getRightOperand());
1016+
// A guard cannot just be `false` (not valid Java)
1017+
if (isBooleanLiteral(rightOperandNoParentheses)
1018+
&& rightOperandNoParentheses instanceof LiteralTree literalTree
1019+
&& literalTree.getValue() instanceof Boolean b
1020+
&& !b) {
1021+
return Optional.empty();
9931022
}
1023+
// A guard cannot reference a local variable that is not final nor effectively final;
1024+
// see JLS 21 §14.11.1.
1025+
if (getReferencedLocalVariablesInTree(rightOperandNoParentheses).stream()
1026+
.anyMatch(varSymbol -> !isConsideredFinal(varSymbol))) {
1027+
return Optional.empty();
1028+
}
1029+
// Update last case to attach the guard
1030+
cases.set(
1031+
currentCasesSize,
1032+
new CaseIr(
1033+
/* hasCaseNull= */ oldLastCase.hasCaseNull(),
1034+
/* hasDefault= */ oldLastCase.hasDefault(),
1035+
/* instanceOfOptional= */ oldLastCase.instanceOfOptional(),
1036+
/* guardOptional= */ Optional.of(binaryTree.getRightOperand()),
1037+
/* expressionsOptional= */ oldLastCase.expressionsOptional(),
1038+
/* arrowRhsOptional= */ oldLastCase.arrowRhsOptional(),
1039+
/* caseSourceCodeRange= */ oldLastCase.caseSourceCodeRange()));
1040+
return rv;
9941041
}
1042+
9951043
} else if (!mustBeInstanceOf && predicateIsConditionalOr) {
9961044
// Maybe the predicate is something like `x == 1 || x == 2`.
9971045
return validateConditionalOrsForSubject(binaryTree, params);
@@ -1006,6 +1054,10 @@ private Optional<ExpressionTree> validatePredicateForSubject(
10061054
return Optional.empty();
10071055
}
10081056

1057+
private static boolean isBooleanLiteral(ExpressionTree tree) {
1058+
return tree.getKind() == Kind.BOOLEAN_LITERAL;
1059+
}
1060+
10091061
/**
10101062
* Validates whether the {@code binaryTree} represents a series of conditional-ORs that can be
10111063
* converted to a single switch case having multiple expressions, returning the subject if so.
@@ -1644,7 +1696,7 @@ public static boolean isDominatedBy(
16441696
boolean isPrimitive = getType(constantExpression).isPrimitive();
16451697
if (isPrimitive) {
16461698
// Guarded patterns cannot dominate primitives
1647-
if (lhs.guardOptional().isPresent()) {
1699+
if (!isUnguarded(lhs)) {
16481700
continue;
16491701
}
16501702
if (lhs.instanceOfOptional().isPresent()) {
@@ -1676,7 +1728,7 @@ public static boolean isDominatedBy(
16761728
}
16771729
boolean isEnum = isEnumValue(constantExpression, state);
16781730
if (isEnum) {
1679-
if (lhs.guardOptional().isPresent()) {
1731+
if (!isUnguarded(lhs)) {
16801732
// Guarded patterns cannot dominate enum values
16811733
continue;
16821734
}
@@ -1697,7 +1749,7 @@ public static boolean isDominatedBy(
16971749
// RHS must be a reference
16981750
// The rhs-reference code would be needed to support e.g. String literals. It is included
16991751
// for completeness.
1700-
if (lhs.guardOptional().isPresent()) {
1752+
if (!isUnguarded(lhs)) {
17011753
// Guarded patterns cannot dominate references
17021754
continue;
17031755
}
@@ -1728,7 +1780,7 @@ public static boolean isDominatedBy(
17281780
}
17291781

17301782
// RHS must be a pattern
1731-
if (lhs.guardOptional().isPresent()) {
1783+
if (!isUnguarded(lhs)) {
17321784
// LHS has a guard, so cannot dominate RHS
17331785
return false;
17341786
}

‎core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java‎

Lines changed: 2 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import static com.google.common.collect.Iterables.getLast;
2222
import static com.google.common.collect.Iterables.getOnlyElement;
2323
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
24+
import static com.google.errorprone.bugpatterns.SwitchUtils.getReferencedLocalVariablesInTree;
2425
import static com.google.errorprone.matchers.Description.NO_MATCH;
2526
import static com.google.errorprone.util.ASTHelpers.getStartPosition;
2627
import static com.google.errorprone.util.ASTHelpers.getSymbol;
@@ -557,28 +558,7 @@ private void handle(Tree tree) {
557558
* supplied {@code tree}.
558559
*/
559560
private static boolean hasReadsOrWritesOfVariableInTree(VarSymbol symbol, Tree tree) {
560-
Set<VarSymbol> referencedLocalVariables = new HashSet<>();
561-
new TreeScanner<Void, Void>() {
562-
@Override
563-
public Void visitMemberSelect(MemberSelectTree memberSelect, Void unused) {
564-
handle(memberSelect);
565-
return super.visitMemberSelect(memberSelect, null);
566-
}
567-
568-
@Override
569-
public Void visitIdentifier(IdentifierTree identifier, Void unused) {
570-
handle(identifier);
571-
return super.visitIdentifier(identifier, null);
572-
}
573-
574-
private void handle(Tree tree) {
575-
var symbol = getSymbol(tree);
576-
if (symbol instanceof VarSymbol varSymbol) {
577-
referencedLocalVariables.add(varSymbol);
578-
}
579-
}
580-
}.scan(tree, null);
581-
return referencedLocalVariables.contains(symbol);
561+
return getReferencedLocalVariablesInTree(tree).contains(symbol);
582562
}
583563

584564
/**

‎core/src/main/java/com/google/errorprone/bugpatterns/SwitchUtils.java‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -442,5 +442,30 @@ static String renderNullDefaultKindPrefix(
442442
};
443443
}
444444

445+
public static Set<VarSymbol> getReferencedLocalVariablesInTree(Tree tree) {
446+
Set<VarSymbol> referencedLocalVariables = new HashSet<>();
447+
new TreeScanner<Void, Void>() {
448+
@Override
449+
public Void visitMemberSelect(MemberSelectTree memberSelect, Void unused) {
450+
handle(memberSelect);
451+
return super.visitMemberSelect(memberSelect, null);
452+
}
453+
454+
@Override
455+
public Void visitIdentifier(IdentifierTree identifier, Void unused) {
456+
handle(identifier);
457+
return super.visitIdentifier(identifier, null);
458+
}
459+
460+
private void handle(Tree tree) {
461+
var symbol = getSymbol(tree);
462+
if (symbol instanceof VarSymbol varSymbol) {
463+
referencedLocalVariables.add(varSymbol);
464+
}
465+
}
466+
}.scan(tree, null);
467+
return referencedLocalVariables;
468+
}
469+
445470
private SwitchUtils() {}
446471
}

0 commit comments

Comments
 (0)