Skip to content
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

[StatementSwitchToExpressionSwitch] Combine immediately-preceding variable definitions with assignment switches to the same variable, where feasible #4730

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[StatementSwitchToExpressionSwitch] Combine immediately-preceding variable definitions with assignment switches to the same variable, where feasible #4730

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
250 changes: 231 additions & 19 deletions core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java
View file
Open in desktop
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
Loading