Skip to content

Commit 3047300

Browse files
cpovirkError Prone Team
cpovirk
authored and
Error Prone Team
committed
Suggest this == obj instead of super.equals(obj) when the two are equivalent.
(inspired by Marcono1234's google/guava#6780) PiperOrigin-RevId: 573931267
1 parent ed01de0 commit 3047300

3 files changed

Lines changed: 236 additions & 0 deletions

File tree

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

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
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.common.collect.Iterables.getOnlyElement;
20+
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
21+
import static com.google.errorprone.BugPattern.StandardTags.LIKELY_ERROR;
22+
import static com.google.errorprone.matchers.Description.NO_MATCH;
23+
import static com.google.errorprone.util.ASTHelpers.enclosingClass;
24+
import static com.google.errorprone.util.ASTHelpers.getSymbol;
25+
import static com.sun.source.tree.Tree.Kind.BLOCK;
26+
import static com.sun.source.tree.Tree.Kind.IDENTIFIER;
27+
import static com.sun.source.tree.Tree.Kind.MEMBER_SELECT;
28+
import static com.sun.source.tree.Tree.Kind.METHOD;
29+
import static com.sun.source.tree.Tree.Kind.RETURN;
30+
31+
import com.google.errorprone.BugPattern;
32+
import com.google.errorprone.VisitorState;
33+
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
34+
import com.google.errorprone.fixes.SuggestedFix;
35+
import com.google.errorprone.matchers.Description;
36+
import com.sun.source.tree.BlockTree;
37+
import com.sun.source.tree.IdentifierTree;
38+
import com.sun.source.tree.MemberSelectTree;
39+
import com.sun.source.tree.MethodInvocationTree;
40+
41+
/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */
42+
@BugPattern(
43+
summary = "`super.equals(obj)` is equivalent to `this == obj` here",
44+
severity = WARNING,
45+
tags = LIKELY_ERROR)
46+
public class SuperEqualsIsObjectEquals extends BugChecker implements MethodInvocationTreeMatcher {
47+
@Override
48+
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
49+
var methodSelect = tree.getMethodSelect();
50+
if (methodSelect.getKind() != MEMBER_SELECT) {
51+
return NO_MATCH;
52+
}
53+
var memberSelect = (MemberSelectTree) methodSelect;
54+
var expression = memberSelect.getExpression();
55+
if (expression.getKind() == IDENTIFIER
56+
&& ((IdentifierTree) expression).getName().equals(state.getNames()._super)
57+
// We can't use a Matcher because onExactClass suffers from b/130658266.
58+
&& enclosingClass(getSymbol(tree)) == state.getSymtab().objectType.tsym
59+
&& memberSelect.getIdentifier().equals(state.getNames().equals)
60+
/*
61+
* We ignore an override that is merely `return super.equals(obj)`. Such an override is less
62+
* likely to be a bug because it may exist for the purpose of adding Javadoc.
63+
*
64+
* TODO(cpovirk): Consider flagging even that if the method does *not* have Javadoc.
65+
*/
66+
&& !methodBodyIsOnlyReturnSuperEquals(state)) {
67+
/*
68+
* There will often be better fixes than this, some of which would change behavior. But let's
69+
* at least suggest the simple thing that's always equivalent.
70+
*/
71+
return describeMatch(
72+
tree,
73+
SuggestedFix.replace(
74+
tree, "this == " + state.getSourceForNode(getOnlyElement(tree.getArguments()))));
75+
}
76+
return NO_MATCH;
77+
}
78+
79+
private static boolean methodBodyIsOnlyReturnSuperEquals(VisitorState state) {
80+
var parentPath = state.getPath().getParentPath();
81+
if (parentPath.getLeaf().getKind() != RETURN) {
82+
return false;
83+
}
84+
var grandparentPath = parentPath.getParentPath();
85+
var grandparent = grandparentPath.getLeaf();
86+
if (grandparent.getKind() != BLOCK) {
87+
return false;
88+
}
89+
if (((BlockTree) grandparent).getStatements().size() > 1) {
90+
return false;
91+
}
92+
var greatGrandparent = grandparentPath.getParentPath().getLeaf();
93+
if (greatGrandparent.getKind() != METHOD) {
94+
return false;
95+
}
96+
return true;
97+
}
98+
}

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -351,6 +351,7 @@
351351
import com.google.errorprone.bugpatterns.StringSplitter;
352352
import com.google.errorprone.bugpatterns.StronglyTypeByteString;
353353
import com.google.errorprone.bugpatterns.SubstringOfZero;
354+
import com.google.errorprone.bugpatterns.SuperEqualsIsObjectEquals;
354355
import com.google.errorprone.bugpatterns.SuppressWarningsDeprecated;
355356
import com.google.errorprone.bugpatterns.SuppressWarningsWithoutExplanation;
356357
import com.google.errorprone.bugpatterns.SwigMemoryLeak;
@@ -1039,6 +1040,7 @@ public static ScannerSupplier warningChecks() {
10391040
StringCharset.class,
10401041
StringSplitter.class,
10411042
Suggester.class,
1043+
SuperEqualsIsObjectEquals.class,
10421044
SwigMemoryLeak.class,
10431045
SynchronizeOnNonFinalField.class,
10441046
ThreadJoinLoop.class,

‎core/src/test/java/com/google/errorprone/bugpatterns/SuperEqualsIsObjectEqualsTest.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.CompilationTestHelper;
21+
import org.junit.Test;
22+
import org.junit.runner.RunWith;
23+
import org.junit.runners.JUnit4;
24+
25+
/** {@link SuperEqualsIsObjectEquals}Test */
26+
@RunWith(JUnit4.class)
27+
public class SuperEqualsIsObjectEqualsTest {
28+
@Test
29+
public void positive() {
30+
helper()
31+
.addSourceLines(
32+
"Foo.java",
33+
"class Foo {",
34+
" int i;",
35+
" @Override",
36+
" public boolean equals(Object obj) {",
37+
" if (obj instanceof Foo) {",
38+
" return i == ((Foo) obj).i;",
39+
" }",
40+
" // BUG: Diagnostic contains: ",
41+
" return super.equals(obj);",
42+
" }",
43+
"}")
44+
.doTest();
45+
}
46+
47+
@Test
48+
public void positiveOtherSupertypeWithoutEquals() {
49+
helper()
50+
.addSourceLines(
51+
"Foo.java",
52+
"class Foo extends Exception {",
53+
" int i;",
54+
" @Override",
55+
" public boolean equals(Object obj) {",
56+
" if (obj instanceof Foo) {",
57+
" return i == ((Foo) obj).i;",
58+
" }",
59+
" // BUG: Diagnostic contains: ",
60+
" return super.equals(obj);",
61+
" }",
62+
"}")
63+
.doTest();
64+
}
65+
66+
@Test
67+
public void negativeOtherSupertypeWithEquals() {
68+
helper()
69+
.addSourceLines(
70+
"Foo.java",
71+
"import java.util.AbstractSet;",
72+
"abstract class Foo extends AbstractSet<String> {",
73+
" int i;",
74+
" @Override",
75+
" public boolean equals(Object obj) {",
76+
" if (obj instanceof Foo) {",
77+
" return i == ((Foo) obj).i;",
78+
" }",
79+
" return super.equals(obj);",
80+
" }",
81+
"}")
82+
.doTest();
83+
}
84+
85+
@Test
86+
public void negativePureSuperDelegation() {
87+
helper()
88+
.addSourceLines(
89+
"Foo.java",
90+
"class Foo {",
91+
" @Override",
92+
" public boolean equals(Object obj) {",
93+
" return super.equals(obj);",
94+
" }",
95+
"}")
96+
.doTest();
97+
}
98+
99+
@Test
100+
public void refactoring() {
101+
refactoringHelper()
102+
.addInputLines(
103+
"Foo.java",
104+
"class Foo {",
105+
" int i;",
106+
" @Override",
107+
" public boolean equals(Object obj) {",
108+
" if (obj instanceof Foo) {",
109+
" return i == ((Foo) obj).i;",
110+
" }",
111+
" return super.equals(obj);",
112+
" }",
113+
"}")
114+
.addOutputLines(
115+
"Foo.java",
116+
"class Foo {",
117+
" int i;",
118+
" @Override",
119+
" public boolean equals(Object obj) {",
120+
" if (obj instanceof Foo) {",
121+
" return i == ((Foo) obj).i;",
122+
" }",
123+
" return this == obj;",
124+
" }",
125+
"}")
126+
.doTest();
127+
}
128+
129+
private CompilationTestHelper helper() {
130+
return CompilationTestHelper.newInstance(SuperEqualsIsObjectEquals.class, getClass());
131+
}
132+
133+
private BugCheckerRefactoringTestHelper refactoringHelper() {
134+
return BugCheckerRefactoringTestHelper.newInstance(SuperEqualsIsObjectEquals.class, getClass());
135+
}
136+
}

0 commit comments

Comments
 (0)