StatementSwitchToExpressionSwitch: in certain situations, the generated autofix code for direct conversions may not compile. Add unit tests to guard against regression.

markhbrady · Error Prone Team · commit 8c1a828a1b1d · 2024-07-11T16:24:11.000-07:00
PiperOrigin-RevId: 651569415
diff --git 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<StatementTree> 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<StatementTree> 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<? extends StatementTree> getStatements(CaseTree caseTree) {
 
   /** Transforms code for this case into the code under an expression switch. */
   private static String transformBlock(
-      CaseTree caseTree,
-      VisitorState state,
-      List<? extends CaseTree> cases,
-      int caseIndex,
-      ImmutableList<StatementTree> filteredStatements) {
+      CaseTree caseTree, VisitorState state, ImmutableList<StatementTree> 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<? extends CaseTree> cases, int caseIndex) {
+  private static int getBlockEnd(VisitorState state, CaseTree caseTree) {
 
-    if (caseIndex == cases.size() - 1) {
+    List<? extends StatementTree> 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<? extends StatementTree> 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<StatementTree> statementTrees) {
@@ -995,17 +999,13 @@ private static boolean isSwitchExhaustive(
    * transformed to {@code x+1;}.
    */
   private static String transformReturnOrThrowBlock(
-      CaseTree caseTree,
-      VisitorState state,
-      List<? extends CaseTree> cases,
-      int caseIndex,
-      List<? extends StatementTree> statements) {
+      CaseTree caseTree, VisitorState state, List<? extends StatementTree> 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<? extends CaseTree> cases,
-      int caseIndex,
-      List<? extends StatementTree> statements) {
+      CaseTree caseTree, VisitorState state, List<? extends StatementTree> 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
@@ -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
