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 extends Node> parentClass = primaryExpression.jjtGetParent().getClass();
- if (!(parentClass.equals(ASTExpression.class) || parentClass.equals(ASTConditionalExpression.class) ||
- hasComparisons(primaryExpression))) {
+ Class extends Node> 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;
- }
-
}