Copilot commented on code in PR #2400:
URL: https://github.com/apache/groovy/pull/2400#discussion_r2971246127
##########
src/main/java/org/codehaus/groovy/ast/AnnotationNode.java:
##########
@@ -50,6 +50,8 @@ public class AnnotationNode extends ASTNode {
public static final int TYPE_PARAMETER_TARGET = 1 << 8;
public static final int TYPE_USE_TARGET = 1 << 9;
public static final int RECORD_COMPONENT_TARGET = 1 << 10;
+ /** Groovy-only target for statement-level annotations (e.g. on {@code
for}/{@code while} loops). */
+ public static final int STATEMENT_TARGET = 1 << 11;
public static final int TYPE_TARGET = ANNOTATION_TARGET | 1;
// GROOVY-7151
Review Comment:
`STATEMENT_TARGET` is introduced but isn’t included in the default
allowed-target bitset (`0x4FF`) computed when an annotation has no `@Target`.
If statement-level target checking is added later, this will cause *all*
annotations without `@Target` to be treated as disallowed on statements unless
the default mask is updated accordingly.
##########
src/main/java/org/codehaus/groovy/ast/stmt/Statement.java:
##########
@@ -58,6 +61,43 @@ public void copyStatementLabels(final Statement that) {
});
}
+
//--------------------------------------------------------------------------
+ // Statement-level annotation support (Groovy-only; stored in node
metadata)
+
+ private static final Object STATEMENT_ANNOTATIONS_KEY =
"_statementAnnotations_";
Review Comment:
`STATEMENT_ANNOTATIONS_KEY` is a String literal. Using a unique `new
Object()` key (or a dedicated private static final Object instance) avoids
accidental metadata key collisions with other features that may also use the
same String in `ASTNode` metadata.
```suggestion
private static final Object STATEMENT_ANNOTATIONS_KEY = new Object();
```
##########
src/main/java/groovy/transform/Parallel.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package groovy.transform;
+
+import org.codehaus.groovy.transform.GroovyASTTransformationClass;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+/**
+ * Runs each iteration of an annotated {@code for} loop on a separate thread.
+ * <p>
+ * This annotation is a lightweight demo transform and intentionally favors
simplicity
+ * over production-grade parallel orchestration.
+ *
+ * @since 6.0.0
+ * @see org.codehaus.groovy.transform.ParallelASTTransformation
+ */
+@Documented
+@Retention(RetentionPolicy.SOURCE)
+@GroovyASTTransformationClass("org.codehaus.groovy.transform.ParallelASTTransformation")
+public @interface Parallel {
+}
Review Comment:
PR description notes the sample transforms/annotations aren’t intended to be
merged as production code, but this PR adds them under `src/main/java` (which
will ship as part of the main artifact). If they’re only for experimentation,
consider moving them to test fixtures/samples or excluding them from the merge
to avoid accidentally committing a public API (`groovy.transform.Parallel`) and
behavior that isn’t supported long-term.
##########
src/main/java/org/codehaus/groovy/ast/ClassCodeVisitorSupport.java:
##########
@@ -212,6 +213,7 @@ public void visitExpressionStatement(ExpressionStatement
statement) {
@Override
public void visitForLoop(ForStatement statement) {
visitStatement(statement);
+ visitStatementAnnotations(statement);
if (statement.getValueVariable() != null) {
Review Comment:
These overrides call `visitStatementAnnotations(statement)` unconditionally
for every loop statement. If the intent is only to process statement
annotations when present, consider guarding with `if
(!statement.getStatementAnnotations().isEmpty())` to avoid extra work in hot
visitors (parser/type checker passes) for the common case of unannotated loops.
##########
src/main/java/org/codehaus/groovy/transform/LoopInvariantASTTransformation.java:
##########
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.codehaus.groovy.transform;
+
+import groovy.transform.LoopInvariant;
+import org.codehaus.groovy.ast.ASTNode;
+import org.codehaus.groovy.ast.AnnotationNode;
+import org.codehaus.groovy.ast.expr.ConstantExpression;
+import org.codehaus.groovy.ast.expr.Expression;
+import org.codehaus.groovy.ast.stmt.BlockStatement;
+import org.codehaus.groovy.ast.stmt.LoopingStatement;
+import org.codehaus.groovy.ast.stmt.Statement;
+import org.codehaus.groovy.control.CompilePhase;
+import org.codehaus.groovy.control.SourceUnit;
+import org.codehaus.groovy.control.messages.SyntaxErrorMessage;
+import org.codehaus.groovy.syntax.SyntaxException;
+
+import static org.codehaus.groovy.ast.tools.GeneralUtils.args;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.block;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.callThisX;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.constX;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.stmt;
+
+/**
+ * Handles the {@link LoopInvariant} annotation by inserting an {@code assert}
for the
+ * invariant expression string at the top of the annotated loop body.
+ * <p>
+ * The annotation may be applied to {@code for}, {@code while}, or {@code
do-while} loops.
+ * For this demonstration, the invariant string is shown in an assert message.
A production
+ * implementation would parse and evaluate the invariant expression.
Review Comment:
The class Javadoc says this transform inserts an `assert` for the invariant,
but the implementation currently injects a `println` statement. Update the
documentation to match the behavior (or change the injected statement to an
`assert`) so users don’t get misleading semantics.
```suggestion
* Handles the {@link LoopInvariant} annotation by inserting a {@code
println} that reports the
* invariant expression string at the top of the annotated loop body.
* <p>
* The annotation may be applied to {@code for}, {@code while}, or {@code
do-while} loops.
* For this demonstration, the invariant string is printed for visibility at
runtime. A production
* implementation would typically parse and evaluate the invariant
expression (for example, using an assert).
```
##########
src/main/java/org/codehaus/groovy/transform/LoopInvariantASTTransformation.java:
##########
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.codehaus.groovy.transform;
+
+import groovy.transform.LoopInvariant;
+import org.codehaus.groovy.ast.ASTNode;
+import org.codehaus.groovy.ast.AnnotationNode;
+import org.codehaus.groovy.ast.expr.ConstantExpression;
+import org.codehaus.groovy.ast.expr.Expression;
+import org.codehaus.groovy.ast.stmt.BlockStatement;
+import org.codehaus.groovy.ast.stmt.LoopingStatement;
+import org.codehaus.groovy.ast.stmt.Statement;
+import org.codehaus.groovy.control.CompilePhase;
+import org.codehaus.groovy.control.SourceUnit;
+import org.codehaus.groovy.control.messages.SyntaxErrorMessage;
+import org.codehaus.groovy.syntax.SyntaxException;
+
+import static org.codehaus.groovy.ast.tools.GeneralUtils.args;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.block;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.callThisX;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.constX;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.stmt;
+
+/**
+ * Handles the {@link LoopInvariant} annotation by inserting an {@code assert}
for the
+ * invariant expression string at the top of the annotated loop body.
+ * <p>
+ * The annotation may be applied to {@code for}, {@code while}, or {@code
do-while} loops.
+ * For this demonstration, the invariant string is shown in an assert message.
A production
+ * implementation would parse and evaluate the invariant expression.
+ *
+ * @since 6.0.0
+ * @see LoopInvariant
+ */
+@GroovyASTTransformation(phase = CompilePhase.SEMANTIC_ANALYSIS)
+public class LoopInvariantASTTransformation implements ASTTransformation {
+
+ @Override
+ public void visit(final ASTNode[] nodes, final SourceUnit source) {
+ if (nodes.length != 2) return;
+ if (!(nodes[0] instanceof AnnotationNode annotation)) return;
+ if
(!annotation.getClassNode().getName().equals(LoopInvariant.class.getName()))
return;
+
+ if (!(nodes[1] instanceof LoopingStatement loopStatement)) {
+ source.getErrorCollector().addError(new SyntaxErrorMessage(
+ new SyntaxException("@LoopInvariant may only be applied to
for, while, or do-while loop statements",
+ annotation.getLineNumber(),
annotation.getColumnNumber()), source));
+ return;
+ }
+
+ Expression invariantExpr = annotation.getMember("value");
+ if (!(invariantExpr instanceof ConstantExpression)) {
+ source.getErrorCollector().addError(new SyntaxErrorMessage(
+ new SyntaxException("@LoopInvariant value must be a
constant string expression",
+ annotation.getLineNumber(),
annotation.getColumnNumber()), source));
+ return;
+ }
+
+ String invariant = (String) ((ConstantExpression)
invariantExpr).getValue();
Review Comment:
`invariantExpr` is only checked to be a `ConstantExpression`, but its value
isn’t verified to be a `String` before casting. A non-string constant would
cause a `ClassCastException` during compilation. Consider checking
`((ConstantExpression) invariantExpr).getValue() instanceof String` and
reporting a syntax error otherwise.
```suggestion
Object invariantValue = ((ConstantExpression)
invariantExpr).getValue();
if (!(invariantValue instanceof String)) {
source.getErrorCollector().addError(new SyntaxErrorMessage(
new SyntaxException("@LoopInvariant value must be a
constant string expression",
annotation.getLineNumber(),
annotation.getColumnNumber()), source));
return;
}
String invariant = (String) invariantValue;
```
##########
src/main/java/org/apache/groovy/parser/antlr4/SemanticPredicates.java:
##########
@@ -186,8 +191,39 @@ public static boolean
isInvalidLocalVariableDeclaration(TokenStream ts) {
!(BuiltInPrimitiveType == tokenType ||
Arrays.binarySearch(MODIFIER_ARRAY, tokenType) >= 0)
&& !Character.isUpperCase(nextCodePoint)
&& nextCodePoint != '@'
- && !(ASSIGN == tokenType3 || (LT == tokenType2 ||
LBRACK == tokenType2));
+ && !(ASSIGN == tokenType3 || (LT == tokenType2 ||
LBRACK == tokenType2))
+ || (nextCodePoint == '@' && isAnnotatedLoopStatement(ts));
}
+ /**
+ * When the input starts with one or more annotations, scan past them and
check whether
+ * the first non-annotation token is a loop keyword ({@code for}, {@code
while}, {@code do}).
+ * If so, the construct is an annotated loop statement, NOT a local
variable declaration.
+ *
+ * @param ts the token stream positioned at the first annotation {@code @}
token
+ * @return {@code true} if annotations are followed by a loop keyword
+ */
+ static boolean isAnnotatedLoopStatement(TokenStream ts) {
+ int idx = 1; // ts.LT(1) is '@'
+ while (ts.LT(idx).getType() == AT) {
+ idx += 2; // skip AT and annotation name
+ // skip qualifier parts of a fully-qualified annotation name, e.g.
@java.lang.Deprecated
+ while (ts.LT(idx).getType() == DOT) {
+ idx += 2; // skip DOT and next name element
+ }
+ // skip annotation arguments (parenthesised), handling nesting
+ if (ts.LT(idx).getType() == LPAREN) {
+ idx++;
+ int depth = 1;
+ while (depth > 0 && ts.LT(idx).getType() !=
org.antlr.v4.runtime.Token.EOF) {
+ int t = ts.LT(idx++).getType();
+ if (t == LPAREN) depth++;
+ else if (t == RPAREN) depth--;
+ }
+ }
+ }
+ int afterAnnotations = ts.LT(idx).getType();
+ return afterAnnotations == FOR || afterAnnotations == WHILE ||
afterAnnotations == DO;
Review Comment:
`isAnnotatedLoopStatement` doesn’t skip `NL` tokens between annotations (or
between the last annotation and the loop keyword). Because `annotationsOpt`
explicitly allows newlines, common formatting like `@Anno\nfor (...)` may not
be recognized here, reintroducing the local-variable-declaration ambiguity this
predicate is meant to avoid. Consider skipping `NL*` (and any other tokens used
by `nls`) while scanning.
##########
src/main/java/org/codehaus/groovy/control/ResolveVisitor.java:
##########
@@ -1393,6 +1393,17 @@ public void visitCatchStatement(final CatchStatement cs)
{
super.visitCatchStatement(cs);
}
+ /**
+ * Resolves the class nodes of any annotations attached to a loop statement
+ * (stored in statement metadata rather than in {@link
org.codehaus.groovy.ast.AnnotatedNode}).
+ */
+ @Override
+ protected void visitStatementAnnotations(final Statement statement) {
+ for (AnnotationNode annotation : statement.getStatementAnnotations()) {
+ visitAnnotation(annotation);
+ }
+ }
+
Review Comment:
`visitStatementAnnotations` currently only resolves statement-level
annotations but does not validate them (e.g., retention/allowed usage). With
the grammar change allowing annotations before loop keywords, non-transform
annotations like `@Deprecated` can now be written on loops and will be silently
ignored, which is inconsistent with existing annotation target checking on
`AnnotatedNode`s. Consider adding validation here (e.g., require SOURCE
retention and/or presence of `@GroovyASTTransformationClass`, and emit a syntax
error otherwise).
```suggestion
visitAnnotation(annotation);
validateStatementAnnotation(annotation, statement);
}
}
/**
* Validates that a statement-level annotation is of an allowed kind.
* <p>
* Currently, only Groovy AST transformation annotations (i.e.
annotations
* annotated with {@code @GroovyASTTransformationClass}) are supported on
* statements such as loops. Other annotations (e.g. {@code @Deprecated})
* would be silently ignored, so we report them as errors for consistency
* with annotation target checking on {@link
org.codehaus.groovy.ast.AnnotatedNode}s.
*/
private void validateStatementAnnotation(final AnnotationNode
annotation, final Statement statement) {
ClassNode annotationType = annotation.getClassNode();
if (annotationType == null) return;
annotationType = annotationType.redirect();
if (annotationType == null) return;
// Only allow Groovy AST transformation annotations on statements.
ClassNode groovyAstTransformClass =
ClassHelper.make(org.codehaus.groovy.transform.GroovyASTTransformationClass.class);
List<AnnotationNode> transformMeta =
annotationType.getAnnotations(groovyAstTransformClass);
if (transformMeta == null || transformMeta.isEmpty()) {
addError("Annotation @" + annotationType.getNameWithoutPackage()
+ " is not a Groovy AST transformation and cannot be used on
statements", annotation);
}
}
```
##########
src/main/java/groovy/transform/LoopInvariant.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package groovy.transform;
+
+import org.codehaus.groovy.transform.GroovyASTTransformationClass;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+/**
+ * Specifies a loop invariant for a {@code for} or {@code while} loop.
+ * <p>
+ * The invariant is a Groovy boolean expression that must hold at the start of
each
+ * iteration. The AST transform inserts an {@code assert} for this condition
at the
+ * top of the loop body, providing lightweight design-by-contract support
inspired
+ * by languages such as Dafny.
Review Comment:
The annotation Javadoc describes injecting an `assert` for the invariant
condition, but the current transform implementation injects a `println`. Align
this documentation with the actual behavior (or update the transform) to avoid
promising contract checking that isn’t happening.
##########
src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java:
##########
@@ -470,7 +470,9 @@ public ForStatement visitForStmtAlt(final ForStmtAltContext
ctx) {
Statement loopBody = this.unpackStatement((Statement)
this.visit(ctx.statement()));
- return configureAST(maker.apply(loopBody), ctx);
+ ForStatement forStatement = configureAST(maker.apply(loopBody), ctx);
+
visitAnnotationsOpt(ctx.annotationsOpt()).forEach(forStatement::addStatementAnnotation);
+ return forStatement;
Review Comment:
This change introduces new syntax/AST behavior (annotations on loop
statements + statement-level annotation plumbing) but no corresponding tests
were added. There are existing parser tests under
`src/test/groovy/org/apache/groovy/parser/antlr4/`; consider adding coverage
that parses annotated `for`/`while`/`do-while` (including multi-line
annotations) and verifies the annotations are attached to the loop statement
and that local AST transforms are triggered.
--
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]
