Fix #4569 forloopcanbeforeach FP with non-unit stride

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/JavaAstUtils.java

@@ -65,6 +65,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTUnaryExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTVariableAccess;
 import net.sourceforge.pmd.lang.java.ast.ASTVariableId;
 import net.sourceforge.pmd.lang.java.ast.Annotatable;
+import net.sourceforge.pmd.lang.java.ast.AssignmentOp;
 import net.sourceforge.pmd.lang.java.ast.BinaryOp;
 import net.sourceforge.pmd.lang.java.ast.JavaNode;
 import net.sourceforge.pmd.lang.java.ast.JavaTokenKinds;
@@ -738,6 +739,18 @@ public final class JavaAstUtils {
         return false;
     }
 
+
+    /**
+     * Tests if the node is an {@link ASTAssignmentExpression} with the given operator.
+     */
+    public static boolean isAssignmentExprWithOperator(@Nullable JavaNode e, AssignmentOp operator) {
+        if (e instanceof ASTAssignmentExpression) {
+            ASTAssignmentExpression infix = (ASTAssignmentExpression) e;
+            return operator == infix.getOperator();
+        }
+        return false;
+    }
+
     /**
      * Returns true if the given token is a Java comment.
      */

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/ForLoopCanBeForeachRule.java

@@ -15,8 +15,10 @@ import org.checkerframework.checker.nullness.qual.Nullable;
 import net.sourceforge.pmd.lang.ast.NodeStream;
 import net.sourceforge.pmd.lang.java.ast.ASTArgumentList;
 import net.sourceforge.pmd.lang.java.ast.ASTArrayAccess;
+import net.sourceforge.pmd.lang.java.ast.ASTAssignableExpr;
 import net.sourceforge.pmd.lang.java.ast.ASTAssignableExpr.ASTNamedReferenceExpr;
 import net.sourceforge.pmd.lang.java.ast.ASTAssignableExpr.AccessType;
+import net.sourceforge.pmd.lang.java.ast.ASTAssignmentExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTFieldAccess;
 import net.sourceforge.pmd.lang.java.ast.ASTForStatement;
@@ -28,6 +30,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTStatementExpressionList;
 import net.sourceforge.pmd.lang.java.ast.ASTUnaryExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTVariableAccess;
 import net.sourceforge.pmd.lang.java.ast.ASTVariableId;
+import net.sourceforge.pmd.lang.java.ast.AssignmentOp;
 import net.sourceforge.pmd.lang.java.ast.BinaryOp;
 import net.sourceforge.pmd.lang.java.ast.internal.JavaAstUtils;
 import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
@@ -88,7 +91,9 @@ public class ForLoopCanBeForeachRule extends AbstractJavaRulechainRule {
     }
 
 
-    /** Finds the declaration of the index variable and its occurrences, null to abort */
+    /**
+     * Finds the declaration of the index variable and its occurrences, null to abort
+     */
     private @Nullable ASTVariableId getIndexVarDeclaration(@Nullable ASTStatement init, ASTStatementExpressionList update) {
         if (init == null) {
             return guessIndexVarFromUpdate(update);
@@ -113,8 +118,20 @@ public class ForLoopCanBeForeachRule extends AbstractJavaRulechainRule {
     private @Nullable ASTVariableId guessIndexVarFromUpdate(ASTStatementExpressionList update) {
         return NodeStream.of(update)
                          .filter(it -> it.getNumChildren() == 1)
-                         .firstChild(ASTUnaryExpression.class)
-                         .map(this::asIPlusPlus)
+                         .map(it -> it.getFirstChild())
+                         .map(it -> {
+                             if (it instanceof ASTUnaryExpression)
+                                 return asIPlusPlus(it);
+                             else if (JavaAstUtils.isAssignmentExprWithOperator(it, AssignmentOp.ADD_ASSIGN)
+                                 && JavaAstUtils.isLiteralInt(((ASTAssignmentExpression) it).getRightOperand(), 1)) {
+                                 // x += 1
+                                 ASTAssignableExpr lhs = ((ASTAssignmentExpression) it).getLeftOperand();
+                                 JVariableSymbol sym = lhs instanceof ASTNamedReferenceExpr
+                                     ? ((ASTNamedReferenceExpr) lhs).getReferencedSym() : null;
+                                 return sym == null ? null : sym.tryGetNode();
+                             }
+                             return null;
+                         })
                          .first();
     }
 
@@ -135,7 +152,6 @@ public class ForLoopCanBeForeachRule extends AbstractJavaRulechainRule {
      * Gets the name of the iterable array or list. The condition has the form i < arr.length or i < coll.size()
      *
      * @param indexVar The index variable
-     *
      * @return The name, or null if it couldn't be found or the guard condition is not safe to refactor (then abort)
      */
     private @Nullable ASTNamedReferenceExpr findIterableFromCondition(ASTExpression guardCondition, ASTVariableId indexVar) {
@@ -185,11 +201,17 @@ public class ForLoopCanBeForeachRule extends AbstractJavaRulechainRule {
     }
 
 
+    private boolean isSimpleIncrementUpdate(ASTStatementExpressionList update, ASTVariableId indexVar) {
+        ASTVariableId guess = guessIndexVarFromUpdate(update);
+        return guess != null && guess == indexVar;
+    }
+
     private boolean isReplaceableArrayLoop(ASTForStatement loop,
                                            ASTVariableId index,
                                            ASTNamedReferenceExpr arrayDeclaration) {
 
         return arrayDeclaration.getTypeMirror().isArray()
+            && isSimpleIncrementUpdate(loop.getUpdate(), index)
             && occurrencesMatch(loop, index, arrayDeclaration, (i, iterable, expr) -> isArrayAccessIndex(expr, iterable));
 
     }
@@ -200,6 +222,7 @@ public class ForLoopCanBeForeachRule extends AbstractJavaRulechainRule {
                                           ASTNamedReferenceExpr listDeclaration) {
 
         return TypeTestUtil.isA(List.class, listDeclaration.getTypeMirror())
+            && isSimpleIncrementUpdate(loop.getUpdate(), index)
             && occurrencesMatch(loop, index, listDeclaration, (i, iterable, expr) -> isListGetIndex(expr, iterable));
     }
 

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/ForLoopCanBeForeach.xml

@@ -426,4 +426,30 @@ public class Test {
             }
             ]]></code>
     </test-code>
+    <test-code>
+        <description>#4569 non-unit stride</description>
+        <expected-problems>1</expected-problems>
+        <expected-linenumbers>11</expected-linenumbers>
+        <code><![CDATA[
+            import java.util.*;
+
+            public class Test {
+                public static void main(String[] args) {
+                    List<Integer> list = List.of(1, 2, 3, 4, 5, 6);
+
+                    for (int i = 0; i < list.size(); i += 2) { // Ok
+                        System.out.println(list.get(i));
+                    }
+
+                    for (int i = 0; i < list.size(); i += 1) { // Not Ok
+                        System.out.println(list.get(i));
+                    }
+
+                    for (int i = 0; i < list.size(); i += 3) { // Ok
+                        System.out.println(list.get(i));
+                    }
+                }
+            }
+            ]]></code>
+    </test-code>
 </test-data>