Skip to content

Commit 6f94a97

Browse files
kevinoconnor7Error Prone Team
kevinoconnor7
authored and
Error Prone Team
committed
Tolerate default cases in switches as being present to handle version skew
If the default branch of a switch is commented with the phrase "skew", we'll now assume that the developer added it explicitly to handle version skew concerns. As a result: * `UnnecessaryDefaultInEnumSwitch` will no longer complain about the `default` branch being unnecessary if it's already otherwise exhaustive. * `MissingCasesInEnumSwitch` will continue to check to ensure all cases are handled despite the default branch being present. This is intended to provide better safety for cases where developers have legitimate concerns about version skew, but don't want to give up the benefit of checking to ensure all cases are covered. If a user doesn't want to be exhaustive then they should not comment the default branch as being present for skew as it's clearly intended to handle more cases than that. PiperOrigin-RevId: 748697198
1 parent 0223abb commit 6f94a97

File tree

5 files changed

+326
-9
lines changed

5 files changed

+326
-9
lines changed

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

+20-4
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,12 @@
1818

1919
import static com.google.common.collect.ImmutableSet.toImmutableSet;
2020
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
21-
import static com.google.errorprone.util.ASTHelpers.isSwitchDefault;
21+
import static com.google.errorprone.bugpatterns.Switches.isDefaultCaseForSkew;
2222

2323
import com.google.common.collect.ImmutableSet;
2424
import com.google.common.collect.Sets;
2525
import com.google.errorprone.BugPattern;
26+
import com.google.errorprone.ErrorProneFlags;
2627
import com.google.errorprone.VisitorState;
2728
import com.google.errorprone.bugpatterns.BugChecker.SwitchTreeMatcher;
2829
import com.google.errorprone.matchers.Description;
@@ -32,18 +33,27 @@
3233
import com.sun.source.tree.SwitchTree;
3334
import com.sun.tools.javac.code.Type;
3435
import java.util.List;
36+
import java.util.Optional;
3537
import java.util.Set;
3638
import java.util.stream.Collectors;
39+
import javax.inject.Inject;
3740
import javax.lang.model.element.ElementKind;
3841

