[
https://issues.apache.org/jira/browse/GROOVY-12157?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18095805#comment-18095805
]
ASF GitHub Bot commented on GROOVY-12157:
-----------------------------------------
codecov-commenter commented on PR #2702:
URL: https://github.com/apache/groovy/pull/2702#issuecomment-4955522261
##
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/2702?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.1155%. 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
([`e0fe323`](https://app.codecov.io/gh/apache/groovy/commit/e0fe323119f96a2559ce75bce5dbbd4be302dc17?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/2702?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2702 +/- ##
==================================================
+ Coverage 69.1077% 69.1155% +0.0077%
- Complexity 34235 34239 +4
==================================================
Files 1537 1537
Lines 129356 129356
Branches 23503 23503
==================================================
+ Hits 89395 89405 +10
+ Misses 31941 31933 -8
+ Partials 8020 8018 -2
```
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/2702?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Coverage Δ | |
|---|---|---|
|
[...in/java/org/codehaus/groovy/classgen/Verifier.java](https://app.codecov.io/gh/apache/groovy/pull/2702?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2FVerifier.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL1ZlcmlmaWVyLmphdmE=)
| `90.0938% <100.0000%> (ø)` | |
... and [7 files with indirect coverage
changes](https://app.codecov.io/gh/apache/groovy/pull/2702/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>
> Covariant bridge methods are emitted in hash order, not the order they are
> found
> --------------------------------------------------------------------------------
>
> Key: GROOVY-12157
> URL: https://issues.apache.org/jira/browse/GROOVY-12157
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> h2. Description
> {{Verifier.addCovariantMethods}} collects the bridge methods it needs to
> generate into unordered maps and then iterates them to emit the methods:
> {code:java}
> // Verifier.java:1679-1680
> Map<String, MethodNode> absInterfaceMethods = new HashMap<>();
> Map<String, MethodNode> allInterfaceMethods = new HashMap<>();
> ...
> // Verifier.java:1719
> Map<String, MethodNode> methodsToAdd = new HashMap<>();
> addCovariantMethods(classNode, declaredMethods, absInterfaceMethods,
> methodsToAdd, genericsSpec);
> if (!methodsToAdd.isEmpty()) {
> ...
> for (Map.Entry<String, MethodNode> entry : methodsToAdd.entrySet()) {
> // we skip bridge methods implemented in current class already
> MethodNode mn = declaredMethodsMap.get(entry.getKey());
> if (mn == null || !mn.getDeclaringClass().equals(classNode)) {
> addPropertyMethod(entry.getValue()); // <-- emits into the
> class file
> }
> }
> }
> {code}
> The three maps chain into each other — {{allInterfaceMethods}} is iterated to
> populate {{absInterfaceMethods}} ({{:1699}}), which is iterated as
> {{interfaceMethods.values()}} ({{:1750}}) to populate {{methodsToAdd}}, which
> is iterated to call {{addPropertyMethod}}. So *the order of the generated
> bridge methods in the emitted class file is the hash order of the
> type-descriptor keys*, not the order in which the methods were discovered.
> h3. Demonstration
> {code:java}
> interface Alpha { Object alpha() }
> interface Beta { Object beta() }
> interface Gamma { Object gamma() }
> interface Delta { Object delta() }
> interface Epsilon { Object epsilon() }
> class Sample implements Alpha, Beta, Gamma, Delta, Epsilon {
> String alpha() { null }
> Integer beta() { null }
> Long gamma() { null }
> Double delta() { null }
> Short epsilon() { null }
> }
> {code}
> {{javap -p Sample.class}} shows the five declared methods in declaration
> order, followed by the five generated bridges in an arbitrary order:
> {noformat}
> public java.lang.String alpha();
> public java.lang.Integer beta();
> public java.lang.Long gamma();
> public java.lang.Double delta();
> public java.lang.Short epsilon();
> public java.lang.Object alpha(); <-- bridges, in hash order:
> public java.lang.Object gamma(); alpha, gamma, epsilon, beta, delta
> public java.lang.Object epsilon();
> public java.lang.Object beta();
> public java.lang.Object delta();
> {noformat}
> h3. Impact
> The keys are type-descriptor {{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. Two reasons it
> still matters:
> * The *insertion* order into these maps comes from {{ClassNode.getMethods()}}
> of the 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 bridge order is coupled to that nondeterminism.
> * Even where it is stable, the emitted order is arbitrary rather than
> meaningful, which makes generated class files harder to diff and reason about.
> h3. Proposed fix
> Use insertion-ordered maps for the three that are iterated to produce output:
> {code:java}
> Map<String, MethodNode> absInterfaceMethods = new LinkedHashMap<>();
> Map<String, MethodNode> allInterfaceMethods = new LinkedHashMap<>();
> ...
> Map<String, MethodNode> methodsToAdd = new LinkedHashMap<>();
> {code}
> Bridge methods are then emitted in the order they are discovered (declaration
> order in the example above: {{alpha, beta, gamma, delta, epsilon}}). The
> method *set* is unchanged — only the order.
> {{declaredMethodsMap}} ({{:1724}}) is lookup-only and {{defaultMethods}}
> ({{:323}}) feeds duplicate-default-method diagnostics only, so neither needs
> to change.
> h3. Test
> {{src/test/groovy/org/codehaus/groovy/classgen/CovariantBridgeMethodOrderTest.groovy}}
> compiles the source above via {{CompilationUnit}}, reads the emitted bytes
> with an ASM {{ClassReader}} (the pattern already used by
> {{Groovy11776.groovy}}), and asserts the bridge methods appear in declaration
> order. It fails on the current code with {{['alpha', 'gamma', 'epsilon',
> 'beta', 'delta']}} and passes with the fix.
> h3. Related
> GROOVY-12149 (nondeterministic reflection order flows into generated
> bytecode) — feeds the insertion order of these maps. GROOVY-12156
> (chooseBestMethod returns candidates in identity-hash order) — same class of
> defect in the static type checker.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
