Skip to content

[RefactorSwitch] align dead code deletion after a return switch with javac's dead code semantics #6220

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
copybara-service merged 1 commit into from
Oct 26, 2026
+624 −143
Merged

[RefactorSwitch] align dead code deletion after a return switch with javac's dead code semantics #6220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 94 additions & 31 deletions core/src/main/java/com/google/errorprone/bugpatterns/RefactorSwitch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import static com.google.errorprone.bugpatterns.SwitchUtils.getHandledEnumValues;
import static com.google.errorprone.bugpatterns.SwitchUtils.getPrecedingStatementsInBlock;
import static com.google.errorprone.bugpatterns.SwitchUtils.getStatements;
import static com.google.errorprone.bugpatterns.SwitchUtils.hasBreakOutOfTree;
import static com.google.errorprone.bugpatterns.SwitchUtils.hasContinueOutOfTree;
import static com.google.errorprone.bugpatterns.SwitchUtils.isCompatibleWithFirstAssignment;
import static com.google.errorprone.bugpatterns.SwitchUtils.isSwitchCoveringAllEnumValues;
Expand All @@ -38,6 +39,7 @@
import static com.google.errorprone.matchers.Description.NO_MATCH;
import static com.google.errorprone.util.ASTHelpers.getStartPosition;
import static com.google.errorprone.util.ASTHelpers.hasImplicitType;
import static com.google.errorprone.util.Reachability.CanCompleteNormallyPatch.CANNOT_COMPLETE_NORMALLY_ANALYZE_INSIDE;
import static com.google.errorprone.util.Reachability.CanCompleteNormallyPatch.CANNOT_COMPLETE_NORMALLY_SKIP_INSIDE;
import static java.util.stream.Collectors.joining;

Expand All @@ -48,6 +50,7 @@
import com.google.errorprone.BugPattern;
import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.errorprone.bugpatterns.BugChecker.SwitchExpressionTreeMatcher;
import com.google.errorprone.bugpatterns.BugChecker.SwitchTreeMatcher;
import com.google.errorprone.bugpatterns.SwitchUtils.CaseQualifications;
Expand All @@ -57,6 +60,7 @@
import com.google.errorprone.util.ASTHelpers;
import com.google.errorprone.util.ErrorProneComment;
import com.google.errorprone.util.Reachability;
import com.google.errorprone.util.Reachability.CanCompleteNormallyPatch;
import com.google.errorprone.util.SourceVersion;
import com.sun.source.tree.AssignmentTree;
import com.sun.source.tree.BlockTree;
Expand All @@ -78,17 +82,20 @@
import com.sun.source.tree.YieldTree;
import com.sun.source.util.TreePath;
import com.sun.source.util.TreePathScanner;
import com.sun.source.util.TreeScanner;
import com.sun.tools.javac.code.Type;
import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Stream;
import javax.inject.Inject;
import org.jspecify.annotations.Nullable;

