Copilot commented on code in PR #2378:
URL: https://github.com/apache/groovy/pull/2378#discussion_r2776569136
##########
src/testFixtures/groovy/org/codehaus/groovy/ast/builder/AstAssert.groovy:
##########
@@ -237,7 +237,10 @@ class AstAssert {
assertSyntaxTree([expected.arguments], [actual.arguments])
},
ForStatement : { expected, actual ->
- assertSyntaxTree([expected.variable], [actual.variable])
+ if (expected.hasValueVariable()) {
+ assertSyntaxTree([expected.valueVariable],
[actual.valueVariable])
+ }
+ assertSyntaxTree([expected.indexVariable],
[actual.indexVariable])
Review Comment:
The ForStatement assertion only compares valueVariable when
`expected.hasValueVariable()` is true, but it doesn't assert that `actual` has
the same presence/absence. This can let a regression slip through where the
actual AST unexpectedly has a value variable (or vice versa) and the test still
passes. Consider first asserting `expected.hasValueVariable() ==
actual.hasValueVariable()`, and then comparing `valueVariable` when present (or
asserting it is null when absent).
##########
src/main/java/org/codehaus/groovy/ast/stmt/ForStatement.java:
##########
@@ -74,7 +83,14 @@ public Parameter getIndexVariable() {
* @since 5.0.0
*/
public Parameter getValueVariable() {
- return valueVariable != FOR_LOOP_DUMMY ? valueVariable : null;
+ return valueVariable != DUMMY_VALUE_VARIABLE ? valueVariable : null;
+ }
+
+ /**
+ * @since 6.0.0
+ */
+ public boolean hasValueVariable() {
+ return valueVariable != null && valueVariable != DUMMY_VALUE_VARIABLE;
Review Comment:
`hasValueVariable()` checks `valueVariable != null`, but `valueVariable` is
always non-null due to `requireNonNull(valueVariable)` in the constructor.
Dropping the null check would simplify the method and better reflect the
invariant (only the dummy sentinel indicates absence).
```suggestion
return valueVariable != DUMMY_VALUE_VARIABLE;
```
--
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]
