[
https://issues.apache.org/jira/browse/GROOVY-11878?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18067448#comment-18067448
]
ASF GitHub Bot commented on GROOVY-11878:
-----------------------------------------
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.
> Allow AST transforms to be applicable in more places, initially loop
> statements
> -------------------------------------------------------------------------------
>
> Key: GROOVY-11878
> URL: https://issues.apache.org/jira/browse/GROOVY-11878
> Project: Groovy
> Issue Type: New Feature
> Reporter: Paul King
> Priority: Major
>
> The JVM doesn't support adding annotations in the bytecode for statements and
> expressions. That doesn't mean that Groovy couldn't support source-level AST
> transforms in those places. Code injection can happen in the normal way, and
> the transform will be "forgotten" after compilation.
> Examples might be:
> {code}
> @Parallel
> for (int i in 1..4) {
> println i ** 2
> }
> {code}
> which might have this output:
> {noformat}
> 1
> 16
> 9
> 4
> {noformat}
> Or these:
> {code}
> int sum = 0
> @Invariant({ 0 <= i && i <= 4 })
> for (int i in 0..4) {
> sum += i
> }
> assert sum == 10
> {code}
> {code}
> int i = 10
> @Invariant({ i >= 0 })
> @Ensures({ i > old.i }) // i decreases
> while (i > 0) {
> i = i - 1
> }
> assert i == 0
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
