Skip to content

Commit a2df97f

Browse files
markhbradyError Prone Team
markhbrady
authored and
Error Prone Team
committed
[StatementSwitchToExpressionSwitch] fix bug where case null can sometimes be missing from generated auto-fix
PiperOrigin-RevId: 806362870
1 parent e691be5 commit a2df97f

File tree

2 files changed

+442
-28
lines changed

2 files changed

+442
-28
lines changed

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

Lines changed: 98 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,20 @@ enum CaseQualifications {
160160
SOME_OR_ALL_CASES_DONT_QUALIFY
161161
}
162162

163+
/**
164+
* The kind of null/default cases included within a single CaseTree.
165+
*
166+
* <p>This enum is used to classify whether a CaseTree includes a null and/or default. Referencing
167+
* JLS 21 §14.11.1, the `SwitchLabel:` production has specific rules applicable to null/default
168+
* cases: `case null, [default]` and `default`. All other scenarios are lumped into KIND_NEITHER.
169+
*/
170+
enum NullDefaultKind {
171+
KIND_NULL_AND_DEFAULT,
172+
KIND_DEFAULT,
173+
KIND_NULL,
174+
KIND_NEITHER
175+
}
176+
163177
private final boolean enableDirectConversion;
164178
private final boolean enableReturnSwitchConversion;
165179
private final boolean enableAssignmentSwitchConversion;
@@ -293,12 +307,15 @@ private static AnalysisResult analyzeSwitchTree(SwitchTree switchTree, VisitorSt
293307
// Case patterns are not currently supported by the checker.
294308
return DEFAULT_ANALYSIS_RESULT;
295309
}
296-
boolean isDefaultCase = caseTree.getExpressions().isEmpty();
310+
311+
NullDefaultKind nullDefaultKind = analyzeCaseForNullAndDefault(caseTree);
312+
boolean isDefaultCase =
313+
nullDefaultKind.equals(NullDefaultKind.KIND_DEFAULT)
314+
|| nullDefaultKind.equals(NullDefaultKind.KIND_NULL_AND_DEFAULT);
297315
isNullCase.set(
298316
caseIndex,
299-
!isDefaultCase
300-
&& caseTree.getExpressions().stream()
301-
.anyMatch(expressionTree -> expressionTree.getKind() == Kind.NULL_LITERAL));
317+
nullDefaultKind.equals(NullDefaultKind.KIND_NULL)
318+
|| nullDefaultKind.equals(NullDefaultKind.KIND_NULL_AND_DEFAULT));
302319
hasDefaultCase |= isDefaultCase;
303320

304321
// Null case can never be grouped with a preceding case
@@ -606,6 +623,21 @@ private static String renderJavaSourceOfAssignment(ExpressionTree tree) {
606623
return pretty.operatorName(jcAssignOp.getTag().noAssignOp()) + EQUALS_STRING;
607624
}
608625

