Try and stabilize Reflect.scala sortInPlace (#4287)

Tries to fix the instability in
https://github.com/com-lihaoyi/mill/actions/runs/12719802399/job/35460719291?pr=4272

It seems `sortInPlaceWith` uses `Ordering.fromLessThan`, which means it
must return `false` when both sides are equal. The previous
implementation returned `true` when both `getDeclaringClass` and
`getReturnType` were equal, which violates that invariant and could
possible cause that crash depending on how the sort executes

Author: lihaoyi

.github/workflows/pre-build.yml

@@ -26,16 +26,27 @@ jobs:
           java-version: ${{ inputs.java-version }}
           distribution: temurin
 
+      # For normal PR jobs, just checkout the base_ref the PR is against
       - uses: actions/checkout@v4
         with:
           ref: ${{ github.base_ref }}
-        if: ${{ !(github.event.action == 'push' && github.repository != 'com-lihaoyi/mill') }}
+        if: ${{ !(github.event_name == 'push' && github.repository != 'com-lihaoyi/mill') }}
 
+      # For fork push jobs, first checkout the version being pushed, then look for the
+      # merge-base where the current version forks off from the upstream main branch
       - uses: actions/checkout@v4
-        with:
-          ref: main
-          repository: https://github.com/com-lihaoyi/mill
-        if: ${{ github.event.action == 'push' && github.repository != 'com-lihaoyi/mill' }}
+        with: { fetch-depth: 0 }
+        if: ${{ github.event_name == 'push' && github.repository != 'com-lihaoyi/mill' }}
+
+      - run: |
+          git fetch https://github.com/com-lihaoyi/mill main
+          MERGE_BASE=$(git merge-base FETCH_HEAD HEAD)
+          # pretty-print the path between the FETCH_HEAD (main), HEAD, and the merge-base
+          git log --graph --pretty=format:"%h %d %ar %s %n" --ancestry-path $MERGE_BASE^1..HEAD --ancestry-path $MERGE_BASE^1..FETCH_HEAD
+
+          git checkout $MERGE_BASE
+        shell: bash
+        if: ${{ github.event_name == 'push' && github.repository != 'com-lihaoyi/mill' }}
 
       - run: echo temurin:${{ inputs.java-version }} > .mill-jvm-version
 

main/define/src/mill/define/Reflect.scala

@@ -29,6 +29,11 @@ private[mill] object Reflect {
       if isLegalIdentifier(n) && (m.getModifiers & Modifier.STATIC) == 0
     } yield (m, n)
 
+  private val classSeqOrdering =
+    Ordering.Implicits.seqOrdering[Seq, Class[_]]((c1, c2) =>
+      if (c1 == c2) 0 else if (c1.isAssignableFrom(c2)) 1 else -1
+    )
+
   def reflect(
       outer: Class[_],
       inner: Class[_],
@@ -56,19 +61,21 @@ private[mill] object Reflect {
     //    which messes up the comparison since all forwarders will have the
     //    same `getDeclaringClass`. To handle these scenarios, also sort by
     //    return type, so we can identify the most specific override
-
+    //
+    // 3. A class can have multiple methods with same name/return-type if they
+    //    differ in parameter types, so sort by those as well
     arr.sortInPlaceWith((m1, m2) =>
-      if (m1.getDeclaringClass.equals(m2.getDeclaringClass)) {
-        m1.getReturnType.isAssignableFrom(m2.getReturnType)
+      if (m1.getName != m2.getName) m1.getName < m2.getName
+      else if (m1.getDeclaringClass != m2.getDeclaringClass) {
+        !m1.getDeclaringClass.isAssignableFrom(m2.getDeclaringClass)
+      } else if (m1.getReturnType != m2.getReturnType) {
+        !m1.getReturnType.isAssignableFrom(m2.getReturnType)
       } else {
-        m1.getDeclaringClass.isAssignableFrom(m2.getDeclaringClass)
+        classSeqOrdering.lt(m1.getParameterTypes, m2.getParameterTypes)
       }
     )
 
-    val res = arr.reverseIterator.distinctBy(_.getName).toArray
-    // Sometimes `getMethods` returns stuff in odd orders, make sure to sort for determinism
-    res.sortInPlaceBy(_.getName)
-    res
+    arr.distinctBy(_.getName)
   }
 
   // For some reason, this fails to pick up concrete `object`s nested directly within