Skip to content

Commit

Permalink
[StatementSwitchToExpressionSwitch] Combine immediately-preceding var…
Browse files Browse the repository at this point in the history
…iable definitions with assignment switches to the same variable, where feasible

PiperOrigin-RevId: 705971103
  • Loading branch information
markhbrady authored and Error Prone Team committed Dec 20, 2024
1 parent 87eec2b commit afe0fbb
Show file tree
Hide file tree
Showing 2 changed files with 641 additions and 31 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,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.getSymbol;
import static com.google.errorprone.util.ASTHelpers.hasImplicitType;
import static com.google.errorprone.util.ASTHelpers.isSwitchDefault;
import static com.sun.source.tree.Tree.Kind.BLOCK;
import static com.sun.source.tree.Tree.Kind.BREAK;
Expand All @@ -36,13 +37,16 @@
import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
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.bugpatterns.BugChecker.SwitchTreeMatcher;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.CompileTimeConstantExpressionMatcher;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.matchers.Matchers;
import com.google.errorprone.util.ASTHelpers;
import com.google.errorprone.util.ErrorProneComment;
Expand All @@ -56,13 +60,18 @@
import com.sun.source.tree.ExpressionStatementTree;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.IdentifierTree;
import com.sun.source.tree.MemberSelectTree;
import com.sun.source.tree.ModifiersTree;
import com.sun.source.tree.ReturnTree;
import com.sun.source.tree.StatementTree;
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;
import com.sun.source.util.TreePath;
import com.sun.source.util.TreePathScanner;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Symbol.VarSymbol;
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.tree.JCTree.JCAssign;
import com.sun.tools.javac.tree.JCTree.JCAssignOp;
Expand All @@ -80,6 +89,7 @@
import java.util.Set;
import java.util.function.Predicate;
import java.util.regex.Pattern;
import java.util.stream.Stream;
import javax.inject.Inject;
import javax.lang.model.element.ElementKind;

Expand All @@ -101,6 +111,7 @@ public final class StatementSwitchToExpressionSwitch extends BugChecker
private static final AssignmentSwitchAnalysisResult DEFAULT_ASSIGNMENT_SWITCH_ANALYSIS_RESULT =
AssignmentSwitchAnalysisResult.of(
/* canConvertToAssignmentSwitch= */ false,
/* canCombineWithPrecedingVariableDeclaration= */ false,
/* assignmentTargetOptional= */ Optional.empty(),
/* assignmentKindOptional= */ Optional.empty(),
/* assignmentSourceCodeOptional= */ Optional.empty());
Expand All @@ -112,6 +123,8 @@ public final class StatementSwitchToExpressionSwitch extends BugChecker
DEFAULT_ASSIGNMENT_SWITCH_ANALYSIS_RESULT,
/* groupedWithNextCase= */ ImmutableList.of());
private static final String EQUALS_STRING = "=";
private static final Matcher<ExpressionTree> COMPILE_TIME_CONSTANT_MATCHER =
CompileTimeConstantExpressionMatcher.instance();

