[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.

PiperOrigin-RevId: 735958054

Author: markhbrady

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 statement.accept(new CanCompleteNormallyVisitor(/* patches= */ ImmutableMap.of()), null);
+  }
+
+  /**
+   * Returns {@code true} if 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 statement.accept(new CanCompleteNormallyVisitor(patches), null);
   }
 
   /**
@@ -100,10 +114,21 @@ 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) {
-        completes = scan(tree);
+        if (patches.containsKey(tree)) {
+          completes = patches.get(tree);
+        } else {
+          completes = scan(tree);
+        }
       }
       return completes;
     }
@@ -112,12 +137,18 @@ 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);
     }
 
     /* A break statement cannot complete normally. */
     @Override
     public Boolean visitBreak(BreakTree tree, Void unused) {
+      if (patches.containsKey(tree)) {
+        return patches.get(tree);
+      }
       breaks.add(skipLabel(requireNonNull(((JCTree.JCBreak) tree).target)));
       return false;
     }
@@ -126,6 +157,9 @@ public Boolean visitBreak(BreakTree tree, Void unused) {
     @Override
     public Boolean visitContinue(ContinueTree tree, Void unused) {
       continues.add(skipLabel(requireNonNull(((JCTree.JCContinue) tree).target)));
+      if (patches.containsKey(tree)) {
+        return patches.get(tree);
+      }
       return false;
     }
 
@@ -135,29 +169,44 @@ private static Tree skipLabel(JCTree tree) {
 
     @Override
     public Boolean visitBlock(BlockTree tree, Void unused) {
+      if (patches.containsKey(tree)) {
+        return patches.get(tree);
+      }
       return scan(tree.getStatements());
     }
 
     /* A local class declaration statement can complete normally iff it is reachable. */
     @Override
     public Boolean visitClass(ClassTree tree, Void unused) {
+      if (patches.containsKey(tree)) {
+        return patches.get(tree);
+      }
       return true;
     }
 
     /* A local variable declaration statement can complete normally iff it is reachable. */
     @Override
     public Boolean visitVariable(VariableTree tree, Void unused) {
+      if (patches.containsKey(tree)) {
+        return patches.get(tree);
+      }
       return true;
     }
 
     /* An empty statement can complete normally iff it is reachable. */
     @Override
     public Boolean visitEmptyStatement(EmptyStatementTree tree, Void unused) {
+      if (patches.containsKey(tree)) {
+        return patches.get(tree);
+      }
       return true;
     }
 
     @Override
     public Boolean visitLabeledStatement(LabeledStatementTree tree, Void unused) {
+      if (patches.containsKey(tree)) {
+        return patches.get(tree);
+      }
       // break/continue targets have already been resolved by javac, so
       // there's nothing to do here
       return scan(tree.getStatement());
@@ -166,6 +215,9 @@ public Boolean visitLabeledStatement(LabeledStatementTree tree, Void unused) {
     /* An expression statement can complete normally iff it is reachable. */
     @Override
     public Boolean visitExpressionStatement(ExpressionStatementTree tree, Void unused) {
+      if (patches.containsKey(tree)) {
+        return patches.get(tree);
+      }
       if (isSystemExit(tree.getExpression())) {
         // The spec doesn't have any special handling for {@code System.exit}, but in practice it
         // cannot complete normally
@@ -198,6 +250,9 @@ private static boolean isSystemExit(ExpressionTree expression) {
      */
     @Override
     public Boolean visitIf(IfTree tree, Void unused) {
+      if (patches.containsKey(tree)) {
+        return patches.get(tree);
+      }
       boolean thenCompletes = scan(tree.getThenStatement());
       boolean elseCompletes = tree.getElseStatement() == null || scan(tree.getElseStatement());
       return thenCompletes || elseCompletes;
@@ -206,6 +261,9 @@ public Boolean visitIf(IfTree tree, Void unused) {
     /* An assert statement can complete normally iff it is reachable. */
     @Override
     public Boolean visitAssert(AssertTree tree, Void unused) {
+      if (patches.containsKey(tree)) {
+        return patches.get(tree);
+      }
       return true;
     }
 
@@ -232,6 +290,10 @@ public Boolean visitAssert(AssertTree tree, Void unused) {
      */
     @Override
     public Boolean visitSwitch(SwitchTree tree, Void unused) {
+      if (patches.containsKey(tree)) {
+        return patches.get(tree);
+      }
+
       // (1)
       if (tree.getCases().stream().noneMatch(c -> isSwitchDefault(c))) {
         return true;
@@ -294,6 +356,9 @@ public Boolean visitSwitch(SwitchTree tree, Void unused) {
      */
     @Override
     public Boolean visitWhileLoop(WhileLoopTree tree, Void unused) {
+      if (patches.containsKey(tree)) {
+        return patches.get(tree);
+      }
       Boolean condValue = ASTHelpers.constValue(tree.getCondition(), Boolean.class);
       if (!Objects.equals(condValue, false)) {
         scan(tree.getStatement());
@@ -333,6 +398,9 @@ public Boolean visitWhileLoop(WhileLoopTree tree, Void unused) {
      */
     @Override
     public Boolean visitDoWhileLoop(DoWhileLoopTree that, Void unused) {
+      if (patches.containsKey(that)) {
+        return patches.get(that);
+      }
       boolean completes = scan(that.getStatement());
       boolean conditionIsAlwaysTrue =
           firstNonNull(ASTHelpers.constValue(that.getCondition(), Boolean.class), false);
@@ -367,6 +435,9 @@ public Boolean visitDoWhileLoop(DoWhileLoopTree that, Void unused) {
      */
     @Override
     public Boolean visitForLoop(ForLoopTree that, Void unused) {
+      if (patches.containsKey(that)) {
+        return patches.get(that);
+      }
       Boolean condValue = ASTHelpers.constValue(that.getCondition(), Boolean.class);
       if (!Objects.equals(condValue, false)) {
         scan(that.getStatement());
@@ -385,19 +456,28 @@ public Boolean visitForLoop(ForLoopTree that, Void unused) {
     /* An enhanced for statement can complete normally iff it is reachable. */
     @Override
     public Boolean visitEnhancedForLoop(EnhancedForLoopTree that, Void unused) {
+      if (patches.containsKey(that)) {
+        return patches.get(that);
+      }
       scan(that.getStatement());
       return true;
     }
 
     /* A return statement cannot complete normally. */
     @Override
     public Boolean visitReturn(ReturnTree tree, Void unused) {
+      if (patches.containsKey(tree)) {
+        return patches.get(tree);
+      }
       return false;
     }
 
     /* A throw statement cannot complete normally. */
     @Override
     public Boolean visitThrow(ThrowTree tree, Void unused) {
+      if (patches.containsKey(tree)) {
+        return patches.get(tree);
+      }
       return false;
     }
 
@@ -410,6 +490,9 @@ public Boolean visitThrow(ThrowTree tree, Void unused) {
      */
     @Override
     public Boolean visitSynchronized(SynchronizedTree tree, Void unused) {
+      if (patches.containsKey(tree)) {
+        return patches.get(tree);
+      }
       return scan(tree.getBlock());
     }
 
@@ -424,6 +507,9 @@ public Boolean visitSynchronized(SynchronizedTree tree, Void unused) {
      */
     @Override
     public Boolean visitTry(TryTree that, Void unused) {
+      if (patches.containsKey(that)) {
+        return patches.get(that);
+      }
       boolean completes = scan(that.getBlock());
       // assume all catch blocks are reachable; javac has already rejected unreachable
       // checked exception handlers

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));
   }
 
   /**