GROOVY-12149: order reflection calls to ensure Annotation members are ordered

[jdaugherty]

Code changes:

• Added ReflectionUtils#getDeclaredMethodsSorted(Class), #getDeclaredFieldsSorted(Class), and #getDeclaredConstructorsSorted(Class): deterministic-order variants of the JDK reflection calls (methods sorted by name then signature, fields by name, constructors by signature), with javadoc documenting the nondeterminism and why callers whose output feeds bytecode generation must use them.
• Java8#configureClassNode now uses the sorted variants for methods, fields, and constructors, so member order in reflection-backed ClassNodes — and everything derived from it — is deterministic.
• Refactored the GROOVY-12146 fix in Java8#configureAnnotation to reuse getDeclaredMethodsSorted instead of its inline sort, centralizing the sorting so future callers don't repeat it ad hoc.

Sorting is behavior-neutral: member order carries no semantic meaning in these paths; it only affects the byte layout of emitted class files.

[jdaugherty]

@paulk-asert I found more issues after verifying the restage. MOP bridge methods & Woven trait methods. I used a similar fix and extracted the logic to a shared location.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.2484%. Comparing base (ff64047) to head (56048d4).
⚠️ Report is 1 commits behind head on GROOVY_5_0_X.

Additional details and impacted files

@@                  Coverage Diff                   @@
##             GROOVY_5_0_X      #2690        +/-   ##
======================================================
- Coverage         67.2505%   67.2484%   -0.0021%     
- Complexity          29569      29578         +9     
======================================================
  Files                1382       1382                
  Lines              116991     117008        +17     
  Branches            20551      20556         +5     
======================================================
+ Hits                78677      78686         +9     
- Misses              31787      31794         +7     
- Partials             6527       6528         +1     

Files with missing lines	Coverage Δ
...rg/codehaus/groovy/reflection/ReflectionUtils.java	50.9259% <100.0000%> (+9.8148%)	⬆️
...in/java/org/codehaus/groovy/vmplugin/v8/Java8.java	78.4483% <100.0000%> (-0.0618%)	⬇️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[paulk-asert]

Claude recommends the following:

Here's the concrete recommendation for #2690 — a single total, spec-stable ordering used by both helpers.

The comparator

/**
 * 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 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.

[jdaugherty]

On the drop return type note:

Because that "tie" is exactly the failure. Sorting only by name + parameters means a bridge method and the method it bridges compare equal (return 0). Arrays.sort is stable — TimSort — so equal elements keep their input order. And the input order is getDeclaredMethods(), the very thing that's nondeterministic and varies between JVM runs. So the tie doesn't get resolved; it silently inherits the nondeterminism you're trying to eliminate.

Concretely, covariant returns / generic erasure produce this:

class Base { T get() { ... } }
class Sub extends Base {
String get() { ... } // real method
// Object get() // synthetic bridge — same name, same (empty) params
}

Sub has two get methods: same name, identical parameter types, differing only in return type (String vs Object). With a name+params comparator they tie, and whichever the JVM happened to enumerate first wins — per build. Adding the return-type key breaks that last tie, so no two distinct methods ever compare equal.

That's what "total order" buys: the JVM forbids two methods in one class sharing a name and descriptor, and the descriptor is precisely parameters + return type. Compare on the full descriptor and every element has a unique sort key, so the output is fully determined no matter what order getDeclaredMethods() hands back. Leave out the return type and the ordering is merely partial — total everywhere except the bridge pairs, which are the one place reproducibility actually needed it.

(Constructors don't need it: name is always and return type always void, so parameters alone are a total order there — which is why getDeclaredConstructorsSorted stops at compareParameterTypes.)

[jdaugherty]

Should the order match the CachedMethods order or should return be last in the sort is my only question. Currently this implementation is comparing the return last, while CachedMethods compares it in the middle.

[jdaugherty]

There is an error grabbing grapes, which leads me to believe this was a transient issue. Can someone please rekick off the tests for this PR?

[Copilot]

Pull request overview

This PR aims to make reflection-derived member enumeration deterministic so that Groovy’s compile-time processing of precompiled classes yields reproducible (byte-identical) generated class files, specifically addressing annotation member ordering and related reflection-driven ClassNode construction.

Changes:

• Added deterministic sorting helpers in ReflectionUtils for declared methods and constructors.
• Updated Java8 VM plugin code paths to use the sorted reflection results for annotation members, methods, and constructors.
• Removed the inline annotation-member sorting logic in favor of the centralized helper.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/main/java/org/codehaus/groovy/vmplugin/v8/Java8.java	Uses new ReflectionUtils sorted reflection helpers for annotation member processing and for class method/constructor enumeration.
src/main/java/org/codehaus/groovy/reflection/ReflectionUtils.java	Introduces deterministic ordering APIs for getDeclaredMethods() and getDeclaredConstructors() via internal comparison helpers.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.