From 8c1a828a1b1deb6ed1dfca04032fb732f2563f0a Mon Sep 17 00:00:00 2001 From: markbrady Date: Thu, 11 Jul 2024 16:22:20 -0700 Subject: [PATCH] StatementSwitchToExpressionSwitch: in certain situations, the generated autofix code for direct conversions may not compile. Add unit tests to guard against regression. PiperOrigin-RevId: 651569415 --- .../StatementSwitchToExpressionSwitch.java | 54 +++---- ...StatementSwitchToExpressionSwitchTest.java | 150 ++++++++++++++++++ 2 files changed, 175 insertions(+), 29 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 c1b74e1024d..82011eb8dc7 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java @@ -63,7 +63,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; @@ -529,8 +528,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 +621,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 +723,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 +758,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 +868,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 +907,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) { - if (caseIndex == cases.size() - 1) { + List statements = caseTree.getStatements(); + if (statements == null || statements.size() != 1) { + return state.getEndPosition(caseTree); + } + + // 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 +999,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 +1028,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..d1e2d6c144e 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,156 @@ 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_surroundingBracesEmpty_error() { + // Test handling of cases with surrounding braces that are empty. The braces around OBVERSE + // can be removed because throw is 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.", + " 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