// Tri-state to represent the fall-thru control flow of a particular case of a particular
// statement switch
Expand Down Expand Up @@ -282,11 +295,46 @@ private static AnalysisResult analyzeSwitchTree(SwitchTree switchTree, VisitorSt
// The switch must be exhaustive (at compile time)
&& exhaustive;

List<StatementTree> precedingStatements = getPrecedingStatementsInBlock(switchTree, state);
Optional<ExpressionTree> assignmentTarget =
assignmentSwitchAnalysisState.assignmentTargetOptional();

// If present, the variable tree that can be combined with the switch block
Optional<VariableTree> combinableVariableTree =
precedingStatements.isEmpty()
? Optional.empty()
: Optional.of(precedingStatements)
// Don't try to combine when multiple variables are declared together
.filter(
StatementSwitchToExpressionSwitch
::precedingTwoStatementsNotInSameVariableDeclaratorList)
// Extract the immediately preceding statement
.map(Iterables::getLast)
// Preceding statement must be a variable declaration
.filter(precedingStatement -> precedingStatement instanceof VariableTree)
.map(precedingStatement -> (VariableTree) precedingStatement)
// Variable must be uninitialized, or initialized with a compile-time constant
.filter(
variableTree ->
(variableTree.getInitializer() == null)
|| COMPILE_TIME_CONSTANT_MATCHER.matches(
variableTree.getInitializer(), state))
// If we are reading the initialized value in the switch block, we can't remove it
.filter(
variableTree -> noReadsOfVariable(ASTHelpers.getSymbol(variableTree), state))
// The variable and the switch's assignment must be compatible
.filter(
variableTree ->
isVariableCompatibleWithAssignment(assignmentTarget, variableTree));
boolean canCombineWithPrecedingVariableDeclaration =
canConvertToAssignmentSwitch && combinableVariableTree.isPresent();

return AnalysisResult.of(
/* canConvertDirectlyToExpressionSwitch= */ allCasesHaveDefiniteControlFlow,
canConvertToReturnSwitch,
AssignmentSwitchAnalysisResult.of(
canConvertToAssignmentSwitch,
canCombineWithPrecedingVariableDeclaration,
assignmentSwitchAnalysisState.assignmentTargetOptional(),
assignmentSwitchAnalysisState.assignmentExpressionKindOptional(),
assignmentSwitchAnalysisState
Expand All @@ -295,6 +343,43 @@ private static AnalysisResult analyzeSwitchTree(SwitchTree switchTree, VisitorSt
ImmutableList.copyOf(groupedWithNextCase));
}

/**
* Determines whether local variable {@code symbol} has no reads within the scope of the {@code
* VisitorState}. (Writes to the variable are ignored.)
*/
private static boolean noReadsOfVariable(VarSymbol symbol, VisitorState state) {
Set<VarSymbol> referencedLocalVariables = new HashSet<>();
new TreePathScanner<Void, Void>() {

@Override
public Void visitAssignment(AssignmentTree tree, Void unused) {
// Only looks at the right-hand side of the assignment
return scan(tree.getExpression(), null);
}

@Override
public Void visitMemberSelect(MemberSelectTree memberSelect, Void unused) {
handle(memberSelect);
return super.visitMemberSelect(memberSelect, null);
}

@Override
public Void visitIdentifier(IdentifierTree identifier, Void unused) {
handle(identifier);
return super.visitIdentifier(identifier, null);
}

private void handle(Tree tree) {
var symbol = getSymbol(tree);
if (symbol instanceof VarSymbol varSymbol) {
referencedLocalVariables.add(varSymbol);
}
}
}.scan(state.getPath(), null);

return !referencedLocalVariables.contains(symbol);
}

/**
* Renders the Java source code for a [compound] assignment operator. The parameter must be either
* an {@code AssignmentTree} or a {@code CompoundAssignmentTree}.
Expand Down Expand Up @@ -488,6 +573,24 @@ private static boolean isCompatibleWithFirstAssignment(
return Objects.equals(assignmentTargetSymbol, caseAssignmentTargetSymbol);
}

/**
* Determines whether a variable definition is compatible with an assignment target (e.g. of a
* switch statement). Compatibility means that the assignment is being made to to the same
* variable that is being defined.
*/
private static boolean isVariableCompatibleWithAssignment(
Optional<ExpressionTree> assignmentTargetOptional, VariableTree variableDefinition) {
// No assignment target, so not compatible
if (assignmentTargetOptional.isEmpty()) {
return false;
}

Symbol assignmentTargetSymbol = getSymbol(assignmentTargetOptional.get());
Symbol definedSymbol = ASTHelpers.getSymbol(variableDefinition);

return Objects.equals(assignmentTargetSymbol, definedSymbol);
}

/**
* Determines whether the supplied case's {@code statements} are capable of being mapped to an
* equivalent expression switch case (without repeating code), returning {@code true} if so.
Expand Down Expand Up @@ -740,6 +843,40 @@ private static SuggestedFix convertToReturnSwitch(
return suggestedFixBuilder.build();
}

/**
* Retrieves a list of all statements (if any) preceding the supplied {@code SwitchTree} in its
* lowest-ancestor statement block (if any).
*/
private static List<StatementTree> getPrecedingStatementsInBlock(
SwitchTree switchTree, VisitorState state) {

List<StatementTree> precedingStatements = new ArrayList<>();

// NOMUTANTS--performance/early return only; correctness unchanged
if (!Matchers.previousStatement(Matchers.<StatementTree>anything())
.matches(switchTree, state)) {
// No lowest-ancestor block or no preceding statements
return precedingStatements;
}

// 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 = 0; i < findBlockStatementIndex(rootToSwitchPath, blockTree); i++) {
precedingStatements.add(blockTree.getStatements().get(i));
}
}
}
// Should have returned above
return precedingStatements;
}

