From 1e6c79c19fe716ced57f41389959db490ced7037 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 19 Nov 2020 21:08:15 +0100 Subject: [PATCH] [java] CompareObjectsWithEquals / UseEqualsToCompareStrings - Fix FN Fixes false negatives in case fields are referenced with this. Type resolution on PrimaryExpression works well enough to simply check the type. --- .../CompareObjectsWithEqualsRule.java | 113 ------------------ .../resources/category/java/errorprone.xml | 20 +++- .../ClassWithFields.java | 14 +++ .../CompareObjectsWithEqualsSample.java | 13 ++ .../ClassWithStringFields.java | 12 ++ .../UseEqualsToCompareStringsSample.java | 9 ++ .../xml/CompareObjectsWithEquals.xml | 45 ++++++- .../xml/UseEqualsToCompareStrings.xml | 53 +++++++- 8 files changed, 149 insertions(+), 130 deletions(-) delete mode 100644 pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CompareObjectsWithEqualsRule.java create mode 100644 pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/compareobjectswithequals/ClassWithFields.java create mode 100644 pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/compareobjectswithequals/CompareObjectsWithEqualsSample.java create mode 100644 pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/useequalstocomparestrings/ClassWithStringFields.java create mode 100644 pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/useequalstocomparestrings/UseEqualsToCompareStringsSample.java diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CompareObjectsWithEqualsRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CompareObjectsWithEqualsRule.java deleted file mode 100644 index 62f4206976..0000000000 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CompareObjectsWithEqualsRule.java +++ /dev/null @@ -1,113 +0,0 @@ -/** - * BSD-style license; for more info see http://pmd.sourceforge.net/license.html - */ - -package net.sourceforge.pmd.lang.java.rule.errorprone; - -import net.sourceforge.pmd.lang.ast.Node; -import net.sourceforge.pmd.lang.java.ast.ASTAllocationExpression; -import net.sourceforge.pmd.lang.java.ast.ASTEqualityExpression; -import net.sourceforge.pmd.lang.java.ast.ASTInitializer; -import net.sourceforge.pmd.lang.java.ast.ASTName; -import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix; -import net.sourceforge.pmd.lang.java.ast.ASTPrimarySuffix; -import net.sourceforge.pmd.lang.java.ast.ASTReferenceType; -import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; -import net.sourceforge.pmd.lang.java.symboltable.VariableNameDeclaration; - -public class CompareObjectsWithEqualsRule extends AbstractJavaRule { - - private boolean hasName(Node n) { - return n.getNumChildren() > 0 && n.getChild(0) instanceof ASTName; - } - - /** - * Indicate whether this node is allocating a new object. - * - * @param n - * node that might be allocating a new object - * @return true if child 0 is an AllocationExpression - */ - private boolean isAllocation(Node n) { - return n.getNumChildren() > 0 && n.getChild(0) instanceof ASTAllocationExpression - && n.getParent().getNumChildren() == 1; - } - - @Override - public Object visit(ASTEqualityExpression node, Object data) { - Node c0 = node.getChild(0).getChild(0); - Node c1 = node.getChild(1).getChild(0); - - // If either side is allocating a new object, there's no way an - // equals expression is correct - if (isAllocation(c0) || isAllocation(c1)) { - addViolation(data, node); - return data; - } - - // skip if either child is not a simple name - if (!hasName(c0) || !hasName(c1)) { - return data; - } - - // skip if either is a qualified name - if (isQualifiedName(c0.getChild(0)) || isQualifiedName(c1.getChild(0))) { - return data; - } - - // skip if either is part of a qualified name - if (isPartOfQualifiedName(node.getChild(0)) || isPartOfQualifiedName(node.getChild(1))) { - return data; - } - - // skip static initializers... missing some cases here - if (!node.getParentsOfType(ASTInitializer.class).isEmpty()) { - return data; - } - - ASTName n0 = (ASTName) c0.getChild(0); - ASTName n1 = (ASTName) c1.getChild(0); - - if (n0.getNameDeclaration() instanceof VariableNameDeclaration - && n1.getNameDeclaration() instanceof VariableNameDeclaration) { - VariableNameDeclaration nd0 = (VariableNameDeclaration) n0.getNameDeclaration(); - VariableNameDeclaration nd1 = (VariableNameDeclaration) n1.getNameDeclaration(); - - // skip array dereferences... this misses some cases - // FIXME catch comparisons btwn array elements of reference types - if (nd0.isArray() || nd1.isArray()) { - return data; - } - - if (nd0.isReferenceType() && nd1.isReferenceType()) { - ASTReferenceType type0 = ((Node) nd0.getAccessNodeParent()) - .getFirstDescendantOfType(ASTReferenceType.class); - ASTReferenceType type1 = ((Node) nd1.getAccessNodeParent()) - .getFirstDescendantOfType(ASTReferenceType.class); - // skip, if it is an enum - if (type0.getType() != null && type0.getType().equals(type1.getType()) - // It may be a custom enum class or an explicit Enum class usage - && (type0.getType().isEnum() || type0.getType() == java.lang.Enum.class)) { - return data; - } - - addViolation(data, node); - } - } - - return data; - } - - /** - * Checks whether the given node contains a qualified name, consisting of - * one ASTPrimaryPrefix and one or more ASTPrimarySuffix nodes. - * - * @param node - * the node - * @return true if it is a qualified name - */ - private boolean isPartOfQualifiedName(Node node) { - return node.getChild(0) instanceof ASTPrimaryPrefix - && !node.findChildrenOfType(ASTPrimarySuffix.class).isEmpty(); - } -} diff --git a/pmd-java/src/main/resources/category/java/errorprone.xml b/pmd-java/src/main/resources/category/java/errorprone.xml index d0ac48edb3..9a66253506 100644 --- a/pmd-java/src/main/resources/category/java/errorprone.xml +++ b/pmd-java/src/main/resources/category/java/errorprone.xml @@ -1097,12 +1097,23 @@ public class Bar { language="java" since="3.2" message="Use equals() to compare object references." - class="net.sourceforge.pmd.lang.java.rule.errorprone.CompareObjectsWithEqualsRule" + class="net.sourceforge.pmd.lang.rule.XPathRule" externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_errorprone.html#compareobjectswithequals"> Use equals() to compare object references; avoid comparing them with ==. 3 + + + + + + + + diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/compareobjectswithequals/ClassWithFields.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/compareobjectswithequals/ClassWithFields.java new file mode 100644 index 0000000000..77a7cd62b2 --- /dev/null +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/compareobjectswithequals/ClassWithFields.java @@ -0,0 +1,14 @@ +/* + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.java.rule.errorprone.compareobjectswithequals; + +public class ClassWithFields { + private Object a; + private Object b; + + boolean test1() { + return false; + } +} diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/compareobjectswithequals/CompareObjectsWithEqualsSample.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/compareobjectswithequals/CompareObjectsWithEqualsSample.java new file mode 100644 index 0000000000..406fc7da92 --- /dev/null +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/compareobjectswithequals/CompareObjectsWithEqualsSample.java @@ -0,0 +1,13 @@ +/* + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.java.rule.errorprone.compareobjectswithequals; + +public class CompareObjectsWithEqualsSample { + boolean bar(String a, String b) { + return false; + } + + void array(int[] a, String[] b) { } +} diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/useequalstocomparestrings/ClassWithStringFields.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/useequalstocomparestrings/ClassWithStringFields.java new file mode 100644 index 0000000000..5b85f10384 --- /dev/null +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/useequalstocomparestrings/ClassWithStringFields.java @@ -0,0 +1,12 @@ +/* + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.java.rule.errorprone.useequalstocomparestrings; + +public class ClassWithStringFields { + private String string1 = "a"; + private String string2 = "a"; + + public void bar() { } +} diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/useequalstocomparestrings/UseEqualsToCompareStringsSample.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/useequalstocomparestrings/UseEqualsToCompareStringsSample.java new file mode 100644 index 0000000000..245842ba6e --- /dev/null +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/useequalstocomparestrings/UseEqualsToCompareStringsSample.java @@ -0,0 +1,9 @@ +/* + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.java.rule.errorprone.useequalstocomparestrings; + +public class UseEqualsToCompareStringsSample { + void bar(String x) {} +} diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CompareObjectsWithEquals.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CompareObjectsWithEquals.xml index 987dbbbd85..b1cf841d28 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CompareObjectsWithEquals.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CompareObjectsWithEquals.xml @@ -53,10 +53,12 @@ public class Foo { - more qualified name skippage + charAt return char - that's ok 0 array element comparison - 0 + 1 + 5 @@ -109,6 +117,8 @@ public class Foo { Comparing against new object should always return false 1 + + + + False negative - class with fields + 4 + 8,9,10,11 + diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/UseEqualsToCompareStrings.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/UseEqualsToCompareStrings.xml index e001d193a4..528b67d746 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/UseEqualsToCompareStrings.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/UseEqualsToCompareStrings.xml @@ -7,8 +7,11 @@ failure case using == 1 + 5 failure case using != 1 + 5 using equals, OK 0 using compareTo, OK 0 using length, OK 0 + + + + false negatives with fields + 8 + 8,9,10,11,13,14,15,16 +