[
https://issues.apache.org/jira/browse/GROOVY-12158?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18095807#comment-18095807
]
ASF GitHub Bot commented on GROOVY-12158:
-----------------------------------------
paulk-asert opened a new pull request, #2703:
URL: https://github.com/apache/groovy/pull/2703
… order they are found
> @AutoImplement generates methods in hash order, not the order they are found
> ----------------------------------------------------------------------------
>
> Key: GROOVY-12158
> URL: https://issues.apache.org/jira/browse/GROOVY-12158
> Project: Groovy
> Issue Type: Improvement
> Components: AST Transforms
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> h2. Description
> {{AutoImplementASTTransformation.getAllCorrectedMethodsMap}} collects the
> methods that need implementing into an unordered map, and the callers iterate
> its values to generate members:
> {code:java}
> // AutoImplementASTTransformation.java:157-161
> static Map<String, MethodNode> getAllCorrectedMethodsMap(final ClassNode
> cNode) {
> Map<String, MethodNode> result = new HashMap<>();
> for (MethodNode mn : getMethodsWithGenerated(cNode)) {
> result.put(methodDescriptorWithoutReturnType(mn), mn);
> }
> ...
> }
> // AutoImplementASTTransformation.java:110
> for (MethodNode candidate : getAllCorrectedMethodsMap(cNode).values()) {
> if (candidate.isAbstract()) {
> MethodNode mNode = addGeneratedMethod(cNode, ...); // <-- emits
> into the class file
> {code}
> The keys are method descriptors, so *the generated methods are emitted in the
> hash order of those descriptors* rather than in the order the methods were
> discovered. Two consumers iterate this map, so both are affected:
> * {{AutoImplementASTTransformation.createMethods}} ({{:110}}) — the generated
> methods in the class file.
> * {{AutoImplementASTStubber}} ({{:75}}) — the placeholder methods in
> joint-compilation Java stubs.
> h3. Demonstration
> {code:java}
> interface Face {
> void alpha()
> void beta()
> void gamma()
> void delta()
> void epsilon()
> void zeta()
> void eta()
> void theta()
> }
> @groovy.transform.AutoImplement
> class Impl implements Face {}
> {code}
> Reading the emitted {{Impl.class}} with an ASM {{ClassReader}}, the generated
> methods appear as:
> {noformat}
> delta, zeta, eta, theta, alpha, epsilon, beta, gamma
> {noformat}
> rather than the declared order {{alpha, beta, gamma, delta, epsilon, zeta,
> eta, theta}}.
> h3. Impact
> The keys are {{String}} values 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 for two
> reasons:
> * The *insertion* order into the map comes from {{ClassNode.getMethods()}} of
> the class and its supertypes, which for reflection-configured {{ClassNode}}s
> is the unspecified JDK reflection order being addressed in GROOVY-12149.
> Within a colliding bucket, {{HashMap}} chain order follows insertion order,
> so the emitted method order is coupled to that nondeterminism.
> * Even where it is stable, the emitted order is arbitrary rather than
> meaningful, which makes generated class files and generated stubs harder to
> diff and reason about.
> h3. Proposed fix
> Use an insertion-ordered map:
> {code:java}
> Map<String, MethodNode> result = new LinkedHashMap<>();
> {code}
> The generated methods are then emitted in the order they are found
> (declaration order in the example above). The method *set* is unchanged —
> only the order.
> Note {{LinkedHashMap.put}} on an existing key retains the original insertion
> position, so the walk's replacement of weaker candidates with stronger ones
> from supertypes ({{:172-173}}, {{:191-192}}) keeps each method where it was
> first seen. {{updatedGenericsSpec}} ({{:179}}) is a lookup-only map and does
> not need to change.
> h3. Test
> {{src/test/groovy/org/codehaus/groovy/transform/AutoImplementMethodOrderTest.groovy}}
> compiles the source above via {{CompilationUnit}}, reads the emitted bytes
> with an ASM {{ClassReader}}, and asserts the generated methods appear in
> declaration order. It fails on the current code with the scrambled order
> shown above and passes with the fix.
> h3. Related
> GROOVY-12149 (nondeterministic reflection order flows into generated
> bytecode) — feeds the insertion order of this map. Same class of defect:
> {{Verifier.addCovariantMethods}} (covariant bridge methods emitted in hash
> order), {{AsmClassGenerator}} {{referencedClasses}} ({{$class$}} synthetic
> fields), {{AnnotationCollectorTransform.makeListOfAnnotations}} (annotation
> member order for precompiled collectors), and GROOVY-12156
> ({{chooseBestMethod}} candidates in identity-hash order). These could
> reasonably be folded into a single issue.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