3942
/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */
4043
@BugPattern(
4144
summary = "Switches on enum types should either handle all values, or have a default case.",
4245
severity = WARNING)
4346
public class MissingCasesInEnumSwitch extends BugChecker implements SwitchTreeMatcher {
44-
4547
public static final int MAX_CASES_TO_PRINT = 5;
4648

49+
private final boolean checkSwitchesWithDefaultForSkew;
50+
51+
@Inject
52+
MissingCasesInEnumSwitch(ErrorProneFlags flags) {
53+
checkSwitchesWithDefaultForSkew =
54+
flags.getBoolean("MissingCasesInEnumSwitch:CheckSwitchesWithDefaultForSkew").orElse(true);
55+
}
56+
4757
@Override
4858
public Description matchSwitch(SwitchTree tree, VisitorState state) {
4959
ExpressionTree expression = tree.getExpression();
@@ -52,8 +62,14 @@ public Description matchSwitch(SwitchTree tree, VisitorState state) {
5262
if (switchType.asElement().getKind() != ElementKind.ENUM) {
5363
return Description.NO_MATCH;
5464
}
55-
// default case is present
56-
if (cases.stream().anyMatch(c -> isSwitchDefault(c))) {
65+
Optional<? extends CaseTree> defaultCase =
66+
cases.stream().filter(ASTHelpers::isSwitchDefault).findFirst();
67+
// Continue to perform the check only if:
68+
// - there is no default case present or
69+
// - the default case only exists for potential version.
70+
if (defaultCase.isPresent()
71+
&& (!checkSwitchesWithDefaultForSkew
72+
|| !isDefaultCaseForSkew(tree, defaultCase.get(), state))) {
5773
return Description.NO_MATCH;
5874
}
5975
ImmutableSet<String> handled =

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

+112
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
/*
2+
* Copyright 2025 The Error Prone Authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.google.errorprone.bugpatterns;
18+
19+
import static com.google.common.base.Preconditions.checkArgument;
20+
import static com.google.errorprone.util.ASTHelpers.getStartPosition;
21+
import static com.google.errorprone.util.ASTHelpers.isSwitchDefault;
22+
23+
import com.google.errorprone.VisitorState;
24+
import com.google.errorprone.util.ErrorProneTokens;
25+
import com.sun.source.tree.CaseTree;
26+
import com.sun.source.tree.SwitchExpressionTree;
27+
import com.sun.source.tree.SwitchTree;
28+
import java.util.regex.Pattern;
29+
30+
final class Switches {
31+
private static final Pattern SKEW_PATTERN =
32+
Pattern.compile("\\bskew\\b", Pattern.CASE_INSENSITIVE);
33+
34+
/**
35+
* Whether the default case for the given switch is a fallback for potential version skew.
36+
*
37+
* <p>This is purely a heuristic that looks for the word "skew" in the comment of the default
38+
* case.
39+
*/
40+
static boolean isDefaultCaseForSkew(
41+
SwitchTree switchTree, CaseTree defaultCase, VisitorState state) {
42+
checkArgument(isSwitchDefault(defaultCase));
43+
44+
int indexOfDefault = switchTree.getCases().indexOf(defaultCase);
45+
46+
// Start position will either be from the end of the previous case, or from the end of the
47+
// expression being switched on if this is the first case.
48+
int startPos =
49+
indexOfDefault > 0
50+
? state.getEndPosition(switchTree.getCases().get(indexOfDefault - 1))
51+
: state.getEndPosition(switchTree.getExpression());
52+
53+
// End position will be the start of the body/first statement, the start of the next case, or
54+
// fallback to the end of the switch.
55+
int endPos;
56+
if (defaultCase.getBody() != null) {
57+
endPos = getStartPosition(defaultCase.getBody());
58+
} else if (!defaultCase.getStatements().isEmpty()) {
59+
endPos = getStartPosition(defaultCase.getStatements().get(0));
60+
} else if (indexOfDefault + 1 < switchTree.getCases().size()) {
61+
endPos = getStartPosition(switchTree.getCases().get(indexOfDefault + 1));
62+
} else {
63+
endPos = state.getEndPosition(switchTree);
64+
}
65+
66+
var tokens =
67+
ErrorProneTokens.getTokens(
68+
state.getSourceCode().subSequence(startPos, endPos).toString(),
69+
startPos,
70+
state.context);
71+
72+
return tokens.stream()
73+
.flatMap(token -> token.comments().stream())
74+
.anyMatch(comment -> SKEW_PATTERN.matcher(comment.getText()).find());
75+
}
76+
77+
/**
78+
* Whether the default case for the given switch is a fallback for potential version skew.
79+
*
80+
* <p>This is purely a heuristic that looks for the word "skew" in the comment of the default
81+
* case.
82+
*/
83+
static boolean isDefaultCaseForSkew(
84+
SwitchExpressionTree switchTree, CaseTree defaultCase, VisitorState state) {
85+
checkArgument(isSwitchDefault(defaultCase));
86+
87+
int indexOfDefault = switchTree.getCases().indexOf(defaultCase);
88+
89+
// Start position will either be from the end of the previous case, or from the end of the
90+
// expression being switched on.
91+
int startPos =
92+
indexOfDefault > 0
93+
? state.getEndPosition(switchTree.getCases().get(indexOfDefault - 1))
94+
: state.getEndPosition(switchTree.getExpression());
95+
96+
// End position will be the start of the body of the default case. In switch expressions the
97+
// default case will always have a body as it cannot be combined with other cases.
98+
int endPos = getStartPosition(defaultCase.getBody());
99+
100+
var tokens =
101+
ErrorProneTokens.getTokens(
102+
state.getSourceCode().subSequence(startPos, endPos).toString(),
103+
startPos,
104+
state.context);
105+
106+
return tokens.stream()
107+
.flatMap(token -> token.comments().stream())
108+
.anyMatch(comment -> SKEW_PATTERN.matcher(comment.getText()).find());
109+
}
110+
111+
private Switches() {}
112+
}

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

+27-5
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import static com.google.common.collect.Iterables.getLast;
2121
import static com.google.common.collect.Iterables.getOnlyElement;
2222
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
23+
import static com.google.errorprone.bugpatterns.Switches.isDefaultCaseForSkew;
2324
import static com.google.errorprone.matchers.Description.NO_MATCH;
2425
import static com.google.errorprone.util.ASTHelpers.getStartPosition;
2526
import static com.google.errorprone.util.ASTHelpers.getType;
@@ -31,6 +32,7 @@
3132
import com.google.common.collect.Sets;
3233
import com.google.common.collect.Sets.SetView;
3334
import com.google.errorprone.BugPattern;
35+
import com.google.errorprone.ErrorProneFlags;
3436
import com.google.errorprone.VisitorState;
3537
import com.google.errorprone.bugpatterns.BugChecker.SwitchExpressionTreeMatcher;
3638
import com.google.errorprone.bugpatterns.BugChecker.SwitchTreeMatcher;
@@ -46,6 +48,7 @@
4648
import com.sun.source.tree.Tree;
4749
import com.sun.tools.javac.code.Symbol.TypeSymbol;
4850
import java.util.List;
51+
import javax.inject.Inject;
4952
import javax.lang.model.element.ElementKind;
5053

5154
/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */
@@ -72,6 +75,14 @@ public class UnnecessaryDefaultInEnumSwitch extends BugChecker
7275
+ "to `UNRECOGNIZED` to enable compile-time enforcement that the switch statement is "
7376
+ "exhaustive";
7477

78+
private final boolean allowDefaultForSkew;
79+
80+
@Inject
81+
UnnecessaryDefaultInEnumSwitch(ErrorProneFlags flags) {
82+
allowDefaultForSkew =
83+
flags.getBoolean("UnnecessaryDefaultInEnumSwitch:AllowDefaultForSkew").orElse(true);
84+
}
85+
7586
@Override
7687
public Description matchSwitchExpression(SwitchExpressionTree tree, VisitorState state) {
7788
TypeSymbol switchType = getType(tree.getExpression()).asElement();
@@ -83,6 +94,10 @@ public Description matchSwitchExpression(SwitchExpressionTree tree, VisitorState
8394
if (defaultCase == null) {
8495
return NO_MATCH;
8596
}
97+
if (allowDefaultForSkew && isDefaultCaseForSkew(tree, defaultCase, state)) {
98+
// default is explicitly commented as being present for skew, it can stay.
99+
return NO_MATCH;
100+
}
86101
SetView<String> unhandledCases = unhandledCases(tree.getCases(), switchType);
87102
if (unhandledCases.equals(ImmutableSet.of("UNRECOGNIZED"))) {
88103
// switch handles all values of a proto-generated enum except for 'UNRECOGNIZED'.
@@ -134,12 +149,19 @@ public Description matchSwitch(SwitchTree switchTree, VisitorState state) {
134149
// switch handles all values of a proto-generated enum except for 'UNRECOGNIZED'.
135150
return fixUnrecognized(switchTree, defaultCase, state);
136151
}
137-
if (unhandledCases.isEmpty()) {
138-
// switch is exhaustive, remove the default if we can.
139-
return fixDefault(switchTree, caseBeforeDefault, defaultCase, state);
152+
153+
if (!unhandledCases.isEmpty()) {
154+
// switch is non-exhaustive, default can stay.
155+
return NO_MATCH;
140156
}
141-
// switch is non-exhaustive, default can stay.
142-
return NO_MATCH;
157+
158+
if (allowDefaultForSkew && isDefaultCaseForSkew(switchTree, defaultCase, state)) {
159+
// default is explicitly commented as being present for skew, it can stay.
160+
return NO_MATCH;
161+
}
162+
163+
// switch is exhaustive, remove the default if we can.
164+
return fixDefault(switchTree, caseBeforeDefault, defaultCase, state);
143165
}
144166

145167
private Description fixDefault(

‎core/src/test/java/com/google/errorprone/bugpatterns/MissingCasesInEnumSwitchTest.java

+29
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,35 @@ void m(Case c) {
134134
.doTest();
135135
}
136136

137+
@Test
138+
public void nonExhaustive_withDefaultForSkew() {
139+
compilationHelper
140+
.addSourceLines(
141+
"Test.java",
142+
"""
143+
class Test {
144+
enum Case {
145+
ONE,
146+
TWO,
147+
THREE
148+
}
149+
150+
void m(Case c) {
151+
// BUG: Diagnostic contains: THREE
152+
switch (c) {
153+
case ONE:
154+
case TWO:
155+
System.err.println("found it!");
156+
break;
157+
default: // fallback for library skew
158+
break;
159+
}
160+
}
161+
}
162+
""")
163+
.doTest();
164+
}
165+
137166
@Test
138167
public void nonExhaustive() {
139168
compilationHelper

0 commit comments

Comments
 (0)