[
https://issues.apache.org/jira/browse/GROOVY-11884?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18069175#comment-18069175
]
ASF GitHub Bot commented on GROOVY-11884:
-----------------------------------------
Copilot commented on code in PR #2407:
URL: https://github.com/apache/groovy/pull/2407#discussion_r3003915697
##########
src/main/java/org/codehaus/groovy/ast/AnnotationNode.java:
##########
@@ -159,9 +164,39 @@ public boolean isTargetAllowed(final int target) {
return 0x4FF; // GROOVY-11838: JLS 9.6.4.1
});
+ // check @ExtendedTarget for Groovy-specific element types
+ allowedTargets |=
classNode.redirect().getNodeMetaData(ExtendedTarget.class, (k) -> {
+ for (AnnotationNode an : classNode.getAnnotations()) {
+ if
("groovy.lang.annotation.ExtendedTarget".equals(an.getClassNode().getName())) {
+ Expression member = an.getMember("value");
+ int bits = 0;
+ if (member instanceof ListExpression list) {
+ for (Expression e : list.getExpressions()) {
+ bits |= extendedElementTypeBits(e);
+ }
+ } else {
+ bits |= extendedElementTypeBits(member);
+ }
+ return bits;
+ }
+ }
+ return 0;
+ });
+
return (target & allowedTargets) == target;
}
+ private static int extendedElementTypeBits(final Expression e) {
+ if (e instanceof PropertyExpression item) {
+ String name = item.getPropertyAsString();
+ return switch (ExtendedElementType.valueOf(name)) {
+ case IMPORT -> IMPORT_TARGET;
+ case LOOP -> STATEMENT_TARGET;
+ };
Review Comment:
`extendedElementTypeBits` calls `ExtendedElementType.valueOf(name)` without
guarding against `name == null` or unknown values. If an invalid enum constant
is used in `@ExtendedTarget(...)`, the compiler may throw
`IllegalArgumentException`/`NPE` here (potentially masking the existing
validation error) instead of reporting a clean compilation error. Consider
handling null/invalid names (e.g., check for null and catch
`IllegalArgumentException`) and returning 0 or producing a targeted error
message.
```suggestion
if (name == null) {
return 0;
}
try {
return switch (ExtendedElementType.valueOf(name)) {
case IMPORT -> IMPORT_TARGET;
case LOOP -> STATEMENT_TARGET;
};
} catch (IllegalArgumentException ignore) {
// unknown ExtendedElementType value; no extended element
type bits
return 0;
}
```
##########
src/main/java/org/codehaus/groovy/classgen/ExtendedVerifier.java:
##########
@@ -221,6 +232,32 @@ public void visitMethod(final MethodNode node) {
}
}
+ @Override
+ protected void visitStatementAnnotations(final Statement statement) {
+ for (AnnotationNode annotation : statement.getStatementAnnotations()) {
+ ErrorCollector errorCollector = new
ErrorCollector(source.getConfiguration());
+ AnnotationVisitor visitor = new AnnotationVisitor(source,
errorCollector);
+ AnnotationNode visited = visitor.visit(annotation);
+ source.getErrorCollector().addCollectorContents(errorCollector);
+
+ if (!visited.isTargetAllowed(STATEMENT_TARGET)) {
+ addError("Annotation @" + visited.getClassNode().getName() + "
is not allowed on element "
+ + AnnotationNode.targetToName(STATEMENT_TARGET),
visited);
+ }
+ }
+ }
Review Comment:
This PR adds new validation for statement annotations via
`visitStatementAnnotations`, but the test additions only cover import-level
annotations. Consider adding a focused test that (1) rejects a
non-`@ExtendedTarget(LOOP)` annotation on a loop and (2) accepts an annotation
that declares `@ExtendedTarget(LOOP)`, to ensure this new validation path is
exercised and doesn’t regress.
##########
src/main/java/org/codehaus/groovy/classgen/ExtendedVerifier.java:
##########
@@ -43,23 +45,27 @@
import org.objectweb.asm.Opcodes;
import java.util.ArrayList;
+import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
+import java.util.Set;
Review Comment:
Unused import: `java.util.Collections` is added but not referenced in this
file. Please remove it to keep the import list clean and avoid warnings.
> Close gaps in Groovy annotation validation
> ------------------------------------------
>
> Key: GROOVY-11884
> URL: https://issues.apache.org/jira/browse/GROOVY-11884
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
> Labels: breaking
>
> Groovy lets you have annotations like @Grab on various nodes and various
> statements including imports. Targeting imports is not possible in Java, but
> this is part of Groovy's stance of providing more flexibility.
> Groovy also does validation that annotations don't appear in incorrect
> places. Currently there is a gap in our validation - we ignore things like
> checking what things can be on imports, e.g. the following is currently valid
> but totally ignored:
> {code}
> @Deprecated
> import java.lang.String
> {code}
> As well as @Grab, @Newify and @BaseScript can also appear on imports.
> With the new incubating AST transforms on loops, @Invariant can now appear on
> loops.
> I propose we create some additional classes to capture these extended
> annotation targets and close the gaps in validation. Something like:
> {code}
> @Retention(RetentionPolicy.RUNTIME)
> @Target(ElementType.ANNOTATION_TYPE)
> public @interface ExtendedTarget {
> ExtendedElementType[] value();
> }
> {code}
> And:
> {code}
> public enum ExtendedElementType {
> /** Import statement. */
> IMPORT,
> /** Loop statement ({@code for}, {@code while}, {@code do-while}). */
> LOOP
> }
> {code}
> And then things like @BaseScript would change from this:
> {code}
> @Retention(RetentionPolicy.SOURCE)
> @Target({ElementType.LOCAL_VARIABLE, ElementType.PACKAGE, ElementType.TYPE
> /*, ElementType.IMPORT*/})
> @GroovyASTTransformationClass("org.codehaus.groovy.transform.BaseScriptASTTransformation")
> public @interface BaseScript {
> Class value() default Script.class;
> }
> {code}
> to this:
> {code}
> @Retention(RetentionPolicy.SOURCE)
> @Target({ElementType.LOCAL_VARIABLE, ElementType.PACKAGE, ElementType.TYPE})
> @ExtendedTarget(ExtendedElementType.IMPORT)
> @GroovyASTTransformationClass("org.codehaus.groovy.transform.BaseScriptASTTransformation")
> public @interface BaseScript {
> Class value() default Script.class;
> }
> {code}
> This would be a breaking change if any codebases are relying on Groovy's
> leniency, e.g. maybe they had a convention of putting @Deprecated on an
> import if they were hoping to eventually get rid of the related dependency.
> We don't know anyone doing this, but in such cases, folks would need to now
> make and use an annotation with @ExtendedTarget.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
