Added more tests

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/ForLoopShouldBeForeachRule.java

@@ -18,7 +18,6 @@ import net.sourceforge.pmd.lang.java.ast.ASTName;
 import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix;
 import net.sourceforge.pmd.lang.java.ast.ASTPrimarySuffix;
 import net.sourceforge.pmd.lang.java.ast.ASTRelationalExpression;
-import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId;
 import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
 import net.sourceforge.pmd.lang.java.symboltable.VariableNameDeclaration;
 import net.sourceforge.pmd.lang.symboltable.NameOccurrence;
@@ -29,69 +28,63 @@ import net.sourceforge.pmd.lang.symboltable.Scope;
  */
 public class ForLoopShouldBeForeachRule extends AbstractJavaRule {
 
-
     @Override
     public Object visit(ASTForStatement node, Object data) {
 
-        ASTForInit init = node.getFirstChildOfType(ASTForInit.class);
+        final ASTForInit init = node.getFirstChildOfType(ASTForInit.class);
+        final ASTForUpdate update = node.getFirstChildOfType(ASTForUpdate.class);
+        final ASTExpression guardCondition = node.getFirstChildOfType(ASTExpression.class);
 
-        if (init == null) {
+        if (init == null && update == null || guardCondition == null) { // The loop may be replaced with a while
             return super.visit(node, data);
         }
 
-        String itName = init.getFirstDescendantOfType(ASTVariableDeclaratorId.class).getImage();
+        VariableNameDeclaration index = getIndexVarDeclaration(init, update);
-
+        if (index == null || !"int".equals(index.getTypeImage())) {
-
-        ASTExpression guardCondition = node.getFirstChildOfType(ASTExpression.class);
-
-        if (guardCondition == null || !isForUpdateSimpleEnough(node.getFirstChildOfType(ASTForUpdate.class), itName)) {
             return super.visit(node, data);
         }
 
-        String iterableName = getIterableNameAndCheckGuardExpression(guardCondition, itName);
+        String itName = index.getName();
+        String iterableName = getIterableNameOrNullToAbort(guardCondition, itName);
 
 
-        if (iterableName == null) {
+        if (!isForUpdateSimpleEnough(update, itName) || iterableName == null) {
             return super.visit(node, data);
         }
 
-        List<NameOccurrence> occurrences = getIteratorOccurencesAndCheckInit(init);
+
+        List<NameOccurrence> occurrences = guardCondition.getScope().getDeclarations(VariableNameDeclaration.class).get(index);
 
         if (occurrences == null) {
             return super.visit(node, data);
         }
 
-
         VariableNameDeclaration iterableDeclaration = findDeclaration(iterableName, node.getScope());
 
         if (iterableDeclaration == null) {
             return super.visit(node, data);
         }
 
-        if (iterableDeclaration.isArray()) {
+        if (iterableDeclaration.isArray() && loopOverArrayCanBeReplaced(node)) {
-            loopOverArrayCanBeReplaced(node);
+            addViolation(data, node);
-        } else if ("List".equals(iterableDeclaration.getTypeImage())) {
+        } else if ("List".equals(iterableDeclaration.getTypeImage())
-            if (loopOverListCanBeReplaced(node, occurrences, iterableDeclaration)) {
+            && loopOverListCanBeReplaced(node, occurrences, iterableDeclaration)) {
             addViolation(data, node);
-            }
         }
 
         return super.visit(node, data);
     }
 
 
-    /**
+    /* Finds the declaration of the index variable to find its name occurrences */
-     * Gets the name occurences of the iterator in the code block, and checks that the iterator is of type int
+    private VariableNameDeclaration getIndexVarDeclaration(ASTForInit init, ASTForUpdate update) {
-     *
+        if (init == null) {
-     * @param init For init
+            return guessIndexVarFromUpdate(update);
-     *
+        }
-     * @return name occurences or null (then abort)
+
-     */
-    private List<NameOccurrence> getIteratorOccurencesAndCheckInit(ASTForInit init) {
         Map<VariableNameDeclaration, List<NameOccurrence>> decls = init.getScope().getDeclarations(VariableNameDeclaration.class);
         VariableNameDeclaration theIterator = null;
 
-
         for (VariableNameDeclaration decl : decls.keySet()) {
             ASTForInit declInit = decl.getNode().getFirstParentOfType(ASTForInit.class);
             if (declInit == init) {
@@ -100,11 +93,26 @@ public class ForLoopShouldBeForeachRule extends AbstractJavaRule {
             }
         }
 
-        if (theIterator == null || !"int".equals(theIterator.getTypeImage())) {
+        return theIterator;
+
+    }
+
+
+    /** Does a best guess to find the index variable, gives up if the update has several statements */
+    private VariableNameDeclaration guessIndexVarFromUpdate(ASTForUpdate update) {
+
+        Node name;
+        try {
+            name = update.findChildNodesWithXPath(getSimpleForStatementXpath(null)).get(0);
+        } catch (JaxenException je) {
+            throw new RuntimeException(je);
+        }
+
+        if (name == null || name.getImage() == null) {
             return null;
         }
 
-        return decls.get(theIterator);
+        return findDeclaration(name.getImage(), update.getScope().getParent());
     }
 
 
@@ -112,24 +120,29 @@ public class ForLoopShouldBeForeachRule extends AbstractJavaRule {
      * @return true if there's only one update statement of the form i++ or ++i.
      */
     private boolean isForUpdateSimpleEnough(ASTForUpdate update, String itName) {
-        return update.hasDescendantMatchingXPath("//StatementExpressionList[count(*)=1]"
+        return update.hasDescendantMatchingXPath(getSimpleForStatementXpath(itName));
+    }
+
+
+    private String getSimpleForStatementXpath(String itName) {
+        return "//StatementExpressionList[count(*)=1]"
             + "/StatementExpression"
             + "/*[self::PostfixExpression and @Image='++' or self::PreIncrementExpression]"
             + "/PrimaryExpression"
             + "/PrimaryPrefix"
-                                                     + "/Name[@Image='" + itName + "']");
+            + "/Name"
+            + (itName == null ? "" : ("[@Image='" + itName + "']"));
     }
 
 
     /**
      * Gets the name of the iterable array or list.
      *
-     * @param guardCondition The guard condition
      * @param itName The name of the iterator variable
      *
      * @return The name, or null if it couldn't be found or the guard condition is not safe to refactor (then abort)
      */
-    private String getIterableNameAndCheckGuardExpression(ASTExpression guardCondition, String itName) {
+    private String getIterableNameOrNullToAbort(ASTExpression guardCondition, String itName) {
 
 
         if (guardCondition.jjtGetNumChildren() > 0
@@ -137,13 +150,11 @@ public class ForLoopShouldBeForeachRule extends AbstractJavaRule {
 
             ASTRelationalExpression relationalExpression = (ASTRelationalExpression) guardCondition.jjtGetChild(0);
 
-            if (relationalExpression.hasImageEqualTo("<")) {
+            if (relationalExpression.hasImageEqualTo("<") || relationalExpression.hasImageEqualTo("<=")) {
 
                 try {
-                    List<Node> left
+                    List<Node> left = guardCondition.findChildNodesWithXPath(
-                        = guardCondition.findChildNodesWithXPath(
+                        "//RelationalExpression/PrimaryExpression/PrimaryPrefix/Name[@Image='" + itName + "']");
-                        "//RelationalExpression[@Image='<']/PrimaryExpression/PrimaryPrefix/Name[@Image='" + itName
-                            + "']");
 
                     List<Node> right = guardCondition.findChildNodesWithXPath(
                         "//RelationalExpression[@Image='<']/PrimaryExpression/PrimaryPrefix"
@@ -161,9 +172,8 @@ public class ForLoopShouldBeForeachRule extends AbstractJavaRule {
                         return null;
                     }
 
-                } catch (JaxenException e) {
+                } catch (JaxenException je) {
-                    e.printStackTrace();
+                    throw new RuntimeException(je);
-                    return null;
                 }
             }
         }
@@ -177,7 +187,7 @@ public class ForLoopShouldBeForeachRule extends AbstractJavaRule {
     }
 
 
-    private boolean loopOverListCanBeReplaced(ASTForStatement node, List<NameOccurrence> occurrences,
+    private boolean loopOverListCanBeReplaced(ASTForStatement stmt, List<NameOccurrence> occurrences,
                                               VariableNameDeclaration listDeclaration) {
 
         String listName = listDeclaration.getName();
@@ -185,9 +195,9 @@ public class ForLoopShouldBeForeachRule extends AbstractJavaRule {
 
         for (NameOccurrence occ : occurrences) {
 
-
             if (occ.getLocation().getFirstParentOfType(ASTForUpdate.class) == null
-                && occ.getLocation().getFirstParentOfType(ASTExpression.class) != node.getFirstChildOfType(ASTExpression.class)
+                && occ.getLocation().getFirstParentOfType(ASTExpression.class)
+                != stmt.getFirstChildOfType(ASTExpression.class)
                 && !occurenceIsListGet(occ, listName)) {
                 return false;
             }
@@ -236,6 +246,4 @@ public class ForLoopShouldBeForeachRule extends AbstractJavaRule {
 
         return null;
     }
-
-
 }

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/ForLoopShouldBeForeach.xml

@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <test-data>
     <test-code>
-        <description>Simple failure case</description>
+        <description>Positive with list</description>
         <expected-problems>1</expected-problems>
         <code><![CDATA[
         public class MyClass {
@@ -13,4 +13,97 @@
         }
         ]]></code>
     </test-code>
+
+    <test-code>
+        <description>Positive with lower or equal</description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+        public class Foo {
+          void loop(List<String> lo) {
+            for (int i = 0; i <= lo.size() - 1; i++) {
+              System.out.println(lo.get(i));
+            }
+          }
+        }
+        ]]></code>
+    </test-code>
+-
+    <test-code>
+        <description>Usage of index var outside get</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+        public class MyClass {
+          void loop(List<String> l) {
+            for (int i = 0; i < l.size(); i++) {
+              System.out.println(i + ": " + l.get(i));
+
+            }
+          }
+        }
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Subclass of List</description>
+        <!-- TODO -> violation, may use typeresolution -->
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+        public class MyClass {
+          void loop(ArrayList<String> l) {
+            for (int i = 0; i < l.size(); i++) {
+              System.out.println(l.get(i));
+
+            }
+          }
+        }
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Get called on another list</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+        public class MyClass {
+          void loop(List<String> l) {
+            List<String> l2 = new ArrayList<>(l);
+            for (int i = 0; i < l.size(); i++) {
+              System.out.println(l2.get(i));
+            }
+          }
+        }
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Backwards iteration</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+        public class MyClass {
+          void loop(List<String> l) {
+            for (int i = l.size() - 1; i > 0; i--) {
+              System.out.println(i + ": " + l.get(i));
+            }
+          }
+        }
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Index var initialized outside for init</description>
+        <!-- TODO -> violation? maybe it's not even in the scope of this rule -> create AvoidEmptyForInit? -->
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+        public class MyClass {
+          void loop(List<String> l) {
+            int i = 0;
+            for (; i < l.size(); i++) {
+              System.out.println(l.get(i));
+            }
+          }
+        }
+        ]]></code>
+    </test-code>
+
+
+
 </test-data>