Merge branch 'pr-2651'

[java] False negative: LiteralsFirstInComparisons for methods... (2569) #2651

docs/pages/release_notes.md

@@ -30,6 +30,8 @@ This is a {{ site.pmd.release_type }} release.
 *   core
     *   [#710](https://github.com/pmd/pmd/issues/710): \[core] Review used dependencies
     *   [#2594](https://github.com/pmd/pmd/issues/2594): \[core] Update exec-maven-plugin and align it in all project
+*   java-bestpractices
+    *   [#2569](https://github.com/pmd/pmd/issues/2569): \[java] LiteralsFirstInComparisons: False negative for methods returning Strings
 *   java-design
     *   [#2174](https://github.com/pmd/pmd/issues/2174): \[java] LawOfDemeter: False positive with 'this' pointer
     *   [#2189](https://github.com/pmd/pmd/issues/2189): \[java] LawOfDemeter: False positive when casting to derived class
@@ -62,6 +64,7 @@ This is a {{ site.pmd.release_type }} release.
 *   [#2640](https://github.com/pmd/pmd/pull/2640): \[java] NullPointerException in rule ProperCloneImplementation - [Mykhailo Palahuta](https://github.com/Drofff)
 *   [#2641](https://github.com/pmd/pmd/pull/2641): \[java] AvoidThrowingNullPointerException marks all NullPointerException… - [Mykhailo Palahuta](https://github.com/Drofff)
 *   [#2643](https://github.com/pmd/pmd/pull/2643): \[java] AvoidCallingFinalize detects some false positives (2578) - [Mykhailo Palahuta](https://github.com/Drofff)
+*   [#2651](https://github.com/pmd/pmd/pull/2651): \[java] False negative: LiteralsFirstInComparisons for methods... (2569) - [Mykhailo Palahuta](https://github.com/Drofff)
 
 {% endtocmaker %}
 

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/LiteralsFirstInComparisonsRule.java

@@ -4,7 +4,8 @@
 
 package net.sourceforge.pmd.lang.java.rule.bestpractices;
 
-import net.sourceforge.pmd.lang.ast.Node;
+import java.util.List;
+
 import net.sourceforge.pmd.lang.java.ast.ASTArgumentList;
 import net.sourceforge.pmd.lang.java.ast.ASTArguments;
 import net.sourceforge.pmd.lang.java.ast.ASTConditionalAndExpression;
@@ -29,59 +30,79 @@ public class LiteralsFirstInComparisonsRule extends AbstractJavaRule {
     }
 
     @Override
-    public Object visit(ASTPrimaryExpression node, Object data) {
+    public Object visit(ASTPrimaryExpression expression, Object data) {
-        ASTPrimaryPrefix primaryPrefix = node.getFirstChildOfType(ASTPrimaryPrefix.class);
+        if (violatesLiteralsFirstInComparisonsRule(expression)) {
-        ASTPrimarySuffix primarySuffix = node.getFirstChildOfType(ASTPrimarySuffix.class);
+            addViolation(data, expression);
-        if (primaryPrefix != null && primarySuffix != null) {
+        }
-            ASTName name = primaryPrefix.getFirstChildOfType(ASTName.class);
-            if (name == null || isIrrelevantImage(name.getImage())) {
         return data;
     }
-            if (!isSingleStringLiteralArgument(primarySuffix)) {
-                return data;
-            }
-            if (isWithinNullComparison(node)) {
-                return data;
-            }
-            addViolation(data, node);
-        }
-        return node;
-    }
 
-    private boolean isIrrelevantImage(String image) {
+    private boolean violatesLiteralsFirstInComparisonsRule(ASTPrimaryExpression expression) {
+        return !hasStringLiteralFirst(expression) && isNullableComparisonWithStringLiteral(expression);
+    }
+
+    private boolean hasStringLiteralFirst(ASTPrimaryExpression expression) {
+        ASTPrimaryPrefix primaryPrefix = expression.getFirstChildOfType(ASTPrimaryPrefix.class);
+        ASTLiteral firstLiteral = primaryPrefix.getFirstDescendantOfType(ASTLiteral.class);
+        return firstLiteral != null && firstLiteral.isStringLiteral();
+    }
+
+    private boolean isNullableComparisonWithStringLiteral(ASTPrimaryExpression expression) {
+        String opName = getOperationName(expression);
+        ASTPrimarySuffix argsSuffix = getSuffixOfArguments(expression);
+        return opName != null && argsSuffix != null && isStringLiteralComparison(opName, argsSuffix)
+                && isNotWithinNullComparison(expression);
+    }
+
+    private String getOperationName(ASTPrimaryExpression primaryExpression) {
+        return isMethodsChain(primaryExpression) ? getOperationNameBySuffix(primaryExpression)
+                : getOperationNameByPrefix(primaryExpression);
+    }
+
+    private boolean isMethodsChain(ASTPrimaryExpression primaryExpression) {
+        return primaryExpression.getNumChildren() > 2;
+    }
+
+    private String getOperationNameBySuffix(ASTPrimaryExpression primaryExpression) {
+        ASTPrimarySuffix opAsSuffix = getPrimarySuffixAtIndexFromEnd(primaryExpression, 1);
+        if (opAsSuffix != null) {
+            String opName = opAsSuffix.getImage(); // name of pattern "operation"
+            return "." + opName;
+        }
+        return null;
+    }
+
+    private String getOperationNameByPrefix(ASTPrimaryExpression primaryExpression) {
+        ASTPrimaryPrefix opAsPrefix = primaryExpression.getFirstChildOfType(ASTPrimaryPrefix.class);
+        if (opAsPrefix != null) {
+            ASTName opName = opAsPrefix.getFirstChildOfType(ASTName.class); // name of pattern "*.operation"
+            return opName != null ? opName.getImage() : null;
+        }
+        return null;
+    }
+
+    private ASTPrimarySuffix getSuffixOfArguments(ASTPrimaryExpression primaryExpression) {
+        return getPrimarySuffixAtIndexFromEnd(primaryExpression, 0);
+    }
+
+    private ASTPrimarySuffix getPrimarySuffixAtIndexFromEnd(ASTPrimaryExpression primaryExpression, int indexFromEnd) {
+        List<ASTPrimarySuffix> primarySuffixes = primaryExpression.findChildrenOfType(ASTPrimarySuffix.class);
+        if (!primarySuffixes.isEmpty()) {
+            int suffixIndex = primarySuffixes.size() - 1 - indexFromEnd;
+            return primarySuffixes.get(suffixIndex);
+        }
+        return null;
+    }
+
+    private boolean isStringLiteralComparison(String opName, ASTPrimarySuffix argsSuffix) {
+        return isComparisonOperation(opName) && isSingleStringLiteralArgument(argsSuffix);
+    }
+
+    private boolean isComparisonOperation(String op) {
         for (String comparisonOp : COMPARISON_OPS) {
-            if (image.endsWith(comparisonOp)) {
+            if (op.endsWith(comparisonOp)) {
-                return false;
-            }
-        }
                 return true;
             }
-
-    private boolean isWithinNullComparison(ASTPrimaryExpression node) {
-        for (ASTExpression parentExpr : node.getParentsOfType(ASTExpression.class)) {
-            if (isComparisonWithNull(parentExpr, "==", ASTConditionalOrExpression.class)
-                    || isComparisonWithNull(parentExpr, "!=", ASTConditionalAndExpression.class)) {
-                return true;
-            }
-        }
-        return false;
-    }
-
-    /*
-     * Expression/ConditionalAndExpression//EqualityExpression(@Image='!=']//NullLiteral
-     * Expression/ConditionalOrExpression//EqualityExpression(@Image='==']//NullLiteral
-     */
-    private boolean isComparisonWithNull(ASTExpression parentExpr, String equalOperator, Class<? extends JavaNode> condition) {
-        Node condExpr = null;
-        ASTEqualityExpression eqExpr = null;
-        if (parentExpr != null) {
-            condExpr = parentExpr.getFirstChildOfType(condition);
-        }
-        if (condExpr != null) {
-            eqExpr = condExpr.getFirstDescendantOfType(ASTEqualityExpression.class);
-        }
-        if (eqExpr != null) {
-            return eqExpr.hasImageEqualTo(equalOperator) && eqExpr.hasDescendantOfType(ASTNullLiteral.class);
         }
         return false;
     }
@@ -93,35 +114,77 @@ public class LiteralsFirstInComparisonsRule extends AbstractJavaRule {
      * ( count(../PrimarySuffix/Arguments/ArgumentList/Expression) = 1 )
      */
     private boolean isSingleStringLiteralArgument(ASTPrimarySuffix primarySuffix) {
-        if (!primarySuffix.isArguments() || primarySuffix.getArgumentCount() != 1) {
+        return isSingleArgumentSuffix(primarySuffix) && isStringLiteralFirstArgumentOfSuffix(primarySuffix);
-            return false;
     }
-        Node node = primarySuffix;
+
-        node = node.getFirstChildOfType(ASTArguments.class);
+    private boolean isSingleArgumentSuffix(ASTPrimarySuffix primarySuffix) {
-        if (node != null) {
+        return primarySuffix.getArgumentCount() == 1;
-            node = node.getFirstChildOfType(ASTArgumentList.class);
+    }
-            if (node.getNumChildren() != 1) {
+
+    private boolean isStringLiteralFirstArgumentOfSuffix(ASTPrimarySuffix primarySuffix) {
+        try {
+            JavaNode firstArg = getFirstArgument(primarySuffix);
+            return isStringLiteral(firstArg);
+        } catch (NullPointerException e) {
             return false;
         }
     }
-        if (node != null) {
+
-            node = node.getFirstChildOfType(ASTExpression.class);
+    private JavaNode getFirstArgument(ASTPrimarySuffix primarySuffix) {
+        ASTArguments arguments = primarySuffix.getFirstChildOfType(ASTArguments.class);
+        ASTArgumentList argumentList = arguments.getFirstChildOfType(ASTArgumentList.class);
+        ASTExpression expression = argumentList.getFirstChildOfType(ASTExpression.class);
+        ASTPrimaryExpression primaryExpression = expression.getFirstChildOfType(ASTPrimaryExpression.class);
+        ASTPrimaryPrefix primaryPrefix = primaryExpression.getFirstChildOfType(ASTPrimaryPrefix.class);
+        return primaryPrefix.getFirstChildOfType(ASTLiteral.class);
     }
-        if (node != null) {
+
-            node = node.getFirstChildOfType(ASTPrimaryExpression.class);
+    private boolean isStringLiteral(JavaNode node) {
-        }
+        if (node instanceof ASTLiteral) {
-        if (node != null) {
-            node = node.getFirstChildOfType(ASTPrimaryPrefix.class);
-        }
-        if (node != null) {
-            node = node.getFirstChildOfType(ASTLiteral.class);
-        }
-        if (node != null) {
             ASTLiteral literal = (ASTLiteral) node;
-            if (literal.isStringLiteral()) {
+            return literal.isStringLiteral();
+        }
+        return false;
+    }
+
+    private boolean isNotWithinNullComparison(ASTPrimaryExpression node) {
+        return !isWithinNullComparison(node);
+    }
+
+    /*
+     * Expression/ConditionalAndExpression//EqualityExpression(@Image='!=']//NullLiteral
+     * Expression/ConditionalOrExpression//EqualityExpression(@Image='==']//NullLiteral
+     */
+    private boolean isWithinNullComparison(ASTPrimaryExpression node) {
+        for (ASTExpression parentExpr : node.getParentsOfType(ASTExpression.class)) {
+            if (isNullComparison(parentExpr)) {
                 return true;
             }
         }
         return false;
     }
+
+    private boolean isNullComparison(ASTExpression expression) {
+        return isAndNotNullComparison(expression) || isOrNullComparison(expression);
+    }
+
+    private boolean isAndNotNullComparison(ASTExpression expression) {
+        ASTConditionalAndExpression andExpression = expression
+                .getFirstChildOfType(ASTConditionalAndExpression.class);
+        return andExpression != null && hasEqualityExpressionWithNullLiteral(andExpression, "!=");
+    }
+
+    private boolean isOrNullComparison(ASTExpression expression) {
+        ASTConditionalOrExpression orExpression = expression
+                .getFirstChildOfType(ASTConditionalOrExpression.class);
+        return orExpression != null && hasEqualityExpressionWithNullLiteral(orExpression, "==");
+    }
+
+    private boolean hasEqualityExpressionWithNullLiteral(JavaNode node, String equalityOp) {
+        ASTEqualityExpression equalityExpression = node.getFirstDescendantOfType(ASTEqualityExpression.class);
+        if (equalityExpression != null && equalityExpression.hasImageEqualTo(equalityOp)) {
+            return equalityExpression.hasDescendantOfType(ASTNullLiteral.class);
+        }
+        return false;
+    }
 }

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LiteralsFirstInComparisons.xml

@@ -272,6 +272,49 @@ public class Foo {
     boolean bar(String x) {
         return contentEquals("2");
     }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>bad, testing false negative at the end of a chain</description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+public class Foo {
+    public boolean bar() {
+        File f;
+        return f.getFileType().equals("testStr");
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>ok, should be ignored in case both operands are string literals</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Foo {
+    boolean isFoo;
+    public void bar() {
+        this.isFoo = "Hello".equals("World");
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Equals on method result with String argument</description>
+        <expected-problems>1</expected-problems>
+        <expected-linenumbers>6</expected-linenumbers>
+        <code><![CDATA[
+public class Foo {
+    private String getStr(String a) {
+        return "a" + a;
+    }
+    public void bar() {
+        if (getStr("b").equals("ab")) { }   // nok
+        if ("ab".equals(getStr("b"))) { }   // ok
+    }
 }
         ]]></code>
     </test-code>