Skip to content

Commit

Permalink
[StatementSwitchToExpressionSwitch] for return switch pattern, if the…
Browse files Browse the repository at this point in the history 
… switch has a `default:` clause and is otherwise exhaustive (even without said `default:` clause), then propose an alternative fix which removes the `default:` clause and its code.

The first fix always retains the `default:` clause.

PiperOrigin-RevId: 713397195
  • Loading branch information
@markhbrady
markhbrady authored and Error Prone Team committed Jan 9, 2025
1 parent 91ff1af commit 3b783e0
Show file tree
Hide file tree
Showing 2 changed files with 220 additions and 16 deletions.
47 changes: 32 additions & 15 deletions core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,6 @@ public final class StatementSwitchToExpressionSwitch extends BugChecker
AssignmentSwitchAnalysisResult.of(
/* canConvertToAssignmentSwitch= */ false,
/* canCombineWithPrecedingVariableDeclaration= */ false,
/* canRemoveDefault= */ false,
/* assignmentTargetOptional= */ Optional.empty(),
/* assignmentKindOptional= */ Optional.empty(),
/* assignmentSourceCodeOptional= */ Optional.empty());
Expand All @@ -121,11 +120,14 @@ public final class StatementSwitchToExpressionSwitch extends BugChecker
AnalysisResult.of(
/* canConvertDirectlyToExpressionSwitch= */ false,
/* canConvertToReturnSwitch= */ false,
/* canRemoveDefault= */ false,
DEFAULT_ASSIGNMENT_SWITCH_ANALYSIS_RESULT,
/* groupedWithNextCase= */ ImmutableList.of());
private static final String EQUALS_STRING = "=";
private static final Matcher<ExpressionTree> COMPILE_TIME_CONSTANT_MATCHER =
CompileTimeConstantExpressionMatcher.instance();
private static final String REMOVE_DEFAULT_CASE_SHORT_DESCRIPTION =
"Remove default case because all enum values handled";