/** Checks for refactorings that can be applied to simplify arrow switches. */
@BugPattern(severity = WARNING, summary = "This switch can be refactored to be more readable")
Expand Down Expand Up @@ -235,6 +242,9 @@ private AnalysisResult analyzeSwitchTree(SwitchTree switchTree, VisitorState sta
// Continuing out of a switch statement is possible, but continuing out of a switch expression
// is not
boolean hasContinueOut = hasContinueOutOfTree(switchTree, state);
// Likewise for breaking out of the switch (or out of an enclosing statement): a `break` cannot
// exit a switch expression
boolean hasBreakOut = hasBreakOutOfTree(switchTree, state);

boolean canConvertToAssignmentSwitch =
// The switch must be known to be exhaustive at compile time
Expand All @@ -252,7 +262,12 @@ private AnalysisResult analyzeSwitchTree(SwitchTree switchTree, VisitorState sta
(!Reachability.canCompleteNormally(switchTree)
|| (!anyCaseCanCompleteNormally && coversAllEnumValues))
&& returnSwitchCaseQualifications.equals(CaseQualifications.ALL_CASES_QUALIFY)
&& !hasContinueOut;
&& !hasContinueOut
&& !hasBreakOut
// The conversion makes the code after the switch dead, so it must be deletable. Decide
// that now, by running the deletion on a fix that is then discarded, so that a
// conversion that would have to be abandoned is never proposed.
&& deleteDeadCodeInAncestors(switchTree, state, SuggestedFix.builder());

ImmutableList<StatementTree> precedingStatements = getPrecedingStatementsInBlock(state);
Optional<ExpressionTree> assignmentTarget =
Expand Down Expand Up @@ -625,61 +640,109 @@ private static SuggestedFix convertToReturnSwitch(

// 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.
// (`analyzeSwitchTree` only proposes this conversion if that deletion succeeds.)
deleteDeadCodeInAncestors(switchTree, state, suggestedFixBuilder);

// The `return switch ...` will always return or throw
Tree cannotCompleteNormallyTree = switchTree;
if (removeDefault) {
suggestedFixBuilder.setShortDescription(REMOVE_DEFAULT_CASE_SHORT_DESCRIPTION);
}
return suggestedFixBuilder.build();
}

/**
* Deletes the code that becomes dead once {@code switchTree} is converted to a {@code return
* switch}, which cannot complete normally: the statements after the switch in its block, and in
* turn those after each enclosing statement that can then no longer complete normally either.
* Returns true if successful, or false if the conversion should be abandoned (in which case
* {@code suggestedFixBuilder} may hold some of the deletions and should be discarded).
*/
@CanIgnoreReturnValue
private static boolean deleteDeadCodeInAncestors(
SwitchTree switchTree, VisitorState state, SuggestedFix.Builder suggestedFixBuilder) {
// `Reachability` sees only the original code, so the `return switch` is represented by the
// switch it will be built from. That switch is given to `Reachability` as a patch that cannot
// complete normally, because the `return switch` cannot, but that is still analyzed inside, so
// that the jumps it contains are recorded. Statements that are to be deleted are not analyzed
// inside, so that any jumps they contain are not recorded. Every other statement is analyzed
// as written, so that a jump that survives the fix (such as a `break` before the switch, which
// still exits an enclosing loop) is recorded.
Set<Tree> deletedTrees = new HashSet<>();
// Search up the AST for enclosing statement blocks, marking any newly-dead code for deletion
// along the way
Tree prev = state.getPath().getLeaf();
// along the way. (A colon-style `case` also holds statements directly, but no switch encloses
// this one: `analyzeSwitchTree` rejects a switch inside another switch.)
Tree childTree = state.getPath().getLeaf();
for (Tree tree : state.getPath().getParentPath()) {
if (tree instanceof BlockTree blockTree) {
var statements = blockTree.getStatements();
int indexInBlock = statements.indexOf(prev);
// A single mock of the immediate child statement block (or switch) is sufficient to
// analyze reachability here; deeper-nested statements are not relevant.
List<? extends StatementTree> statements = blockTree.getStatements();
int indexInBlock = statements.indexOf(childTree);
ImmutableMap.Builder<Tree, CanCompleteNormallyPatch> patches = ImmutableMap.builder();
patches.put(switchTree, CANNOT_COMPLETE_NORMALLY_ANALYZE_INSIDE);
deletedTrees.forEach(
deletedTree -> patches.put(deletedTree, CANNOT_COMPLETE_NORMALLY_SKIP_INSIDE));
boolean nextStatementReachable =
Reachability.canCompleteNormally(
statements.get(indexInBlock),
ImmutableMap.of(cannotCompleteNormallyTree, CANNOT_COMPLETE_NORMALLY_SKIP_INSIDE));
// If we continue to the ancestor statement block, it will be because the end of this
// statement block is not reachable
cannotCompleteNormallyTree = blockTree;
Reachability.canCompleteNormally(statements.get(indexInBlock), patches.buildOrThrow());
// Everything after this statement in this block is about to be deleted, so accumulate it
// for the ancestors' analyses. Only deleted statements are patched this way: a statement
// that survives must still be analyzed inside.
List<? extends StatementTree> deletedStatements =
statements.subList(indexInBlock + 1, statements.size());
deletedTrees.addAll(deletedStatements);
if (nextStatementReachable) {
break;
}

// If a next statement in this block exists, then it is not reachable.
if (indexInBlock < statements.size() - 1) {
if (!deletedStatements.isEmpty()) {
if (deletedStatements.stream().anyMatch(RefactorSwitch::hasSwitchInTree)) {
// This code is now unreachable, so leaving it in place would not compile, but it
// contains a switch that this check may rewrite under a separate finding whose fix
// would overlap this deletion. Decline to convert instead.
return false;
}
int deletionStartPosition = state.getEndPosition(statements.get(indexInBlock));
int deletionEndPosition = state.getEndPosition(blockTree);
String deletedRegion =
state
.getSourceCode()
.subSequence(
state.getEndPosition(statements.get(indexInBlock)),
state.getEndPosition(blockTree))
.subSequence(deletionStartPosition, deletionEndPosition)
.toString();
// If the region we would delete looks interesting, bail out and just delete the orphaned
// statements.
if (deletedRegion.contains("LINT.")) {
statements
.subList(indexInBlock + 1, statements.size())
.forEach(suggestedFixBuilder::delete);
deletedStatements.forEach(suggestedFixBuilder::delete);
} else {
// If the region doesn't seem to contain interesting comments, delete it along with
// comments: those comments are often just of the form "Unreachable code".
suggestedFixBuilder.replace(
state.getEndPosition(statements.get(indexInBlock)),
state.getEndPosition(blockTree),
"}");
suggestedFixBuilder.replace(deletionStartPosition, deletionEndPosition, "}");
}
}
}
prev = tree;
childTree = tree;
}
return true;
}

if (removeDefault) {
suggestedFixBuilder.setShortDescription(REMOVE_DEFAULT_CASE_SHORT_DESCRIPTION);
}
return suggestedFixBuilder.build();
/** Returns whether {@code tree} contains a switch statement or switch expression. */
private static boolean hasSwitchInTree(Tree tree) {
return Objects.equals(
new TreeScanner<Boolean, Void>() {
@Override
public Boolean visitSwitch(SwitchTree switchTree, Void unused) {
return true;
}

@Override
public Boolean visitSwitchExpression(
SwitchExpressionTree switchExpressionTree, Void unused) {
return true;
}

@Override
public Boolean reduce(@Nullable Boolean left, @Nullable Boolean right) {
return Objects.equals(left, true) || Objects.equals(right, true);
}
}.scan(tree, null),
true);
}

/**
Expand Down
Loading
Loading