Merge pull request #4540 from LynnBroe:issue4457

[java] Fix #4457: false negative about OverrideBothEqualsAndHashcode #4540

docs/pages/release_notes.md

@@ -39,6 +39,12 @@ for all.</p>
 This section lists the most important changes from the last release candidate.
 The remaining section describes the complete release notes for 7.0.0.
 
+#### Fixed issues
+
+* java-errorprone
+  * [#4457](https://github.com/pmd/pmd/issues/4457): \[java] OverrideBothEqualsAndHashcode: false negative with anonymous classes
+  * [#4546](https://github.com/pmd/pmd/issues/4546): \[java] OverrideBothEqualsAndHashCode ignores records
+
 #### API Changes
 
 * The following previously deprecated classes have been removed:
@@ -48,6 +54,10 @@ The remaining section describes the complete release notes for 7.0.0.
     * `net.sourceforge.pmd.cli.PMDParameters`
     * `net.sourceforge.pmd.cli.PmdParametersParseResult`
 
+#### External Contributions
+
+* [#4540](https://github.com/pmd/pmd/pull/4540): \[java] Fix #4457: false negative about OverrideBothEqualsAndHashcode - [AnnaDev](https://github.com/LynnBroe) (@LynnBroe)
+
 ### 🚀 Major Features and Enhancements
 
 #### New official logo
@@ -501,10 +511,12 @@ Language specific fixes:
     * [#3843](https://github.com/pmd/pmd/issues/3843): \[java] UseEqualsToCompareStrings should consider return type
     * [#4356](https://github.com/pmd/pmd/pull/4356):   \[java] Fix NPE in CloseResourceRule
     * [#4449](https://github.com/pmd/pmd/issues/4449): \[java] AvoidAccessibilityAlteration: Possible false positive in AvoidAccessibilityAlteration rule when using Lambda expression
+    * [#4457](https://github.com/pmd/pmd/issues/4457): \[java] OverrideBothEqualsAndHashcode: false negative with anonymous classes
     * [#4493](https://github.com/pmd/pmd/issues/4493): \[java] MissingStaticMethodInNonInstantiatableClass: false-positive about @<!-- -->Inject
     * [#4505](https://github.com/pmd/pmd/issues/4505): \[java] ImplicitSwitchFallThrough NPE in PMD 7.0.0-rc1
     * [#4513](https://github.com/pmd/pmd/issues/4513): \[java] UselessOperationOnImmutable various false negatives with String
     * [#4514](https://github.com/pmd/pmd/issues/4514): \[java] AvoidLiteralsInIfCondition false positive and negative for String literals when ignoreExpressions=true
+    * [#4546](https://github.com/pmd/pmd/issues/4546): \[java] OverrideBothEqualsAndHashCode ignores records
 * java-multithreading
     * [#2537](https://github.com/pmd/pmd/issues/2537): \[java] DontCallThreadRun can't detect the case that call run() in `this.run()`
     * [#2538](https://github.com/pmd/pmd/issues/2538): \[java] DontCallThreadRun can't detect the case that call run() in `foo.bar.run()`
@@ -548,6 +560,7 @@ Language specific fixes:
 * [#4494](https://github.com/pmd/pmd/pull/4494): \[java] Fix #4487: A false-positive about UnnecessaryConstructor and @<!-- -->Inject and @<!-- -->Autowired - [AnnaDev](https://github.com/LynnBroe) (@LynnBroe)
 * [#4495](https://github.com/pmd/pmd/pull/4495): \[java] Fix #4493: false-positive about MissingStaticMethodInNonInstantiatableClass and @<!-- -->Inject - [AnnaDev](https://github.com/LynnBroe) (@LynnBroe)
 * [#4520](https://github.com/pmd/pmd/pull/4520): \[doc] Fix typo: missing closing quotation mark after CPD-END - [João Dinis Ferreira](https://github.com/joaodinissf) (@joaodinissf)
+* [#4540](https://github.com/pmd/pmd/pull/4540): \[java] Fix #4457: false negative about OverrideBothEqualsAndHashcode - [AnnaDev](https://github.com/LynnBroe) (@LynnBroe)
 
 ### 📈 Stats
 * 4557 commits

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/JavaAstUtils.java

@@ -676,6 +676,19 @@ public final class JavaAstUtils {
             && !node.isStatic();
     }
 
+    public static boolean isEqualsMethod(ASTMethodDeclaration node) {
+        return "equals".equals(node.getName())
+            && node.getArity() == 1
+            && node.getFormalParameters().get(0).getTypeMirror().isTop()
+            && !node.isStatic();
+    }
+
+    public static boolean isHashCodeMethod(ASTMethodDeclaration node) {
+        return "hashCode".equals(node.getName())
+            && node.getArity() == 0
+            && !node.isStatic();
+    }
+
     public static boolean isArrayLengthFieldAccess(ASTExpression node) {
         if (node instanceof ASTFieldAccess) {
             ASTFieldAccess field = (ASTFieldAccess) node;

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/OverrideBothEqualsAndHashcodeRule.java

@@ -4,70 +4,68 @@
 
 package net.sourceforge.pmd.lang.java.rule.errorprone;
 
-import net.sourceforge.pmd.lang.ast.Node;
+import net.sourceforge.pmd.lang.java.ast.ASTAnonymousClassDeclaration;
+import net.sourceforge.pmd.lang.java.ast.ASTAnyTypeDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration;
-import net.sourceforge.pmd.lang.java.ast.ASTFormalParameter;
-import net.sourceforge.pmd.lang.java.ast.ASTImplementsList;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
-import net.sourceforge.pmd.lang.java.ast.TypeNode;
+import net.sourceforge.pmd.lang.java.ast.ASTRecordDeclaration;
-import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
+import net.sourceforge.pmd.lang.java.ast.internal.JavaAstUtils;
+import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
 import net.sourceforge.pmd.lang.java.types.TypeTestUtil;
 
-public class OverrideBothEqualsAndHashcodeRule extends AbstractJavaRule {
+public class OverrideBothEqualsAndHashcodeRule extends AbstractJavaRulechainRule {
 
-    private boolean implementsComparable = false;
+    public OverrideBothEqualsAndHashcodeRule() {
+        super(ASTClassOrInterfaceDeclaration.class,
+              ASTRecordDeclaration.class,
+              ASTAnonymousClassDeclaration.class);
+    }
 
-    private boolean containsEquals = false;
+    private void visitTypeDecl(ASTAnyTypeDeclaration node, Object data) {
+        if (TypeTestUtil.isA(Comparable.class, node)) {
+            return;
+        }
+        ASTMethodDeclaration equalsMethod = null;
+        ASTMethodDeclaration hashCodeMethod = null;
+        for (ASTMethodDeclaration m : node.getDeclarations(ASTMethodDeclaration.class)) {
+            if (JavaAstUtils.isEqualsMethod(m)) {
+                equalsMethod = m;
+                if (hashCodeMethod != null) {
+                    break; // shortcut
+                }
+            } else if (JavaAstUtils.isHashCodeMethod(m)) {
+                hashCodeMethod = m;
+                if (equalsMethod != null) {
+                    break; // shortcut
+                }
+            }
+        }
 
-    private boolean containsHashCode = false;
+        if (hashCodeMethod != null ^ equalsMethod != null) {
+            ASTMethodDeclaration nonNullNode =
+                equalsMethod == null ? hashCodeMethod : equalsMethod;
+            asCtx(data).addViolation(nonNullNode);
+        }
+    }
 
-    private Node nodeFound = null;
+    @Override
+    public Object visit(ASTAnonymousClassDeclaration node, Object data) {
+        visitTypeDecl(node, data);
+        return null;
+    }
 
     @Override
     public Object visit(ASTClassOrInterfaceDeclaration node, Object data) {
         if (node.isInterface()) {
-            return data;
+            return null;
         }
-        super.visit(node, data);
+        visitTypeDecl(node, data);
-        if (!implementsComparable && containsEquals ^ containsHashCode) {
+        return null;
-            if (nodeFound == null) {
-                nodeFound = node;
-            }
-            addViolation(data, nodeFound);
-        }
-        implementsComparable = false;
-        containsEquals = false;
-        containsHashCode = false;
-        nodeFound = null;
-        return data;
     }
 
     @Override
-    public Object visit(ASTImplementsList node, Object data) {
+    public Object visit(ASTRecordDeclaration node, Object data) {
-        implementsComparable = node.children().filter(child -> TypeTestUtil.isA(Comparable.class, (TypeNode) child)).nonEmpty();
+        visitTypeDecl(node, data);
-        return super.visit(node, data);
+        return null;
-    }
-
-    @Override
-    public Object visit(ASTMethodDeclaration node, Object data) {
-        if (implementsComparable) {
-            return data;
-        }
-
-        int formalParamsCount = node.getFormalParameters().size();
-        ASTFormalParameter formalParam = null;
-        if (formalParamsCount > 0) {
-            formalParam = node.getFormalParameters().get(0);
-        }
-
-        if (formalParamsCount == 0 && "hashCode".equals(node.getName())) {
-            containsHashCode = true;
-            nodeFound = node;
-        } else if (formalParamsCount == 1 && "equals".equals(node.getName())
-                && TypeTestUtil.isExactlyA(Object.class, formalParam)) {
-            containsEquals = true;
-            nodeFound = node;
-        }
-        return super.visit(node, data);
     }
 }

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/OverrideBothEqualsAndHashcode.xml

@@ -180,4 +180,34 @@ public class Foo implements Runnable {
 }
         ]]></code>
     </test-code>
+
+    <test-code>
+        <description>[java] OverrideBothEqualsAndHashcode: false negative with anonymous classes #4457</description>
+        <expected-problems>2</expected-problems>
+        <code><![CDATA[
+public class Foo {
+  public class B {
+      Object o = new Object() {
+          public boolean equals(Object other) {   // report no warning
+              return false;
+          }
+      };
+      public int hashCode() {
+          return 1;
+      }
+  }
+}
+        ]]></code>
+    </test-code>
+    <test-code>
+        <description>[java] OverrideBothEqualsAndHashCode ignores records #4546</description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+            public record Foo(int x) {
+                    public int hashCode() {
+                        return 1;
+                    }
+            }
+            ]]></code>
+    </test-code>
 </test-data>