// Tri-state to represent the fall-thru control flow of a particular case of a particular
// statement switch
Expand Down Expand Up @@ -171,14 +173,20 @@ public Description matchSwitch(SwitchTree switchTree, VisitorState state) {

List<SuggestedFix> suggestedFixes = new ArrayList<>();
if (enableReturnSwitchConversion && analysisResult.canConvertToReturnSwitch()) {
suggestedFixes.add(convertToReturnSwitch(switchTree, state, analysisResult));
suggestedFixes.add(
convertToReturnSwitch(switchTree, state, analysisResult, /* removeDefault= */ false));

if (analysisResult.canRemoveDefault()) {
suggestedFixes.add(
convertToReturnSwitch(switchTree, state, analysisResult, /* removeDefault= */ true));
}
}
if (enableAssignmentSwitchConversion
&& analysisResult.assignmentSwitchAnalysisResult().canConvertToAssignmentSwitch()) {
suggestedFixes.add(
convertToAssignmentSwitch(switchTree, state, analysisResult, /* removeDefault= */ false));

if (analysisResult.assignmentSwitchAnalysisResult().canRemoveDefault()) {
if (analysisResult.canRemoveDefault()) {
suggestedFixes.add(
convertToAssignmentSwitch(
switchTree, state, analysisResult, /* removeDefault= */ true));
Expand Down Expand Up @@ -344,10 +352,10 @@ && isSwitchExhaustiveWithoutDefault(
return AnalysisResult.of(
/* canConvertDirectlyToExpressionSwitch= */ allCasesHaveDefiniteControlFlow,
canConvertToReturnSwitch,
canRemoveDefault,
AssignmentSwitchAnalysisResult.of(
canConvertToAssignmentSwitch,
canCombineWithPrecedingVariableDeclaration,
canRemoveDefault,
assignmentSwitchAnalysisState.assignmentTargetOptional(),
assignmentSwitchAnalysisState.assignmentExpressionKindOptional(),
assignmentSwitchAnalysisState
Expand Down Expand Up @@ -759,7 +767,10 @@ private static SuggestedFix convertDirectlyToExpressionSwitch(
* conversion is possible.
*/
private static SuggestedFix convertToReturnSwitch(
SwitchTree switchTree, VisitorState state, AnalysisResult analysisResult) {
SwitchTree switchTree,
VisitorState state,
AnalysisResult analysisResult,
boolean removeDefault) {

List<StatementTree> statementsToDelete = new ArrayList<>();
List<? extends CaseTree> cases = switchTree.getCases();
Expand All @@ -778,6 +789,10 @@ private static SuggestedFix convertToReturnSwitch(
for (int caseIndex = 0; caseIndex < cases.size(); caseIndex++) {
CaseTree caseTree = cases.get(caseIndex);
boolean isDefaultCase = isSwitchDefault(caseTree);
if (removeDefault && isDefaultCase) {
// Skip default case
continue;
}

String transformedBlockSource =
transformReturnOrThrowBlock(caseTree, state, getStatements(caseTree));
Expand Down Expand Up @@ -849,8 +864,11 @@ private static SuggestedFix convertToReturnSwitch(
// removed.
statementsToDelete.addAll(followingStatementsInBlock(switchTree, state));

SuggestedFix.Builder suggestedFixBuilder =
SuggestedFix.builder().replace(switchTree, replacementCodeBuilder.toString());
SuggestedFix.Builder suggestedFixBuilder = SuggestedFix.builder();
if (removeDefault) {
suggestedFixBuilder.setShortDescription(REMOVE_DEFAULT_CASE_SHORT_DESCRIPTION);
}
suggestedFixBuilder.replace(switchTree, replacementCodeBuilder.toString());
// Delete trailing statements, leaving comments where feasible
statementsToDelete.forEach(deleteMe -> suggestedFixBuilder.replace(deleteMe, ""));
return suggestedFixBuilder.build();
Expand Down Expand Up @@ -1128,8 +1146,7 @@ private static SuggestedFix convertToAssignmentSwitch(

SuggestedFix.Builder suggestedFixBuilder = SuggestedFix.builder();
if (removeDefault) {
suggestedFixBuilder.setShortDescription(
"Remove default case because all enum values handled");
suggestedFixBuilder.setShortDescription(REMOVE_DEFAULT_CASE_SHORT_DESCRIPTION);
}
suggestedFixBuilder.replace(switchTree, replacementCodeBuilder.toString());
statementsToDelete.forEach(deleteMe -> suggestedFixBuilder.replace(deleteMe, ""));
Expand Down Expand Up @@ -1473,6 +1490,10 @@ abstract static class AnalysisResult {
// Whether the statement switch can be converted to a return switch
abstract boolean canConvertToReturnSwitch();

// Whether the assignment switch is exhaustive even in the absence of the default case that
// exists in the original switch statement
abstract boolean canRemoveDefault();

// Results of the analysis for conversion to an assignment switch
abstract AssignmentSwitchAnalysisResult assignmentSwitchAnalysisResult();

Expand All @@ -1482,11 +1503,13 @@ abstract static class AnalysisResult {
static AnalysisResult of(
boolean canConvertDirectlyToExpressionSwitch,
boolean canConvertToReturnSwitch,
boolean canRemoveDefault,
AssignmentSwitchAnalysisResult assignmentSwitchAnalysisResult,
ImmutableList<Boolean> groupedWithNextCase) {
return new AutoValue_StatementSwitchToExpressionSwitch_AnalysisResult(
canConvertDirectlyToExpressionSwitch,
canConvertToReturnSwitch,
canRemoveDefault,
assignmentSwitchAnalysisResult,
groupedWithNextCase);
}
Expand All @@ -1501,10 +1524,6 @@ abstract static class AssignmentSwitchAnalysisResult {
// declaration
abstract boolean canCombineWithPrecedingVariableDeclaration();

// Whether the assignment switch is exhaustive even in the absence of the default case that
// exists in the original switch statement
abstract boolean canRemoveDefault();

// Target of the assignment switch, if any
abstract Optional<ExpressionTree> assignmentTargetOptional();

Expand All @@ -1517,14 +1536,12 @@ abstract static class AssignmentSwitchAnalysisResult {
static AssignmentSwitchAnalysisResult of(
boolean canConvertToAssignmentSwitch,
boolean canCombineWithPrecedingVariableDeclaration,
boolean canRemoveDefault,
Optional<ExpressionTree> assignmentTargetOptional,
Optional<Tree.Kind> assignmentKindOptional,
Optional<String> assignmentSourceCodeOptional) {
return new AutoValue_StatementSwitchToExpressionSwitch_AssignmentSwitchAnalysisResult(
canConvertToAssignmentSwitch,
canCombineWithPrecedingVariableDeclaration,
canRemoveDefault,
assignmentTargetOptional,
assignmentKindOptional,
assignmentSourceCodeOptional);
Expand Down
Loading

0 comments on commit 3b783e0

Please sign in to comment.