/**
* Retrieves a list of all statements (if any) following the supplied {@code SwitchTree} in its
* lowest-ancestor statement block (if any).
Expand Down Expand Up @@ -790,6 +927,34 @@ private static int findBlockStatementIndex(TreePath treePath, BlockTree blockTre
return -1;
}

/**
* Determines whether the last two preceding statements are not variable declarations within the
* same VariableDeclaratorList, for example {@code int x, y;}. VariableDeclaratorLists are defined
* in e.g. JLS 21 § 14.4. Precondition: all preceding statements are taken from the same {@code
* BlockTree}.
*/
private static boolean precedingTwoStatementsNotInSameVariableDeclaratorList(
List<StatementTree> precedingStatements) {

if (precedingStatements.size() < 2) {
return true;
}

StatementTree secondToLastStatement = precedingStatements.get(precedingStatements.size() - 2);
StatementTree lastStatement = Iterables.getLast(precedingStatements);
if (!(secondToLastStatement instanceof VariableTree)
|| !(lastStatement instanceof VariableTree)) {
return true;
}

VariableTree variableTree1 = (VariableTree) secondToLastStatement;
VariableTree variableTree2 = (VariableTree) lastStatement;

// Start positions will vary if the variable declarations are in the same
// VariableDeclaratorList.
return getStartPosition(variableTree1) != getStartPosition(variableTree2);
}