626+
/**
627+
* Renders the Java source prefix needed for the supplied {@code nullDefaultKind}, incorporating
628+
* whether the `default` case should be removed.
629+
*/
630+
private static String renderNullDefaultKindPrefix(
631+
NullDefaultKind nullDefaultKind, boolean removeDefault) {
632+
633+
return switch (nullDefaultKind) {
634+
case KIND_NULL_AND_DEFAULT -> removeDefault ? "case null" : "case null, default";
635+
case KIND_NULL -> "case null";
636+
case KIND_DEFAULT -> removeDefault ? "" : "default";
637+
case KIND_NEITHER -> "case ";
638+
};
639+
}
640+
609641
/**
610642
* Analyze a single {@code case} of a single {@code switch} statement to determine whether it is
611643
* convertible to a return switch. The supplied {@code previousCaseQualifications} is updated and
@@ -951,10 +983,10 @@ private static SuggestedFix convertDirectlyToExpressionSwitch(
951983
boolean firstCaseInGroup = true;
952984
for (int caseIndex = 0; caseIndex < cases.size(); caseIndex++) {
953985
CaseTree caseTree = cases.get(caseIndex);
954-
boolean isDefaultCase = isSwitchDefault(caseTree);
986+
NullDefaultKind nullDefaultKind = analyzeCaseForNullAndDefault(caseTree);
955987

956-
if (removeDefault && isDefaultCase) {
957-
// Skip default case
988+
if (removeDefault && nullDefaultKind.equals(NullDefaultKind.KIND_DEFAULT)) {
989+
// Skip removed default (and its code) entirely
958990
continue;
959991
}
960992

@@ -971,14 +1003,19 @@ private static SuggestedFix convertDirectlyToExpressionSwitch(
9711003
? extractCommentsBeforeFirstCase(switchTree, allSwitchComments).orElse("")
9721004
: "");
9731005

974-
replacementCodeBuilder.append("\n ");
975-
if (!isDefaultCase) {
976-
replacementCodeBuilder.append("case ");
1006+
replacementCodeBuilder
1007+
.append("\n ")
1008+
.append(renderNullDefaultKindPrefix(nullDefaultKind, removeDefault));
1009+
} else {
1010+
// Second or later case in our group
1011+
if (nullDefaultKind.equals(NullDefaultKind.KIND_DEFAULT)) {
1012+
replacementCodeBuilder.append("default");
9771013
}
9781014
}
979-
replacementCodeBuilder.append(
980-
isDefaultCase ? "default" : printCaseExpressions(caseTree, state));
9811015

1016+
if (nullDefaultKind.equals(NullDefaultKind.KIND_NEITHER)) {
1017+
replacementCodeBuilder.append(printCaseExpressions(caseTree, state));
1018+
}
9821019
Optional<String> commentsAfterCaseOptional =
9831020
extractCommentsAfterCase(switchTree, allSwitchComments, state, caseIndex);
9841021
if (analysisResult.groupedWithNextCase().get(caseIndex)) {
@@ -1095,9 +1132,9 @@ private static SuggestedFix convertToReturnSwitch(
10951132
boolean firstCaseInGroup = true;
10961133
for (int caseIndex = 0; caseIndex < cases.size(); caseIndex++) {
10971134
CaseTree caseTree = cases.get(caseIndex);
1098-
boolean isDefaultCase = isSwitchDefault(caseTree);
1099-
if (removeDefault && isDefaultCase) {
1100-
// Skip default case
1135+
NullDefaultKind nullDefaultKind = analyzeCaseForNullAndDefault(caseTree);
1136+
if (removeDefault && nullDefaultKind.equals(NullDefaultKind.KIND_DEFAULT)) {
1137+
// Skip removed default (and its code) entirely
11011138
continue;
11021139
}
11031140

@@ -1111,13 +1148,19 @@ private static SuggestedFix convertToReturnSwitch(
11111148
? extractCommentsBeforeFirstCase(switchTree, allSwitchComments).orElse("")
11121149
: "");
11131150

1114-
replacementCodeBuilder.append("\n ");
1115-
if (!isDefaultCase) {
1116-
replacementCodeBuilder.append("case ");
1151+
replacementCodeBuilder
1152+
.append("\n ")
1153+
.append(renderNullDefaultKindPrefix(nullDefaultKind, removeDefault));
1154+
} else {
1155+
// Second or later case in our group
1156+
if (nullDefaultKind.equals(NullDefaultKind.KIND_DEFAULT)) {
1157+
replacementCodeBuilder.append("default");
11171158
}
11181159
}
1119-
replacementCodeBuilder.append(
1120-
isDefaultCase ? "default" : printCaseExpressions(caseTree, state));
1160+
1161+
if (nullDefaultKind.equals(NullDefaultKind.KIND_NEITHER)) {
1162+
replacementCodeBuilder.append(printCaseExpressions(caseTree, state));
1163+
}
11211164

11221165
Optional<String> commentsAfterCaseOptional =
11231166
extractCommentsAfterCase(switchTree, allSwitchComments, state, caseIndex);
@@ -1323,9 +1366,10 @@ private static SuggestedFix convertToAssignmentSwitch(
13231366
boolean firstCaseInGroup = true;
13241367
for (int caseIndex = 0; caseIndex < cases.size(); caseIndex++) {
13251368
CaseTree caseTree = cases.get(caseIndex);
1326-
boolean isDefaultCase = isSwitchDefault(caseTree);
1327-
if (removeDefault && isDefaultCase) {
1328-
// Remove `default:` case (and its code, if any) from the SuggestedFix
1369+
NullDefaultKind nullDefaultKind = analyzeCaseForNullAndDefault(caseTree);
1370+
1371+
if (removeDefault && nullDefaultKind.equals(NullDefaultKind.KIND_DEFAULT)) {
1372+
// Skip removed default (and its code) entirely
13291373
continue;
13301374
}
13311375
ImmutableList<StatementTree> filteredStatements = filterOutRedundantBreak(caseTree);
@@ -1340,13 +1384,19 @@ private static SuggestedFix convertToAssignmentSwitch(
13401384
? extractCommentsBeforeFirstCase(switchTree, allSwitchComments).orElse("")
13411385
: "");
13421386

1343-
replacementCodeBuilder.append("\n ");
1344-
if (!isDefaultCase) {
1345-
replacementCodeBuilder.append("case ");
1387+
replacementCodeBuilder
1388+
.append("\n ")
1389+
.append(renderNullDefaultKindPrefix(nullDefaultKind, removeDefault));
1390+
} else {
1391+
// Second or later case in our group
1392+
if (nullDefaultKind.equals(NullDefaultKind.KIND_DEFAULT)) {
1393+
replacementCodeBuilder.append("default");
13461394
}
13471395
}
1348-
replacementCodeBuilder.append(
1349-
isDefaultCase ? "default" : printCaseExpressions(caseTree, state));
1396+
1397+
if (nullDefaultKind.equals(NullDefaultKind.KIND_NEITHER)) {
1398+
replacementCodeBuilder.append(printCaseExpressions(caseTree, state));
1399+
}
13501400

13511401
Optional<String> commentsAfterCaseOptional =
13521402
extractCommentsAfterCase(switchTree, allSwitchComments, state, caseIndex);
@@ -1778,6 +1828,26 @@ private static String transformAssignOrThrowBlock(
17781828
return transformedBlockBuilder.toString();
17791829
}
17801830

1831+
/**
1832+
* Determines whether the supplied {@code caseTree} case contains `case null` and/or `default`.
1833+
*/
1834+
private static NullDefaultKind analyzeCaseForNullAndDefault(CaseTree caseTree) {
1835+
boolean hasDefault = isSwitchDefault(caseTree);
1836+
boolean hasNull =
1837+
caseTree.getExpressions().stream()
1838+
.anyMatch(expression -> expression.getKind().equals(Tree.Kind.NULL_LITERAL));
1839+
1840+
if (hasNull && hasDefault) {
1841+
return NullDefaultKind.KIND_NULL_AND_DEFAULT;
1842+
} else if (hasNull) {
1843+
return NullDefaultKind.KIND_NULL;
1844+
} else if (hasDefault) {
1845+
return NullDefaultKind.KIND_DEFAULT;
1846+
}
1847+
1848+
return NullDefaultKind.KIND_NEITHER;
1849+
}
1850+
17811851
@AutoValue
17821852
abstract static class AnalysisResult {
17831853
/** Whether the statement switch can be directly converted to an expression switch */

0 commit comments

Comments
 (0)