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
index 8cc1e1b84a5..94ee5913215 100644
--- 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 {
* 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.
+ *
+ *
An exception is made for {@code System.exit}, which cannot complete normally in practice.
+ */
+ public static boolean canCompleteNormally(
+ StatementTree statement, ImmutableMap patches) {
+ return new CanCompleteNormallyVisitor(patches).scan(statement);
}
/**
@@ -100,6 +114,13 @@ private static class CanCompleteNormallyVisitor extends SimpleTreeVisitor continues = new HashSet<>();
+ /** Trees that are patched to have a specific completion result. */
+ private final ImmutableMap patches;
+
+ public CanCompleteNormallyVisitor(ImmutableMap patches) {
+ this.patches = patches;
+ }
+
boolean scan(List trees) {
boolean completes = true;
for (StatementTree tree : trees) {
@@ -112,6 +133,9 @@ boolean scan(List 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
index 306f29d65b0..fc6ba2215e8 100644
--- 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 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 followingStatementsInBlock(
- SwitchTree switchTree, VisitorState state) {
- List followingStatements = new ArrayList<>();
-
- // NOMUTANTS--for performance/early return only; correctness unchanged
- if (!Matchers.nextStatement(Matchers.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
index cf6a9ecb951..2fb5ca4658d 100644
--- 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