Applying patch [ 1966013 ] Missing cases in UselessOperationOnImmutable

git-svn-id: https://pmd.svn.sourceforge.net/svnroot/pmd/branches/pmd/4.2.x@6197 51baf565-9d33-0410-a72c-fc3788e3496d

pmd/etc/changelog.txt

@@ -2,9 +2,11 @@
 
 Fixes for exclude-pattern
 Updates to RuleChain to honor RuleSet exclude-pattern
+Upgrading UselessOperationOnImmutable to detect more use cases, especially on String
 Fixed bug 1988829 - Violation reported without source file name (actually a fix to ConsecutiveLiteralAppends)
 Fixed bug 1989814 - false +: ConsecutiveLiteralAppends
 
+
 May 20, 2008 - 4.2.2:
 
 Fixed false positive in UnusedImports: javadoc comments are parsed to check @see and other tags

pmd/regress/test/net/sourceforge/pmd/rules/basic/xml/UselessOperationOnImmutable.xml

@@ -10,7 +10,7 @@ useless operation on BigDecimal
 public class Foo {
  public void foo() {
   BigDecimal bd = new BigDecimal(5);
-  bd.add(new BigDecimal(5));
+  bd.divideToIntegralValue(new BigDecimal(5));
  }
 }
      ]]></code>
@@ -24,7 +24,7 @@ useless operation on BigInteger
 public class Foo {
  public void foo() {
   BigInteger bi = new BigInteger(5);
-  bi.add(new BigInteger(5));
+  bi.modPow(new BigInteger(1), new BigInteger(5));
  }
 }
      ]]></code>

pmd/rulesets/basic.xml

@@ -789,11 +789,11 @@ public class Test {
 
     <rule  name="UselessOperationOnImmutable"
            since="3.5"
-           message="An operation on an Immutable object (BigDecimal or BigInteger) won't change the object itself"
+           message="An operation on an Immutable object (String, BigDecimal or BigInteger) won't change the object itself"
            class="net.sourceforge.pmd.rules.UselessOperationOnImmutable"
            externalInfoUrl="http://pmd.sourceforge.net/rules/basic.html#UselessOperationOnImmutable">
       <description>
-    An operation on an Immutable object (BigDecimal or BigInteger) won't change the object itself. The
+    An operation on an Immutable object (String, BigDecimal or BigInteger) won't change the object itself. The
     result of the operation is a new object. Therefore, ignoring the operation result is an error.
       </description>
       <priority>3</priority>

pmd/src/net/sourceforge/pmd/rules/UselessOperationOnImmutable.java

@@ -1,9 +1,12 @@
 package net.sourceforge.pmd.rules;
 
+import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 
+import net.sourceforge.pmd.AbstractJavaRule;
 import net.sourceforge.pmd.AbstractRule;
 import net.sourceforge.pmd.ast.ASTConditionalExpression;
 import net.sourceforge.pmd.ast.ASTExpression;
@@ -18,20 +21,39 @@ import net.sourceforge.pmd.symboltable.NameOccurrence;
 import net.sourceforge.pmd.util.CollectionUtil;
 
 /**
- * An operation on an Immutable object (BigDecimal or BigInteger) won't change
+ * An operation on an Immutable object (String, BigDecimal or BigInteger) won't change
  * the object itself. The result of the operation is a new object. Therefore,
  * ignoring the operation result is an error.
  */
-public class UselessOperationOnImmutable extends AbstractRule {
+public class UselessOperationOnImmutable extends AbstractJavaRule {
+	
     /**
-     * These are the methods which are immutable
+     * These are the BigDecimal methods which are immutable
      */
-    private static final Set<String> targetMethods = CollectionUtil.asSet(new String[] { ".add", ".multiply", ".divide", ".subtract", ".setScale", ".negate", ".movePointLeft", ".movePointRight", ".pow", ".shiftLeft", ".shiftRight" });
+    private static final Set<String> 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<String> 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<String> 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
      */
-    private static final Set<String> targetClasses = CollectionUtil.asSet(new String[] { "java.math.BigDecimal", "BigDecimal", "java.math.BigInteger", "BigInteger" });
+    private static final Map<String, Set<String>> mapClasses = new HashMap<String, Set<String>>();
+    static {
+        mapClasses.put("java.math.BigDecimal", decMethods);
+        mapClasses.put("BigDecimal", decMethods);
+        mapClasses.put("java.math.BigInteger", intMethods);
+        mapClasses.put("BigInteger", intMethods);
+        mapClasses.put("java.lang.String", strMethods);
+        mapClasses.put("String", strMethods);
+    }
 	
     public Object visit(ASTLocalVariableDeclaration node, Object data) {
 
@@ -49,7 +71,7 @@ public class UselessOperationOnImmutable extends AbstractRule {
             if (!(parentClass.equals(ASTExpression.class) || parentClass.equals(ASTConditionalExpression.class) || 
             		hasComparisons(primaryExpression))) {
                 String methodCall = sn.getImage().substring(variableName.length());
-                if (targetMethods.contains(methodCall)) {
+                if (mapClasses.get(getType(node)).contains(methodCall)) {
                     addViolation(data, sn);
                 }
             }
@@ -83,10 +105,26 @@ public class UselessOperationOnImmutable extends AbstractRule {
      * @return ASTVariableDeclaratorId
      */
     private ASTVariableDeclaratorId getDeclaration(ASTLocalVariableDeclaration node) {
-        ASTType type = node.getTypeNode();
-        if (targetClasses.contains(type.getTypeImage())) {
+        String type = getType(node);
+        if (mapClasses.keySet().contains(type)) {
             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;
+    }
+	
 }