Skip to content

Commit baf0e0a

Browse files
graememorganError Prone Team
graememorgan
authored and
Error Prone Team
committed
Discourage assignment expressions.
PiperOrigin-RevId: 737034262
1 parent f635aa8 commit baf0e0a

File tree

4 files changed

+260
-0
lines changed

4 files changed

+260
-0
lines changed

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

+88
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
/*
2+
* Copyright 2025 The Error Prone Authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.google.errorprone.bugpatterns;
18+
19+
import static com.google.errorprone.util.ASTHelpers.getSymbol;
20+
21+
import com.google.errorprone.BugPattern;
22+
import com.google.errorprone.BugPattern.SeverityLevel;
23+
import com.google.errorprone.VisitorState;
24+
import com.google.errorprone.bugpatterns.BugChecker.AssignmentTreeMatcher;
25+
import com.google.errorprone.fixes.SuggestedFix;
26+
import com.google.errorprone.matchers.Description;
27+
import com.sun.source.tree.AnnotationTree;
28+
import com.sun.source.tree.AssignmentTree;
29+
import com.sun.source.tree.BinaryTree;
30+
import com.sun.source.tree.ExpressionStatementTree;
31+
import com.sun.source.tree.IdentifierTree;
32+
import com.sun.source.tree.LambdaExpressionTree;
33+
import com.sun.source.tree.ParenthesizedTree;
34+
import com.sun.source.tree.Tree;
35+
import com.sun.source.tree.VariableTree;
36+
import com.sun.tools.javac.code.Symbol.VarSymbol;
37+
38+
/** A BugPattern; see the summary. */
39+
@BugPattern(
40+
summary =
41+
"The use of an assignment expression can be surprising and hard to read; consider factoring"
42+
+ " out the assignment to a separate statement.",
43+
severity = SeverityLevel.WARNING)
44+
public final class AssignmentExpression extends BugChecker implements AssignmentTreeMatcher {
45+
46+
@Override
47+
public Description matchAssignment(AssignmentTree tree, VisitorState state) {
48+
Tree parent = state.getPath().getParentPath().getLeaf();
49+
if (parent instanceof ExpressionStatementTree) {
50+
return Description.NO_MATCH;
51+
}
52+
if (parent instanceof AnnotationTree) {
53+
return Description.NO_MATCH;
54+
}
55+
if (parent instanceof LambdaExpressionTree) {
56+
return Description.NO_MATCH;
57+
}
58+
// Exempt the C-ism of (foo = getFoo()) != null, etc.
59+
if (parent instanceof ParenthesizedTree
60+
&& state.getPath().getParentPath().getParentPath().getLeaf() instanceof BinaryTree) {
61+
return Description.NO_MATCH;
62+
}
63+
// Detect duplicate assignments: a = a = foo() so that we can generate a fix.
64+
if (isDuplicateAssignment(tree, parent)) {
65+
return describeMatch(
66+
tree, SuggestedFix.replace(tree, state.getSourceForNode(tree.getExpression())));
67+
}
68+
// If we got here it's something like x = y = 0, which is odd but not disallowed.
69+
if (parent instanceof AssignmentTree) {
70+
return Description.NO_MATCH;
71+
}
72+
return describeMatch(tree);
73+
}
74+
75+
private static boolean isDuplicateAssignment(AssignmentTree tree, Tree parent) {
76+
if (!(tree.getVariable() instanceof IdentifierTree
77+
&& getSymbol(tree.getVariable()) instanceof VarSymbol varSymbol)) {
78+
return false;
79+
}
80+
return switch (parent.getKind()) {
81+
case ASSIGNMENT ->
82+
((AssignmentTree) parent).getVariable() instanceof IdentifierTree identifierTree
83+
&& varSymbol.equals(getSymbol(identifierTree));
84+
case VARIABLE -> varSymbol.equals(getSymbol((VariableTree) parent));
85+
default -> false;
86+
};
87+
}
88+
}

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

