Fixed bug 1423429 - ImmutableField no longer reports false positives on variables which can be set via an anonymous inner class that is created in the constructor.

git-svn-id: https://pmd.svn.sourceforge.net/svnroot/pmd/trunk@4390 51baf565-9d33-0410-a72c-fc3788e3496d

pmd/etc/changelog.txt

@@ -12,6 +12,7 @@ Fixed bug 1114003 - UnusedPrivateMethod no longer reports false positives when t
 Fixed bug 1472843 - UnusedPrivateMethod no longer reports false positives when a private method is only called from a method that contains a variable with the same name as that method.
 Fixed bug 1461442 - UseAssertSameInsteadOfAssertTrue now ignores comparisons to null; UseAssertNullInsteadOfAssertTrue will report those.
 Fixed bug 1474778 - UnnecessaryCaseChange no longer flags usages of toUpperCase(Locale).
+Fixed bug 1423429 - ImmutableField no longer reports false positives on variables which can be set via an anonymous inner class that is created in the constructor.
 Fixed major bug in CPD; it was not picking up files other than .java or .jsp.
 Fixed a bug in CallSuperInConstructor; it now checks inner classes/enums more carefully.
 Fixed a bug in VariableNamingConventions; it was not setting the warning message properly.

pmd/regress/test/net/sourceforge/pmd/rules/design/ImmutableFieldTest.java