/**
* Transforms the supplied statement switch into an assignment switch style of expression switch.
* In this conversion, each nontrivial statement block is mapped one-to-one to a new expression on
Expand All @@ -800,30 +965,68 @@ private static int findBlockStatementIndex(TreePath treePath, BlockTree blockTre
private static SuggestedFix convertToAssignmentSwitch(
SwitchTree switchTree, VisitorState state, AnalysisResult analysisResult) {

List<StatementTree> statementsToDelete = new ArrayList<>();
StringBuilder replacementCodeBuilder = new StringBuilder();

if (analysisResult
.assignmentSwitchAnalysisResult()
.canCombineWithPrecedingVariableDeclaration()) {

List<StatementTree> precedingStatements = getPrecedingStatementsInBlock(switchTree, state);
VariableTree variableTree = (VariableTree) Iterables.getLast(precedingStatements);
statementsToDelete.add(variableTree);

// Render flags such as "final"
ModifiersTree modifiersTree = variableTree.getModifiers();
StringBuilder flagsBuilder = new StringBuilder();
if (!modifiersTree.getFlags().isEmpty()) {
flagsBuilder.append(
modifiersTree.getFlags().stream().map(flag -> flag + " ").collect(joining("")));
}

// Add variable comments and annotations, followed by rendered flags to the
// beginning of the expression switch
ImmutableList<ErrorProneComment> allVariableComments =
state.getTokensForNode(variableTree).stream()
.flatMap(errorProneToken -> errorProneToken.comments().stream())
.collect(toImmutableList());
replacementCodeBuilder.append(
Streams.concat(
allVariableComments.stream()
.filter(comment -> !comment.getText().isEmpty())
.map(ErrorProneComment::getText),
modifiersTree.getAnnotations().stream().map(state::getSourceForNode),
Stream.of(flagsBuilder.toString()))
.collect(joining("\n")));

// Local variables declared with "var" must unfortunately be handled as a special case because
// getSourceForNode() returns null for the source code of a "var" declaration.
String sourceForType =
hasImplicitType(variableTree, state)
? "var"
: state.getSourceForNode(variableTree.getType());

replacementCodeBuilder.append(sourceForType).append(" ");
}

List<? extends CaseTree> cases = switchTree.getCases();
ImmutableList<ErrorProneComment> allSwitchComments =
state.getTokensForNode(switchTree).stream()
.flatMap(errorProneToken -> errorProneToken.comments().stream())
.collect(toImmutableList());

StringBuilder replacementCodeBuilder =
new StringBuilder(
state.getSourceForNode(
analysisResult
.assignmentSwitchAnalysisResult()
.assignmentTargetOptional()
.get()))
.append(" ")
// Invariant: always present when a finding exists
.append(
analysisResult
.assignmentSwitchAnalysisResult()
.assignmentSourceCodeOptional()
.get())
.append(" ")
.append("switch ")
.append(state.getSourceForNode(switchTree.getExpression()))
.append(" {");
replacementCodeBuilder
.append(
state.getSourceForNode(
analysisResult.assignmentSwitchAnalysisResult().assignmentTargetOptional().get()))
.append(" ")
// Invariant: always present when a finding exists
.append(
analysisResult.assignmentSwitchAnalysisResult().assignmentSourceCodeOptional().get())
.append(" ")
.append("switch ")
.append(state.getSourceForNode(switchTree.getExpression()))
.append(" {");

StringBuilder groupedCaseCommentsAccumulator = null;
boolean firstCaseInGroup = true;
Expand Down Expand Up @@ -903,7 +1106,10 @@ private static SuggestedFix convertToAssignmentSwitch(
// Close the switch statement
replacementCodeBuilder.append("\n};");

return SuggestedFix.builder().replace(switchTree, replacementCodeBuilder.toString()).build();
SuggestedFix.Builder suggestedFixBuilder =
SuggestedFix.builder().replace(switchTree, replacementCodeBuilder.toString());
statementsToDelete.forEach(deleteMe -> suggestedFixBuilder.replace(deleteMe, ""));
return suggestedFixBuilder.build();
}

/**
Expand Down Expand Up @@ -1258,6 +1464,10 @@ abstract static class AssignmentSwitchAnalysisResult {
// Whether the statement switch can be converted to an assignment switch
abstract boolean canConvertToAssignmentSwitch();

// Whether the assignment switch can be combined with the immediately preceding variable
// declaration
abstract boolean canCombineWithPrecedingVariableDeclaration();

// Target of the assignment switch, if any
abstract Optional<ExpressionTree> assignmentTargetOptional();

Expand All @@ -1269,11 +1479,13 @@ abstract static class AssignmentSwitchAnalysisResult {

static AssignmentSwitchAnalysisResult of(
boolean canConvertToAssignmentSwitch,
boolean canCombineWithPrecedingVariableDeclaration,
Optional<ExpressionTree> assignmentTargetOptional,
Optional<Tree.Kind> assignmentKindOptional,
Optional<String> assignmentSourceCodeOptional) {
return new AutoValue_StatementSwitchToExpressionSwitch_AssignmentSwitchAnalysisResult(
canConvertToAssignmentSwitch,
canCombineWithPrecedingVariableDeclaration,
assignmentTargetOptional,
assignmentKindOptional,
assignmentSourceCodeOptional);
Expand Down
Loading

0 comments on commit afe0fbb

Please sign in to comment.