pmd: bugfix for 3574133: False+ : SingularField

hooperbloob · Oct 6, 2012 · 28b37d6 · 28b37d6
1 parent 051824b
commit 28b37d6
Show file tree

Hide file tree

Showing 9 changed files with 163 additions and 73 deletions.
diff --git a/pmd/etc/changelog.txt b/pmd/etc/changelog.txt
@@ -38,6 +38,7 @@ Fixed bug 3530124: pmd: parsing of generic method call with super fails
 Fixed bug 3496028: pmd-4.2.6: MissingBreakInSwitch fails to report violation
 Fixed bug 3484404: Invalid NPath calculation in return statement. Thanks to Prabhjot Singh for the patch.
 Fixed bug 3560464: c/c++ \ as a continuation character not supported
+Fixed bug 3574133: False+ : SingularField
 Improved JSP parser to be less strict with not valid XML documents (like HTML). Thanks to Victor Bucutea.
 Fixed bgastviewer not working. Thanks to Victor Bucutea.
 

diff --git a/pmd/etc/grammar/Java.jjt b/pmd/etc/grammar/Java.jjt
@@ -1,5 +1,10 @@
 /**
+ * Enhance grammar to use LocalVariableDeclaration in a for-each loop.
+ * This enhances the symbol table to recognize variables declared in such
+ * a for-each loop.
  *
+ * Andreas Dangel 10/2012
+ * ===================================================================
  * Fix parser problem #3530124 with generics
  *
  * Modified the grammar, so that the different usages of generics work.
@@ -1911,7 +1916,7 @@ void ForStatement() :
 (
       LOOKAHEAD(Modifiers() Type() <IDENTIFIER> ":")
       {checkForBadJDK15ForLoopSyntaxArgumentsUsage();}
-      Modifiers() Type() <IDENTIFIER> ":" Expression()
+      Modifiers() LocalVariableDeclaration() ":" Expression()
 |
   [ ForInit() ] ";"
   [ Expression() ] ";"

diff --git a/pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/coupling/LawOfDemeterRule.java b/pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/coupling/LawOfDemeterRule.java
@@ -13,6 +13,7 @@
 import net.sourceforge.pmd.lang.java.ast.ASTAllocationExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTAssignmentOperator;
 import net.sourceforge.pmd.lang.java.ast.ASTBlock;
+import net.sourceforge.pmd.lang.java.ast.ASTForStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTName;
 import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression;
@@ -209,7 +210,10 @@ private void checkViolation() {
 
             if (SCOPE_LOCAL.equals(baseScope)) {
                 Assignment lastAssignment = determineLastAssignment();
-                if (lastAssignment != null && !lastAssignment.allocation && !lastAssignment.iterator) {
+                if (lastAssignment != null
+                    && !lastAssignment.allocation
+                    && !lastAssignment.iterator
+                    && !lastAssignment.forLoop) {
                     violation = true;
                     violationReason = REASON_OBJECT_NOT_CREATED_LOCALLY;
                 }
@@ -289,7 +293,8 @@ private Assignment determineLastAssignment() {
                 if (variableDeclaratorId.hasImageEqualTo(baseName)) {
                     boolean allocationFound = declarator.getFirstDescendantOfType(ASTAllocationExpression.class) != null;
                     boolean iterator = isIterator();
-                    assignments.add(new Assignment(declarator.getBeginLine(), allocationFound, iterator));
+                    boolean forLoop = isForLoop(declarator);
+                    assignments.add(new Assignment(declarator.getBeginLine(), allocationFound, iterator, forLoop));
                 }
             }
 
@@ -298,7 +303,7 @@ private Assignment determineLastAssignment() {
                 if (stmt.hasImageEqualTo(SIMPLE_ASSIGNMENT_OPERATOR)) {
                     boolean allocationFound = stmt.jjtGetParent().getFirstDescendantOfType(ASTAllocationExpression.class) != null;
                     boolean iterator = isIterator();
-                    assignments.add(new Assignment(stmt.getBeginLine(), allocationFound, iterator));
+                    assignments.add(new Assignment(stmt.getBeginLine(), allocationFound, iterator, false));
                 }
             }
 
@@ -319,6 +324,10 @@ private boolean isIterator() {
             return iterator;
         }
 
+        private boolean isForLoop(ASTVariableDeclarator declarator) {
+            return declarator.jjtGetParent().jjtGetParent() instanceof ASTForStatement;
+        }
+
         public ASTPrimaryExpression getExpression() {
             return expression;
         }
@@ -351,17 +360,19 @@ private static class Assignment implements Comparable<Assignment> {
         private int line;
         private boolean allocation;
         private boolean iterator;
+        private boolean forLoop;
 
-        public Assignment(int line, boolean allocation, boolean iterator) {
+        public Assignment(int line, boolean allocation, boolean iterator, boolean forLoop) {
             this.line = line;
             this.allocation = allocation;
             this.iterator = iterator;
+            this.forLoop = forLoop;
         }
 
         @Override
         public String toString() {
             return "assignment: line=" + line + " allocation:" + allocation
-                + " iterator:" + iterator;
+                + " iterator:" + iterator + " forLoop: " + forLoop;
         }
 
         public int compareTo(Assignment o) {

diff --git a/pmd/src/main/java/net/sourceforge/pmd/lang/java/symboltable/Search.java b/pmd/src/main/java/net/sourceforge/pmd/lang/java/symboltable/Search.java
@@ -36,17 +36,17 @@ public NameDeclaration getResult() {
 
     private NameDeclaration searchUpward(NameOccurrence nameOccurrence, Scope scope) {
         if (TRACE) {
-            System.out.println("checking scope " + scope + " for name occurrence " + nameOccurrence);
+            System.out.println(" checking scope " + scope + " for name occurrence " + nameOccurrence);
         }
         if (!scope.contains(nameOccurrence) && scope.getParent() != null) {
             if (TRACE) {
-        	System.out.println("moving up fm " + scope + " to " + scope.getParent());
+                System.out.println(" moving up from " + scope + " to " + scope.getParent());
             }
             return searchUpward(nameOccurrence, scope.getParent());
         }
         if (scope.contains(nameOccurrence)) {
             if (TRACE) {
-        	System.out.println("found it!");
+                System.out.println(" found it!");
             }
             return scope.addVariableNameOccurrence(nameOccurrence);
         }

diff --git a/pmd/src/main/resources/rulesets/java/basic.xml b/pmd/src/main/resources/rulesets/java/basic.xml
@@ -63,6 +63,7 @@ Some for loops can be simplified to while loops, this makes them more concise.
                 <![CDATA[
 //ForStatement
  [count(*) > 1]
+ [not(LocalVariableDeclaration)]
  [not(ForInit)]
  [not(ForUpdate)]
  [not(Type and Expression and Statement)]

diff --git a/pmd/src/test/java/net/sourceforge/pmd/symboltable/AcceptanceTest.java b/pmd/src/test/java/net/sourceforge/pmd/symboltable/AcceptanceTest.java
@@ -6,32 +6,32 @@
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 
 import net.sourceforge.pmd.PMD;
-import net.sourceforge.pmd.lang.ast.Node;
 import net.sourceforge.pmd.lang.java.ast.ASTBlock;
 import net.sourceforge.pmd.lang.java.ast.ASTCatchStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTEqualityExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTInitializer;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
+import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId;
+import net.sourceforge.pmd.lang.java.ast.JavaNode;
+import net.sourceforge.pmd.lang.java.symboltable.NameOccurrence;
 import net.sourceforge.pmd.lang.java.symboltable.Scope;
 import net.sourceforge.pmd.lang.java.symboltable.VariableNameDeclaration;
 
-import org.junit.Ignore;
 import org.junit.Test;
 public class AcceptanceTest extends STBBaseTst {
 
-    @Ignore
     @Test
     public void testClashingSymbols() {
         parseCode(TEST1);
     }
 
-    @Ignore
     @Test
     public void testInitializer() {
         parseCode(TEST_INITIALIZERS);
@@ -41,83 +41,94 @@ public void testInitializer() {
         assertTrue(a.isStatic());
     }
 
-    @Ignore
     @Test
     public void testCatchBlocks() {
         parseCode(TEST_CATCH_BLOCKS);
         ASTCatchStatement c = acu.findDescendantsOfType(ASTCatchStatement.class).get(0);
         ASTBlock a = c.findDescendantsOfType(ASTBlock.class).get(0);
         Scope s = a.getScope();
-        Map vars = s.getParent().getVariableDeclarations();
+        Map<VariableNameDeclaration, List<NameOccurrence>> vars = s.getParent().getVariableDeclarations();
         assertEquals(1, vars.size());
-        VariableNameDeclaration v = (VariableNameDeclaration)vars.keySet().iterator().next();
+        VariableNameDeclaration v = vars.keySet().iterator().next();
         assertEquals("e", v.getImage());
-        assertEquals(1, ((List)vars.get(v)).size());
+        assertEquals(1, (vars.get(v)).size());
     }
 
-    @Ignore
     @Test
     public void testEq() {
         parseCode(TEST_EQ);
         ASTEqualityExpression e = acu.findDescendantsOfType(ASTEqualityExpression.class).get(0);
         ASTMethodDeclaration method = e.getFirstParentOfType(ASTMethodDeclaration.class);
         Scope s = method.getScope();
-        Map m = s.getVariableDeclarations();
-        for (Iterator i = m.keySet().iterator(); i.hasNext();) {
-            VariableNameDeclaration vnd = (VariableNameDeclaration)i.next();
-            Node node = vnd.getNode();
-            //System.out.println();
+        Map<VariableNameDeclaration, List<NameOccurrence>> m = s.getVariableDeclarations();
+        assertEquals(2, m.size());
+        for (Map.Entry<VariableNameDeclaration, List<NameOccurrence>> entry : m.entrySet()) {
+            VariableNameDeclaration vnd = entry.getKey();
+            List<NameOccurrence> usages = entry.getValue();
+
+            if (vnd.getImage().equals("a") || vnd.getImage().equals("b")) {
+                assertEquals(1, usages.size());
+                assertEquals(3, usages.get(0).getLocation().getBeginLine());
+            } else {
+                fail("Unkown variable " + vnd);
+            }
         }
-        //System.out.println(m.size());
     }
 
     @Test
     public void testFieldFinder() {
-        //FIXME - Does this test do anything?
-        //Not really, I think it's just a demo -- Tom
-
-/*
-        System.out.println(TEST_FIELD);
         parseCode(TEST_FIELD);
-
-        List<ASTVariableDeclaratorId> variableDeclaratorIds = acu.findDescendantsOfType(ASTVariableDeclaratorId.class);
-        ASTVariableDeclaratorId declaration = null;
-        for (Iterator iter = variableDeclaratorIds.iterator(); iter.hasNext();) {
-            declaration = (ASTVariableDeclaratorId) iter.next();
-            if ("b".equals(declaration.getImage()))
-                break;
-        }
-        NameOccurrence no = declaration.getUsages().iterator().next();
-        SimpleNode location = no.getLocation();
-        System.out.println("variable " + declaration.getImage() + " is used here: " + location.getImage());
-*/
+//        System.out.println(TEST_FIELD);
+
+        ASTVariableDeclaratorId declaration = acu.findDescendantsOfType(ASTVariableDeclaratorId.class).get(1);
+        assertEquals(3, declaration.getBeginLine());
+        assertEquals("bbbbbbbbbb", declaration.getImage());
+        assertEquals(1, declaration.getUsages().size());
+        NameOccurrence no = declaration.getUsages().get(0);
+        JavaNode location = no.getLocation();
+        assertEquals(6, location.getBeginLine());
+//        System.out.println("variable " + declaration.getImage() + " is used here: " + location.getImage());
     }
 
