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/trunk@6227 51baf565-9d33-0410-a72c-fc3788e3496d

pmd/etc/changelog.txt

@@ -299,7 +299,7 @@ bin and java14/bin scripts:
 	retroweaver version was not correct in java14/bin scripts
 	support for extra languages in cpd.sh
 	standard unix scripts can be used with cygwin
-Upgrading UselessOperationOnImmutable to detect more use cases, especially on String
+Upgrading UselessOperationOnImmutable to detect more use cases, especially on String and fix false positives
 AvoidDuplicateLiteralRule now has 'skipAnnotations' boolean property
 Fixed false positive in UnusedImports: javadoc comments are parsed to check @see and other tags
 Fixed parsing bug: constant fields in annotation classes

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

@@ -129,4 +129,37 @@ public class RuleViolator {
 }
              ]]></code>
     </test-code>
+    <test-code>
+        <description><![CDATA[
+String calls in expressions
+     ]]></description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Foo {
+ public void foo() {
+	String s;
+	String s2 = "foo" + s.substring( 0, delimiterIndex ) + "/";
+	s2 = "foo" + s.substring( 0, delimiterIndex );
+	if (s.trim().length() > 0) {
+	}
+
+ }
+}
+     ]]></code>
+    </test-code>
+    <test-code>
+        <description><![CDATA[
+BigInteger calls in expression
+     ]]></description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Foo {
+ public void foo() {
+	BigInteger temp = BigInteger.valueOf((long) startMonth).add(dMonths);
+        setMonth(temp.subtract(BigInteger.ONE).mod(TWELVE).intValue() + 1);
+ }
+}
+     ]]></code>
+    </test-code>
+
 </test-data>

pmd/src/net/sourceforge/pmd/lang/java/rule/basic/UselessOperationOnImmutableRule.java

@@ -1,17 +1,12 @@
 package net.sourceforge.pmd.lang.java.rule.basic;
 
 import java.util.HashMap;
-import java.util.Iterator;
-import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
 import net.sourceforge.pmd.lang.ast.Node;
-import net.sourceforge.pmd.lang.java.ast.ASTConditionalExpression;
-import net.sourceforge.pmd.lang.java.ast.ASTExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration;
-import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression;
-import net.sourceforge.pmd.lang.java.ast.ASTPrimarySuffix;
+import net.sourceforge.pmd.lang.java.ast.ASTStatementExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTType;
 import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId;
 import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
@@ -24,12 +19,12 @@ import net.sourceforge.pmd.util.CollectionUtil;
  * ignoring the operation result is an error.
  */
 public class UselessOperationOnImmutableRule extends AbstractJavaRule {
-	
+
     /**
      * These are the BigDecimal methods which are immutable
      */
     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
      */
@@ -53,6 +48,7 @@ public class UselessOperationOnImmutableRule extends AbstractJavaRule {
         mapClasses.put("String", strMethods);
     }
 
+    @Override
     public Object visit(ASTLocalVariableDeclaration node, Object data) {
 
         ASTVariableDeclaratorId var = getDeclaration(node);
@@ -61,13 +57,12 @@ public class UselessOperationOnImmutableRule extends AbstractJavaRule {
         }
         String variableName = var.getImage();
         for (NameOccurrence no: var.getUsages()) {
-            // FIXME - getUsages will return everything with the same name as the variable, 
+            // FIXME - getUsages will return everything with the same name as the variable,
             // see JUnit test, case 6. Changing to Node below, revisit when getUsages is fixed
             Node 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());
                 ASTType nodeType = node.getTypeNode();
                 if ( nodeType != null ) {
@@ -80,27 +75,10 @@ public class UselessOperationOnImmutableRule extends AbstractJavaRule {
         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<ASTPrimarySuffix> suffixes = ((ASTPrimaryExpression)primaryExpression).findChildrenOfType(ASTPrimarySuffix.class);
-			for (Iterator<ASTPrimarySuffix> 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
-     * 
+     *
      * @param node
      *            The ASTLocalVariableDeclaration which is a problem
      * @return ASTVariableDeclaratorId