Skip to content

[StatementSwitchToExpressionSwitch] align dead-code deletion after a return switch with javac's reachability #6228

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

Open
copybara-service wants to merge 1 commit into
base: master
Choose a base branch
from
+1,016 −384
Open

[StatementSwitchToExpressionSwitch] align dead-code deletion after a return switch with javac's reachability #6228

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
173 changes: 12 additions & 161 deletions core/src/main/java/com/google/errorprone/bugpatterns/IfChainToSwitch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
import static com.google.errorprone.bugpatterns.SwitchUtils.KINDS_CONVERTIBLE_WITHOUT_BRACES;
import static com.google.errorprone.bugpatterns.SwitchUtils.deleteDeadCodeInAncestors;
import static com.google.errorprone.bugpatterns.SwitchUtils.getReferencedLocalVariablesInTree;
import static com.google.errorprone.bugpatterns.SwitchUtils.hasBreakOutOfTree;
import static com.google.errorprone.bugpatterns.SwitchUtils.isEnumValue;
Expand All @@ -35,15 +36,11 @@
import static com.google.errorprone.util.ASTHelpers.sameVariable;
import static com.google.errorprone.util.ASTHelpers.stripParentheses;
import static com.google.errorprone.util.ASTHelpers.unboxedType;
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.lang.Math.max;
import static java.lang.Math.min;
import static java.util.Collections.disjoint;
import static java.util.stream.Collectors.joining;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Range;
import com.google.common.collect.Streams;
Expand All @@ -59,20 +56,17 @@
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.BinaryTree;
import com.sun.source.tree.BindingPatternTree;
import com.sun.source.tree.BlockTree;
import com.sun.source.tree.BreakTree;
import com.sun.source.tree.CaseTree;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.IfTree;
import com.sun.source.tree.InstanceOfTree;
import com.sun.source.tree.LiteralTree;
import com.sun.source.tree.StatementTree;
import com.sun.source.tree.SwitchExpressionTree;
import com.sun.source.tree.SwitchTree;
import com.sun.source.tree.Tree;
import com.sun.source.tree.Tree.Kind;
import com.sun.source.tree.VariableTree;
Expand Down Expand Up @@ -671,7 +665,17 @@ private Optional<List<CaseIr>> maybeFixDefaultNullAndUnconditional(
// If javac believes the switch can complete normally, then it believes the statements that
// follow it are still reachable, so they are left in place.
if (emptyRhsBlockCount + canCompleteNormallyBlockCount == 0 && javacSeesSwitchAsExhaustive) {
if (!deleteDeadCodeInAncestors(ifTree, state, suggestedFixBuilder.get(), numberPulledUp)) {
// The switch is built from the if-chain (whose branches become its cases) and any
// pulled-up statements that follow it (which become its `default` case and so are not
// deleted). Dead code containing an `if` is not deleted either: this check may rewrite
// that `if` under a separate finding whose fix would overlap the deletion, so decline to
// convert instead.
if (!deleteDeadCodeInAncestors(
ifTree,
state,
suggestedFixBuilder.get(),
numberPulledUp,
IfChainToSwitch::hasIfInTree)) {
return Optional.empty();
}
}
Expand All @@ -684,159 +688,6 @@ private Optional<List<CaseIr>> maybeFixDefaultNullAndUnconditional(
: Optional.of(cases);
}

/**
* Deletes dead code in ancestors after a switch that cannot complete normally. Returns true if
* successful, false if the refactoring should be aborted.
*/
private static boolean deleteDeadCodeInAncestors(
StatementTree ifTree,
VisitorState state,
SuggestedFix.Builder suggestedFixBuilder,
int numberPulledUp) {
// No case can complete normally and javac sees the switch as exhaustive, so the switch
// cannot complete normally, and we need to do reachability analysis. `Reachability` sees
// only the original code, so the switch is represented by the trees it will be built from:
// the if-chain (whose branches become its cases) and any pulled-up statements that follow
// it (which become its `default` case).
//
// The if-chain is given to `Reachability` as a patch that cannot complete normally, because
// the 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.
Set<Tree> deletedTrees = new HashSet<>();
// Search up the AST for enclosing statement lists, marking any newly-dead code for
// deletion along the way
Tree childTree = state.getPath().getLeaf();
for (TreePath path = state.getPath().getParentPath();
path != null;
path = path.getParentPath()) {
Tree tree = path.getLeaf();
List<? extends StatementTree> statements =
getEnclosedStatements(tree).orElse(ImmutableList.of());
int indexInStatements = statements.indexOf(childTree);
if (indexInStatements >= 0) {
ImmutableMap.Builder<Tree, CanCompleteNormallyPatch> patches = ImmutableMap.builder();
patches.put(ifTree, CANNOT_COMPLETE_NORMALLY_ANALYZE_INSIDE);
deletedTrees.forEach(
deletedTree -> patches.put(deletedTree, CANNOT_COMPLETE_NORMALLY_SKIP_INSIDE));
boolean nextStatementReachable =
Reachability.canCompleteNormally(
statements.get(indexInStatements), patches.buildOrThrow());
// Everything after this statement in this list (other than any pulled-up statements,
// which move into the switch) 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.
int firstDeletedIndex = indexInStatements + 1 + numberPulledUp;
deletedTrees.addAll(statements.subList(firstDeletedIndex, statements.size()));
if (nextStatementReachable) {
break;
}
// If a next statement in this statement list exists, then it is not reachable.
if (firstDeletedIndex < statements.size()) {
List<? extends StatementTree> deletedStatements =
statements.subList(firstDeletedIndex, statements.size());
if (deletedStatements.stream().anyMatch(IfChainToSwitch::hasIfInTree)) {
// This code is now unreachable, so leaving it in place would not compile, but it
// contains an `if` that this check may rewrite under a separate finding whose fix
// would overlap this deletion. Decline to convert instead.
return false;
}
// A block is deleted up to and including its closing brace, which is then written
// back. A colon-style `case` has no block nor closing brace, so its dead statements
// are deleted up to the last of them.
int deletionEndPosition;
String replacement;
switch (tree) {
case BlockTree blockTree -> {
deletionEndPosition = state.getEndPosition(blockTree);
replacement = "}";
}
case CaseTree caseTree -> {
// A local variable declared in a colon-style `case` remains in scope in the later
// case groups of the same switch block. If any of them refers to it, then
// deleting it would break them. Nor can it be kept, because it is now
// unreachable. Decline to produce a finding instead
if (isDeletedVariableReferencedLater(
path.getParentPath().getLeaf(), caseTree, deletedStatements)) {
return false;
}
deletionEndPosition = state.getEndPosition(statements.getLast());
replacement = "";
}
// Only a block or a colon-style `case` holds statements (see
// `getEnclosedStatements`)
default -> throw new AssertionError("Unexpected tree type: " + tree.getKind());
}
int deletionStartPosition = state.getEndPosition(statements.get(firstDeletedIndex - 1));
String deletedRegion =
state
.getSourceCode()
.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(firstDeletedIndex, statements.size())
.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(deletionStartPosition, deletionEndPosition, replacement);
}
}
}
// Only applies in lowest statement list
numberPulledUp = 0;
childTree = tree;
}
return true;
}

/**
* Returns the statements that {@code tree} holds directly, or empty if it holds none. Statements
* can only be held by a block or a colon-style {@code case}.
*/
private static Optional<List<? extends StatementTree>> getEnclosedStatements(Tree tree) {
return switch (tree) {
case BlockTree blockTree -> Optional.of(blockTree.getStatements());
// Empty for an arrow-style `case`, whose body is a single statement rather than a list
case CaseTree caseTree -> Optional.ofNullable(caseTree.getStatements());
default -> Optional.empty();
};
}

/** Returns the cases that follow {@code caseTree} in {@code switchTree}, its enclosing switch. */
private static List<? extends CaseTree> getSubsequentCases(Tree switchTree, CaseTree caseTree) {
List<? extends CaseTree> cases =
switch (switchTree) {
case SwitchTree s -> s.getCases();
case SwitchExpressionTree s -> s.getCases();
default -> throw new IllegalArgumentException("not a switch: " + switchTree.getKind());
};
return cases.subList(cases.indexOf(caseTree) + 1, cases.size());
}

/**
* Returns whether a local variable declared by {@code deletedStatements}, which are deleted from
* {@code caseTree}, is referred to by a later case of {@code switchTree} (its enclosing
* `switch`).
*/
private static boolean isDeletedVariableReferencedLater(
Tree switchTree, CaseTree caseTree, List<? extends StatementTree> deletedStatements) {
ImmutableSet<Symbol> deletedVariableSymbols =
deletedStatements.stream()
.filter(VariableTree.class::isInstance)
.map(s -> getSymbol((VariableTree) s))
.collect(toImmutableSet());
return !deletedVariableSymbols.isEmpty()
&& getSubsequentCases(switchTree, caseTree).stream()
.anyMatch(
laterCase ->
!disjoint(
getReferencedLocalVariablesInTree(laterCase), deletedVariableSymbols));
}

/**
* Analyzes the supplied case IRs for a switch statement. If deemed likely possible, this method
* pulls up the statement subsequent to the if statement into the switch under a default case, and
Expand Down
86 changes: 33 additions & 53 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 @@ -22,10 +22,12 @@
import static com.google.errorprone.bugpatterns.SwitchUtils.KINDS_RETURN_OR_THROW;
import static com.google.errorprone.bugpatterns.SwitchUtils.REMOVE_DEFAULT_CASE_SHORT_DESCRIPTION;
import static com.google.errorprone.bugpatterns.SwitchUtils.analyzeCaseForNullAndDefault;
import static com.google.errorprone.bugpatterns.SwitchUtils.deleteDeadCodeInAncestors;
import static com.google.errorprone.bugpatterns.SwitchUtils.findCombinableVariableTree;
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,16 +40,15 @@
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_SKIP_INSIDE;
import static java.util.stream.Collectors.joining;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Range;
import com.google.common.collect.Streams;
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 Down Expand Up @@ -235,6 +236,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 +256,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.
&& deleteDeadCodeAfterReturnSwitch(switchTree, state, SuggestedFix.builder());

ImmutableList<StatementTree> precedingStatements = getPrecedingStatementsInBlock(state);
Optional<ExpressionTree> assignmentTarget =
Expand Down Expand Up @@ -625,63 +634,34 @@ 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.

// 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
Tree prev = 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.
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;
if (nextStatementReachable) {
break;
}

// If a next statement in this block exists, then it is not reachable.
if (indexInBlock < statements.size() - 1) {
String deletedRegion =
state
.getSourceCode()
.subSequence(
state.getEndPosition(statements.get(indexInBlock)),
state.getEndPosition(blockTree))
.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);
} 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),
"}");
}
}
}
prev = tree;
}
// (`analyzeSwitchTree` only proposes this conversion if that deletion succeeds.)
deleteDeadCodeAfterReturnSwitch(switchTree, state, suggestedFixBuilder);

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. 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 deleteDeadCodeAfterReturnSwitch(
SwitchTree switchTree, VisitorState state, SuggestedFix.Builder suggestedFixBuilder) {
// A switch in the code that becomes dead may get a separate finding from this check, whose fix
// would overlap the deletion; then decline to convert.
return deleteDeadCodeInAncestors(
switchTree,
state,
suggestedFixBuilder,
/* numberPulledUp= */ 0,
/* declineIfDeleted= */ SwitchUtils::hasSwitchInTree);
}

/**
* Converts a single `yield` statement to a single expression within the {@code
* suggestedFixBuilder}, for example `case null -> {yield 0;}` would become `case null -> 0;`.
Expand Down
Loading
Loading