From 3047300568e312c574a60db608622730abcce9c6 Mon Sep 17 00:00:00 2001 From: cpovirk Date: Mon, 16 Oct 2023 14:30:45 -0700 Subject: [PATCH] Suggest `this == obj` instead of `super.equals(obj)` when the two are equivalent. (inspired by Marcono1234's https://github.com/google/guava/issues/6780) PiperOrigin-RevId: 573931267 --- .../SuperEqualsIsObjectEquals.java | 98 +++++++++++++ .../scanner/BuiltInCheckerSuppliers.java | 2 + .../SuperEqualsIsObjectEqualsTest.java | 136 ++++++++++++++++++ 3 files changed, 236 insertions(+) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/SuperEqualsIsObjectEquals.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/SuperEqualsIsObjectEqualsTest.java diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/SuperEqualsIsObjectEquals.java b/core/src/main/java/com/google/errorprone/bugpatterns/SuperEqualsIsObjectEquals.java new file mode 100644 index 00000000000..b87e717ea66 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/SuperEqualsIsObjectEquals.java @@ -0,0 +1,98 @@ +/* + * Copyright 2023 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.common.collect.Iterables.getOnlyElement; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.BugPattern.StandardTags.LIKELY_ERROR; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.util.ASTHelpers.enclosingClass; +import static com.google.errorprone.util.ASTHelpers.getSymbol; +import static com.sun.source.tree.Tree.Kind.BLOCK; +import static com.sun.source.tree.Tree.Kind.IDENTIFIER; +import static com.sun.source.tree.Tree.Kind.MEMBER_SELECT; +import static com.sun.source.tree.Tree.Kind.METHOD; +import static com.sun.source.tree.Tree.Kind.RETURN; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.sun.source.tree.BlockTree; +import com.sun.source.tree.IdentifierTree; +import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.MethodInvocationTree; + +/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ +@BugPattern( + summary = "`super.equals(obj)` is equivalent to `this == obj` here", + severity = WARNING, + tags = LIKELY_ERROR) +public class SuperEqualsIsObjectEquals extends BugChecker implements MethodInvocationTreeMatcher { + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + var methodSelect = tree.getMethodSelect(); + if (methodSelect.getKind() != MEMBER_SELECT) { + return NO_MATCH; + } + var memberSelect = (MemberSelectTree) methodSelect; + var expression = memberSelect.getExpression(); + if (expression.getKind() == IDENTIFIER + && ((IdentifierTree) expression).getName().equals(state.getNames()._super) + // We can't use a Matcher because onExactClass suffers from b/130658266. + && enclosingClass(getSymbol(tree)) == state.getSymtab().objectType.tsym + && memberSelect.getIdentifier().equals(state.getNames().equals) + /* + * We ignore an override that is merely `return super.equals(obj)`. Such an override is less + * likely to be a bug because it may exist for the purpose of adding Javadoc. + * + * TODO(cpovirk): Consider flagging even that if the method does *not* have Javadoc. + */ + && !methodBodyIsOnlyReturnSuperEquals(state)) { + /* + * There will often be better fixes than this, some of which would change behavior. But let's + * at least suggest the simple thing that's always equivalent. + */ + return describeMatch( + tree, + SuggestedFix.replace( + tree, "this == " + state.getSourceForNode(getOnlyElement(tree.getArguments())))); + } + return NO_MATCH; + } + + private static boolean methodBodyIsOnlyReturnSuperEquals(VisitorState state) { + var parentPath = state.getPath().getParentPath(); + if (parentPath.getLeaf().getKind() != RETURN) { + return false; + } + var grandparentPath = parentPath.getParentPath(); + var grandparent = grandparentPath.getLeaf(); + if (grandparent.getKind() != BLOCK) { + return false; + } + if (((BlockTree) grandparent).getStatements().size() > 1) { + return false; + } + var greatGrandparent = grandparentPath.getParentPath().getLeaf(); + if (greatGrandparent.getKind() != METHOD) { + return false; + } + return true; + } +} diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index 4fdb6d2f5f9..672e0a77d6c 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -351,6 +351,7 @@ import com.google.errorprone.bugpatterns.StringSplitter; import com.google.errorprone.bugpatterns.StronglyTypeByteString; import com.google.errorprone.bugpatterns.SubstringOfZero; +import com.google.errorprone.bugpatterns.SuperEqualsIsObjectEquals; import com.google.errorprone.bugpatterns.SuppressWarningsDeprecated; import com.google.errorprone.bugpatterns.SuppressWarningsWithoutExplanation; import com.google.errorprone.bugpatterns.SwigMemoryLeak; @@ -1039,6 +1040,7 @@ public static ScannerSupplier warningChecks() { StringCharset.class, StringSplitter.class, Suggester.class, + SuperEqualsIsObjectEquals.class, SwigMemoryLeak.class, SynchronizeOnNonFinalField.class, ThreadJoinLoop.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/SuperEqualsIsObjectEqualsTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/SuperEqualsIsObjectEqualsTest.java new file mode 100644 index 00000000000..49d2625e5a8 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/SuperEqualsIsObjectEqualsTest.java @@ -0,0 +1,136 @@ +/* + * Copyright 2023 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; + +/** {@link SuperEqualsIsObjectEquals}Test */ +@RunWith(JUnit4.class) +public class SuperEqualsIsObjectEqualsTest { + @Test + public void positive() { + helper() + .addSourceLines( + "Foo.java", + "class Foo {", + " int i;", + " @Override", + " public boolean equals(Object obj) {", + " if (obj instanceof Foo) {", + " return i == ((Foo) obj).i;", + " }", + " // BUG: Diagnostic contains: ", + " return super.equals(obj);", + " }", + "}") + .doTest(); + } + + @Test + public void positiveOtherSupertypeWithoutEquals() { + helper() + .addSourceLines( + "Foo.java", + "class Foo extends Exception {", + " int i;", + " @Override", + " public boolean equals(Object obj) {", + " if (obj instanceof Foo) {", + " return i == ((Foo) obj).i;", + " }", + " // BUG: Diagnostic contains: ", + " return super.equals(obj);", + " }", + "}") + .doTest(); + } + + @Test + public void negativeOtherSupertypeWithEquals() { + helper() + .addSourceLines( + "Foo.java", + "import java.util.AbstractSet;", + "abstract class Foo extends AbstractSet {", + " int i;", + " @Override", + " public boolean equals(Object obj) {", + " if (obj instanceof Foo) {", + " return i == ((Foo) obj).i;", + " }", + " return super.equals(obj);", + " }", + "}") + .doTest(); + } + + @Test + public void negativePureSuperDelegation() { + helper() + .addSourceLines( + "Foo.java", + "class Foo {", + " @Override", + " public boolean equals(Object obj) {", + " return super.equals(obj);", + " }", + "}") + .doTest(); + } + + @Test + public void refactoring() { + refactoringHelper() + .addInputLines( + "Foo.java", + "class Foo {", + " int i;", + " @Override", + " public boolean equals(Object obj) {", + " if (obj instanceof Foo) {", + " return i == ((Foo) obj).i;", + " }", + " return super.equals(obj);", + " }", + "}") + .addOutputLines( + "Foo.java", + "class Foo {", + " int i;", + " @Override", + " public boolean equals(Object obj) {", + " if (obj instanceof Foo) {", + " return i == ((Foo) obj).i;", + " }", + " return this == obj;", + " }", + "}") + .doTest(); + } + + private CompilationTestHelper helper() { + return CompilationTestHelper.newInstance(SuperEqualsIsObjectEquals.class, getClass()); + } + + private BugCheckerRefactoringTestHelper refactoringHelper() { + return BugCheckerRefactoringTestHelper.newInstance(SuperEqualsIsObjectEquals.class, getClass()); + } +}