@@ -168,7 +168,7 @@ public class ImmutableFieldTest extends SimpleAggregatorTst {
 
     private static final String TEST16 =
             "public class Foo {" + PMD.EOL +
-            " private int x;" + PMD.EOL +
+            " private int x = 2 ;" + PMD.EOL +
             " public Foo() {" + PMD.EOL +
             "   mouseListener = new MouseAdapter() {" + PMD.EOL +
             "      public void mouseClicked(MouseEvent e) {" + PMD.EOL +

pmd/rulesets/design.xml

@@ -654,7 +654,7 @@ public class Foo {
     </rule>
 
     <rule name="ImmutableField"
-          message="Private field could be made final.  It is only initialized in the declaration or constructor."
+          message="Private field ''{0}'' could be made final; it is only initialized in the declaration or constructor."
           class="net.sourceforge.pmd.rules.design.ImmutableField"
           externalInfoUrl="http://pmd.sourceforge.net/rules/design.html#ImmutableField">
       <description>

pmd/rulesets/scratchpad.xml

@@ -10,49 +10,6 @@ These are new rules that are still in progress
   </description>
 
 
-    <rule name="SimplifyBooleanAssertion"
-	    message="assertTrue(!expr) can be replaced by assertFalse(expr)"
-        class="net.sourceforge.pmd.rules.XPathRule"
-       externalInfoUrl="http://pmd.sourceforge.net/rules/junit.html#SimplifyBooleanAssertion">
-      <description>
-Avoid negation in an assertTrue or assertFalse test.
-For example, rephrase:
-assertTrue(!expr);
-as:
-assertFalse(expr);
-      </description>
-      <properties>
-          <property name="xpath">
-              <value>
-    <![CDATA[
-//StatementExpression
-[
-.//Name[@Image='assertTrue' or  @Image='assertFalse']
-and
-PrimaryExpression/PrimarySuffix/Arguments/ArgumentList
- /Expression/UnaryExpressionNotPlusMinus[@Image='!']
-/PrimaryExpression/PrimaryPrefix
-]
-]]>
-              </value>
-          </property>
-      </properties>
-        <priority>3</priority>
-      <example>
-<![CDATA[
-public class SimpleTest extends TestCase {
- public void testX() {
-  assertTrue("not empty", !r.isEmpty()); // replace with assertFalse("not empty", r.isEmpty())
-
-  assertFalse(!r.isEmpty()); // replace with assertFalse(r.isEmpty())
- }
-}
-]]>
-          </example>
-    </rule>
-
- 
-
 
     <!--
     <rule name="UselessAssignment"

pmd/src/net/sourceforge/pmd/rules/design/ImmutableField.java

@@ -22,6 +22,7 @@ import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.ArrayList;
 
 /**
  * @author Olander
@@ -34,7 +35,7 @@ public class ImmutableField extends AbstractRule {
 
     public Object visit(ASTClassOrInterfaceDeclaration node, Object data) {
         Map vars = node.getScope().getVariableDeclarations();
-        Set constructors = findAllConstructors(node);
+        List constructors = findAllConstructors(node);
         for (Iterator i = vars.keySet().iterator(); i.hasNext();) {
             VariableNameDeclaration field = (VariableNameDeclaration) i.next();
             if (field.getAccessNodeParent().isStatic() || !field.getAccessNodeParent().isPrivate() || field.getAccessNodeParent().isFinal()) {
@@ -45,15 +46,19 @@ public class ImmutableField extends AbstractRule {
             if (result == MUTABLE) {
                 continue;
             }
-            if (result == IMMUTABLE || ((result == CHECKDECL) && !field.getAccessNodeParent().findChildrenOfType(ASTVariableInitializer.class).isEmpty())) {
+            if (result == IMMUTABLE || (result == CHECKDECL && initializedWhenDeclared(field))) {
                 addViolation(data, field.getNode(), field.getImage());
             }
         }
         return super.visit(node, data);
     }
 
+    private boolean initializedWhenDeclared(VariableNameDeclaration field) {
+        return !field.getAccessNodeParent().findChildrenOfType(ASTVariableInitializer.class).isEmpty();
+    }
+
     private int initializedInConstructor(List usages, Set allConstructors) {
-        int rc = MUTABLE, methodInitCount = 0;
+        int result = MUTABLE, methodInitCount = 0;
         Set consSet = new HashSet();
         for (Iterator j = usages.iterator(); j.hasNext();) {
             NameOccurrence occ = (NameOccurrence) j.next();
@@ -65,9 +70,10 @@ public class ImmutableField extends AbstractRule {
                         continue;
                     }
                     if (inAnonymousInnerClass(node)) {
-                        continue;
+                        methodInitCount++;
-                    }
+                    } else {
                         consSet.add(constructor);
+                    }
                 } else {
                     if (node.getFirstParentOfType(ASTMethodDeclaration.class) != null) {
                         methodInitCount++;
@@ -76,14 +82,14 @@ public class ImmutableField extends AbstractRule {
             }
         }
         if (usages.isEmpty() || ((methodInitCount == 0) && consSet.isEmpty())) {
-            rc = CHECKDECL;
+            result = CHECKDECL;
         } else {
             allConstructors.removeAll(consSet);
             if (allConstructors.isEmpty() && (methodInitCount == 0)) {
-                rc = IMMUTABLE;
+                result = IMMUTABLE;
             }
         }
-        return rc;
+        return result;
     }
 
     private boolean inLoopOrTry(SimpleNode node) {
@@ -98,12 +104,9 @@ public class ImmutableField extends AbstractRule {
         return parent != null && parent.isAnonymousInnerClass();
     }
 
-    /**
+    private List findAllConstructors(ASTClassOrInterfaceDeclaration node) {
-     * construct a set containing all ASTConstructorDeclaration nodes for this class
+        List cons = new ArrayList();
-     */
+        node.findChildrenOfType(ASTConstructorDeclaration.class, cons, false);
-    private Set findAllConstructors(ASTClassOrInterfaceDeclaration node) {
+        return cons;
-        Set set = new HashSet();
-        set.addAll(node.findChildrenOfType(ASTConstructorDeclaration.class));
-        return set;
     }
 }

pmd/xdocs/credits.xml

@@ -48,6 +48,7 @@
         </subsection>
         <subsection name="Contributors">
         <ul>
+            <li>Noel Grandin - bug report for ImmutableField, bug report for MissingStaticMethodInNonInstantiatableClass, bug report for MissingBreakInSwitch, EqualsNull rule, bug report for IfElseStmtsMustUseBracesRule</li>
             <li><a href="http://www.orablogs.com/olaf/">Olaf Heimburger</a> - wrote the UseProperClassLoader rule, code changes to get JDeveloper plugin working under JDev 10.1.3 EA, reported a possible NPE in ReportTree</li>
             <li>Mohammad Farooq - Reported new JavaNCSS URL</li>
             <li>Jeff Jensen - Reported missing XML schema references in documentation, wrote new XML schema, reported missing schema refs in example rulesets, suggested posting XML schema on PMD site, discussion of 'comments in catch block' feature, suggested description attribute in property element</li>
@@ -93,7 +94,6 @@
             <li>Jean-Marc Vanel - suggested enhancements to the PMD scoreboard</li>
             <li><a href="http://www.websoup.net/">Andriy Rozeluk</a> - suggested UseStringBufferLength, bug report 1306180 for AvoidConcatenatingNonLiteralsInStringBuffer, reported bug 1293157 for UnusedPrivateMethod, suggested UnnecessaryCaseChange, bug report for SimplifyConditional, suggested UnnecessaryLocalBeforeReturn, suggestions for improving BooleanInstantiation, UnnecessaryReturn, AvoidDuplicateLiterals RFEs and bug reports, various other RFEs and thoughtful discussions as well</li>
             <li>Bruno Juillet - suggested reporting suppressed warnings, bug report for missing package/class/method names, patch for Ant task's excludeMarker attribute, bug report on ruleset overrides</li>
-            <li>Noel Grandin - bug report for MissingStaticMethodInNonInstantiatableClass, bug report for MissingBreakInSwitch, EqualsNull rule, bug report for IfElseStmtsMustUseBracesRule</li>
             <li>Derek Hofmann - suggestion for adding --skip-duplicate-files option for CPD, bug report for CPD skipping header files when in C/C++ mode</li>
             <li>Mark Holczhammer - bug report for InefficientStringBuffering</li>
             <li>Raja Rajan - 2 bug reports for CompareObjectswithEquals</li>