[
https://issues.apache.org/jira/browse/GROOVY-12158?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18095817#comment-18095817
]
ASF GitHub Bot commented on GROOVY-12158:
-----------------------------------------
codecov-commenter commented on PR #2703:
URL: https://github.com/apache/groovy/pull/2703#issuecomment-4955760765
##
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/2703?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 69.1054%. Comparing base
([`86c6648`](https://app.codecov.io/gh/apache/groovy/commit/86c6648d1e61436fe513afaa85d8d0c403de3cda?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
to head
([`cd6aaa1`](https://app.codecov.io/gh/apache/groovy/commit/cd6aaa144573a83908cef035691754473233e71f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/2703?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2703 +/- ##
==================================================
- Coverage 69.1077% 69.1054% -0.0023%
Complexity 34235 34235
==================================================
Files 1537 1537
Lines 129356 129356
Branches 23503 23503
==================================================
- Hits 89395 89392 -3
- Misses 31941 31942 +1
- Partials 8020 8022 +2
```
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/2703?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Coverage Δ | |
|---|---|---|
|
[...oovy/transform/AutoImplementASTTransformation.java](https://app.codecov.io/gh/apache/groovy/pull/2703?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Ftransform%2FAutoImplementASTTransformation.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3RyYW5zZm9ybS9BdXRvSW1wbGVtZW50QVNUVHJhbnNmb3JtYXRpb24uamF2YQ==)
| `88.0597% <100.0000%> (ø)` | |
... and [5 files with indirect coverage
changes](https://app.codecov.io/gh/apache/groovy/pull/2703/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test
Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests,
report on failures, and find test suite problems.
- :package: [JS Bundle
Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save
yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
> @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)