-    @Ignore
     @Test
     public void testDemo() {
         parseCode(TEST_DEMO);
-        System.out.println(TEST_DEMO);
+//        System.out.println(TEST_DEMO);
         ASTMethodDeclaration node = acu.findDescendantsOfType(ASTMethodDeclaration.class).get(0);
         Scope s = node.getScope();
-        Map m = s.getVariableDeclarations();
-        for (Iterator i = m.keySet().iterator(); i.hasNext();) {
-            VariableNameDeclaration d = (VariableNameDeclaration) i.next();
-            System.out.println("Variable: " + d.getImage());
-            System.out.println("Type: " + d.getTypeImage());
+        Map<VariableNameDeclaration, List<NameOccurrence>> m = s.getVariableDeclarations();
+        for (Iterator<VariableNameDeclaration> i = m.keySet().iterator(); i.hasNext();) {
+            VariableNameDeclaration d = i.next();
+            assertEquals("buz", d.getImage());
+            assertEquals("ArrayList", d.getTypeImage());
+            List<NameOccurrence> u = m.get(d);
+            assertEquals(1, u.size());
+            NameOccurrence o = u.get(0);
+            int beginLine = o.getLocation().getBeginLine();
+            assertEquals(3, beginLine);
+
+//            System.out.println("Variable: " + d.getImage());
+//            System.out.println("Type: " + d.getTypeImage());
+//            System.out.println("Usages: " + u.size());
+//            System.out.println("Used in line " + beginLine);
         }
     }
-/*
-            List u = (List)m.get(d);
-            System.out.println("Usages: " + u.size());
-            NameOccurrence o = (NameOccurrence)u.get(0);
-            int beginLine = o.getLocation().getBeginLine();
-            System.out.println("Used in line " + beginLine);
-*/
+
+    @Test
+    public void testEnum() {
+	parseCode(NameOccurrencesTest.TEST_ENUM);
+
+	ASTVariableDeclaratorId vdi = acu.findDescendantsOfType(ASTVariableDeclaratorId.class).get(0);
+	List<NameOccurrence> usages = vdi.getUsages();
+	assertEquals(2, usages.size());
+	assertEquals(5, usages.get(0).getLocation().getBeginLine());
+	assertEquals(9, usages.get(1).getLocation().getBeginLine());
+    }
 
     private static final String TEST_DEMO =
             "public class Foo  {" + PMD.EOL +
             " void bar(ArrayList buz) { " + PMD.EOL +
+            "  buz.add(\"foo\");" + PMD.EOL +
             " } " + PMD.EOL +
             "}" + PMD.EOL;
 
@@ -154,10 +165,15 @@ public void testDemo() {
             "}" + PMD.EOL;
 
     private static final String TEST_FIELD =
-    "public class MyClass {" + PMD.EOL +
-    " private int a; " + PMD.EOL +
-    " boolean b = MyClass.ASCENDING; " + PMD.EOL +
-    "}" + PMD.EOL;
+            "public class MyClass {" + PMD.EOL +
+            " private int aaaaaaaaaa; " + PMD.EOL +
+            " boolean bbbbbbbbbb = MyClass.ASCENDING; " + PMD.EOL +
+            " private int zzzzzzzzzz;" + PMD.EOL +
+            " private void doIt() {" + PMD.EOL +
+            "  if (bbbbbbbbbb) {" + PMD.EOL +
+            "  }" + PMD.EOL +
+            " }" + PMD.EOL +
+            "}" + PMD.EOL;
 
     public static junit.framework.Test suite() {
         return new junit.framework.JUnit4TestAdapter(AcceptanceTest.class);