Skip to content

Commit

Permalink
[StatementSwitchToExpressionSwitch] for direct conversion pattern, if…
Browse files Browse the repository at this point in the history 
… the 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: 713693052
  • Loading branch information
@markhbrady
markhbrady authored and Error Prone Team committed Jan 9, 2025
1 parent 91ff1af commit c2cb0f8
Show file tree
Hide file tree
Showing 2 changed files with 591 additions and 186 deletions.
77 changes: 58 additions & 19 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,21 +173,35 @@ 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));
}
}
if (enableDirectConversion && analysisResult.canConvertDirectlyToExpressionSwitch()) {
suggestedFixes.add(convertDirectlyToExpressionSwitch(switchTree, state, analysisResult));
suggestedFixes.add(
convertDirectlyToExpressionSwitch(
switchTree, state, analysisResult, /* removeDefault= */ false));

if (analysisResult.canRemoveDefault()) {
suggestedFixes.add(
convertDirectlyToExpressionSwitch(
switchTree, state, analysisResult, /* removeDefault= */ true));
}
}

return suggestedFixes.isEmpty()
Expand Down Expand Up @@ -344,10 +360,10 @@ && isSwitchExhaustiveWithoutDefault(
return AnalysisResult.of(
/* canConvertDirectlyToExpressionSwitch= */ allCasesHaveDefiniteControlFlow,
canConvertToReturnSwitch,
canRemoveDefault,
AssignmentSwitchAnalysisResult.of(
canConvertToAssignmentSwitch,
canCombineWithPrecedingVariableDeclaration,
canRemoveDefault,
assignmentSwitchAnalysisState.assignmentTargetOptional(),
assignmentSwitchAnalysisState.assignmentExpressionKindOptional(),
assignmentSwitchAnalysisState
Expand Down Expand Up @@ -631,10 +647,14 @@ private static boolean areStatementsConvertibleToExpressionSwitch(
/**
* Transforms the supplied statement switch into an expression switch directly. In this
* conversion, each nontrivial statement block is mapped one-to-one to a new {@code Expression} or
* {@code StatementBlock} on the right-hand side. Comments are preserved where possible.
* {@code StatementBlock} on the right-hand side (the `default:` case is removed if {@code
* removeDefault} is true). Comments are preserved where possible.
*/
private static SuggestedFix convertDirectlyToExpressionSwitch(
SwitchTree switchTree, VisitorState state, AnalysisResult analysisResult) {
SwitchTree switchTree,
VisitorState state,
AnalysisResult analysisResult,
boolean removeDefault) {

List<? extends CaseTree> cases = switchTree.getCases();
ImmutableList<ErrorProneComment> allSwitchComments =
Expand All @@ -654,6 +674,11 @@ private static SuggestedFix convertDirectlyToExpressionSwitch(
CaseTree caseTree = cases.get(caseIndex);
boolean isDefaultCase = isSwitchDefault(caseTree);

if (removeDefault && isDefaultCase) {
// Skip default case
continue;
}

// For readability, filter out trailing unlabelled break statement because these become a
// No-Op when used inside expression switches
ImmutableList<StatementTree> filteredStatements = filterOutRedundantBreak(caseTree);
Expand Down Expand Up @@ -748,7 +773,12 @@ private static SuggestedFix convertDirectlyToExpressionSwitch(
// Close the switch statement
replacementCodeBuilder.append("\n}");

return SuggestedFix.builder().replace(switchTree, replacementCodeBuilder.toString()).build();
SuggestedFix.Builder suggestedFixBuilder = SuggestedFix.builder();
if (removeDefault) {
suggestedFixBuilder.setShortDescription(REMOVE_DEFAULT_CASE_SHORT_DESCRIPTION);
}
suggestedFixBuilder.replace(switchTree, replacementCodeBuilder.toString());
return suggestedFixBuilder.build();
}

/**
Expand All @@ -759,7 +789,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 +811,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 +886,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 +1168,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 +1512,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 +1525,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 +1546,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 +1558,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 c2cb0f8

Please sign in to comment.