GROOVY-11857: encapsulate ForStatement without value variable

paulk-asert · paulk-asert · commit 47fd44803ed2 · 2026-02-08T21:18:10.000+10:00
diff --git a/src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java b/src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java
@@ -521,7 +521,7 @@ public Function<Statement, ForStatement> visitOriginalForControl(final OriginalF
           .map(e -> (Expression) this.visit(e)).orElse(EmptyExpression.INSTANCE));
         closureListExpression.addExpression(this.visitForUpdate(ctx.forUpdate()));
 
-        return (body) -> new ForStatement(ForStatement.FOR_LOOP_DUMMY, closureListExpression, body);
+        return (body) -> new ForStatement(closureListExpression, body);
     }
 
     @Override
diff --git a/src/main/java/org/codehaus/groovy/ast/ClassCodeExpressionTransformer.java b/src/main/java/org/codehaus/groovy/ast/ClassCodeExpressionTransformer.java
@@ -137,7 +137,9 @@ public void visitExpressionStatement(final ExpressionStatement stmt) {
 
     @Override
     public void visitForLoop(final ForStatement stmt) {
-        visitAnnotations(stmt.getVariable()); // "for(T x : y)" or "for(x in y)"
+        if (stmt.getValueVariable() != null) {
+            visitAnnotations(stmt.getValueVariable()); // "for(T x : y)" or "for(x in y)"
+        }
         stmt.setCollectionExpression(transform(stmt.getCollectionExpression()));
         stmt.getLoopBlock().visit(this);
     }
diff --git a/src/main/java/org/codehaus/groovy/ast/ClassCodeVisitorSupport.java b/src/main/java/org/codehaus/groovy/ast/ClassCodeVisitorSupport.java
@@ -215,7 +215,9 @@ public void visitExpressionStatement(ExpressionStatement statement) {
     @Override
     public void visitForLoop(ForStatement statement) {
         visitStatement(statement);
-        visitAnnotations(statement.getVariable());
+        if (statement.getValueVariable() != null) {
+            visitAnnotations(statement.getValueVariable());
+        }
         super.visitForLoop(statement);
     }
 
diff --git a/src/main/java/org/codehaus/groovy/ast/stmt/ForStatement.java b/src/main/java/org/codehaus/groovy/ast/stmt/ForStatement.java
@@ -23,6 +23,7 @@
 import org.codehaus.groovy.ast.GroovyCodeVisitor;
 import org.codehaus.groovy.ast.Parameter;
 import org.codehaus.groovy.ast.VariableScope;
+import org.codehaus.groovy.ast.expr.ClosureListExpression;
 import org.codehaus.groovy.ast.expr.Expression;
 
 import static java.util.Objects.requireNonNull;
@@ -32,7 +33,9 @@
  */
 public class ForStatement extends Statement implements LoopingStatement {
 
-    public static final Parameter FOR_LOOP_DUMMY = new Parameter(ClassHelper.OBJECT_TYPE, "forLoopDummyParameter");
+    private static final Parameter DUMMY_VALUE_VARIABLE = new Parameter(ClassHelper.OBJECT_TYPE, "forLoopDummyParameter");
+    @Deprecated(since = "6.0.0")
+    public static final Parameter FOR_LOOP_DUMMY = DUMMY_VALUE_VARIABLE;
 
     private final Parameter indexVariable, valueVariable;
     private Expression collectionExpression;
@@ -52,6 +55,15 @@ public ForStatement(final Parameter variable, final Expression collectionExpress
         this(null, variable, collectionExpression, loopBlock);
     }
 
+    /**
+     * A constructor for C-style for loops.
+     *
+     * @since 6.0.0
+     */
+    public ForStatement(final ClosureListExpression closureListExpression, final Statement loopBlock) {
+        this(DUMMY_VALUE_VARIABLE, closureListExpression, loopBlock);
+    }
+
     public void setCollectionExpression(final Expression collectionExpression) {
         this.collectionExpression = requireNonNull(collectionExpression);
     }
@@ -74,7 +86,7 @@ public Parameter getIndexVariable() {
      * @since 5.0.0
      */
     public Parameter getValueVariable() {
-        return valueVariable != FOR_LOOP_DUMMY ? valueVariable : null;
+        return valueVariable != DUMMY_VALUE_VARIABLE ? valueVariable : null;
     }
 
     @Deprecated(since = "5.0.0")
diff --git a/src/main/java/org/codehaus/groovy/classgen/ClassCompletionVerifier.java b/src/main/java/org/codehaus/groovy/classgen/ClassCompletionVerifier.java
@@ -792,9 +792,15 @@ public void visitCatchStatement(final CatchStatement cs) {
 
     @Override
     public void visitForLoop(final ForStatement fs) {
-        List<String> modifiers = new ArrayList<>();
-        int mods = fs.getVariable().getModifiers();
+        if (fs.getValueVariable() != null) {
+            checkModifiers(fs, fs.getValueVariable());
+        }
+        super.visitForLoop(fs);
+    }
 
+    private void checkModifiers(ForStatement fs, Parameter p) {
+        List<String> modifiers = new ArrayList<>();
+        int mods = p.getModifiers();
         if (isAbstract (mods)) modifiers.add("abstract" );
         if (isPrivate  (mods)) modifiers.add("private"  );
         if (isProtected(mods)) modifiers.add("protected");
@@ -803,10 +809,8 @@ public void visitForLoop(final ForStatement fs) {
         if (isStrict   (mods)) modifiers.add("strictfp" );
 
         for (String modifier : modifiers) {
-            addError("The variable '" + fs.getVariable().getName() + "' has invalid modifier " + modifier + ".", fs);
+            addError("The variable '" + p.getName() + "' has invalid modifier " + modifier + ".", fs);
         }
-
-        super.visitForLoop(fs);
     }
 
     @Override
diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesStatementWriter.java b/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesStatementWriter.java
@@ -97,7 +97,7 @@ protected void writeForInLoop(final ForStatement loop) {
         ClassNode collectionType = controller.getTypeChooser().resolveType(collectionExpression, controller.getClassNode());
 
         int mark = operandStack.getStackLength();
-        if (collectionType.isArray() && collectionType.getComponentType().equals(loop.getVariableType())) {
+        if (collectionType.isArray() && collectionType.getComponentType().equals(loop.getValueVariable().getType())) {
             writeOptimizedForEachLoop(loop, collectionExpression, collectionType);
         } else if (GeneralUtils.isOrImplements(collectionType, ENUMERATION_CLASSNODE)) {
             writeEnumerationBasedForEachLoop(loop, collectionExpression, collectionType);
diff --git a/src/main/java/org/codehaus/groovy/control/ResolveVisitor.java b/src/main/java/org/codehaus/groovy/control/ResolveVisitor.java
@@ -1392,7 +1392,9 @@ public void visitCatchStatement(final CatchStatement cs) {
 
     @Override
     public void visitForLoop(final ForStatement forLoop) {
-        resolveOrFail(forLoop.getVariableType(), forLoop);
+        if (forLoop.getValueVariable() != null) {
+            resolveOrFail(forLoop.getValueVariable().getType(), forLoop);
+        }
         super.visitForLoop(forLoop);
     }
 
diff --git a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
@@ -2176,7 +2176,7 @@ public void visitForLoop(final ForStatement forLoop) {
             visitStatement(forLoop);
             collectionExpression.visit(this);
             ClassNode collectionType = getType(collectionExpression);
-            ClassNode forLoopVariableType = forLoop.getVariableType();
+            ClassNode forLoopVariableType = forLoop.getValueVariable().getType();
             ClassNode componentType;
             if (isStringType(collectionType)) {
                 if (isWrapperCharacter(getWrapper(forLoopVariableType))) {
diff --git a/src/testFixtures/groovy/org/codehaus/groovy/ast/builder/AstAssert.groovy b/src/testFixtures/groovy/org/codehaus/groovy/ast/builder/AstAssert.groovy
@@ -237,7 +237,11 @@ class AstAssert {
                 assertSyntaxTree([expected.arguments], [actual.arguments])
             },
             ForStatement : { expected, actual ->
-                assertSyntaxTree([expected.variable], [actual.variable])
+                assert expected.valueVariable.asBoolean() == actual.valueVariable.asBoolean()
+                if (expected.valueVariable) {
+                    assertSyntaxTree([expected.valueVariable], [actual.valueVariable])
+                }
+                assertSyntaxTree([expected.indexVariable], [actual.indexVariable])
                 assertSyntaxTree([expected.collectionExpression], [actual.collectionExpression])
                 assertSyntaxTree([expected.loopBlock], [actual.loopBlock])
             },
diff --git a/subprojects/groovy-astbuilder/src/test/groovy/org/codehaus/groovy/ast/builder/AstBuilderFromCodeTest.groovy b/subprojects/groovy-astbuilder/src/test/groovy/org/codehaus/groovy/ast/builder/AstBuilderFromCodeTest.groovy
@@ -413,39 +413,38 @@ class AstBuilderFromCodeTest {
         }
 
         def expected = new BlockStatement([new ForStatement(
-                new Parameter(ClassHelper.OBJECT_TYPE, "forLoopDummyParameter"),
-                new ClosureListExpression(
-                        [
-                                new DeclarationExpression(
-                                        new VariableExpression("x"),
-                                        new Token(Types.EQUALS, "=", -1, -1),
-                                        new ConstantExpression(0)
-                                ),
-                                new BinaryExpression(
-                                        new VariableExpression("x"),
-                                        new Token(Types.COMPARE_LESS_THAN, "<", -1, -1),
-                                        new ConstantExpression(10)
-                                ),
-                                new PostfixExpression(
-                                        new VariableExpression("x"),
-                                        new Token(Types.PLUS_PLUS, "++", -1, -1)
-                                )
-                        ]
-                ),
-                new BlockStatement(
-                        [
-                                new ExpressionStatement(
-                                        new MethodCallExpression(
-                                                new VariableExpression("this"),
-                                                new ConstantExpression("println"),
-                                                new ArgumentListExpression(
-                                                        new VariableExpression("x"),
-                                                )
-                                        )
-                                )
-                        ],
-                        new VariableScope()
-                )
+            new ClosureListExpression(
+                [
+                    new DeclarationExpression(
+                        new VariableExpression("x"),
+                        new Token(Types.EQUALS, "=", -1, -1),
+                        new ConstantExpression(0)
+                    ),
+                    new BinaryExpression(
+                        new VariableExpression("x"),
+                        new Token(Types.COMPARE_LESS_THAN, "<", -1, -1),
+                        new ConstantExpression(10)
+                    ),
+                    new PostfixExpression(
+                        new VariableExpression("x"),
+                        new Token(Types.PLUS_PLUS, "++", -1, -1)
+                    )
+                ]
+            ),
+            new BlockStatement(
+                [
+                    new ExpressionStatement(
+                        new MethodCallExpression(
+                            new VariableExpression("this"),
+                            new ConstantExpression("println"),
+                            new ArgumentListExpression(
+                                new VariableExpression("x"),
+                            )
+                        )
+                    )
+                ],
+                new VariableScope()
+            )
         )], new VariableScope())
 
         AstAssert.assertSyntaxTree([expected], result)

Original file line number	Diff line number	Diff line change
`@@ -521,7 +521,7 @@ public Function<Statement, ForStatement> visitOriginalForControl(final OriginalF`
`521`	`521`	`.map(e -> (Expression) this.visit(e)).orElse(EmptyExpression.INSTANCE));`
`522`	`522`	`closureListExpression.addExpression(this.visitForUpdate(ctx.forUpdate()));`
`523`	`523`
`524`		`- return (body) -> new ForStatement(ForStatement.FOR_LOOP_DUMMY, closureListExpression, body);`
	`524`	`+ return (body) -> new ForStatement(closureListExpression, body);`
`525`	`525`	`}`
`526`	`526`
`527`	`527`	`@Override`
Original file line number	Diff line number	Diff line change
`@@ -1392,7 +1392,9 @@ public void visitCatchStatement(final CatchStatement cs) {`
`1392`	`1392`
`1393`	`1393`	`@Override`
`1394`	`1394`	`public void visitForLoop(final ForStatement forLoop) {`
`1395`		`- resolveOrFail(forLoop.getVariableType(), forLoop);`
	`1395`	`+ if (forLoop.getValueVariable() != null) {`
	`1396`	`+ resolveOrFail(forLoop.getValueVariable().getType(), forLoop);`
	`1397`	`+ }`
`1396`	`1398`	`super.visitForLoop(forLoop);`
`1397`	`1399`	`}`
`1398`	`1400`