Discourage assignment expressions.

core/src/main/java/com/google/errorprone/bugpatterns/AssignmentExpression.java

@@ -0,0 +1,88 @@
+/*
+ * Copyright 2025 The Error Prone Authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.errorprone.bugpatterns;
+
+import static com.google.errorprone.util.ASTHelpers.getSymbol;
+
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.BugPattern.SeverityLevel;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker.AssignmentTreeMatcher;
+import com.google.errorprone.fixes.SuggestedFix;
+import com.google.errorprone.matchers.Description;
+import com.sun.source.tree.AnnotationTree;
+import com.sun.source.tree.AssignmentTree;
+import com.sun.source.tree.BinaryTree;
+import com.sun.source.tree.ExpressionStatementTree;
+import com.sun.source.tree.IdentifierTree;
+import com.sun.source.tree.LambdaExpressionTree;
+import com.sun.source.tree.ParenthesizedTree;
+import com.sun.source.tree.Tree;
+import com.sun.source.tree.VariableTree;
+import com.sun.tools.javac.code.Symbol.VarSymbol;
+
+/** A BugPattern; see the summary. */
+@BugPattern(
+    summary =
+        "The use of an assignment expression can be surprising and hard to read; consider factoring"
+            + " out the assignment to a separate statement.",
+    severity = SeverityLevel.WARNING)
+public final class AssignmentExpression extends BugChecker implements AssignmentTreeMatcher {
+
+  @Override
+  public Description matchAssignment(AssignmentTree tree, VisitorState state) {
+    Tree parent = state.getPath().getParentPath().getLeaf();
+    if (parent instanceof ExpressionStatementTree) {
+      return Description.NO_MATCH;
+    }
+    if (parent instanceof AnnotationTree) {
+      return Description.NO_MATCH;
+    }
+    if (parent instanceof LambdaExpressionTree) {
+      return Description.NO_MATCH;
+    }
+    // Exempt the C-ism of (foo = getFoo()) != null, etc.
+    if (parent instanceof ParenthesizedTree
+        && state.getPath().getParentPath().getParentPath().getLeaf() instanceof BinaryTree) {
+      return Description.NO_MATCH;
+    }
+    // Detect duplicate assignments: a = a = foo() so that we can generate a fix.
+    if (isDuplicateAssignment(tree, parent)) {
+      return describeMatch(
+          tree, SuggestedFix.replace(tree, state.getSourceForNode(tree.getExpression())));
+    }
+    // If we got here it's something like x = y = 0, which is odd but not disallowed.
+    if (parent instanceof AssignmentTree) {
+      return Description.NO_MATCH;
+    }
+    return describeMatch(tree);
+  }
+
+  private static boolean isDuplicateAssignment(AssignmentTree tree, Tree parent) {
+    if (!(tree.getVariable() instanceof IdentifierTree
+        && getSymbol(tree.getVariable()) instanceof VarSymbol varSymbol)) {
+      return false;
+    }
+    return switch (parent.getKind()) {
+      case ASSIGNMENT ->
+          ((AssignmentTree) parent).getVariable() instanceof IdentifierTree identifierTree
+              && varSymbol.equals(getSymbol(identifierTree));
+      case VARIABLE -> varSymbol.equals(getSymbol((VariableTree) parent));
+      default -> false;
+    };
+  }
+}

core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java

@@ -40,6 +40,7 @@
 import com.google.errorprone.bugpatterns.AssertFalse;
 import com.google.errorprone.bugpatterns.AssertThrowsMultipleStatements;
 import com.google.errorprone.bugpatterns.AssertionFailureIgnored;
+import com.google.errorprone.bugpatterns.AssignmentExpression;
 import com.google.errorprone.bugpatterns.AsyncCallableReturnsNull;
 import com.google.errorprone.bugpatterns.AsyncFunctionReturnsNull;
 import com.google.errorprone.bugpatterns.AttemptedNegativeZero;
