Skip to content

Commit a57309b

Browse files
graememorganError Prone Team
graememorgan
authored and
Error Prone Team
committed
Add a check to reverse Yoda conditions.
PiperOrigin-RevId: 500187575
1 parent 181f991 commit a57309b

File tree

3 files changed

+251
-1
lines changed

3 files changed

+251
-1
lines changed

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

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
/*
2+
* Copyright 2023 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.BugPattern.SeverityLevel.WARNING;
20+
import static com.google.errorprone.fixes.SuggestedFixes.qualifyType;
21+
import static com.google.errorprone.matchers.Description.NO_MATCH;
22+
import static com.google.errorprone.matchers.Matchers.instanceEqualsInvocation;
23+
import static com.google.errorprone.matchers.Matchers.staticEqualsInvocation;
24+
import static com.google.errorprone.util.ASTHelpers.constValue;
25+
import static com.google.errorprone.util.ASTHelpers.getReceiver;
26+
import static com.google.errorprone.util.ASTHelpers.getSymbol;
27+
import static java.lang.String.format;
28+
29+
import com.google.errorprone.BugPattern;
30+
import com.google.errorprone.VisitorState;
31+
import com.google.errorprone.bugpatterns.BugChecker.BinaryTreeMatcher;
32+
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
33+
import com.google.errorprone.fixes.SuggestedFix;
34+
import com.google.errorprone.matchers.Description;
35+
import com.sun.source.tree.BinaryTree;
36+
import com.sun.source.tree.MethodInvocationTree;
37+
import com.sun.source.tree.Tree;
38+
import com.sun.tools.javac.code.Symbol.VarSymbol;
39+
import java.util.Objects;
40+
41+
/** See the summary. */
42+
@BugPattern(
43+
summary = "The non-constant portion of an equals check generally comes first.",
44+
severity = WARNING)
45+
public final class YodaCondition extends BugChecker
46+
implements BinaryTreeMatcher, MethodInvocationTreeMatcher {
47+
@Override
48+
public Description matchBinary(BinaryTree tree, VisitorState state) {
49+
switch (tree.getKind()) {
50+
case EQUAL_TO:
51+
case NOT_EQUAL_TO:
52+
return fix(
53+
tree,
54+
tree.getLeftOperand(),
55+
tree.getRightOperand(),
56+
/* provideNullSafeFix= */ false,
57+
state);
58+
default:
59+
return NO_MATCH;
60+
}
61+
}
62+
63+
@Override
64+
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
65+
if (staticEqualsInvocation().matches(tree, state)) {
66+
return fix(
67+
tree,
68+
tree.getArguments().get(0),
69+
tree.getArguments().get(1),
70+
/* provideNullSafeFix= */ false,
71+
state);
72+
}
73+
if (instanceEqualsInvocation().matches(tree, state)) {
74+
return fix(
75+
tree,
76+
getReceiver(tree),
77+
tree.getArguments().get(0),
78+
/* provideNullSafeFix= */ true,
79+
state);
80+
}
81+
return NO_MATCH;
82+
}
83+
84+
private Description fix(
85+
Tree tree, Tree lhs, Tree rhs, boolean provideNullSafeFix, VisitorState state) {
86+
if (seemsConstant(lhs) && !seemsConstant(rhs)) {
87+
var description = buildDescription(lhs).addFix(SuggestedFix.swap(lhs, rhs));
88+
if (provideNullSafeFix) {
89+
var fix = SuggestedFix.builder().setShortDescription("null-safe fix");
90+
description.addFix(
91+
fix.replace(
92+
tree,
93+
format(
94+
"%s.equals(%s, %s)",
95+
qualifyType(state, fix, Objects.class.getName()),
96+
state.getSourceForNode(rhs),
97+
state.getSourceForNode(lhs)))
98+
.build());
99+
}
100+
return description.build();
101+
}
102+
return NO_MATCH;
103+
}
104+
105+
private static boolean seemsConstant(Tree tree) {
106+
if (constValue(tree) != null) {
107+
return true;
108+
}
109+
var symbol = getSymbol(tree);
110+
return symbol instanceof VarSymbol && symbol.isEnum();
111+
}
112+
}

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -412,6 +412,7 @@
412412
import com.google.errorprone.bugpatterns.WithSignatureDiscouraged;
413413
import com.google.errorprone.bugpatterns.WrongOneof;
414414
import com.google.errorprone.bugpatterns.XorPower;
415+
import com.google.errorprone.bugpatterns.YodaCondition;
415416
import com.google.errorprone.bugpatterns.android.BinderIdentityRestoredDangerously;
416417
import com.google.errorprone.bugpatterns.android.BundleDeserializationCast;
417418
import com.google.errorprone.bugpatterns.android.FragmentInjection;
@@ -1164,7 +1165,8 @@ public static ScannerSupplier errorChecks() {
11641165
VarChecker.class,
11651166
Varifier.class,
11661167
VoidMissingNullable.class,
1167-
WildcardImport.class
1168+
WildcardImport.class,
1169+
YodaCondition.class
11681170
// keep-sorted end
11691171
);
11701172

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

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
/*
2+
* Copyright 2023 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.BugCheckerRefactoringTestHelper.FixChoosers;
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 YodaConditionTest {
27+
private final BugCheckerRefactoringTestHelper refactoring =
28+
BugCheckerRefactoringTestHelper.newInstance(YodaCondition.class, getClass());
29+
30+
@Test
31+
public void primitive() {
32+
refactoring
33+
.addInputLines(
34+
"Test.java",
35+
"class Test {",
36+
" boolean yoda(int a) {",
37+
" return 4 == a;",
38+
" }",
39+
" boolean notYoda(int a) {",
40+
" return a == 4;",
41+
" }",
42+
"}")
43+
.addOutputLines(
44+
"Test.java",
45+
"class Test {",
46+
" boolean yoda(int a) {",
47+
" return a == 4;",
48+
" }",
49+
" boolean notYoda(int a) {",
50+
" return a == 4;",
51+
" }",
52+
"}")
53+
.doTest();
54+
}
55+
56+
@Test
57+
public void enums() {
58+
refactoring
59+
.addInputLines(
60+
"E.java",
61+
"enum E {",
62+
" A, B;",
63+
" boolean foo(E e) {",
64+
" return this == e;",
65+
" }",
66+
"}")
67+
.expectUnchanged()
68+
.addInputLines(
69+
"Test.java",
70+
"class Test {",
71+
" boolean yoda(E a) {",
72+
" return E.A == a;",
73+
" }",
74+
" boolean notYoda(E a) {",
75+
" return a == E.A;",
76+
" }",
77+
"}")
78+
.addOutputLines(
79+
"Test.java",
80+
"class Test {",
81+
" boolean yoda(E a) {",
82+
" return a == E.A;",
83+
" }",
84+
" boolean notYoda(E a) {",
85+
" return a == E.A;",
86+
" }",
87+
"}")
88+
.doTest();
89+
}
90+
91+
@Test
92+
public void nullUnsafeFix() {
93+
refactoring
94+
.addInputLines("E.java", "enum E {A, B}")
95+
.expectUnchanged()
96+
.addInputLines(
97+
"Test.java",
98+
"class Test {",
99+
" boolean yoda(E a) {",
100+
" return E.A.equals(a);",
101+
" }",
102+
"}")
103+
.addOutputLines(
104+
"Test.java",
105+
"class Test {",
106+
" boolean yoda(E a) {",
107+
" return a.equals(E.A);",
108+
" }",
109+
"}")
110+
.doTest();
111+
}
112+
113+
@Test
114+
public void nullSafeFix() {
115+
refactoring
116+
.addInputLines("E.java", "enum E {A, B}")
117+
.expectUnchanged()
118+
.addInputLines(
119+
"Test.java",
120+
"class Test {",
121+
" boolean yoda(E a) {",
122+
" return E.A.equals(a);",
123+
" }",
124+
"}")
125+
.addOutputLines(
126+
"Test.java",
127+
"import java.util.Objects;",
128+
"class Test {",
129+
" boolean yoda(E a) {",
130+
" return Objects.equals(a, E.A);",
131+
" }",
132+
"}")
133+
.setFixChooser(FixChoosers.SECOND)
134+
.doTest();
135+
}
136+
}

0 commit comments

Comments
 (0)