From 4583fde181fe2b1eb164ecbafb9a9adbe522414a Mon Sep 17 00:00:00 2001 From: Xavier Le Vourch Date: Fri, 20 Jun 2008 19:58:54 +0000 Subject: [PATCH] fix false positives in UselessOperationOnImmutable false positives included string were used in expressions and BigInteger as method arguments. The immutable object is ignored only if expression's parent is a statement expression. git-svn-id: https://pmd.svn.sourceforge.net/svnroot/pmd/branches/pmd/4.2.x@6237 51baf565-9d33-0410-a72c-fc3788e3496d --- pmd/etc/changelog.txt | 2 +- .../basic/xml/UselessOperationOnImmutable.xml | 33 ++++++++++ .../rules/UselessOperationOnImmutable.java | 60 +++++-------------- 3 files changed, 49 insertions(+), 46 deletions(-) diff --git a/pmd/etc/changelog.txt b/pmd/etc/changelog.txt index 7b44d78888..5345b379ca 100644 --- a/pmd/etc/changelog.txt +++ b/pmd/etc/changelog.txt @@ -2,7 +2,7 @@ Fixes for exclude-pattern Updates to RuleChain to honor RuleSet exclude-pattern -Upgrading UselessOperationOnImmutable to detect more use cases, especially on String +Upgrading UselessOperationOnImmutable to detect more use cases, especially on String and fix false positives Fixed bug 1988829 - Violation reported without source file name (actually a fix to ConsecutiveLiteralAppends) Fixed bug 1989814 - false +: ConsecutiveLiteralAppends Fixed bug 1977230 - false positive: UselessOverridingMethod diff --git a/pmd/regress/test/net/sourceforge/pmd/rules/basic/xml/UselessOperationOnImmutable.xml b/pmd/regress/test/net/sourceforge/pmd/rules/basic/xml/UselessOperationOnImmutable.xml index f176373e60..7a6a98eec3 100644 --- a/pmd/regress/test/net/sourceforge/pmd/rules/basic/xml/UselessOperationOnImmutable.xml +++ b/pmd/regress/test/net/sourceforge/pmd/rules/basic/xml/UselessOperationOnImmutable.xml @@ -129,4 +129,37 @@ public class RuleViolator { } ]]> + + + 0 + 0) { + } + + } +} + ]]> + + + + 0 + + + diff --git a/pmd/src/net/sourceforge/pmd/rules/UselessOperationOnImmutable.java b/pmd/src/net/sourceforge/pmd/rules/UselessOperationOnImmutable.java index 13169d3698..0f384f84b7 100644 --- a/pmd/src/net/sourceforge/pmd/rules/UselessOperationOnImmutable.java +++ b/pmd/src/net/sourceforge/pmd/rules/UselessOperationOnImmutable.java @@ -13,6 +13,7 @@ import net.sourceforge.pmd.ast.ASTExpression; import net.sourceforge.pmd.ast.ASTLocalVariableDeclaration; import net.sourceforge.pmd.ast.ASTPrimaryExpression; import net.sourceforge.pmd.ast.ASTPrimarySuffix; +import net.sourceforge.pmd.ast.ASTStatementExpression; import net.sourceforge.pmd.ast.ASTType; import net.sourceforge.pmd.ast.ASTVariableDeclaratorId; import net.sourceforge.pmd.ast.Node; @@ -26,22 +27,22 @@ import net.sourceforge.pmd.util.CollectionUtil; * ignoring the operation result is an error. */ public class UselessOperationOnImmutable extends AbstractJavaRule { - + /** * These are the BigDecimal methods which are immutable */ private static final Set decMethods = CollectionUtil.asSet(new String[] { ".abs", ".add", ".divide", ".divideToIntegralValue", ".max", ".min", ".movePointLeft", ".movePointRight", ".multiply", ".negate", ".plus", ".pow", ".remainder", ".round", ".scaleByPowerOfTen", ".setScale", ".stripTrailingZeros", ".subtract", ".ulp" }); - + /** * These are the BigInteger methods which are immutable */ private static final Set intMethods = CollectionUtil.asSet(new String[] { ".abs", ".add", ".and", ".andNot", ".clearBit", ".divide", ".flipBit", ".gcd", ".max", ".min", ".mod", ".modInverse", ".modPow", ".multiply", ".negate", ".nextProbablePrine", ".not", ".or", ".pow", ".remainder", ".setBit", ".shiftLeft", ".shiftRight", ".subtract", ".xor" }); - + /** * These are the String methods which are immutable */ private static final Set strMethods = CollectionUtil.asSet(new String[] { ".concat", ".intern", ".replace", ".replaceAll", ".replaceFirst", ".substring", ".toLowerCase", ".toString", ".toUpperCase", ".trim" }); - + /** * These are the classes that the rule can apply to */ @@ -54,7 +55,7 @@ public class UselessOperationOnImmutable extends AbstractJavaRule { mapClasses.put("java.lang.String", strMethods); mapClasses.put("String", strMethods); } - + public Object visit(ASTLocalVariableDeclaration node, Object data) { ASTVariableDeclaratorId var = getDeclaration(node); @@ -67,35 +68,20 @@ public class UselessOperationOnImmutable extends AbstractJavaRule { // see JUnit test, case 6. Changing to SimpleNode below, revisit when getUsages is fixed SimpleNode sn = no.getLocation(); Node primaryExpression = sn.jjtGetParent().jjtGetParent(); - Class parentClass = primaryExpression.jjtGetParent().getClass(); - if (!(parentClass.equals(ASTExpression.class) || parentClass.equals(ASTConditionalExpression.class) || - hasComparisons(primaryExpression))) { + Class parentClass = primaryExpression.jjtGetParent().getClass(); + if (parentClass.equals(ASTStatementExpression.class)) { String methodCall = sn.getImage().substring(variableName.length()); - if (mapClasses.get(getType(node)).contains(methodCall)) { - addViolation(data, sn); + ASTType nodeType = node.getTypeNode(); + if ( nodeType != null ) { + if ( mapClasses.get(nodeType.getTypeImage()).contains(methodCall)) { + addViolation(data, sn); + } } } } return super.visit(node, data); } - /** - * Check whether the Immutable is compareTo'd something - */ - private boolean hasComparisons(Node primaryExpression) { - if (primaryExpression.getClass().equals(ASTPrimaryExpression.class)) { - List suffixes = ((ASTPrimaryExpression)primaryExpression).findChildrenOfType(ASTPrimarySuffix.class); - for (Iterator iterator = suffixes.iterator(); iterator.hasNext();) { - ASTPrimarySuffix suffix = iterator.next(); - if ("compareTo".equals(suffix.getImage())) - return true; - } - } else { - //Some weird usage of the Immutable - } - return false; //No comparison - } - /** * This method checks the variable declaration if it is on a class we care * about. If it is, it returns the DeclaratorId @@ -105,26 +91,10 @@ public class UselessOperationOnImmutable extends AbstractJavaRule { * @return ASTVariableDeclaratorId */ private ASTVariableDeclaratorId getDeclaration(ASTLocalVariableDeclaration node) { - String type = getType(node); - if (mapClasses.keySet().contains(type)) { + ASTType type = node.getTypeNode(); + if (mapClasses.keySet().contains(type.getTypeImage())) { return (ASTVariableDeclaratorId) node.jjtGetChild(1).jjtGetChild(0); } return null; } - - /** - * This method returns the name of the node's class. - * - * @param node - * The ASTLocalVariableDeclaration which is a problem - * @return the class name - */ - private static String getType(ASTLocalVariableDeclaration node) { - ASTType type = node.getTypeNode(); - if (type != null) { - return type.getTypeImage(); - } - return null; - } - }