Paul King created GROOVY-12156:
----------------------------------
Summary: chooseBestMethod returns candidates in identity-hash
order, making static-compilation output path-dependent
Key: GROOVY-12156
URL: https://issues.apache.org/jira/browse/GROOVY-12156
Project: Groovy
Issue Type: Improvement
Components: Static compilation
Reporter: Paul King
Assignee: Paul King
### Description
`StaticTypeCheckingSupport.chooseBestMethod` accumulates its result in a
`HashSet` and returns it as a list:
```java
// StaticTypeCheckingSupport.java:1066
Set<MethodNode> bestMethods = new HashSet<>(); // choose best method(s) for
each possible receiver
for (ClassNode rcvr : duckType ? ((UnionTypeClassNode) receiver).getDelegates()
: new ClassNode[]{receiver}) {
...
bestMethods.addAll(view);
}
return new LinkedList<>(bestMethods); // assumes caller wants remove to be
inexpensive
```
`MethodNode` overrides neither `equals()` nor `hashCode()` (unlike `ClassNode`,
whose `hashCode()` is text-based and stable). The `HashSet` therefore buckets
by **identity hash code**, and the returned candidate order is whatever
identity hashing produces.
#### Why this is not benign
HotSpot's default identity hash (`-XX:hashCode=5`) is a **thread-local xorshift
PRNG**. The value an object receives depends on how many identity hashes that
thread has already handed out, and each thread has its own state. So the order
is not random per run — it is *path-dependent*:
```
main thread, 0 prior hashes : [m5, m3, m1, m2, m4]
main thread, 3 prior hashes : [m5, m2, m4, m3, m1]
main thread, 7 prior hashes : [m2, m5, m4, m1, m3]
worker thread : [m1, m2, m3, m5, m4]
worker thread : [m5, m3, m1, m4, m2]
```
(5 objects with no `hashCode()` override, added to a `HashSet`, iterated.) A
fresh single-threaded JVM repeating identical work reproduces the same order —
but the candidate order changes when the compile runs on a different thread or
after a different amount of prior work. In practice that means a **long-lived,
reused Gradle daemon**, **parallel compile workers**, or simply **a different
set of classes compiled earlier in the same JVM** can each shift the order.
Reproducible builds require identical output from identical sources regardless
of that state. The nondeterministic reflection order in GROOVY-12149 also
perturbs the sequence of `hashCode()` calls, so the two defects amplify each
other.
#### Where the order reaches generated code
Most callers reduce to a single candidate and raise an ambiguity error
otherwise (`StaticTypeCheckingVisitor:4397`, `:3581`, `findMethodOrFail:5694`)
— those are safe, since an ambiguity error emits no bytecode. But three callers
accept a multi-candidate list **without error**:
1. **Union-type receiver (GROOVY-8965), `StaticTypeCheckingVisitor:4385`** —
the case that *deliberately* fills the set with one best method per union
delegate, and so is a non-error path by construction:
```java
if (mn.size() > 1 && obj instanceof UnionTypeClassNode) {
ClassNode returnType = mn.stream().map(MethodNode::getReturnType)
.reduce(WideningCategories::lowestUpperBound).get();
call.putNodeMetaData(DYNAMIC_RESOLUTION, returnType);
```
`Stream.reduce` folds in list order, and `lowestUpperBound` is not reliably
order-insensitive (folding interface types can build differing synthetic LUB
nodes). The resulting inferred type drives casts and return types in emitted
bytecode.
2. **Method pointers / method references (`::`),
`StaticTypeCheckingVisitor:3236-3268`** — on more than one candidate it calls
`extension.handleAmbiguousMethods`, whose default implementation
(`TypeCheckingExtension:190`) returns the list unchanged, then applies the same
order-sensitive `lowestUpperBound` fold to type the closure.
3. **Macro dispatch, `MacroCallTransformingVisitor:106-119`** —
`findMacroMethods` returns `chooseBestMethod(...)` directly and the caller
loops over the candidates until one succeeds, so **the first candidate wins**.
When two macro methods both apply, identity-hash order decides which one
expands the call — a *different generated AST*, not merely a different byte
layout.
Ambiguity **error messages** also list candidates in this order, so diagnostics
are unstable too.
### Proposed fix
Use an insertion-ordered set:
```java
Set<MethodNode> bestMethods = new LinkedHashSet<>();
```
`MethodNode` has identity `equals`/`hashCode`, so this is a
semantics-preserving drop-in — deduplication behaviour is unchanged. It yields
a *meaningful* order rather than an arbitrary one: candidates follow the
`duckType` loop, i.e. the union's declared delegate order.
`StaticTypeCheckingVisitor:671` already uses `LinkedHashSet<MethodNode>` for
`PV_METHODS_ACCESS`, so this matches existing practice.
### Suggested test
A `@CompileStatic` union-type receiver (per GROOVY-8965) whose delegates
declare the same method with different return types, asserting the inferred
`DYNAMIC_RESOLUTION` type; and a macro case with two applicable macro methods,
asserting which expansion wins. To exercise the path-dependence, run the
compile on a worker thread and/or after a varying number of identity-hash
allocations — with the `HashSet` the selection shifts, with `LinkedHashSet` it
does not.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
