[
https://issues.apache.org/jira/browse/GROOVY-12158?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Paul King updated GROOVY-12158:
-------------------------------
Description:
TLDR this is mostly a reproducibility (bytecode ordering) issue not a
correctness issue.
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.
was:
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.
> @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
>
> TLDR this is mostly a reproducibility (bytecode ordering) issue not a
> correctness issue.
> 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)
