Re: [PR] GROOVY-12149: order reflection calls to ensure Annotation members are ordered [groovy]

["paulk-asert (via GitHub)" <gi...@apache.org>]

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.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org