[
https://issues.apache.org/jira/browse/GROOVY-12160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18096029#comment-18096029
]
ASF GitHub Bot commented on GROOVY-12160:
-----------------------------------------
Copilot commented on code in PR #2705:
URL: https://github.com/apache/groovy/pull/2705#discussion_r3575111004
##########
src/main/java/org/codehaus/groovy/transform/AnnotationCollectorTransform.java:
##########
@@ -47,6 +47,7 @@
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
+import java.util.LinkedHashMap;
Review Comment:
The java.util.HashMap import is now unused after switching to LinkedHashMap;
keeping it adds noise and can trigger IDE/compiler warnings.
> @AnnotationCollector emits collected annotation members in hash order when
> the collector is precompiled
> -------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12160
> URL: https://issues.apache.org/jira/browse/GROOVY-12160
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> h2. Description
> When a collector annotation is precompiled, {{AnnotationCollectorTransform}}
> reads its preset members back from the generated {{CollectorHelper}} and
> copies them onto the annotations the collector expands to. That copy goes
> through an unordered map:
> {code:java}
> // AnnotationCollectorTransform.java:316-337 (makeListOfAnnotations)
> for (Object[] inner : data) {
> Class<?> anno = (Class<?>) inner[0];
> AnnotationNode toAdd = new AnnotationNode(ClassHelper.make(anno));
> ret.add(toAdd);
> Map<String,Object> member = (Map<String, Object>) inner[1];
> if (member.isEmpty()) {
> continue;
> }
> Map<String, Expression> generated = new HashMap<>(member.size());
> for (Map.Entry<String, Object> entry : member.entrySet()) {
> generated.put(entry.getKey(), makeExpression(entry.getValue()));
> }
> copyMembers(generated, toAdd); // iterates `generated` ->
> toAdd.addMember(...)
> }
> {code}
> {{copyMembers}} ({{:274}}) iterates that map into
> {{AnnotationNode.addMember}}, and those members are written to the class
> file's annotation attribute in that order. So *the members of the expanded
> annotation are emitted in the hash order of their names*.
> Declaration order is intact everywhere else in the chain and is lost only
> here: {{serialize(AnnotationNode)}} ({{:184}}) builds the helper's map by
> iterating {{AnnotationNode.getMembers()}}, which is a {{LinkedHashMap}}, so
> the order the members were declared on the collector survives into the
> precompiled {{CollectorHelper}}.
> h3. Reachability
> Only when the collector is *precompiled* — i.e. resolved from the classpath,
> so the members are read reflectively from its {{CollectorHelper}} via
> {{getTargetListFromClass}} ({{:280-290}}). A collector declared in the same
> compilation unit takes the AST-based path, which preserves order. The
> precompiled case is the normal one for any collector shipped in a library,
> including Groovy's own {{@Immutable}} and {{@Canonical}}.
> h3. Demonstration
> Compilation unit 1 (the collector, presetting five members):
> {code:java}
> import groovy.transform.AnnotationCollector
> import java.lang.annotation.*
> @Retention(RetentionPolicy.RUNTIME) @Target(ElementType.TYPE)
> @interface Marker {
> String alpha() default ''
> String beta() default ''
> String gamma() default ''
> String delta() default ''
> String epsilon() default ''
> }
> @Marker(alpha='a', beta='b', gamma='c', delta='d', epsilon='e')
> @AnnotationCollector
> @interface Coll {}
> {code}
> Compilation unit 2, compiled against the first:
> {code:java}
> @Coll
> class Holder {}
> {code}
> Reading the emitted {{Holder.class}} with an ASM {{ClassReader}}, the members
> of the {{@Marker}} annotation appear as:
> {noformat}
> delta, epsilon, beta, gamma, alpha
> {noformat}
> rather than the declared order {{alpha, beta, gamma, delta, epsilon}}.
> h3. Impact
> The keys are {{String}}s and {{String.hashCode}} is stable, so for a given
> key set the iteration order is reproducible across JVM runs — this is not by
> itself a live reproducible-builds failure. It still matters because the
> emitted order is arbitrary rather than meaningful, it lands in the
> {{RuntimeVisibleAnnotations}} attribute of the class file, and within a
> colliding bucket {{HashMap}} chain order follows insertion order. Of the
> related ordering issues below, this one has the most ordinary trigger: any
> classpath collector with more than one preset member.
> h3. Proposed fix
> Use an insertion-ordered map:
> {code:java}
> Map<String, Expression> generated = new LinkedHashMap<>(member.size());
> {code}
> The collected members are then emitted in the order they were declared on the
> collector. The member *set* and their values are unchanged — only the order.
> h3. Test
> {{src/test/groovy/org/codehaus/groovy/transform/AnnotationCollectorMemberOrderTest.groovy}}
> compiles the annotation and collector into a temp directory as one
> compilation unit, applies the collector from a second unit with that
> directory on the classpath (so the precompiled path is exercised), then reads
> the emitted bytes with an ASM {{ClassReader}} and asserts the annotation's
> members appear in declaration order. It fails on the current code with
> {{['delta', 'epsilon', 'beta', 'gamma', 'alpha']}} and passes with the fix.
> h3. Related
> GROOVY-12149 (nondeterministic reflection order flows into generated
> bytecode). Same class of defect: {{Verifier.addCovariantMethods}} (covariant
> bridge methods emitted in hash order),
> {{AutoImplementASTTransformation.getAllCorrectedMethodsMap}}
> ({{@AutoImplement}} methods emitted in hash order), {{AsmClassGenerator}}
> {{referencedClasses}} ({{$class$}} synthetic fields and accessors), and
> GROOVY-12156 ({{chooseBestMethod}} candidates in identity-hash order).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
