Copilot commented on code in PR #2378:
URL: https://github.com/apache/groovy/pull/2378#discussion_r2779126902
##########
src/main/java/org/codehaus/groovy/ast/stmt/ForStatement.java:
##########
@@ -52,6 +54,13 @@ public ForStatement(final Parameter variable, final
Expression collectionExpress
this(null, variable, collectionExpression, loopBlock);
}
+ /**
+ * @since 6.0.0
+ */
+ public ForStatement(final Expression collectionExpression, final Statement
loopBlock) {
+ this(DUMMY_VALUE_VARIABLE, collectionExpression, loopBlock);
+ }
Review Comment:
The new `ForStatement(Expression collectionExpression, Statement loopBlock)`
constructor accepts any `Expression`, but creates a statement with no value
variable (dummy), which makes the AST invalid for non-`ClosureListExpression`
collections and can lead to downstream NPEs (e.g., visitors assume
`getValueVariable()` is non-null for for-in loops). Consider tightening the
signature to `ClosureListExpression` (or validating `collectionExpression
instanceof ClosureListExpression` and throwing an `IllegalArgumentException`)
and documenting that this constructor is only for C-style
`for(init;cond;update)` loops.
##########
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;
Review Comment:
`forLoop.getValueVariable()` can be `null` (e.g., C-style loops use the
dummy value variable which `getValueVariable()` intentionally hides). While
this branch currently excludes `ClosureListExpression`, a
programmatically-constructed `ForStatement` without a value variable but with a
non-`ClosureListExpression` collection would NPE here. Consider guarding
against `null` and either falling back to `getVariable()`/`getVariableType()`
or producing a clearer static type checking error for malformed loops.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
