Skip to content

Commit

Permalink
[StatementSwitchToExpressionSwitch] when assigning from a switch on e…
Browse files Browse the repository at this point in the history
…num, if 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: 712971198
  • Loading branch information
markhbrady authored and Error Prone Team committed Jan 8, 2025
1 parent 1a14c75 commit 760f9a1
Show file tree
Hide file tree
Showing 2 changed files with 213 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ public final class StatementSwitchToExpressionSwitch extends BugChecker
AssignmentSwitchAnalysisResult.of(
/* canConvertToAssignmentSwitch= */ false,
/* canCombineWithPrecedingVariableDeclaration= */ false,
/* canRemoveDefault= */ false,
/* assignmentTargetOptional= */ Optional.empty(),
/* assignmentKindOptional= */ Optional.empty(),
/* assignmentSourceCodeOptional= */ Optional.empty());
Expand Down Expand Up @@ -174,7 +175,14 @@ public Description matchSwitch(SwitchTree switchTree, VisitorState state) {
}
if (enableAssignmentSwitchConversion
&& analysisResult.assignmentSwitchAnalysisResult().canConvertToAssignmentSwitch()) {
suggestedFixes.add(convertToAssignmentSwitch(switchTree, state, analysisResult));
suggestedFixes.add(
convertToAssignmentSwitch(switchTree, state, analysisResult, /* removeDefault= */ false));

if (analysisResult.assignmentSwitchAnalysisResult().canRemoveDefault()) {
suggestedFixes.add(
convertToAssignmentSwitch(
switchTree, state, analysisResult, /* removeDefault= */ true));
}
}
if (enableDirectConversion && analysisResult.canConvertDirectlyToExpressionSwitch()) {
suggestedFixes.add(convertDirectlyToExpressionSwitch(switchTree, state, analysisResult));
Expand Down Expand Up @@ -277,6 +285,10 @@ private static AnalysisResult analyzeSwitchTree(SwitchTree switchTree, VisitorSt
boolean exhaustive =
isSwitchExhaustive(
hasDefaultCase, handledEnumValues, ASTHelpers.getType(switchTree.getExpression()));
boolean canRemoveDefault =
hasDefaultCase
&& isSwitchExhaustiveWithoutDefault(
handledEnumValues, ASTHelpers.getType(switchTree.getExpression()));

boolean canConvertToReturnSwitch =
// All restrictions for direct conversion apply
Expand Down Expand Up @@ -335,6 +347,7 @@ private static AnalysisResult analyzeSwitchTree(SwitchTree switchTree, VisitorSt
AssignmentSwitchAnalysisResult.of(
canConvertToAssignmentSwitch,
canCombineWithPrecedingVariableDeclaration,
canRemoveDefault,
assignmentSwitchAnalysisState.assignmentTargetOptional(),
assignmentSwitchAnalysisState.assignmentExpressionKindOptional(),
assignmentSwitchAnalysisState
Expand Down Expand Up @@ -958,12 +971,15 @@ private static boolean precedingTwoStatementsNotInSameVariableDeclaratorList(
/**
* Transforms the supplied statement switch into an assignment switch style of expression switch.
* In this conversion, each nontrivial statement block is mapped one-to-one to a new expression on
* the right-hand side of the arrow. Comments are preserved where possible. Precondition: the
* {@code AnalysisResult} for the {@code SwitchTree} must have deduced that this conversion is
* possible.
* the right-hand side of the arrow (if {@code removeDefault} is true, then the {@code default:}
* block is skipped). Comments are preserved where possible. Precondition: the {@code
* AnalysisResult} for the {@code SwitchTree} must have deduced that this conversion is possible.
*/
private static SuggestedFix convertToAssignmentSwitch(
SwitchTree switchTree, VisitorState state, AnalysisResult analysisResult) {
SwitchTree switchTree,
VisitorState state,
AnalysisResult analysisResult,
boolean removeDefault) {

List<StatementTree> statementsToDelete = new ArrayList<>();
StringBuilder replacementCodeBuilder = new StringBuilder();
Expand Down Expand Up @@ -1033,6 +1049,10 @@ private static SuggestedFix convertToAssignmentSwitch(
for (int caseIndex = 0; caseIndex < cases.size(); caseIndex++) {
CaseTree caseTree = cases.get(caseIndex);
boolean isDefaultCase = isSwitchDefault(caseTree);
if (removeDefault && isDefaultCase) {
// Remove `default:` case (and its code, if any) from the SuggestedFix
continue;
}
ImmutableList<StatementTree> filteredStatements = filterOutRedundantBreak(caseTree);

String transformedBlockSource =
Expand Down Expand Up @@ -1106,8 +1126,12 @@ private static SuggestedFix convertToAssignmentSwitch(
// Close the switch statement
replacementCodeBuilder.append("\n};");

SuggestedFix.Builder suggestedFixBuilder =
SuggestedFix.builder().replace(switchTree, replacementCodeBuilder.toString());
SuggestedFix.Builder suggestedFixBuilder = SuggestedFix.builder();
if (removeDefault) {
suggestedFixBuilder.setShortDescription(
"Remove default case because all enum values handled");
}
suggestedFixBuilder.replace(switchTree, replacementCodeBuilder.toString());
statementsToDelete.forEach(deleteMe -> suggestedFixBuilder.replace(deleteMe, ""));
return suggestedFixBuilder.build();
}
Expand Down Expand Up @@ -1352,7 +1376,16 @@ private static boolean isSwitchExhaustive(
return true;
}

// Handles switching on enum (map is bijective)
return isSwitchExhaustiveWithoutDefault(handledEnumValues, switchType);
}

/**
* Ad-hoc algorithm to search for a surjective map from (non-null) values of a {@code switch}'s
* expression to a {@code CaseTree}, not including a {@code default} case (if present).
*/
private static boolean isSwitchExhaustiveWithoutDefault(
Set<String> handledEnumValues, Type switchType) {
// Handles switching on enum only (map is bijective)
if (switchType.asElement().getKind() != ElementKind.ENUM) {
// Give up on search
return false;
Expand Down Expand Up @@ -1468,6 +1501,10 @@ 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 @@ -1480,12 +1517,14 @@ 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 760f9a1

Please sign in to comment.