From 3b783e0f6851d9a2541e2e34d9281cb66072b78e Mon Sep 17 00:00:00 2001 From: markbrady Date: Wed, 8 Jan 2025 13:25:51 -0800 Subject: [PATCH] [StatementSwitchToExpressionSwitch] for return switch pattern, 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: 713397195 --- .../StatementSwitchToExpressionSwitch.java | 47 +++-- ...StatementSwitchToExpressionSwitchTest.java | 189 +++++++++++++++++- 2 files changed, 220 insertions(+), 16 deletions(-) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java b/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java index 9dca068e1df..587cf35b7d1 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java @@ -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()); @@ -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 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 @@ -171,14 +173,20 @@ public Description matchSwitch(SwitchTree switchTree, VisitorState state) { List 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)); @@ -344,10 +352,10 @@ && isSwitchExhaustiveWithoutDefault( return AnalysisResult.of( /* canConvertDirectlyToExpressionSwitch= */ allCasesHaveDefiniteControlFlow, canConvertToReturnSwitch, + canRemoveDefault, AssignmentSwitchAnalysisResult.of( canConvertToAssignmentSwitch, canCombineWithPrecedingVariableDeclaration, - canRemoveDefault, assignmentSwitchAnalysisState.assignmentTargetOptional(), assignmentSwitchAnalysisState.assignmentExpressionKindOptional(), assignmentSwitchAnalysisState @@ -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 statementsToDelete = new ArrayList<>(); List cases = switchTree.getCases(); @@ -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)); @@ -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(); @@ -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, "")); @@ -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(); @@ -1482,11 +1503,13 @@ abstract static class AnalysisResult { static AnalysisResult of( boolean canConvertDirectlyToExpressionSwitch, boolean canConvertToReturnSwitch, + boolean canRemoveDefault, AssignmentSwitchAnalysisResult assignmentSwitchAnalysisResult, ImmutableList groupedWithNextCase) { return new AutoValue_StatementSwitchToExpressionSwitch_AnalysisResult( canConvertDirectlyToExpressionSwitch, canConvertToReturnSwitch, + canRemoveDefault, assignmentSwitchAnalysisResult, groupedWithNextCase); } @@ -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 assignmentTargetOptional(); @@ -1517,14 +1536,12 @@ abstract static class AssignmentSwitchAnalysisResult { static AssignmentSwitchAnalysisResult of( boolean canConvertToAssignmentSwitch, boolean canCombineWithPrecedingVariableDeclaration, - boolean canRemoveDefault, Optional assignmentTargetOptional, Optional assignmentKindOptional, Optional assignmentSourceCodeOptional) { return new AutoValue_StatementSwitchToExpressionSwitch_AssignmentSwitchAnalysisResult( canConvertToAssignmentSwitch, canCombineWithPrecedingVariableDeclaration, - canRemoveDefault, assignmentTargetOptional, assignmentKindOptional, assignmentSourceCodeOptional); diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java index b1649121cd4..49520ef9c5c 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java @@ -2002,6 +2002,7 @@ public int foo(Side side) { .setArgs( ImmutableList.of( "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) + .setFixChooser(Iterables::getOnlyElement) .doTest(); } @@ -2075,6 +2076,7 @@ public int foo(Side side) { .setArgs( ImmutableList.of( "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) + .setFixChooser(Iterables::getOnlyElement) .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } @@ -2154,11 +2156,12 @@ public int foo(Side side) { .setArgs( ImmutableList.of( "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) + .setFixChooser(Iterables::getOnlyElement) .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } @Test - public void switchByEnum_switchInReturnSwitchWithShouldNeverHappen_error() { + public void switchByEnum_switchInReturnSwitchWithShouldNeverHappen_noError() { // No error because the inner switch is the only fixable one helper .addSourceLines( @@ -2312,6 +2315,7 @@ public int foo(Side side) { .setArgs( ImmutableList.of( "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) + .setFixChooser(Iterables::getOnlyElement) .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } @@ -2540,6 +2544,186 @@ public int foo(Side side) { .setArgs( ImmutableList.of( "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) + .setFixChooser(Iterables::getOnlyElement) + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void switchByEnum_returnSwitchWithAllEnumValuesAndDefault_errorRemoveDefault() { + // The return switch has a case for each enum value *and* a default case handler. This test + // asserts that a secondary fix is proposed to remove the default case. Note that the original + // code cannot have a "should never happen" (after the statement switch) because the compiler + // will deduce that such code is unreachable. + + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + enum Side { + HEART, + SPADE, + DIAMOND, + CLUB + }; + + public Test(int foo) {} + + public int invoke() { + return 123; + } + + public int foo(Side side) { + System.out.println("don't delete 0"); + if (invoke() > 0) { + System.out.println("don't delete 1"); + // Preceding comment + switch (side) { + case HEART /* lhs comment */: // rhs comment + // Fall through + case DIAMOND: + return invoke(); + case SPADE: + throw new RuntimeException(); + case CLUB: + throw new NullPointerException(); + default: + throw new NullPointerException(); + } + // Unreachable - no "should never happen" code + } + System.out.println("don't delete 2"); + return 0; + } + } + """) + .addOutputLines( + "Test.java", + """ + class Test { + enum Side { + HEART, + SPADE, + DIAMOND, + CLUB + }; + + public Test(int foo) {} + + public int invoke() { + return 123; + } + + public int foo(Side side) { + System.out.println("don't delete 0"); + if (invoke() > 0) { + System.out.println("don't delete 1"); + // Preceding comment + return switch (side) { + case HEART, DIAMOND -> + /* lhs comment */ + // rhs comment + invoke(); + case SPADE -> throw new RuntimeException(); + case CLUB -> throw new NullPointerException(); + default -> throw new NullPointerException(); + }; + // Unreachable - no "should never happen" code + } + System.out.println("don't delete 2"); + return 0; + } + } + """) + .setArgs( + ImmutableList.of( + "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) + .setFixChooser(FixChoosers.FIRST) + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + + refactoringHelper2 + .addInputLines( + "Test.java", + """ + class Test { + enum Side { + HEART, + SPADE, + DIAMOND, + CLUB + }; + + public Test(int foo) {} + + public int invoke() { + return 123; + } + + public int foo(Side side) { + System.out.println("don't delete 0"); + if (invoke() > 0) { + System.out.println("don't delete 1"); + // Preceding comment + switch (side) { + case HEART /* lhs comment */: // rhs comment + // Fall through + case DIAMOND: + return invoke(); + case SPADE: + throw new RuntimeException(); + case CLUB: + throw new NullPointerException(); + default: + throw new NullPointerException(); + } + // Unreachable - no "should never happen" code + } + System.out.println("don't delete 2"); + return 0; + } + } + """) + .addOutputLines( + "Test.java", + """ + class Test { + enum Side { + HEART, + SPADE, + DIAMOND, + CLUB + }; + + public Test(int foo) {} + + public int invoke() { + return 123; + } + + public int foo(Side side) { + System.out.println("don't delete 0"); + if (invoke() > 0) { + System.out.println("don't delete 1"); + // Preceding comment + return switch (side) { + case HEART, DIAMOND -> + /* lhs comment */ + // rhs comment + invoke(); + case SPADE -> throw new RuntimeException(); + case CLUB -> throw new NullPointerException(); + }; + // Unreachable - no "should never happen" code + } + System.out.println("don't delete 2"); + return 0; + } + } + """) + .setArgs( + ImmutableList.of( + "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) + .setFixChooser(FixChoosers.SECOND) .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } @@ -2682,6 +2866,7 @@ public int foo(Side side) { .setArgs( ImmutableList.of( "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) + .setFixChooser(Iterables::getOnlyElement) .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } @@ -2840,6 +3025,7 @@ public int foo(Side side) { .setArgs( ImmutableList.of( "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) + .setFixChooser(Iterables::getOnlyElement) .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } @@ -2931,6 +3117,7 @@ public int foo(Side side) { .setArgs( ImmutableList.of( "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) + .setFixChooser(Iterables::getOnlyElement) .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); }