Merge branch 'pr/3485' into 7.0.x

.ci/files/all-java.xml

@@ -259,7 +259,7 @@
     <!-- <rule ref="category/java/errorprone.xml/UnnecessaryBooleanAssertion"/> -->
     <!-- <rule ref="category/java/errorprone.xml/UnnecessaryCaseChange"/> -->
     <!-- <rule ref="category/java/errorprone.xml/UnnecessaryConversionTemporary"/> -->
-    <!-- <rule ref="category/java/errorprone.xml/UnusedNullCheckInEquals"/> -->
+    <rule ref="category/java/errorprone.xml/UnusedNullCheckInEquals"/>
     <!-- <rule ref="category/java/errorprone.xml/UseCorrectExceptionLogging"/> -->
     <rule ref="category/java/errorprone.xml/UseEqualsToCompareStrings"/>
     <rule ref="category/java/errorprone.xml/UseLocaleWithCaseConversions"/>

pmd-java/src/main/resources/category/java/errorprone.xml

@@ -3188,20 +3188,19 @@ public String convert(int x) {
           class="net.sourceforge.pmd.lang.rule.XPathRule"
           externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_errorprone.html#unusednullcheckinequals">
         <description>
-After checking an object reference for null, you should invoke equals() on that object rather than passing it to another object's equals() method.
+After checking an object reference for null, you should invoke equals() on that object rather than passing
+it to another object's equals() method.
         </description>
         <priority>3</priority>
         <properties>
             <property name="xpath">
                 <value>
 <![CDATA[
-(//PrimaryPrefix[ends-with(Name/@Image, '.equals') and Name/@Image != 'Arrays.equals'] | //PrimarySuffix[@Image='equals' and not(../PrimaryPrefix/Literal)])
- /following-sibling::PrimarySuffix/Arguments/ArgumentList/Expression
- /PrimaryExpression[not(PrimarySuffix)]/PrimaryPrefix
- /Name[@Image = ./../../../../../../../../../../Expression/ConditionalAndExpression
- /EqualityExpression[@Image="!=" and not(./preceding-sibling::*) and
- ./PrimaryExpression/PrimaryPrefix/Literal/NullLiteral]
-  /PrimaryExpression/PrimaryPrefix/Name/@Image]
+//InfixExpression[@Operator = '&&']
+  /MethodCall[pmd-java:matchesSig("java.lang.Object#equals(java.lang.Object)")]
+             [not(StringLiteral)]
+             [not(VariableAccess[@CompileTimeConstant = true()])]
+             [ArgumentList/VariableAccess/@Name = ..//InfixExpression[@Operator = '!='][NullLiteral]/VariableAccess/@Name]
 ]]>
                 </value>
             </property>

pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/UnusedNullCheckInEqualsTest.java

@@ -6,7 +6,6 @@ package net.sourceforge.pmd.lang.java.rule.errorprone;
 
 import net.sourceforge.pmd.testframework.PmdRuleTst;
 
-@org.junit.Ignore("Rule has not been updated yet")
 public class UnusedNullCheckInEqualsTest extends PmdRuleTst {
     // no additional unit tests
 }

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

@@ -7,10 +7,12 @@
     <test-code>
         <description>failure case</description>
         <expected-problems>1</expected-problems>
+        <expected-linenumbers>4</expected-linenumbers>
         <code><![CDATA[
 public class Foo {
-    public void bar() {
-        if (x != null && foo.getBar().equals(x)) {}
+    public Foo getBar(Foo x) {
+        Foo foo = new Foo();
+        if (x != null && foo.getBar(foo).equals(x)) {}
     }
 }
         ]]></code>
@@ -21,7 +23,8 @@ public class Foo {
         <expected-problems>0</expected-problems>
         <code><![CDATA[
 public class Foo {
-    public void bar() {
+    public void bar(Foo x, Foo y) {
+        Foo foo = new Foo();
         if (x != null && foo.equals(y)) {}
     }
 }
@@ -33,7 +36,7 @@ public class Foo {
         <expected-problems>0</expected-problems>
         <code><![CDATA[
 public class Foo {
-    public void bar() {
+    public void bar(Foo x, Foo y) {
         if (x != null && x.equals(y)) {}
     }
 }
@@ -45,7 +48,7 @@ public class Foo {
         <expected-problems>0</expected-problems>
         <code><![CDATA[
 public class Foo {
-    public void bar() {
+    public void bar(String x, String y) {
         if (x != null && "Foo".equals(y)) {}
         if (y.equals(x)) {}
     }
@@ -58,8 +61,10 @@ public class Foo {
         <expected-problems>0</expected-problems>
         <code><![CDATA[
 public class Foo {
-    public void bar() {
-        if (c != null && A.b(c).equals(d)) {}
+    public static Foo A = new Foo();
+    public void bar(String c) {
+        String d = "bar";
+        if (c != null && A.bar(c).equals(d)) {}
     }
 }
         ]]></code>
@@ -70,10 +75,11 @@ public class Foo {
         <expected-problems>0</expected-problems>
         <code><![CDATA[
 public class Foo {
-    public void bar() {
+    public void bar(Foo[] o1, Foo[] o2) {
         if (o1 != null && o1[0] != null && o2[0].getName() != null && o2[0].getName().equals(o1[0].getName())) { }
         if (o1 != null && o2.equals(o1.getName())) { }
     }
+    public String getName() { return ""; }
 }
         ]]></code>
     </test-code>
@@ -81,9 +87,10 @@ public class Foo {
     <test-code>
         <description>shouldn't this fail? Yes, it should. Fixed it, so that method calls to equals on variables are considered, too.</description>
         <expected-problems>1</expected-problems>
+        <expected-linenumbers>3</expected-linenumbers>
         <code><![CDATA[
 public class Foo {
-    public void bar() {
+    public void bar(String x, String y) {
         if (x != null && y.equals(x)) {}
     }
 }
@@ -94,10 +101,59 @@ public class Foo {
         <description>Arrays can't be compared directly but with Arrays.equals().</description>
         <expected-problems>0</expected-problems>
         <code><![CDATA[
+import java.util.Arrays;
+
 public class Foo {
     private static boolean isSame(Object[] a1, Object[] a2) {
         return a1 == a2 || (a1 != null && a2 != null && Arrays.equals(a1, a2));
     }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>False negative with another condition in the middle</description>
+        <expected-problems>3</expected-problems>
+        <expected-linenumbers>3,7,14</expected-linenumbers>
+        <code><![CDATA[
+public class Foo {
+    private boolean matches(String thatString, Object other, Class<?> otherClass) {
+        return thatString != null && otherClass == thatString.getClass() && other.equals(thatString);
+    }
+
+    private boolean matches_if(String thatString, Object other, Class<?> otherClass) {
+        if (thatString != null && otherClass == thatString.getClass() && other.equals(thatString)) {
+            return true;
+        }
+        return false;
+    }
+
+    private boolean matches_var(String thatString, Object other, Class<?> otherClass) {
+        boolean result = thatString != null && otherClass == thatString.getClass() && other.equals(thatString);
+        return result;
+    }
+
+    private boolean matchesCorrected(String thatString, Object other, Class<?> otherClass) {
+        return thatString != null && otherClass == thatString.getClass() && thatString.equals(other);
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>False positive with string literals / constants</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Foo {
+    private static final String CONST = "";
+
+    private boolean test(String val) {
+        return val != null && "".equals(val);
+    }
+
+    private boolean test_const(String val) {
+        return val != null && CONST.equals(val);
+    }
 }
         ]]></code>
     </test-code>