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 c1b74e1024d..1b45646e741 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java @@ -46,7 +46,6 @@ import com.google.errorprone.matchers.Matchers; import com.google.errorprone.util.ASTHelpers; import com.google.errorprone.util.Reachability; -import com.google.errorprone.util.SourceVersion; import com.sun.source.tree.AssignmentTree; import com.sun.source.tree.BlockTree; import com.sun.source.tree.BreakTree; @@ -63,7 +62,6 @@ import com.sun.source.util.TreePath; import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Type; -import com.sun.tools.javac.tree.JCTree; import com.sun.tools.javac.tree.JCTree.JCAssign; import com.sun.tools.javac.tree.JCTree.JCAssignOp; import com.sun.tools.javac.tree.Pretty; @@ -135,7 +133,10 @@ static enum CaseQualifications { @Inject StatementSwitchToExpressionSwitch(ErrorProneFlags flags) { this.enableDirectConversion = - flags.getBoolean("StatementSwitchToExpressionSwitch:EnableDirectConversion").orElse(false); + true + || flags + .getBoolean("StatementSwitchToExpressionSwitch:EnableDirectConversion") + .orElse(false); this.enableReturnSwitchConversion = flags .getBoolean("StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion") @@ -148,9 +149,9 @@ static enum CaseQualifications { @Override public Description matchSwitch(SwitchTree switchTree, VisitorState state) { - if (!SourceVersion.supportsSwitchExpressions(state.context)) { - return NO_MATCH; - } + // if (!SourceVersion.supportsSwitchExpressions(state.context)) { + // return NO_MATCH; + // } AnalysisResult analysisResult = analyzeSwitchTree(switchTree, state); @@ -529,8 +530,7 @@ private static SuggestedFix convertDirectlyToExpressionSwitch( // For readability, filter out trailing unlabelled break statement because these become a // No-Op when used inside expression switches ImmutableList filteredStatements = filterOutRedundantBreak(caseTree); - String transformedBlockSource = - transformBlock(caseTree, state, cases, caseIndex, filteredStatements); + String transformedBlockSource = transformBlock(caseTree, state, filteredStatements); if (firstCaseInGroup) { groupedCaseCommentsAccumulator = new StringBuilder(); @@ -623,7 +623,7 @@ private static SuggestedFix convertToReturnSwitch( boolean isDefaultCase = caseTree.getExpression() == null; String transformedBlockSource = - transformReturnOrThrowBlock(caseTree, state, cases, caseIndex, getStatements(caseTree)); + transformReturnOrThrowBlock(caseTree, state, getStatements(caseTree)); if (firstCaseInGroup) { groupedCaseCommentsAccumulator = new StringBuilder(); @@ -725,7 +725,7 @@ private static int findBlockStatementIndex(TreePath treePath, BlockTree blockTre /** * 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 presevered where possible. Precondition: the + * 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. */ @@ -760,7 +760,7 @@ private static SuggestedFix convertToAssignmentSwitch( ImmutableList filteredStatements = filterOutRedundantBreak(caseTree); String transformedBlockSource = - transformAssignOrThrowBlock(caseTree, state, cases, caseIndex, filteredStatements); + transformAssignOrThrowBlock(caseTree, state, filteredStatements); if (firstCaseInGroup) { groupedCaseCommentsAccumulator = new StringBuilder(); @@ -870,17 +870,13 @@ private static List getStatements(CaseTree caseTree) { /** Transforms code for this case into the code under an expression switch. */ private static String transformBlock( - CaseTree caseTree, - VisitorState state, - List cases, - int caseIndex, - ImmutableList filteredStatements) { + CaseTree caseTree, VisitorState state, ImmutableList filteredStatements) { StringBuilder transformedBlockBuilder = new StringBuilder(); int codeBlockStart = extractLhsComments(caseTree, state, transformedBlockBuilder); int codeBlockEnd = filteredStatements.isEmpty() - ? getBlockEnd(state, caseTree, cases, caseIndex) + ? getBlockEnd(state, caseTree) : state.getEndPosition(Streams.findLast(filteredStatements.stream()).get()); transformedBlockBuilder.append(state.getSourceCode(), codeBlockStart, codeBlockEnd); @@ -913,19 +909,29 @@ private static int extractLhsComments( * Finds the position in source corresponding to the end of the code block of the supplied {@code * caseIndex} within all {@code cases}. */ - private static int getBlockEnd( - VisitorState state, CaseTree caseTree, List cases, int caseIndex) { + private static int getBlockEnd(VisitorState state, CaseTree caseTree) { + + List statements = caseTree.getStatements(); + if (statements == null || statements.size() != 1) { + return state.getEndPosition(caseTree); + } - if (caseIndex == cases.size() - 1) { + // Invariant: statements.size() == 1 + StatementTree onlyStatement = getOnlyElement(statements); + if (!onlyStatement.getKind().equals(BLOCK)) { return state.getEndPosition(caseTree); } - return ((JCTree) cases.get(caseIndex + 1)).getStartPosition(); + // The RHS of the case has a single enclosing block { ... } + List blockStatements = ((BlockTree) onlyStatement).getStatements(); + return blockStatements.isEmpty() + ? state.getEndPosition(caseTree) + : state.getEndPosition(blockStatements.get(blockStatements.size() - 1)); } /** * Determines whether the supplied {@code case}'s logic should be expressed on the right of the - * arrow symbol without braces, incorporating both language and readabilitiy considerations. + * arrow symbol without braces, incorporating both language and readability considerations. */ private static boolean shouldTransformCaseWithoutBraces( ImmutableList statementTrees) { @@ -995,17 +1001,13 @@ private static boolean isSwitchExhaustive( * transformed to {@code x+1;}. */ private static String transformReturnOrThrowBlock( - CaseTree caseTree, - VisitorState state, - List cases, - int caseIndex, - List statements) { + CaseTree caseTree, VisitorState state, List statements) { StringBuilder transformedBlockBuilder = new StringBuilder(); int codeBlockStart; int codeBlockEnd = statements.isEmpty() - ? getBlockEnd(state, caseTree, cases, caseIndex) + ? getBlockEnd(state, caseTree) : state.getEndPosition(Streams.findLast(statements.stream()).get()); if (statements.size() == 1 && statements.get(0).getKind().equals(RETURN)) { @@ -1028,17 +1030,13 @@ private static String transformReturnOrThrowBlock( * {@code >>=}). */ private static String transformAssignOrThrowBlock( - CaseTree caseTree, - VisitorState state, - List cases, - int caseIndex, - List statements) { + CaseTree caseTree, VisitorState state, List statements) { StringBuilder transformedBlockBuilder = new StringBuilder(); int codeBlockStart; int codeBlockEnd = statements.isEmpty() - ? getBlockEnd(state, caseTree, cases, caseIndex) + ? getBlockEnd(state, caseTree) : state.getEndPosition(Streams.findLast(statements.stream()).get()); if (!statements.isEmpty() && statements.get(0).getKind().equals(EXPRESSION_STATEMENT)) { 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 bf05679f332..d0085e2fcb0 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java @@ -1230,6 +1230,155 @@ public void switchByEnum_caseHasOnlyComments_error() { .doTest(); } + @Test + public void switchByEnum_surroundingBracesCannotRemove_error() { + // Can't remove braces around OBVERSE because break statements are not a member of + // KINDS_CONVERTIBLE_WITHOUT_BRACES + assumeTrue(RuntimeVersion.isAtLeast14()); + helper + .addSourceLines( + "Test.java", + "class Test {", + " enum Side {OBVERSE, REVERSE};", + " public Test(int foo) {", + " }", + " ", + " public void foo(Side side) { ", + " // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]", + " switch(side) {", + " case OBVERSE: {", + " // The quick brown fox, jumps over the lazy dog, etc.", + " break;", + " }", + " ", + " default: { ", + " throw new RuntimeException(\"Invalid type.\");", + " }", + " }", + " }", + "}") + .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion") + .doTest(); + + // Check correct generated code + refactoringHelper + .addInputLines( + "Test.java", + "class Test {", + " enum Side {OBVERSE, REVERSE};", + " public Test(int foo) {", + " }", + " ", + " public void foo(Side side) { ", + " switch(side) {", + " case OBVERSE: {", + " // The quick brown fox, jumps over the lazy dog, etc.", + " break;", + " }", + " ", + " default: { ", + " throw new RuntimeException(\"Invalid type.\");", + " }", + " }", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " enum Side {OBVERSE, REVERSE};", + " public Test(int foo) {", + " }", + " ", + " public void foo(Side side) { ", + " switch(side) {", + " case OBVERSE -> {", + " // The quick brown fox, jumps over the lazy dog, etc.", + " break;", + " }", + " ", + " default -> ", + " throw new RuntimeException(\"Invalid type.\");", + " ", + " }", + " }", + "}") + .setArgs( + ImmutableList.of("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")) + .doTest(); + } + + @Test + public void switchByEnum_surroundingBraces_error() { + // Test handling of cases with surrounding braces. + assumeTrue(RuntimeVersion.isAtLeast14()); + + helper + .addSourceLines( + "Test.java", + "class Test {", + " enum Side {OBVERSE, REVERSE};", + " public Test(int foo) {", + " }", + " ", + " public void foo(Side side) { ", + " // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]", + " switch(side) {", + " case OBVERSE: {", + " // The quick brown fox, jumps over the lazy dog, etc.", + " throw new RuntimeException(\"Invalid.\");", + " }", + " ", + " default: { ", + " }", + " }", + " }", + "}") + .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion") + .doTest(); + + // Check correct generated code + refactoringHelper + .addInputLines( + "Test.java", + "class Test {", + " enum Side {OBVERSE, REVERSE};", + " public Test(int foo) {", + " }", + " ", + " public void foo(Side side) { ", + " switch(side) {", + " case OBVERSE: {", + " // The quick brown fox, jumps over the lazy dog, etc.", + " throw new RuntimeException(\"Invalid.\");", + " }", + " ", + " default: { ", + " }", + " }", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " enum Side {OBVERSE, REVERSE};", + " public Test(int foo) {", + " }", + " ", + " public void foo(Side side) { ", + " switch(side) {", + " case OBVERSE -> ", + " // The quick brown fox, jumps over the lazy dog, etc.", + " throw new RuntimeException(\"Invalid.\");", + " default -> {}", + " ", + " }", + " }", + "}") + .setArgs( + ImmutableList.of("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")) + .doTest(); + } + /********************************** * * Return switch test cases