+2
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
import com.google.errorprone.bugpatterns.AssertFalse;
4141
import com.google.errorprone.bugpatterns.AssertThrowsMultipleStatements;
4242
import com.google.errorprone.bugpatterns.AssertionFailureIgnored;
43+
import com.google.errorprone.bugpatterns.AssignmentExpression;
4344
import com.google.errorprone.bugpatterns.AsyncCallableReturnsNull;
4445
import com.google.errorprone.bugpatterns.AsyncFunctionReturnsNull;
4546
import com.google.errorprone.bugpatterns.AttemptedNegativeZero;
@@ -875,6 +876,7 @@ public static ScannerSupplier warningChecks() {
875876
AssertEqualsArgumentOrderChecker.class,
876877
AssertThrowsMultipleStatements.class,
877878
AssertionFailureIgnored.class,
879+
AssignmentExpression.class,
878880
AssistedInjectAndInjectOnSameConstructor.class,
879881
AttemptedNegativeZero.class,
880882
AutoValueBoxedValues.class,

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

+130
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
/*
2+
* Copyright 2025 The Error Prone Authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.google.errorprone.bugpatterns;
18+
19+
import com.google.errorprone.BugCheckerRefactoringTestHelper;
20+
import com.google.errorprone.CompilationTestHelper;
21+
import org.junit.Test;
22+
import org.junit.runner.RunWith;
23+
import org.junit.runners.JUnit4;
24+
25+
@RunWith(JUnit4.class)
26+
public final class AssignmentExpressionTest {
27+
private final CompilationTestHelper helper =
28+
CompilationTestHelper.newInstance(AssignmentExpression.class, getClass());
29+
30+
private final BugCheckerRefactoringTestHelper refactoringHelper =
31+
BugCheckerRefactoringTestHelper.newInstance(AssignmentExpression.class, getClass());
32+
33+
@Test
34+
public void bareAssignment_noFinding() {
35+
helper
36+
.addSourceLines(
37+
"Test.java",
38+
"""
39+
class Test {
40+
void test() {
41+
int a = 1;
42+
a = 2;
43+
for (a = 3; true; ) {
44+
break;
45+
}
46+
}
47+
}
48+
""")
49+
.doTest();
50+
}
51+
52+
@Test
53+
public void comparedToNull_sure() {
54+
helper
55+
.addSourceLines(
56+
"Test.java",
57+
"""
58+
class Test {
59+
void test() {
60+
Object a;
61+
if ((a = new Object()) != null) {
62+
return;
63+
}
64+
}
65+
}
66+
""")
67+
.doTest();
68+
}
69+
70+
@Test
71+
public void multipleAssignments_sure() {
72+
helper
73+
.addSourceLines(
74+
"Test.java",
75+
"""
76+
class Test {
77+
void test() {
78+
Object a;
79+
Object b;
80+
a = b = new Object();
81+
}
82+
}
83+
""")
84+
.doTest();
85+
}
86+
87+
@Test
88+
public void assignmentExpressionUsed_ohNo() {
89+
helper
90+
.addSourceLines(
91+
"Test.java",
92+
"""
93+
class Test {
94+
void test(boolean x) {
95+
// BUG: Diagnostic contains:
96+
test(x = true);
97+
}
98+
}
99+
""")
100+
.doTest();
101+
}
102+
103+
@Test
104+
public void doubleAssignment_removed() {
105+
refactoringHelper
106+
.addInputLines(
107+
"Test.java",
108+
"""
109+
class Test {
110+
void test() {
111+
// BUG: Diagnostic contains:
112+
Object a = a = new Object();
113+
// BUG: Diagnostic contains:
114+
a = a = new Object();
115+
}
116+
}
117+
""")
118+
.addOutputLines(
119+
"Test.java",
120+
"""
121+
class Test {
122+
void test() {
123+
Object a = new Object();
124+
a = new Object();
125+
}
126+
}
127+
""")
128+
.doTest();
129+
}
130+
}

‎docs/bugpattern/AssignmentExpression.md

+40
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
Using the result of an assignment expression can be quite unclear, except for
2+
simple cases where the result is used to initialise another variable, such as
3+
`x = y = 0`.
4+
5+
Consider a common pattern of lazily initialising a field:
6+
7+
```java
8+
class Lazy<T> {
9+
private T t = null;
10+
11+
abstract T create();
12+
13+
public T get() {
14+
if (t != null) {
15+
return t;
16+
}
17+
return t = create();
18+
}
19+
}
20+
```
21+
22+
```java
23+
class Lazy<T> {
24+
private T t = null;
25+
26+
abstract T create();
27+
28+
public T get() {
29+
if (t != null) {
30+
return t;
31+
}
32+
t = create();
33+
return t;
34+
}
35+
}
36+
```
37+
38+
At the cost of another line, it's now clearer what's happening. (Note that
39+
neither the before nor the after are thread-safe; this particular example would
40+
be better served with `Suppliers.memoizing`.)

0 commit comments

Comments
 (0)