[
https://issues.apache.org/jira/browse/GROOVY-12149?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18095825#comment-18095825
]
ASF GitHub Bot commented on GROOVY-12149:
-----------------------------------------
paulk-asert commented on PR #2690:
URL: https://github.com/apache/groovy/pull/2690#issuecomment-4956050602
Claude recommends the following
Here's the concrete recommendation for #2690 — a single total, spec-stable
ordering used by both helpers.
## The comparator
```java
/**
* Orders by name, then parameter types, then return type. This is a total
order:
* the JVM forbids two methods in one class sharing a name and descriptor,
and the
* descriptor is exactly the parameter types plus the return type.
*/
private static int compareMethods(final Method m1, final Method m2) {
int c = m1.getName().compareTo(m2.getName());
if (c != 0) return c;
c = compareParameterTypes(m1.getParameterTypes(),
m2.getParameterTypes());
if (c != 0) return c;
return
m1.getReturnType().getName().compareTo(m2.getReturnType().getName());
}
private static int compareParameterTypes(final Class<?>[] p1, final
Class<?>[] p2) {
int c = Integer.compare(p1.length, p2.length);
if (c != 0) return c;
for (int i = 0; i < p1.length; i += 1) {
c = p1[i].getName().compareTo(p2[i].getName());
if (c != 0) return c;
}
return 0;
}
public static Method[] getDeclaredMethodsSorted(final Class<?> type) {
Method[] methods = type.getDeclaredMethods(); // fresh array; safe to
sort in place
Arrays.sort(methods, ReflectionUtils::compareMethods);
return methods;
}
public static Constructor<?>[] getDeclaredConstructorsSorted(final Class<?>
type) {
Constructor<?>[] constructors = type.getDeclaredConstructors();
// name is always <init> and the return type always void, so the
parameters decide
Arrays.sort(constructors, (c1, c2) ->
compareParameterTypes(c1.getParameterTypes(),
c2.getParameterTypes()));
return constructors;
}
```
## Why this shape
**The return type is mandatory, and it is the whole point.** Leave it out
and a bridge method ties with the method it bridges.
**Parameters before return type**, so a bridge sorts adjacent to the method
it bridges and overloads group by signature.
`CachedMethod.compareToCachedMethod` (`CachedMethod.java:126`) orders name →
return type → parameters instead; either is total, and it's worth citing as the
in-repo precedent that *already* includes the return type for exactly this
reason. Pick one and be consistent.
**Explicit keys rather than `Method::toString`.** The PR's current
`thenComparing(Method::toString)` is genuinely correct — `toString()` includes
the return type, so it is total — but its format is unspecified, which means
byte-identical output across JDK versions rests on an implementation detail,
and it rebuilds a long string on every comparison. Comparing class names is
spec-stable and cheaper.
## And drop the fields
Delete `getDeclaredFieldsSorted` and its use in `configureClassNode`.
HotSpot returns declared fields in class-file order — it's the methods array it
keeps sorted by symbol address, which is why methods and constructors vary and
fields don't. So sorting fields buys no determinism, and it *costs*:
`GeneralUtils.getAllProperties` walks `getFields()` in order, so alphabetizing
a precompiled superclass's fields reorders `@TupleConstructor` parameters. It
also makes the reflection path disagree with the decompiler path, which
preserves class-file order.
Constructors, by contrast, live in the methods array as `<init>` and
genuinely do come back in an arbitrary order — so they belong in the fix
alongside methods.
> Nondeterministic reflection order flows into generated bytecode, breaking
> reproducible builds
> ---------------------------------------------------------------------------------------------
>
> Key: GROOVY-12149
> URL: https://issues.apache.org/jira/browse/GROOVY-12149
> Project: Groovy
> Issue Type: Bug
> Reporter: James Daugherty
> Priority: Major
>
> Follow-up to GROOVY-12146, which fixed nondeterministic ordering of
> annotation members copied from precompiled classes. The same root cause —
> Class.getDeclaredMethods()/getDeclaredFields()/getDeclaredConstructors()
> returning members in an unspecified order that varies between HotSpot runs —
> also affects Java8#configureClassNode, which populates ClassNodes for
> precompiled classes via reflection. The enumeration order is preserved
> through to bytecode generation (e.g. via the LinkedHashMap returned by
> ClassNode#getDeclaredMethodsMap), so two compilations of identical sources on
> the same JDK can still produce byte-different class files.
> Two manifestations were observed while verifying the reproducibility of the
> Apache Grails 8.0.0-M3 release artifacts (same sources, same JDK,
> containerized double-build):
> 1. MOP bridge methods: for classes extending a precompiled Groovy class,
> MopWriter#getSuperMethods iterates the superclass's declared-methods map, so
> the synthetic super$N$… methods are emitted in a different order per build.
> 2. Woven trait methods: methods copied from a precompiled trait (e.g. GORM's
> DirtyCheckable#trackChanges/syncChangedProperties) are woven into
> implementing classes in enumeration order, reordering the emitted methods and
> the constant pool.
> In both cases the bytecode is semantically identical — decompiled sources
> match exactly — only member emission order (and the constant-pool layout that
> follows from it) differs.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
