Skip to content

Commit dbe2258

Browse files
markhbradyError Prone Team
markhbrady
authored and
Error Prone Team
committed
[StatementSwitchToExpressionSwitch] Refactor shared logic into SwitchUtils. No functional changes.
PiperOrigin-RevId: 875189827
1 parent 3d817b0 commit dbe2258

5 files changed

Lines changed: 889 additions & 65 deletions

File tree

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

Lines changed: 277 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -21,12 +21,14 @@
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;
3234
import static com.sun.source.tree.Tree.Kind.EXPRESSION_STATEMENT;
@@ -48,7 +50,6 @@
4850
import com.google.errorprone.fixes.SuggestedFix;
4951
import com.google.errorprone.fixes.SuggestedFixes;
5052
import com.google.errorprone.matchers.Description;
51-
import com.google.errorprone.suppliers.Suppliers;
5253
import com.google.errorprone.util.ASTHelpers;
5354
import com.google.errorprone.util.ErrorProneComment;
5455
import com.google.errorprone.util.Reachability;
@@ -60,6 +61,7 @@
6061
import com.sun.source.tree.ExpressionTree;
6162
import com.sun.source.tree.IfTree;
6263
import com.sun.source.tree.InstanceOfTree;
64+
import com.sun.source.tree.LiteralTree;
6365
import com.sun.source.tree.StatementTree;
6466
import com.sun.source.tree.Tree;
6567
import com.sun.source.tree.Tree.Kind;
@@ -90,6 +92,19 @@ public final class IfChainToSwitch extends BugChecker implements IfTreeMatcher {
9092
// it's either an ExpressionStatement or a Throw. Refer to JLS 14 §14.11.1
9193
private static final ImmutableSet<Kind> KINDS_CONVERTIBLE_WITHOUT_BRACES =
9294
ImmutableSet.of(THROW, EXPRESSION_STATEMENT);
95+
// Types that are allowed for CaseConstant expressions to be assignable to in a switch, as
96+
// specified in JLS 21 §14.11.1.
97+
private static final ImmutableSet<String> ALLOWED_SWITCH_CASE_CONSTANT_TYPES =
98+
ImmutableSet.of(
99+
"char",
100+
"byte",
101+
"short",
102+
"int",
103+
"java.lang.Character",
104+
"java.lang.Byte",
105+
"java.lang.Short",
106+
"java.lang.Integer",
107+
"java.lang.String");
93108

94109
private final boolean enableMain;
95110
private final boolean enableSafe;
@@ -917,6 +932,7 @@ private Optional<ExpressionTree> validatePredicateForSubject(
917932
ExpressionTree rhs = binaryTree.getRightOperand();
918933
boolean predicateIsEquality = binaryTree.getKind().equals(Kind.EQUAL_TO);
919934
boolean predicateIsConditionalAnd = binaryTree.getKind().equals(Kind.CONDITIONAL_AND);
935+
boolean predicateIsConditionalOr = binaryTree.getKind().equals(Kind.CONDITIONAL_OR);
920936

921937
if (!mustBeInstanceOf && predicateIsEquality) {
922938
// Either lhs or rhs must be a compile-time constant.
@@ -954,39 +970,104 @@ private Optional<ExpressionTree> validatePredicateForSubject(
954970

955971
return Optional.empty();
956972
}
957-
} else if (predicateIsConditionalAnd) {
973+
} else if (predicateIsConditionalAnd && !mustBeInstanceOf) {
958974
// Maybe the predicate is something like `a instanceof Foo && predicate`. If so, recurse on
959975
// the left side, and attach the right side of the conditional and as a guard to the
960976
// resulting case.
961-
if (!mustBeInstanceOf && binaryTree.getKind().equals(Kind.CONDITIONAL_AND)) {
962-
int currentCasesSize = cases.size();
963-
var rv =
964-
validatePredicateForSubject(
965-
binaryTree.getLeftOperand(),
966-
subject,
967-
state,
968-
/* mustBeInstanceOf= */ true,
969-
cases,
970-
elseOptional,
977+
int currentCasesSize = cases.size();
978+
var rv =
979+
validatePredicateForSubject(
980+
binaryTree.getLeftOperand(),
981+
subject,
982+
state,
983+
/* mustBeInstanceOf= */ true,
984+
cases,
985+
elseOptional,
986+
arrowRhsOptional,
987+
handledEnumValues,
988+
ifTreeRange,
989+
/* caseStartPosition= */ caseStartPosition);
990+
if (rv.isPresent()) {
991+
CaseIr oldLastCase = cases.get(currentCasesSize);
992+
ExpressionTree rightOperandNoParentheses =
993+
ASTHelpers.stripParentheses(binaryTree.getRightOperand());
994+
// A guard cannot just be `false` (not valid Java)
995+
if (isBooleanLiteral(rightOperandNoParentheses)
996+
&& rightOperandNoParentheses instanceof LiteralTree literalTree
997+
&& literalTree.getValue() instanceof Boolean b
998+
&& !b) {
999+
return Optional.empty();
1000+
}
1001+
// A guard cannot reference a local variable that is not final nor effectively final;
1002+
// see JLS 21 §14.11.1.
1003+
if (getReferencedLocalVariablesInTree(rightOperandNoParentheses).stream()
1004+
.anyMatch(varSymbol -> !isConsideredFinal(varSymbol))) {
1005+
return Optional.empty();
1006+
}
1007+
// Update last case to attach the guard
1008+
cases.set(
1009+
currentCasesSize,
1010+
new CaseIr(
1011+
/* hasCaseNull= */ oldLastCase.hasCaseNull(),
1012+
/* hasDefault= */ oldLastCase.hasDefault(),
1013+
/* instanceOfOptional= */ oldLastCase.instanceOfOptional(),
1014+
/* guardOptional= */ Optional.of(binaryTree.getRightOperand()),
1015+
/* expressionsOptional= */ oldLastCase.expressionsOptional(),
1016+
/* arrowRhsOptional= */ oldLastCase.arrowRhsOptional(),
1017+
/* caseSourceCodeRange= */ oldLastCase.caseSourceCodeRange()));
1018+
return rv;
1019+
}
1020+
1021+
} else if (!mustBeInstanceOf && predicateIsConditionalOr) {
1022+
// Maybe the predicate is something like `x == 1 || x == 2`.
1023+
int initialCasesSize = cases.size();
1024+
Optional<SubjectAndCaseExpressions> orExpressionsOptional =
1025+
validateConditionalOrsForSubject(
1026+
binaryTree,
1027+
subject,
1028+
cases,
1029+
elseOptional,
1030+
arrowRhsOptional,
1031+
ifTreeRange,
1032+
caseStartPosition,
1033+
caseEndPosition,
1034+
hasElse,
1035+
hasElseIf,
1036+
handledEnumValues,
1037+
state);
1038+
if (orExpressionsOptional.isPresent()) {
1039+
SubjectAndCaseExpressions orExpressions = orExpressionsOptional.get();
1040+
// Remove individual cases added, and add a single grouped case covering all of them
1041+
cases.subList(initialCasesSize, cases.size()).clear();
1042+
cases.add(
1043+
new CaseIr(
1044+
/* hasCaseNull= */ false,
1045+
/* hasDefault= */ false,
1046+
/* instanceOfOptional= */ Optional.empty(),
1047+
/* guardOptional= */ Optional.empty(),
1048+
Optional.of(orExpressions.expressions()),
9711049
arrowRhsOptional,
972-
handledEnumValues,
973-
ifTreeRange,
974-
/* caseStartPosition= */ caseStartPosition);
975-
if (rv.isPresent()) {
976-
CaseIr oldLastCase = cases.get(currentCasesSize);
977-
// Update last case to attach the guard
978-
cases.set(
979-
currentCasesSize,
1050+
/* caseSourceCodeRange= */ Range.closedOpen(caseStartPosition, caseEndPosition)));
1051+
1052+
boolean addDefault = hasElse && !hasElseIf;
1053+
if (addDefault) {
1054+
cases.add(
9801055
new CaseIr(
981-
/* hasCaseNull= */ oldLastCase.hasCaseNull(),
982-
/* hasDefault= */ oldLastCase.hasDefault(),
983-
/* instanceOfOptional= */ oldLastCase.instanceOfOptional(),
984-
/* guardOptional= */ Optional.of(binaryTree.getRightOperand()),
985-
/* expressionsOptional= */ oldLastCase.expressionsOptional(),
986-
/* arrowRhsOptional= */ oldLastCase.arrowRhsOptional(),
987-
/* caseSourceCodeRange= */ oldLastCase.caseSourceCodeRange()));
988-
return rv;
1056+
/* hasCaseNull= */ false,
1057+
/* hasDefault= */ true,
1058+
/* instanceOfOptional= */ Optional.empty(),
1059+
/* guardOptional= */ Optional.empty(),
1060+
/* expressionsOptional= */
1061+
/* expressionsOptional= */ Optional.empty(),
1062+
/* arrowRhsOptional= */ elseOptional,
1063+
/* caseSourceCodeRange= */ Range.closedOpen(
1064+
caseEndPosition,
1065+
elseOptional.isPresent()
1066+
? getStartPosition(elseOptional.get())
1067+
: caseEndPosition)));
9891068
}
1069+
1070+
return Optional.of(orExpressions.subject());
9901071
}
9911072
}
9921073
}
@@ -1010,6 +1091,149 @@ private Optional<ExpressionTree> validatePredicateForSubject(
10101091
return Optional.empty();
10111092
}
10121093

1094+
private static boolean isBooleanLiteral(ExpressionTree tree) {
1095+
return tree.getKind() == Kind.BOOLEAN_LITERAL;
1096+
}
1097+
1098+
/**
1099+
* Validates whether the {@code binaryTree} represents a series of conditional-ORs that can be
1100+
* converted to a single switch case having multiple expressions, returning the subject and case
1101+
* expressions if so. Otherwise, returns {@code Optional.empty()}.
1102+
*/
1103+
private Optional<SubjectAndCaseExpressions> validateConditionalOrsForSubject(
1104+
BinaryTree binaryTree,
1105+
Optional<ExpressionTree> subject,
1106+
List<CaseIr> cases,
1107+
Optional<StatementTree> elseOptional,
1108+
Optional<StatementTree> arrowRhsOptional,
1109+
Range<Integer> ifTreeRange,
1110+
int caseStartPosition,
1111+
int caseEndPosition,
1112+
boolean hasElse,
1113+
boolean hasElseIf,
1114+
Set<String> handledEnumValues,
1115+
VisitorState state) {
1116+
1117+
if (!binaryTree.getKind().equals(Kind.CONDITIONAL_OR)) {
1118+
return Optional.empty();
1119+
}
1120+
1121+
// Logical-OR is associative, so we can disregard parentheses
1122+
ExpressionTree lhs = ASTHelpers.stripParentheses(binaryTree.getLeftOperand());
1123+
ExpressionTree rhs = ASTHelpers.stripParentheses(binaryTree.getRightOperand());
1124+
List<ExpressionTree> caseExpressions = new ArrayList<>();
1125+
1126+
ExpressionTree[] sides = {lhs, rhs};
1127+
for (ExpressionTree side : sides) {
1128+
switch (side) {
1129+
case BinaryTree bt when bt.getKind().equals(Kind.EQUAL_TO) -> {
1130+
// Maybe comparing to a non-null compile-time constant? (`case null` not supported here
1131+
// due to Java syntax restrictions)
1132+
if ((COMPILE_TIME_CONSTANT_MATCHER.matches(bt.getLeftOperand(), state)
1133+
&& !isNull(bt.getLeftOperand()))
1134+
|| (COMPILE_TIME_CONSTANT_MATCHER.matches(bt.getRightOperand(), state)
1135+
&& !isNull(bt.getRightOperand()))) {
1136+
subject =
1137+
validateCompileTimeConstantForSubject(
1138+
bt.getLeftOperand(),
1139+
bt.getRightOperand(),
1140+
subject,
1141+
state,
1142+
cases,
1143+
elseOptional,
1144+
arrowRhsOptional,
1145+
ifTreeRange,
1146+
caseEndPosition,
1147+
hasElse,
1148+
hasElseIf);
1149+
1150+
if (subject.isEmpty()) {
1151+
return Optional.empty();
1152+
}
1153+
1154+
var compileTimeConstantExpression =
1155+
COMPILE_TIME_CONSTANT_MATCHER.matches(bt.getLeftOperand(), state)
1156+
? bt.getLeftOperand()
1157+
: bt.getRightOperand();
1158+
caseExpressions.add(compileTimeConstantExpression);
1159+
} else {
1160+
// Maybe comparing to an enum value?
1161+
if ((isEnumValue(bt.getLeftOperand(), state)
1162+
&& ASTHelpers.isEnumConstant(bt.getLeftOperand()))
1163+
|| (isEnumValue(bt.getRightOperand(), state)
1164+
&& ASTHelpers.isEnumConstant(bt.getRightOperand()))) {
1165+
subject =
1166+
validateEnumPredicateForSubject(
1167+
bt.getLeftOperand(),
1168+
bt.getRightOperand(),
1169+
subject,
1170+
state,
1171+
cases,
1172+
elseOptional,
1173+
arrowRhsOptional,
1174+
handledEnumValues,
1175+
ifTreeRange,
1176+
caseEndPosition,
1177+
hasElse,
1178+
hasElseIf);
1179+
1180+
if (subject.isEmpty()) {
1181+
return Optional.empty();
1182+
}
1183+
var enumValueExpression =
1184+
isEnumValue(bt.getLeftOperand(), state)
1185+
&& ASTHelpers.isEnumConstant(bt.getLeftOperand())
1186+
? bt.getLeftOperand()
1187+
: bt.getRightOperand();
1188+
caseExpressions.add(enumValueExpression);
1189+
} else {
1190+
// Unsupported
1191+
return Optional.empty();
1192+
}
1193+
}
1194+
}
1195+
1196+
case BinaryTree bt -> {
1197+
// Maybe multiple comparisons connected by OR? e.g. `x == 1 || x == 2 || ...`
1198+
var subjectAndCaseExpressionsOptional =
1199+
validateConditionalOrsForSubject(
1200+
bt,
1201+
subject,
1202+
cases,
1203+
elseOptional,
1204+
arrowRhsOptional,
1205+
ifTreeRange,
1206+
caseStartPosition,
1207+
caseEndPosition,
1208+
hasElse,
1209+
hasElseIf,
1210+
handledEnumValues,
1211+
state);
1212+
1213+
if (subjectAndCaseExpressionsOptional.isEmpty()) {
1214+
return Optional.empty();
1215+
}
1216+
SubjectAndCaseExpressions subjectAndCaseExpressions =
1217+
subjectAndCaseExpressionsOptional.get();
1218+
subject = Optional.of(subjectAndCaseExpressions.subject());
1219+
caseExpressions.addAll(subjectAndCaseExpressions.expressions());
1220+
}
1221+
default -> {
1222+
// Unsupported
1223+
return Optional.empty();
1224+
}
1225+
}
1226+
}
1227+
1228+
return caseExpressions.isEmpty()
1229+
? Optional.empty()
1230+
: Optional.of(new SubjectAndCaseExpressions(subject.get(), caseExpressions));
1231+
}
1232+
1233+
private static boolean isNull(ExpressionTree expression) {
1234+
return expression.getKind() == Kind.NULL_LITERAL;
1235+
}
1236+
10131237
/**
10141238
* Determines whether the {@code subject} expression "matches" the given {@code expression}. If
10151239
* {@code enableSafe} is true, then matching means that the subject must be referring to the same
@@ -1143,24 +1367,33 @@ private Optional<ExpressionTree> validateCompileTimeConstantForSubject(
11431367
boolean compileTimeConstantOnLhs = COMPILE_TIME_CONSTANT_MATCHER.matches(lhs, state);
11441368
ExpressionTree testExpression = compileTimeConstantOnLhs ? rhs : lhs;
11451369
ExpressionTree compileTimeConstant = compileTimeConstantOnLhs ? lhs : rhs;
1370+
Type compileTimeConstantType = getType(compileTimeConstant);
1371+
Type testExpressionType = getType(testExpression);
11461372

11471373
if (subject.isPresent() && !subjectMatches(subject.get(), testExpression, state)) {
11481374
// Predicate not compatible with predicate of preceding if statement
11491375
return Optional.empty();
11501376
}
11511377

1152-
// Don't support the use of Booleans as switch conditions
1153-
if (isSubtype(getType(testExpression), Suppliers.JAVA_LANG_BOOLEAN_TYPE.get(state), state)) {
1378+
// Don't support the use of String as switch conditions
1379+
if (isSubtype(testExpressionType, state.getSymtab().stringType, state)) {
11541380
return Optional.empty();
11551381
}
11561382

1157-
// Don't support the use of String as switch conditions
1158-
if (isSubtype(getType(testExpression), state.getSymtab().stringType, state)) {
1383+
// The compile time constant must be assignable to the type of the testExpression, which
1384+
// includes the possible use of assignment context conversions
1385+
Types types = state.getTypes();
1386+
if (!types.isAssignable(compileTimeConstantType, testExpressionType)) {
11591387
return Optional.empty();
11601388
}
11611389

1162-
// Switching on primitive long requires later Java version (we don't currently support)
1163-
if (state.getTypes().isSameType(getType(testExpression), state.getSymtab().longType)) {
1390+
// As of Java 21, a CaseConstant must be assignable to one of the following types (this is an
1391+
// outer bound; the checker does not necessarily support all of these)
1392+
boolean caseConstantIsAssignable =
1393+
ALLOWED_SWITCH_CASE_CONSTANT_TYPES.stream()
1394+
.map(state::getTypeFromString)
1395+
.anyMatch(t -> types.isAssignable(compileTimeConstantType, t));
1396+
if (!caseConstantIsAssignable) {
11641397
return Optional.empty();
11651398
}
11661399

@@ -1656,6 +1889,16 @@ record CaseIr(
16561889
}
16571890
}
16581891

1892+
/**
1893+
* Container for the subject (of an if predicate) and a (non-empty) list of expressions that can
1894+
* match that subject in the given case.
1895+
*/
1896+
record SubjectAndCaseExpressions(ExpressionTree subject, List<ExpressionTree> expressions) {
1897+
SubjectAndCaseExpressions {
1898+
checkArgument(!expressions.isEmpty());
1899+
}
1900+
}
1901+
16591902
/** This record represents the current state of the analysis of an if-chain. */
16601903
record IfChainAnalysisState(
16611904
// The expression to be switched on (if known)

0 commit comments

Comments
 (0)