[StatementSwitchToExpressionSwitch] for the return switch pattern, fix a bug where the auto-fix can contain dead code, which will lead to a compile-time error if adopted.

markhbrady · Error Prone Team · commit c139e7f6bcd6 · 2025-03-17T09:50:43.000-07:00
PiperOrigin-RevId: 737649634
diff --git a/check_api/src/main/java/com/google/errorprone/util/Reachability.java b/check_api/src/main/java/com/google/errorprone/util/Reachability.java
@@ -22,6 +22,7 @@
 import static com.google.errorprone.util.ASTHelpers.isSwitchDefault;
 import static java.util.Objects.requireNonNull;
 
+import com.google.common.collect.ImmutableMap;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import com.sun.source.tree.AssertTree;
 import com.sun.source.tree.BlockTree;
@@ -66,7 +67,20 @@ public class Reachability {
    * <p>An exception is made for {@code System.exit}, which cannot complete normally in practice.
    */
   public static boolean canCompleteNormally(StatementTree statement) {
-    return statement.accept(new CanCompleteNormallyVisitor(), null);
+    return new CanCompleteNormallyVisitor(/* patches= */ ImmutableMap.of()).scan(statement);
+  }
+
+  /**
+   * Returns whether the given statement can complete normally, as defined by JLS 14.21, when taking
+   * into account the given {@code patches}. The patches are a (possibly empty) map from {@code
+   * Tree} to a boolean indicating whether that specific {@code Tree} can complete normally. All
+   * relevant tree(s) not present in the patches will be analyzed as per the JLS.
+   *
+   * <p>An exception is made for {@code System.exit}, which cannot complete normally in practice.
+   */
+  public static boolean canCompleteNormally(
+      StatementTree statement, ImmutableMap<Tree, Boolean> patches) {
+    return new CanCompleteNormallyVisitor(patches).scan(statement);
   }
 
   /**
@@ -100,6 +114,13 @@ private static class CanCompleteNormallyVisitor extends SimpleTreeVisitor<Boolea
     /** Trees that are the target of a reachable continue statement. */
     private final Set<Tree> continues = new HashSet<>();
 
+    /** Trees that are patched to have a specific completion result. */
+    private final ImmutableMap<Tree, Boolean> patches;
+
+    public CanCompleteNormallyVisitor(ImmutableMap<Tree, Boolean> patches) {
+      this.patches = patches;
+    }
+
     boolean scan(List<? extends StatementTree> trees) {
       boolean completes = true;
       for (StatementTree tree : trees) {
@@ -112,6 +133,9 @@ boolean scan(List<? extends StatementTree> trees) {
     // don't otherwise affect the result of the reachability analysis.
     @CanIgnoreReturnValue
     private boolean scan(Tree tree) {
+      if (patches.containsKey(tree)) {
+        return patches.get(tree);
+      }
       return tree.accept(this, null);
     }
 
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java b/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java
@@ -20,6 +20,7 @@
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.collect.Iterables.getLast;
 import static com.google.common.collect.Iterables.getOnlyElement;
+import static com.google.common.collect.Streams.stream;
 import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
 import static com.google.errorprone.matchers.Description.NO_MATCH;
 import static com.google.errorprone.util.ASTHelpers.getStartPosition;
@@ -39,6 +40,7 @@
 import com.google.common.collect.HashBiMap;
 import com.google.common.collect.ImmutableBiMap;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Streams;
@@ -1082,17 +1084,43 @@ private static SuggestedFix convertToReturnSwitch(
     // Close the switch statement
     replacementCodeBuilder.append("\n};");
 
-    // Statements in the same block following the switch are currently reachable but will become
-    // unreachable, which would lead to a compile-time error. Therefore, suggest that they be
-    // removed.
-    statementsToDelete.addAll(followingStatementsInBlock(switchTree, state));
+    // The transformed code can cause other existing code to become dead code.  So, we must analyze
+    // and delete such dead code, otherwise the suggested autofix could fail to compile.
+
+    // The `return switch ...` will always return or throw
+    Tree cannotCompleteNormallyTree = switchTree;
+    // Search up the AST for enclosing statement blocks, marking any newly-dead code for deletion
+    // along the way
+    for (Tree tree : state.getPath()) {
+      if (tree instanceof BlockTree blockTree) {
+        TreePath rootToCurrentPath = TreePath.getPath(state.getPath(), switchTree);
+        int indexInBlock = findBlockStatementIndex(rootToCurrentPath, blockTree);
+        // A single mock of the immediate child statement block (or switch) is sufficient to
+        // analyze reachability here; deeper-nested statements are not relevant.
+        boolean nextStatementReachable =
+            Reachability.canCompleteNormally(
+                blockTree.getStatements().get(indexInBlock),
+                ImmutableMap.of(cannotCompleteNormallyTree, false));
+        // If we continue to the ancestor statement block, it will be because the end of this
+        // statement block is not reachable
+        cannotCompleteNormallyTree = blockTree;
+        if (nextStatementReachable) {
+          break;
+        }
+
+        // If the next statement is not reachable, then none of the following statements in this
+        // block are either.  So, we need to delete them all.
+        statementsToDelete.addAll(
+            blockTree.getStatements().subList(indexInBlock + 1, blockTree.getStatements().size()));
+      }
+    }
 
     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
+    // Delete dead code, leaving comments where feasible
     statementsToDelete.forEach(deleteMe -> suggestedFixBuilder.replace(deleteMe, ""));
     return suggestedFixBuilder.build();
   }
@@ -1131,54 +1159,14 @@ private static List<StatementTree> getPrecedingStatementsInBlock(
     return precedingStatements;
   }
 
-  /**
-   * Retrieves a list of all statements (if any) following the supplied {@code SwitchTree} in its
-   * lowest-ancestor statement block (if any).
-   */
-  private static List<StatementTree> followingStatementsInBlock(
-      SwitchTree switchTree, VisitorState state) {
-    List<StatementTree> followingStatements = new ArrayList<>();
-
-    // NOMUTANTS--for performance/early return only; correctness unchanged
-    if (!Matchers.nextStatement(Matchers.<StatementTree>anything()).matches(switchTree, state)) {
-      // No lowest-ancestor block or no following statements
-      return followingStatements;
-    }
-
-    // Fetch the lowest ancestor statement block
-    TreePath pathToEnclosing = state.findPathToEnclosing(BlockTree.class);
-    // NOMUTANTS--should early return above
-    if (pathToEnclosing != null) {
-      Tree enclosing = pathToEnclosing.getLeaf();
-      if (enclosing instanceof BlockTree blockTree) {
-        // Path from root -> switchTree
-        TreePath rootToSwitchPath = TreePath.getPath(pathToEnclosing, switchTree);
-
-        for (int i = findBlockStatementIndex(rootToSwitchPath, blockTree) + 1;
-            (i >= 0) && (i < blockTree.getStatements().size());
-            i++) {
-          followingStatements.add(blockTree.getStatements().get(i));
-        }
-      }
-    }
-    return followingStatements;
-  }
-
   /**
    * Search through the provided {@code BlockTree} to find which statement in that block tree lies
    * along the supplied {@code TreePath}. Returns the index (zero-based) of the matching statement
    * in the block tree, or -1 if not found.
    */
   private static int findBlockStatementIndex(TreePath treePath, BlockTree blockTree) {
-    for (int i = 0; i < blockTree.getStatements().size(); i++) {
-      StatementTree thisStatement = blockTree.getStatements().get(i);
-      // Look for thisStatement along the path from the root to the switch tree
-      TreePath pathFromRootToThisStatement = TreePath.getPath(treePath, thisStatement);
-      if (pathFromRootToThisStatement != null) {
-        return i;
-      }
-    }
-    return -1;
+    return Iterables.indexOf(
+        blockTree.getStatements(), stmt -> stream(treePath).anyMatch(t -> t == stmt));
   }
 
   /**
diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java
@@ -2675,7 +2675,8 @@ public int foo(Side side) {
 
   @Test
   public void switchByEnum_returnSwitchWithShouldNeverHappen_errorAndRemoveShouldNeverHappen() {
-    // The switch has a case for each enum and "should never happen" error handling
+    // The switch has a case for each enum and "should never happen" error handling for code after
+    // the switch in the same block. The "should never happen" code should be removed.
     helper
         .addSourceLines(
             "Test.java",
@@ -2817,6 +2818,193 @@ public int foo(Side side) {
         .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
   }
 
+  @Test
+  public void switchByEnum_deadCodeAnalysis_error() {
+    // Code after the return switch in the same block should be removed, as well as certain code in
+    // the parent block(s)
+    refactoringHelper
+        .addInputLines(
+            "Test.java",
+            """
+            class Test {
+              enum Side {
+                HEART,
+                SPADE,
+                DIAMOND,
+                CLUB
+              };
+
+              public Test(int foo) {}
+
+              public int foo(Side side) {
+                System.out.println("don't delete 0");
+                try {
+                  System.out.println("don't delete 1");
+                  System.out.println("don't delete 11");
+                  switch (side) {
+                    case HEART:
+                    case DIAMOND:
+                    case SPADE:
+                      return 1;
+                    case CLUB:
+                      throw new NullPointerException();
+                  }
+                  System.out.println("delete me");
+                } catch (Throwable e) {
+                  throw new RuntimeException("rethrew");
+                }
+
+                // Becomes unreachable
+                System.out.println("uh-oh");
+                return -1;
+              }
+            }
+            """)
+        .addOutputLines(
+            "Test.java",
+            """
+            class Test {
+              enum Side {
+                HEART,
+                SPADE,
+                DIAMOND,
+                CLUB
+              };
+
+              public Test(int foo) {}
+
+              public int foo(Side side) {
+                System.out.println("don't delete 0");
+                try {
+                  System.out.println("don't delete 1");
+                  System.out.println("don't delete 11");
+                  return switch (side) {
+                    case HEART, DIAMOND, SPADE -> 1;
+                    case CLUB -> throw new NullPointerException();
+                  };
+
+                } catch (Throwable e) {
+                  throw new RuntimeException("rethrew");
+                }
+
+                // Becomes unreachable
+
+              }
+            }
+            """)
+        .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")
+        .setFixChooser(StatementSwitchToExpressionSwitchTest::assertOneFixAndChoose)
+        .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
+  }
+
+  @Test
+  public void switchByEnum_deadCodeAnalysis2_error() {
+    // This test case is similar to switchByEnum_deadCodeAnalysis_error, but with additional
+    // complexity and language elements such as synchronized(), if(), ...
+    refactoringHelper
+        .addInputLines(
+            "Test.java",
+            """
+            class Test {
+              enum Side {
+                HEART,
+                SPADE,
+                DIAMOND,
+                CLUB
+              };
+
+              public Test(int foo) {}
+
+              int x = 3;
+
+              public int foo(Side side) {
+                if (x < 4) {
+                  try {
+                    System.out.println("don't delete 0");
+                    synchronized (this) {
+                      try {
+                        System.out.println("don't delete 1");
+                        System.out.println("don't delete 11");
+                        switch (side) {
+                          case HEART:
+                          case DIAMOND:
+                          case SPADE:
+                            return 1;
+                          case CLUB:
+                            throw new NullPointerException();
+                        }
+                        System.out.println("delete me");
+                      } catch (Throwable e) {
+                        throw new RuntimeException("rethrew");
+                      }
+                    }
+
+                    // Becomes unreachable
+                    System.out.println("uh-oh");
+                  } catch (Throwable e) {
+                    throw new RuntimeException("rethrew");
+                  }
+                  // Also becomes unreachable
+                  System.out.println("delete me too");
+                  System.out.println("delete me three");
+                  return 3;
+                }
+                System.out.println("I'm always reachable");
+                return 4;
+              }
+            }
+            """)
+        .addOutputLines(
+            "Test.java",
+            """
+            class Test {
+              enum Side {
+                HEART,
+                SPADE,
+                DIAMOND,
+                CLUB
+              };
+
+              public Test(int foo) {}
+
+              int x = 3;
+
+              public int foo(Side side) {
+                if (x < 4) {
+                  try {
+                    System.out.println("don't delete 0");
+                    synchronized (this) {
+                      try {
+                        System.out.println("don't delete 1");
+                        System.out.println("don't delete 11");
+                        return switch (side) {
+                          case HEART, DIAMOND, SPADE -> 1;
+                          case CLUB -> throw new NullPointerException();
+                        };
+
+                      } catch (Throwable e) {
+                        throw new RuntimeException("rethrew");
+                      }
+                    }
+
+                    // Becomes unreachable
+
+                  } catch (Throwable e) {
+                    throw new RuntimeException("rethrew");
+                  }
+                  // Also becomes unreachable
+
+                }
+                System.out.println("I'm always reachable");
+                return 4;
+              }
+            }
+            """)
+        .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")
+        .setFixChooser(StatementSwitchToExpressionSwitchTest::assertOneFixAndChoose)
+        .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
