diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index d988529d47..da2f587aef 100644 --- a/docs/pages/release_notes.md +++ b/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 %} diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/LiteralsFirstInComparisonsRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/LiteralsFirstInComparisonsRule.java index 667240f5d8..3419dd8d65 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/LiteralsFirstInComparisonsRule.java +++ b/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,63 +30,83 @@ public class LiteralsFirstInComparisonsRule extends AbstractJavaRule { } @Override - public Object visit(ASTPrimaryExpression node, Object data) { - ASTPrimaryPrefix primaryPrefix = node.getFirstChildOfType(ASTPrimaryPrefix.class); - ASTPrimarySuffix primarySuffix = node.getFirstChildOfType(ASTPrimarySuffix.class); - 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); + public Object visit(ASTPrimaryExpression expression, Object data) { + if (violatesLiteralsFirstInComparisonsRule(expression)) { + addViolation(data, expression); } - return node; + return data; } - 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 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)) { - 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)) { + if (op.endsWith(comparisonOp)) { return true; } } return false; } - /* - * Expression/ConditionalAndExpression//EqualityExpression(@Image='!=']//NullLiteral - * Expression/ConditionalOrExpression//EqualityExpression(@Image='==']//NullLiteral - */ - private boolean isComparisonWithNull(ASTExpression parentExpr, String equalOperator, Class 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; - } - /* * This corresponds to the following XPath expression: * (../PrimarySuffix/Arguments/ArgumentList/Expression/PrimaryExpression/PrimaryPrefix/Literal[@StringLiteral= true()]) @@ -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); + } + + private boolean isSingleArgumentSuffix(ASTPrimarySuffix primarySuffix) { + return primarySuffix.getArgumentCount() == 1; + } + + private boolean isStringLiteralFirstArgumentOfSuffix(ASTPrimarySuffix primarySuffix) { + try { + JavaNode firstArg = getFirstArgument(primarySuffix); + return isStringLiteral(firstArg); + } catch (NullPointerException e) { return false; } - Node node = primarySuffix; - node = node.getFirstChildOfType(ASTArguments.class); - if (node != null) { - node = node.getFirstChildOfType(ASTArgumentList.class); - if (node.getNumChildren() != 1) { - return false; - } - } - if (node != null) { - node = node.getFirstChildOfType(ASTExpression.class); - } - if (node != null) { - node = node.getFirstChildOfType(ASTPrimaryExpression.class); - } - if (node != null) { - node = node.getFirstChildOfType(ASTPrimaryPrefix.class); - } - if (node != null) { - node = node.getFirstChildOfType(ASTLiteral.class); - } - if (node != null) { + } + + 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); + } + + private boolean isStringLiteral(JavaNode node) { + if (node instanceof ASTLiteral) { 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; + } } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LiteralsFirstInComparisons.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LiteralsFirstInComparisons.xml index 024ff927ab..f76ba2658f 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LiteralsFirstInComparisons.xml +++ b/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"); } +} + ]]> + + + + bad, testing false negative at the end of a chain + 1 + + + + + ok, should be ignored in case both operands are string literals + 0 + + + + + Equals on method result with String argument + 1 + 6 +