[RefactorSwitch] fix bug which can result in invalid code generation when a variable is read on the left side of an assignment

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<VariableTree> 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<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);
-  }
-
   /**
    * Determines whether local variable {@code symbol} has reads or writes within the scope of the
    * supplied {@code tree}.

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

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