@@ -875,6 +876,7 @@ public static ScannerSupplier warningChecks() {
           AssertEqualsArgumentOrderChecker.class,
           AssertThrowsMultipleStatements.class,
           AssertionFailureIgnored.class,
+          AssignmentExpression.class,
           AssistedInjectAndInjectOnSameConstructor.class,
           AttemptedNegativeZero.class,
           AutoValueBoxedValues.class,

core/src/test/java/com/google/errorprone/bugpatterns/AssignmentExpressionTest.java

@@ -0,0 +1,130 @@
+/*
+ * Copyright 2025 The Error Prone Authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.errorprone.bugpatterns;
+
+import com.google.errorprone.BugCheckerRefactoringTestHelper;
+import com.google.errorprone.CompilationTestHelper;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+@RunWith(JUnit4.class)
+public final class AssignmentExpressionTest {
+  private final CompilationTestHelper helper =
+      CompilationTestHelper.newInstance(AssignmentExpression.class, getClass());
+
+  private final BugCheckerRefactoringTestHelper refactoringHelper =
+      BugCheckerRefactoringTestHelper.newInstance(AssignmentExpression.class, getClass());
+
+  @Test
+  public void bareAssignment_noFinding() {
+    helper
+        .addSourceLines(
+            "Test.java",
+            """
+            class Test {
+              void test() {
+                int a = 1;
+                a = 2;
+                for (a = 3; true; ) {
+                  break;
+                }
+              }
+            }
+            """)
+        .doTest();
+  }
+
+  @Test
+  public void comparedToNull_sure() {
+    helper
+        .addSourceLines(
+            "Test.java",
+            """
+            class Test {
+              void test() {
+                Object a;
+                if ((a = new Object()) != null) {
+                  return;
+                }
+              }
+            }
+            """)
+        .doTest();
+  }
+
+  @Test
+  public void multipleAssignments_sure() {
+    helper
+        .addSourceLines(
+            "Test.java",
+            """
+            class Test {
+              void test() {
+                Object a;
+                Object b;
+                a = b = new Object();
+              }
+            }
+            """)
+        .doTest();
+  }
+
+  @Test
+  public void assignmentExpressionUsed_ohNo() {
+    helper
+        .addSourceLines(
+            "Test.java",
+            """
+            class Test {
+              void test(boolean x) {
+                // BUG: Diagnostic contains:
+                test(x = true);
+              }
+            }
+            """)
+        .doTest();
+  }
+
+  @Test
+  public void doubleAssignment_removed() {
+    refactoringHelper
+        .addInputLines(
+            "Test.java",
+            """
+            class Test {
+              void test() {
+                // BUG: Diagnostic contains:
+                Object a = a = new Object();
+                // BUG: Diagnostic contains:
+                a = a = new Object();
+              }
+            }
+            """)
+        .addOutputLines(
+            "Test.java",
+            """
+            class Test {
+              void test() {
+                Object a = new Object();
+                a = new Object();
+              }
+            }
+            """)
+        .doTest();
+  }
+}

docs/bugpattern/AssignmentExpression.md

@@ -0,0 +1,40 @@
+Using the result of an assignment expression can be quite unclear, except for
+simple cases where the result is used to initialise another variable, such as
+`x = y = 0`.
+
+Consider a common pattern of lazily initialising a field:
+
+```java
+class Lazy<T> {
+  private T t = null;
+
+  abstract T create();
+
+  public T get() {
+    if (t != null) {
+      return t;
+    }
+    return t = create();
+  }
+}
+```
+
+```java
+class Lazy<T> {
+  private T t = null;
+
+  abstract T create();
+
+  public T get() {
+    if (t != null) {
+      return t;
+    }
+    t = create();
+    return t;
+  }
+}
+```
+
+At the cost of another line, it's now clearer what's happening. (Note that
+neither the before nor the after are thread-safe; this particular example would
+be better served with `Suppliers.memoizing`.)