[
https://issues.apache.org/jira/browse/GROOVY-12157?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18095812#comment-18095812
]
ASF GitHub Bot commented on GROOVY-12157:
-----------------------------------------
testlens-app[bot] commented on PR #2702:
URL: https://github.com/apache/groovy/pull/2702#issuecomment-4955668562
## ✅ All tests passed ✅
🏷️ Commit: e0fe323119f96a2559ce75bce5dbbd4be302dc17
▶️ Tests: 103674 executed
⚪️ Checks: 31/31 completed
---
_Learn more about TestLens at [testlens.app](https://testlens.app)._
> 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)
