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 8665d17bdb7..c5bff462c4d 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java @@ -22,6 +22,7 @@ import static com.google.common.collect.Iterables.getOnlyElement; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import static com.google.errorprone.bugpatterns.SwitchUtils.getReferencedLocalVariablesInTree; +import static com.google.errorprone.bugpatterns.SwitchUtils.noReadsOfVariable; 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; @@ -61,9 +62,7 @@ import com.sun.source.tree.CompoundAssignmentTree; import com.sun.source.tree.ExpressionStatementTree; import com.sun.source.tree.ExpressionTree; -import com.sun.source.tree.IdentifierTree; import com.sun.source.tree.LabeledStatementTree; -import com.sun.source.tree.MemberSelectTree; import com.sun.source.tree.PatternCaseLabelTree; import com.sun.source.tree.ReturnTree; import com.sun.source.tree.StatementTree; @@ -72,7 +71,6 @@ 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.source.util.TreeScanner; import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Symbol.VarSymbol; @@ -516,43 +514,6 @@ private static Optional findCombinableVariableTree( return Optional.of(variableTree); } - /** - * 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 referencedLocalVariables = new HashSet<>(); - new TreePathScanner() { - - @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); - } - /** * Determines whether local variable {@code symbol} has reads or writes within the scope of the * supplied {@code tree}. diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/SwitchUtils.java b/core/src/main/java/com/google/errorprone/bugpatterns/SwitchUtils.java index 59d4c69f2aa..a93dede338f 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/SwitchUtils.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/SwitchUtils.java @@ -191,8 +191,11 @@ static boolean noReadsOfVariable(VarSymbol symbol, VisitorState state) { @Override public Void visitAssignment(AssignmentTree tree, Void unused) { - // Only looks at the right-hand side of the assignment - return scan(tree.getExpression(), null); + if (tree.getVariable() instanceof IdentifierTree) { + // Bare assignments are pure writes to the variable, reads happen only on RHS + return scan(tree.getExpression(), null); + } + return super.visitAssignment(tree, null); } @Override diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/RefactorSwitchTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/RefactorSwitchTest.java index 41ce7a925c3..cd093bece8f 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/RefactorSwitchTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/RefactorSwitchTest.java @@ -199,6 +199,55 @@ public int compareTo(Integer other) { .doTest(); } + @Test + public void switchAssignment_lhsReadCannotCombineWithDeclaration_error() { + // The switch can't be combined with the declaration of `x`, because `x.field` requires `x` to + // be definitely assigned + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + static class Foo { + Foo field; + } + + public void foo(int ui) { + Foo x = null; + switch (ui) { + case 1 -> x = new Foo(); + case 2 -> x = (x.field = new Foo()); + default -> x = new Foo(); + } + } + } + """) + .addOutputLines( + "Test.java", + """ + class Test { + static class Foo { + Foo field; + } + + public void foo(int ui) { + Foo x = null; + x = + switch (ui) { + case 1 -> new Foo(); + case 2 -> (x.field = new Foo()); + default -> new Foo(); + }; + } + } + """) + .setArgs( + "-XepOpt:RefactorSwitch:EnableAssignmentSwitch=true", + "-XepOpt:RefactorSwitch:EnableReturnSwitch=false", + "-XepOpt:RefactorSwitch:EnableSimplifySwitch=false") + .doTest(); + } + @Test public void switchByEnum_hasContinue_noError() { // Continuing out of a switch statement is allowed, but continuing out of a switch expression is