Paul King created GROOVY-12158:
----------------------------------
Summary: @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
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}}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. 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)
