From c9efc7597990a1412a9eb52d6182e98801f0125d Mon Sep 17 00:00:00 2001
From: Myyk Seok <2080820+myyk@users.noreply.github.com>
Date: Wed, 30 Oct 2024 00:47:09 +0800
Subject: [PATCH 01/18] chore: simplify resolve core without functionality
 changes

---
 .../src/mill/resolve/ResolveCore.scala        | 68 +++++++++----------
 1 file changed, 32 insertions(+), 36 deletions(-)

diff --git a/main/resolve/src/mill/resolve/ResolveCore.scala b/main/resolve/src/mill/resolve/ResolveCore.scala
index 5c3a7396a74..604d9737a15 100644
--- a/main/resolve/src/mill/resolve/ResolveCore.scala
+++ b/main/resolve/src/mill/resolve/ResolveCore.scala
@@ -240,31 +240,30 @@ private object ResolveCore {
   }
 
   def resolveTransitiveChildren(
-      rootModule: BaseModule,
-      cls: Class[_],
-      nameOpt: Option[String],
-      segments: Segments,
-      typePattern: Seq[String]
+    rootModule: BaseModule,
+    cls: Class[_],
+    nameOpt: Option[String],
+    segments: Segments,
+    typePattern: Seq[String]
   ): Either[String, Set[Resolved]] = {
-    val direct =
-      resolveDirectChildren(rootModule, cls, nameOpt, segments, typePattern)
-    direct.flatMap { direct =>
-      for {
-        directTraverse <-
-          resolveDirectChildren(rootModule, cls, nameOpt, segments, Nil)
-        indirect0 = directTraverse
-          .collect { case m: Resolved.Module =>
-            resolveTransitiveChildren(
-              rootModule,
-              m.cls,
-              nameOpt,
-              m.segments,
-              typePattern
-            )
-          }
-        indirect <- EitherOps.sequence(indirect0).map(_.flatten)
-      } yield direct ++ indirect
+    val direct = resolveDirectChildren(rootModule, cls, nameOpt, segments, typePattern)
+    val directTraverse = resolveDirectChildren(rootModule, cls, nameOpt, segments, Nil)
+
+    val indirect0 = directTraverse match {
+      case Right(modules) =>
+        modules.flatMap {
+          case m: Resolved.Module =>
+            // TODO: Add cycle detection here
+            Some(resolveTransitiveChildren(rootModule, m.cls, nameOpt, m.segments, typePattern))
+          case _ => None
+        }
+      case Left(err) => Seq(Left(err))
     }
+
+    for {
+      d <- direct
+      i <- EitherOps.sequence(indirect0).map(_.flatten)
+    } yield d ++ i
   }
 
   private def resolveParents(c: Class[_]): Seq[Class[_]] =
@@ -318,21 +317,18 @@ private object ResolveCore {
       }
     } else Right(Nil)
 
-    crossesOrErr.flatMap { crosses =>
-      val filteredCrosses = crosses.filter { c =>
+    for {
+      crosses <- crossesOrErr
+      filteredCrosses = crosses.filter { c =>
         classMatchesTypePred(typePattern)(c.cls)
       }
-
-      resolveDirectChildren0(rootModule, segments, cls, nameOpt, typePattern)
-        .map(
-          _.map {
-            case (Resolved.Module(s, cls), _) => Resolved.Module(segments ++ s, cls)
-            case (Resolved.NamedTask(s), _) => Resolved.NamedTask(segments ++ s)
-            case (Resolved.Command(s), _) => Resolved.Command(segments ++ s)
-          }
-            .toSet
-            .++(filteredCrosses)
-        )
+      direct <- resolveDirectChildren0(rootModule, segments, cls, nameOpt, typePattern)
+    } yield {
+      direct.map {
+        case (Resolved.Module(s, cls), _) => Resolved.Module(segments ++ s, cls)
+        case (Resolved.NamedTask(s), _) => Resolved.NamedTask(segments ++ s)
+        case (Resolved.Command(s), _) => Resolved.Command(segments ++ s)
+      }.toSet ++ filteredCrosses
     }
   }
 

From 1f0675b74a0a0352704f4ba50416986ec0baa909 Mon Sep 17 00:00:00 2001
From: Myyk Seok <2080820+myyk@users.noreply.github.com>
Date: Tue, 29 Oct 2024 16:24:46 +0800
Subject: [PATCH 02/18] test: add test to detect bug for
 https://github.com/com-lihaoyi/mill/issues/3715

---
 .../test/src/mill/main/ResolveTests.scala     | 23 +++++++++
 main/test/src/mill/util/TestGraphs.scala      | 47 +++++++++++++++++++
 .../mill/scalalib/internal/ModuleUtils.scala  |  2 +-
 3 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/main/resolve/test/src/mill/main/ResolveTests.scala b/main/resolve/test/src/mill/main/ResolveTests.scala
index 8c3c1718bac..0683ee9de7f 100644
--- a/main/resolve/test/src/mill/main/ResolveTests.scala
+++ b/main/resolve/test/src/mill/main/ResolveTests.scala
@@ -1110,5 +1110,28 @@ object ResolveTests extends TestSuite {
         Right(Set(_.concrete.tests.inner.foo, _.concrete.tests.inner.innerer.bar))
       )
     }
+    test("cyclicModuleRefInitError") {
+      val check = new Checker(cyclicModuleRefInitError)
+      test - check(
+        "__",
+        Left(
+          "blah blah blah." // TODO: complete this
+        )
+      )
+    }
+    test("nonCyclicModules") {
+      val check = new Checker(nonCyclicModules)
+      test - check(
+        "__",
+        Right(Set()) // TODO: complete this
+      )
+    }
+    test("moduleRefWithNonModuleRefChild") {
+      val check = new Checker(moduleRefWithNonModuleRefChild)
+      test - check(
+        "__",
+        Right(Set()) // TODO: complete this
+      )
+    }
   }
 }
diff --git a/main/test/src/mill/util/TestGraphs.scala b/main/test/src/mill/util/TestGraphs.scala
index 8ca8c563c87..25fc0b74e3f 100644
--- a/main/test/src/mill/util/TestGraphs.scala
+++ b/main/test/src/mill/util/TestGraphs.scala
@@ -222,6 +222,53 @@ class TestGraphs() {
     override lazy val millDiscover = Discover[this.type]
   }
 
+  object cyclicModuleRefInitError extends TestBaseModule {
+    import mill.Agg
+
+    // See issue: https://github.com/com-lihaoyi/mill/issues/3715
+    trait CommonModule extends TestBaseModule {
+      def moduleDeps: Seq[CommonModule] = Seq.empty
+      def a = myA
+      def b = myB
+    }
+
+    object myA extends A
+    trait A extends CommonModule
+    object myB extends B
+    trait B extends CommonModule {
+      override def moduleDeps = super.moduleDeps ++ Agg(a)
+    }
+  }
+
+  // The module names repeat, but it's not actually cyclic and is meant to confuse the cycle detection.
+  object nonCyclicModules extends TestBaseModule {
+    object A extends TestBaseModule {
+      def b = B
+    }
+    object B extends TestBaseModule {
+      object A extends TestBaseModule {
+        def b = B
+      }
+      def a = A
+
+      object B extends TestBaseModule {
+        object B extends TestBaseModule {}
+        object A extends TestBaseModule {
+          def b = B
+        }
+        def a = A
+      }
+    }
+  }
+
+  // This edge case shouldn't be an error
+  object moduleRefWithNonModuleRefChild extends TestBaseModule {
+    def aRef = A
+    def a = ModuleRef(A)
+
+    object A extends TestBaseModule {}
+  }
+
   object overrideModule extends TestBaseModule {
     trait Base extends Module {
       lazy val inner: BaseInnerModule = new BaseInnerModule {}
diff --git a/scalalib/src/mill/scalalib/internal/ModuleUtils.scala b/scalalib/src/mill/scalalib/internal/ModuleUtils.scala
index 54d9f82ae1c..017d2d29c49 100644
--- a/scalalib/src/mill/scalalib/internal/ModuleUtils.scala
+++ b/scalalib/src/mill/scalalib/internal/ModuleUtils.scala
@@ -25,7 +25,7 @@ object ModuleUtils {
    * @param deps A function provided the direct dependencies
    * @throws BuildScriptException if there were cycles in the dependencies
    */
-  // FIMXE: Remove or consolidate with copy in ZincModuleImpl
+  // FIMXE: Remove or consolidate with copy in ZincWorkerImpl
   def recursive[T](name: String, start: T, deps: T => Seq[T]): Seq[T] = {
 
     @tailrec def rec(

From 9fce280cf8c19d90d4ac36a0936586e1707c9cc3 Mon Sep 17 00:00:00 2001
From: Myyk Seok <2080820+myyk@users.noreply.github.com>
Date: Wed, 30 Oct 2024 16:15:54 +0800
Subject: [PATCH 03/18] feat: detect some cyclic references in resolve

---
 .../src/mill/resolve/ResolveCore.scala        | 47 ++++++++++++-------
 .../test/src/mill/main/ResolveTests.scala     |  8 ++--
 main/test/src/mill/util/TestGraphs.scala      | 12 ++---
 3 files changed, 39 insertions(+), 28 deletions(-)

diff --git a/main/resolve/src/mill/resolve/ResolveCore.scala b/main/resolve/src/mill/resolve/ResolveCore.scala
index 604d9737a15..69a834be633 100644
--- a/main/resolve/src/mill/resolve/ResolveCore.scala
+++ b/main/resolve/src/mill/resolve/ResolveCore.scala
@@ -100,7 +100,8 @@ private object ResolveCore {
                     m.cls,
                     None,
                     current.segments,
-                    Nil
+                    Nil,
+                    Set.empty // TODO: We should pass the seenModules set
                   )
 
                 transitiveOrErr.map(transitive => self ++ transitive)
@@ -122,7 +123,8 @@ private object ResolveCore {
                   m.cls,
                   None,
                   current.segments,
-                  typePattern
+                  typePattern,
+                  Set.empty // TODO: We should pass the seenModules set
                 )
 
                 transitiveOrErr.map(transitive => self ++ transitive)
@@ -244,26 +246,37 @@ private object ResolveCore {
     cls: Class[_],
     nameOpt: Option[String],
     segments: Segments,
-    typePattern: Seq[String]
+    typePattern: Seq[String],
+    seenModules: Set[Class[_]],
   ): Either[String, Set[Resolved]] = {
-    val direct = resolveDirectChildren(rootModule, cls, nameOpt, segments, typePattern)
+    val errOrDirect = resolveDirectChildren(rootModule, cls, nameOpt, segments, typePattern)
     val directTraverse = resolveDirectChildren(rootModule, cls, nameOpt, segments, Nil)
 
-    val indirect0 = directTraverse match {
-      case Right(modules) =>
-        modules.flatMap {
-          case m: Resolved.Module =>
-            // TODO: Add cycle detection here
-            Some(resolveTransitiveChildren(rootModule, m.cls, nameOpt, m.segments, typePattern))
-          case _ => None
-        }
-      case Left(err) => Seq(Left(err))
+    val errOrModules = directTraverse.map { modules =>
+      modules.flatMap {
+        case m: Resolved.Module => Some(m)
+        case _ => None
+      }
     }
 
-    for {
-      d <- direct
-      i <- EitherOps.sequence(indirect0).map(_.flatten)
-    } yield d ++ i
+    if (seenModules.contains(cls)) {
+      Left(s"Cyclic module reference detected: ${cls.getName}, it's required to wrap it in ModuleRef. See documentation: https://mill-build.org/mill/0.12.1/fundamentals/modules.html#_abstract_modules_references")
+    } else {
+      val errOrIndirect0 = errOrModules match {
+        case Right(modules) =>
+          modules.flatMap { m =>
+            Some(resolveTransitiveChildren(rootModule, m.cls, nameOpt, m.segments, typePattern, seenModules + cls))
+          }
+        case Left(err) => Seq(Left(err))
+      }
+
+      val errOrIndirect = EitherOps.sequence(errOrIndirect0).map(_.flatten)
+
+      for {
+        direct <- errOrDirect
+        indirect <- errOrIndirect
+      } yield direct ++ indirect
+    }
   }
 
   private def resolveParents(c: Class[_]): Seq[Class[_]] =
diff --git a/main/resolve/test/src/mill/main/ResolveTests.scala b/main/resolve/test/src/mill/main/ResolveTests.scala
index 0683ee9de7f..247c2284a9d 100644
--- a/main/resolve/test/src/mill/main/ResolveTests.scala
+++ b/main/resolve/test/src/mill/main/ResolveTests.scala
@@ -1112,11 +1112,9 @@ object ResolveTests extends TestSuite {
     }
     test("cyclicModuleRefInitError") {
       val check = new Checker(cyclicModuleRefInitError)
-      test - check(
-        "__",
-        Left(
-          "blah blah blah." // TODO: complete this
-        )
+      test - check.checkSeq0(
+        Seq("__"),
+        isShortError(_, "Cyclic module reference detected")
       )
     }
     test("nonCyclicModules") {
diff --git a/main/test/src/mill/util/TestGraphs.scala b/main/test/src/mill/util/TestGraphs.scala
index 25fc0b74e3f..d79a7cf5e8b 100644
--- a/main/test/src/mill/util/TestGraphs.scala
+++ b/main/test/src/mill/util/TestGraphs.scala
@@ -242,18 +242,18 @@ class TestGraphs() {
 
   // The module names repeat, but it's not actually cyclic and is meant to confuse the cycle detection.
   object nonCyclicModules extends TestBaseModule {
-    object A extends TestBaseModule {
+    object A extends Module {
       def b = B
     }
-    object B extends TestBaseModule {
-      object A extends TestBaseModule {
+    object B extends Module {
+      object A extends Module {
         def b = B
       }
       def a = A
 
-      object B extends TestBaseModule {
-        object B extends TestBaseModule {}
-        object A extends TestBaseModule {
+      object B extends Module {
+        object B extends Module {}
+        object A extends Module {
           def b = B
         }
         def a = A

From 8099efe139e5a1b7acb97d3822a7182af75311cc Mon Sep 17 00:00:00 2001
From: Myyk Seok <2080820+myyk@users.noreply.github.com>
Date: Wed, 30 Oct 2024 16:43:57 +0800
Subject: [PATCH 04/18] test: fix the new tests

---
 .../test/src/mill/main/ResolveTests.scala     | 10 +-
 main/test/src/mill/util/TestGraphs.scala      | 97 ++++++++++---------
 2 files changed, 55 insertions(+), 52 deletions(-)

diff --git a/main/resolve/test/src/mill/main/ResolveTests.scala b/main/resolve/test/src/mill/main/ResolveTests.scala
index 247c2284a9d..344556a561e 100644
--- a/main/resolve/test/src/mill/main/ResolveTests.scala
+++ b/main/resolve/test/src/mill/main/ResolveTests.scala
@@ -1111,24 +1111,24 @@ object ResolveTests extends TestSuite {
       )
     }
     test("cyclicModuleRefInitError") {
-      val check = new Checker(cyclicModuleRefInitError)
+      val check = new Checker(TestGraphs.CyclicModuleRefInitError)
       test - check.checkSeq0(
         Seq("__"),
         isShortError(_, "Cyclic module reference detected")
       )
     }
     test("nonCyclicModules") {
-      val check = new Checker(nonCyclicModules)
+      val check = new Checker(TestGraphs.NonCyclicModules)
       test - check(
         "__",
-        Right(Set()) // TODO: complete this
+        Right(Set(_.foo))
       )
     }
     test("moduleRefWithNonModuleRefChild") {
-      val check = new Checker(moduleRefWithNonModuleRefChild)
+      val check = new Checker(TestGraphs.ModuleRefWithNonModuleRefChild)
       test - check(
         "__",
-        Right(Set()) // TODO: complete this
+        Right(Set(_.foo))
       )
     }
   }
diff --git a/main/test/src/mill/util/TestGraphs.scala b/main/test/src/mill/util/TestGraphs.scala
index d79a7cf5e8b..473c8b06e11 100644
--- a/main/test/src/mill/util/TestGraphs.scala
+++ b/main/test/src/mill/util/TestGraphs.scala
@@ -222,53 +222,6 @@ class TestGraphs() {
     override lazy val millDiscover = Discover[this.type]
   }
 
-  object cyclicModuleRefInitError extends TestBaseModule {
-    import mill.Agg
-
-    // See issue: https://github.com/com-lihaoyi/mill/issues/3715
-    trait CommonModule extends TestBaseModule {
-      def moduleDeps: Seq[CommonModule] = Seq.empty
-      def a = myA
-      def b = myB
-    }
-
-    object myA extends A
-    trait A extends CommonModule
-    object myB extends B
-    trait B extends CommonModule {
-      override def moduleDeps = super.moduleDeps ++ Agg(a)
-    }
-  }
-
-  // The module names repeat, but it's not actually cyclic and is meant to confuse the cycle detection.
-  object nonCyclicModules extends TestBaseModule {
-    object A extends Module {
-      def b = B
-    }
-    object B extends Module {
-      object A extends Module {
-        def b = B
-      }
-      def a = A
-
-      object B extends Module {
-        object B extends Module {}
-        object A extends Module {
-          def b = B
-        }
-        def a = A
-      }
-    }
-  }
-
-  // This edge case shouldn't be an error
-  object moduleRefWithNonModuleRefChild extends TestBaseModule {
-    def aRef = A
-    def a = ModuleRef(A)
-
-    object A extends TestBaseModule {}
-  }
-
   object overrideModule extends TestBaseModule {
     trait Base extends Module {
       lazy val inner: BaseInnerModule = new BaseInnerModule {}
@@ -714,4 +667,54 @@ object TestGraphs {
     }
   }
 
+  object CyclicModuleRefInitError extends TestBaseModule {
+    import mill.Agg
+
+    // See issue: https://github.com/com-lihaoyi/mill/issues/3715
+    trait CommonModule extends TestBaseModule {
+      def moduleDeps: Seq[CommonModule] = Seq.empty
+      def a = myA
+      def b = myB
+    }
+
+    object myA extends A
+    trait A extends CommonModule
+    object myB extends B
+    trait B extends CommonModule {
+      override def moduleDeps = super.moduleDeps ++ Agg(a)
+    }
+  }
+
+  // The module names repeat, but it's not actually cyclic and is meant to confuse the cycle detection.
+  object NonCyclicModules extends TestBaseModule {
+    def foo = Task { "foo" }
+
+    object A extends Module {
+      def b = B
+    }
+    object B extends Module {
+      object A extends Module {
+        def b = B
+      }
+      def a = A
+
+      object B extends Module {
+        object B extends Module {}
+        object A extends Module {
+          def b = B
+        }
+        def a = A
+      }
+    }
+  }
+
+  // This edge case shouldn't be an error
+  object ModuleRefWithNonModuleRefChild extends TestBaseModule {
+    def foo = Task { "foo" }
+
+    def aRef = A
+    def a = ModuleRef(A)
+
+    object A extends TestBaseModule {}
+  }
 }

From 7688b395605b2babeefde9d9721378b23f2c1bb2 Mon Sep 17 00:00:00 2001
From: Myyk Seok <2080820+myyk@users.noreply.github.com>
Date: Wed, 30 Oct 2024 16:51:01 +0800
Subject: [PATCH 05/18] test: add a couple more tests

---
 .../resolve/test/src/mill/main/ResolveTests.scala | 14 ++++++++++++++
 main/test/src/mill/util/TestGraphs.scala          | 15 +++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/main/resolve/test/src/mill/main/ResolveTests.scala b/main/resolve/test/src/mill/main/ResolveTests.scala
index 344556a561e..be7abe734db 100644
--- a/main/resolve/test/src/mill/main/ResolveTests.scala
+++ b/main/resolve/test/src/mill/main/ResolveTests.scala
@@ -1117,6 +1117,20 @@ object ResolveTests extends TestSuite {
         isShortError(_, "Cyclic module reference detected")
       )
     }
+    test("cyclicModuleRefInitError2") {
+      val check = new Checker(TestGraphs.CyclicModuleRefInitError2)
+      test - check.checkSeq0(
+        Seq("__"),
+        isShortError(_, "Cyclic module reference detected")
+      )
+    }
+    test("cyclicModuleRefInitError3") {
+      val check = new Checker(TestGraphs.CyclicModuleRefInitError3)
+      test - check.checkSeq0(
+        Seq("__"),
+        isShortError(_, "Cyclic module reference detected")
+      )
+    }
     test("nonCyclicModules") {
       val check = new Checker(TestGraphs.NonCyclicModules)
       test - check(
diff --git a/main/test/src/mill/util/TestGraphs.scala b/main/test/src/mill/util/TestGraphs.scala
index 473c8b06e11..c127d338152 100644
--- a/main/test/src/mill/util/TestGraphs.scala
+++ b/main/test/src/mill/util/TestGraphs.scala
@@ -685,6 +685,21 @@ object TestGraphs {
     }
   }
 
+  object CyclicModuleRefInitError2 extends TestBaseModule {
+    // The cycle is in the child
+    def A = CyclicModuleRefInitError
+  }
+
+  object CyclicModuleRefInitError3 extends TestBaseModule {
+    // The cycle is in directly here
+    object A extends Module {
+      def b = B
+    }
+    object B extends Module {
+      def a = A
+    }
+  }
+
   // The module names repeat, but it's not actually cyclic and is meant to confuse the cycle detection.
   object NonCyclicModules extends TestBaseModule {
     def foo = Task { "foo" }

From a4ae8d65ada3b57d929f2c455e464926a7f7342d Mon Sep 17 00:00:00 2001
From: Myyk Seok <2080820+myyk@users.noreply.github.com>
Date: Wed, 30 Oct 2024 17:02:18 +0800
Subject: [PATCH 06/18] test: add cycle in a cross test case

---
 .../resolve/test/src/mill/main/ResolveTests.scala |  7 +++++++
 main/test/src/mill/util/TestGraphs.scala          | 15 +++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/main/resolve/test/src/mill/main/ResolveTests.scala b/main/resolve/test/src/mill/main/ResolveTests.scala
index be7abe734db..579e9ce2a2b 100644
--- a/main/resolve/test/src/mill/main/ResolveTests.scala
+++ b/main/resolve/test/src/mill/main/ResolveTests.scala
@@ -1131,6 +1131,13 @@ object ResolveTests extends TestSuite {
         isShortError(_, "Cyclic module reference detected")
       )
     }
+    test("crossedCyclicModuleRefInitError") {
+      val check = new Checker(TestGraphs.CrossedCyclicModuleRefInitError)
+      test - check.checkSeq0(
+        Seq("__"),
+        isShortError(_, "Cyclic module reference detected")
+      )
+    }
     test("nonCyclicModules") {
       val check = new Checker(TestGraphs.NonCyclicModules)
       test - check(
diff --git a/main/test/src/mill/util/TestGraphs.scala b/main/test/src/mill/util/TestGraphs.scala
index c127d338152..31bdee150ea 100644
--- a/main/test/src/mill/util/TestGraphs.scala
+++ b/main/test/src/mill/util/TestGraphs.scala
@@ -700,6 +700,21 @@ object TestGraphs {
     }
   }
 
+  object CrossedCyclicModuleRefInitError extends TestBaseModule {
+    object cross extends mill.Cross[Cross]("210", "211", "212")
+    trait Cross extends Cross.Module[String] {
+      def suffix = Task { crossValue }
+      def c2 = cross2
+    }
+
+    object cross2 extends mill.Cross[Cross2]("210", "211", "212")
+    trait Cross2 extends Cross.Module[String] {
+      override def millSourcePath = super.millSourcePath / crossValue
+      def suffix = Task { crossValue }
+      def c1 = cross
+    }
+  }
+
   // The module names repeat, but it's not actually cyclic and is meant to confuse the cycle detection.
   object NonCyclicModules extends TestBaseModule {
     def foo = Task { "foo" }

From 22dd5ce39854f3a0e631766f3a1142b0921c5768 Mon Sep 17 00:00:00 2001
From: Myyk Seok <2080820+myyk@users.noreply.github.com>
Date: Wed, 30 Oct 2024 17:11:44 +0800
Subject: [PATCH 07/18] test: add another ModuleRef cycle test

---
 main/resolve/test/src/mill/main/ResolveTests.scala |  7 +++++++
 main/test/src/mill/util/TestGraphs.scala           | 12 ++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/main/resolve/test/src/mill/main/ResolveTests.scala b/main/resolve/test/src/mill/main/ResolveTests.scala
index 579e9ce2a2b..7ffd361063c 100644
--- a/main/resolve/test/src/mill/main/ResolveTests.scala
+++ b/main/resolve/test/src/mill/main/ResolveTests.scala
@@ -1152,5 +1152,12 @@ object ResolveTests extends TestSuite {
         Right(Set(_.foo))
       )
     }
+    test("moduleRefCycle") {
+      val check = new Checker(TestGraphs.ModuleRefCycle)
+      test - check(
+        "__",
+        Right(Set(_.foo))
+      )
+    }
   }
 }
diff --git a/main/test/src/mill/util/TestGraphs.scala b/main/test/src/mill/util/TestGraphs.scala
index 31bdee150ea..be6ac674de9 100644
--- a/main/test/src/mill/util/TestGraphs.scala
+++ b/main/test/src/mill/util/TestGraphs.scala
@@ -747,4 +747,16 @@ object TestGraphs {
 
     object A extends TestBaseModule {}
   }
+
+  object ModuleRefCycle extends TestBaseModule {
+    def foo = Task { "foo" }
+
+    // The cycle is in directly here
+    object A extends Module {
+      def b = ModuleRef(B)
+    }
+    object B extends Module {
+      def a = ModuleRef(A)
+    }
+  }
 }

From 01e0a869864a1d8cdae90d949d3c7bcdd52bca2d Mon Sep 17 00:00:00 2001
From: Myyk Seok <2080820+myyk@users.noreply.github.com>
Date: Thu, 31 Oct 2024 11:51:56 +0800
Subject: [PATCH 08/18] chore: small cleanup before PR

---
 main/resolve/src/mill/resolve/ResolveCore.scala | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/main/resolve/src/mill/resolve/ResolveCore.scala b/main/resolve/src/mill/resolve/ResolveCore.scala
index 69a834be633..25a05b5cdc3 100644
--- a/main/resolve/src/mill/resolve/ResolveCore.scala
+++ b/main/resolve/src/mill/resolve/ResolveCore.scala
@@ -101,7 +101,7 @@ private object ResolveCore {
                     None,
                     current.segments,
                     Nil,
-                    Set.empty // TODO: We should pass the seenModules set
+                    Set.empty
                   )
 
                 transitiveOrErr.map(transitive => self ++ transitive)
@@ -124,7 +124,7 @@ private object ResolveCore {
                   None,
                   current.segments,
                   typePattern,
-                  Set.empty // TODO: We should pass the seenModules set
+                  Set.empty
                 )
 
                 transitiveOrErr.map(transitive => self ++ transitive)
@@ -341,7 +341,8 @@ private object ResolveCore {
         case (Resolved.Module(s, cls), _) => Resolved.Module(segments ++ s, cls)
         case (Resolved.NamedTask(s), _) => Resolved.NamedTask(segments ++ s)
         case (Resolved.Command(s), _) => Resolved.Command(segments ++ s)
-      }.toSet ++ filteredCrosses
+      }
+        .toSet ++ filteredCrosses
     }
   }
 

From 78ab6ca064568cc4ad095e83e0382723c6569392 Mon Sep 17 00:00:00 2001
From: Myyk Seok <2080820+myyk@users.noreply.github.com>
Date: Thu, 31 Oct 2024 11:55:45 +0800
Subject: [PATCH 09/18] chore: split out change in ModuleUtils to it's own
 branch

---
 scalalib/src/mill/scalalib/internal/ModuleUtils.scala | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scalalib/src/mill/scalalib/internal/ModuleUtils.scala b/scalalib/src/mill/scalalib/internal/ModuleUtils.scala
index 017d2d29c49..54d9f82ae1c 100644
--- a/scalalib/src/mill/scalalib/internal/ModuleUtils.scala
+++ b/scalalib/src/mill/scalalib/internal/ModuleUtils.scala
@@ -25,7 +25,7 @@ object ModuleUtils {
    * @param deps A function provided the direct dependencies
    * @throws BuildScriptException if there were cycles in the dependencies
    */
-  // FIMXE: Remove or consolidate with copy in ZincWorkerImpl
+  // FIMXE: Remove or consolidate with copy in ZincModuleImpl
   def recursive[T](name: String, start: T, deps: T => Seq[T]): Seq[T] = {
 
     @tailrec def rec(

From a13ee360919abaeb6a37e0c9486f93fcbd9a351d Mon Sep 17 00:00:00 2001
From: Myyk Seok <2080820+myyk@users.noreply.github.com>
Date: Fri, 1 Nov 2024 13:17:20 +0800
Subject: [PATCH 10/18] chore: remove link from error message in ResolveCore

---
 main/resolve/src/mill/resolve/ResolveCore.scala | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/main/resolve/src/mill/resolve/ResolveCore.scala b/main/resolve/src/mill/resolve/ResolveCore.scala
index 25a05b5cdc3..86b5e4b1605 100644
--- a/main/resolve/src/mill/resolve/ResolveCore.scala
+++ b/main/resolve/src/mill/resolve/ResolveCore.scala
@@ -260,7 +260,7 @@ private object ResolveCore {
     }
 
     if (seenModules.contains(cls)) {
-      Left(s"Cyclic module reference detected: ${cls.getName}, it's required to wrap it in ModuleRef. See documentation: https://mill-build.org/mill/0.12.1/fundamentals/modules.html#_abstract_modules_references")
+      Left(s"Cyclic module reference detected: ${cls.getName}, it's required to wrap it in ModuleRef.")
     } else {
       val errOrIndirect0 = errOrModules match {
         case Right(modules) =>

From 2448e5bf90ad081cf27455ffe503f1c3077409c3 Mon Sep 17 00:00:00 2001
From: Myyk Seok <2080820+myyk@users.noreply.github.com>
Date: Fri, 1 Nov 2024 15:43:06 +0800
Subject: [PATCH 11/18] chore: add some more passing test cases

---
 .../test/src/mill/main/ResolveTests.scala        | 16 ++++++++++++++++
 main/test/src/mill/util/TestGraphs.scala         |  2 ++
 2 files changed, 18 insertions(+)

diff --git a/main/resolve/test/src/mill/main/ResolveTests.scala b/main/resolve/test/src/mill/main/ResolveTests.scala
index 7ffd361063c..d0da602c6e2 100644
--- a/main/resolve/test/src/mill/main/ResolveTests.scala
+++ b/main/resolve/test/src/mill/main/ResolveTests.scala
@@ -1116,6 +1116,14 @@ object ResolveTests extends TestSuite {
         Seq("__"),
         isShortError(_, "Cyclic module reference detected")
       )
+      test - check.checkSeq0(
+        Seq("myA.__"),
+        isShortError(_, "Cyclic module reference detected")
+      )
+      test - check.checkSeq0(
+        Seq("myA.a.__"),
+        isShortError(_, "Cyclic module reference detected")
+      )
     }
     test("cyclicModuleRefInitError2") {
       val check = new Checker(TestGraphs.CyclicModuleRefInitError2)
@@ -1130,6 +1138,14 @@ object ResolveTests extends TestSuite {
         Seq("__"),
         isShortError(_, "Cyclic module reference detected")
       )
+      test - check.checkSeq0(
+        Seq("A.__"),
+        isShortError(_, "Cyclic module reference detected")
+      )
+      test - check.checkSeq0(
+        Seq("A.b.__.a.b"),
+        isShortError(_, "Cyclic module reference detected")
+      )
     }
     test("crossedCyclicModuleRefInitError") {
       val check = new Checker(TestGraphs.CrossedCyclicModuleRefInitError)
diff --git a/main/test/src/mill/util/TestGraphs.scala b/main/test/src/mill/util/TestGraphs.scala
index be6ac674de9..67c8341953b 100644
--- a/main/test/src/mill/util/TestGraphs.scala
+++ b/main/test/src/mill/util/TestGraphs.scala
@@ -669,9 +669,11 @@ object TestGraphs {
 
   object CyclicModuleRefInitError extends TestBaseModule {
     import mill.Agg
+    def foo = Task { "foo" }
 
     // See issue: https://github.com/com-lihaoyi/mill/issues/3715
     trait CommonModule extends TestBaseModule {
+      def foo = Task { "foo" }
       def moduleDeps: Seq[CommonModule] = Seq.empty
       def a = myA
       def b = myB

From ca775e6d4ae70e95b4b08554fca48d92f6630b0f Mon Sep 17 00:00:00 2001
From: Myyk Seok <2080820+myyk@users.noreply.github.com>
Date: Fri, 1 Nov 2024 15:43:06 +0800
Subject: [PATCH 12/18] chore: add some more passing test cases

---
 .../test/src/mill/main/ResolveTests.scala      | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/main/resolve/test/src/mill/main/ResolveTests.scala b/main/resolve/test/src/mill/main/ResolveTests.scala
index d0da602c6e2..b845675137a 100644
--- a/main/resolve/test/src/mill/main/ResolveTests.scala
+++ b/main/resolve/test/src/mill/main/ResolveTests.scala
@@ -1116,14 +1116,32 @@ object ResolveTests extends TestSuite {
         Seq("__"),
         isShortError(_, "Cyclic module reference detected")
       )
+      test - check(
+        "_",
+        Right(Set(_.foo))
+      )
       test - check.checkSeq0(
         Seq("myA.__"),
         isShortError(_, "Cyclic module reference detected")
       )
+      test - check(
+        "myA.a._",
+        Right(Set(_.foo))
+      )
       test - check.checkSeq0(
         Seq("myA.a.__"),
         isShortError(_, "Cyclic module reference detected")
       )
+//      // FIXME: Cannot test like this because it "Cannot find default task to evaluate"
+//      test - check(
+//        "myA.a._.a",
+//        Right(Set(_.foo))
+//      )
+//      // FIXME: Cannot test like this because it "Cannot find default task to evaluate"
+//      test - check.checkSeq0(
+//        Seq("myA.a.b.a"),
+//        isShortError(_, "Cyclic module reference detected")
+//      )
     }
     test("cyclicModuleRefInitError2") {
       val check = new Checker(TestGraphs.CyclicModuleRefInitError2)

From 04da6aa73d833b7c13d204d2ab97cac3b576c52c Mon Sep 17 00:00:00 2001
From: Myyk Seok <2080820+myyk@users.noreply.github.com>
Date: Fri, 1 Nov 2024 15:44:00 +0800
Subject: [PATCH 13/18] .

---
 main/resolve/test/src/mill/main/ResolveTests.scala | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/main/resolve/test/src/mill/main/ResolveTests.scala b/main/resolve/test/src/mill/main/ResolveTests.scala
index b845675137a..da01878d14d 100644
--- a/main/resolve/test/src/mill/main/ResolveTests.scala
+++ b/main/resolve/test/src/mill/main/ResolveTests.scala
@@ -1124,14 +1124,15 @@ object ResolveTests extends TestSuite {
         Seq("myA.__"),
         isShortError(_, "Cyclic module reference detected")
       )
-      test - check(
-        "myA.a._",
-        Right(Set(_.foo))
-      )
       test - check.checkSeq0(
         Seq("myA.a.__"),
         isShortError(_, "Cyclic module reference detected")
       )
+      // FIXME: Cannot test like this because it "Cannot find default task to evaluate"
+      test - check.checkSeq0(
+        Seq("myA.a._"),
+        isShortError(_, "Cyclic module reference detected")
+      )
 //      // FIXME: Cannot test like this because it "Cannot find default task to evaluate"
 //      test - check(
 //        "myA.a._.a",

From 1064f0427b1641464cb70f05deee88678e5fdf7c Mon Sep 17 00:00:00 2001
From: Myyk Seok <2080820+myyk@users.noreply.github.com>
Date: Fri, 1 Nov 2024 17:18:23 +0800
Subject: [PATCH 14/18] feat: wip - direct dependencies that don't
 StackOverflow will also return an error when cyclic. bug: foo.__.bar patterns
 break this though.

---
 main/resolve/src/mill/resolve/Resolve.scala   |  2 +-
 .../src/mill/resolve/ResolveCore.scala        | 76 ++++++++++++-------
 .../test/src/mill/main/ResolveTests.scala     | 23 +++---
 3 files changed, 63 insertions(+), 38 deletions(-)

diff --git a/main/resolve/src/mill/resolve/Resolve.scala b/main/resolve/src/mill/resolve/Resolve.scala
index 0a8703996e4..68672972a9b 100644
--- a/main/resolve/src/mill/resolve/Resolve.scala
+++ b/main/resolve/src/mill/resolve/Resolve.scala
@@ -73,7 +73,7 @@ object Resolve {
                 rootModule,
                 value.getClass,
                 Some(value.defaultCommandName()),
-                value.millModuleSegments
+                value.millModuleSegments,
               )
 
               directChildrenOrErr.flatMap(directChildren =>
diff --git a/main/resolve/src/mill/resolve/ResolveCore.scala b/main/resolve/src/mill/resolve/ResolveCore.scala
index 86b5e4b1605..c16f558ca67 100644
--- a/main/resolve/src/mill/resolve/ResolveCore.scala
+++ b/main/resolve/src/mill/resolve/ResolveCore.scala
@@ -66,32 +66,53 @@ private object ResolveCore {
       rootModule: BaseModule,
       remainingQuery: List[Segment],
       current: Resolved,
-      querySoFar: Segments
+      querySoFar: Segments,
+      seenModules: Set[Class[_]] = Set.empty
   ): Result = {
+    def moduleClasses(resolved: Iterable[Resolved]): Set[Class[_]] = {
+      resolved.collect {
+        case Resolved.Module(_, cls) => cls
+      }.toSet
+    }
+
     remainingQuery match {
       case Nil => Success(Set(current))
       case head :: tail =>
         def recurse(searchModules: Set[Resolved]): Result = {
-          val (failures, successesLists) = searchModules
-            .map(r => resolve(rootModule, tail, r, querySoFar ++ Seq(head)))
-            .partitionMap { case s: Success => Right(s.value); case f: Failed => Left(f) }
-
-          val (errors, notFounds) = failures.partitionMap {
-            case s: NotFound => Right(s)
-            case s: Error => Left(s.msg)
+          val searchModuleClasses = moduleClasses(searchModules)
+          val checkedSearchModules = if (seenModules.intersect(searchModuleClasses).nonEmpty) {
+            Left(s"Cyclic module reference detected: [${seenModules.intersect(searchModuleClasses).mkString(", ")}], it's required to wrap it in ModuleRef.")
+          } else { Right(searchModules) }
+
+          val errOrResults = for {
+            checkedSearchModules0 <- checkedSearchModules
+            searchModules <- Right(checkedSearchModules0)
+          } yield {
+            searchModules.map(r => resolve(rootModule, tail, r, querySoFar ++ Seq(head), seenModules ++ moduleClasses(Set(r))))
+              .partitionMap { case s: Success => Right(s.value); case f: Failed => Left(f) }
           }
 
-          if (errors.nonEmpty) Error(errors.mkString("\n"))
-          else if (successesLists.flatten.nonEmpty) Success(successesLists.flatten)
-          else notFounds.size match {
-            case 1 => notFounds.head
-            case _ => notFoundResult(rootModule, querySoFar, current, head)
+          errOrResults match {
+            case Left(msg) => Error(msg)
+            case Right(results) =>
+              val (failures, successesLists) = results
+              val (errors, notFounds) = failures.partitionMap {
+                case s: NotFound => Right(s)
+                case s: Error => Left(s.msg)
+              }
+
+              if (errors.nonEmpty) Error(errors.mkString("\n"))
+              else if (successesLists.flatten.nonEmpty) Success(successesLists.flatten)
+              else notFounds.size match {
+                case 1 => notFounds.head
+                case _ => notFoundResult(rootModule, querySoFar, current, head)
+              }
           }
         }
 
         (head, current) match {
           case (Segment.Label(singleLabel), m: Resolved.Module) =>
-            val resOrErr = singleLabel match {
+            val resOrErr: Either[String, Iterable[Resolved]] = singleLabel match {
               case "__" =>
                 val self = Seq(Resolved.Module(m.segments, m.cls))
                 val transitiveOrErr =
@@ -111,7 +132,7 @@ private object ResolveCore {
                   rootModule,
                   m.cls,
                   None,
-                  current.segments
+                  current.segments,
                 )
 
               case pattern if pattern.startsWith("__:") =>
@@ -144,7 +165,7 @@ private object ResolveCore {
                   rootModule,
                   m.cls,
                   Some(singleLabel),
-                  current.segments
+                  current.segments,
                 )
             }
 
@@ -210,7 +231,7 @@ private object ResolveCore {
           rootModule,
           current.millModuleSegments,
           current.getClass,
-          Some(s)
+          Some(s),
         ).flatMap {
           case Seq((_, Some(f))) =>
             val res = f(current)
@@ -318,7 +339,6 @@ private object ResolveCore {
       segments: Segments,
       typePattern: Seq[String] = Nil
   ): Either[String, Set[Resolved]] = {
-
     val crossesOrErr = if (classOf[Cross[_]].isAssignableFrom(cls) && nameOpt.isEmpty) {
       instantiateModule(rootModule, segments).map {
         case cross: Cross[_] =>
@@ -330,19 +350,23 @@ private object ResolveCore {
       }
     } else Right(Nil)
 
+    def expandSegments(direct: Seq[(Resolved, Option[Module => Either[String, Module]])]) = {
+      direct.map {
+        case (Resolved.Module(s, cls), _) => Resolved.Module(segments ++ s, cls)
+        case (Resolved.NamedTask(s), _) => Resolved.NamedTask(segments ++ s)
+        case (Resolved.Command(s), _) => Resolved.Command(segments ++ s)
+      }
+    }
+
     for {
       crosses <- crossesOrErr
       filteredCrosses = crosses.filter { c =>
         classMatchesTypePred(typePattern)(c.cls)
       }
-      direct <- resolveDirectChildren0(rootModule, segments, cls, nameOpt, typePattern)
+      direct0 <- resolveDirectChildren0(rootModule, segments, cls, nameOpt, typePattern)
+      direct <- Right(expandSegments(direct0))
     } yield {
-      direct.map {
-        case (Resolved.Module(s, cls), _) => Resolved.Module(segments ++ s, cls)
-        case (Resolved.NamedTask(s), _) => Resolved.NamedTask(segments ++ s)
-        case (Resolved.Command(s), _) => Resolved.Command(segments ++ s)
-      }
-        .toSet ++ filteredCrosses
+      direct.toSet ++ filteredCrosses
     }
   }
 
@@ -413,7 +437,7 @@ private object ResolveCore {
           rootModule,
           m.cls,
           None,
-          current.segments
+          current.segments,
         ).toOption.get.map(
           _.segments.value.last
         )
diff --git a/main/resolve/test/src/mill/main/ResolveTests.scala b/main/resolve/test/src/mill/main/ResolveTests.scala
index da01878d14d..e15407dca86 100644
--- a/main/resolve/test/src/mill/main/ResolveTests.scala
+++ b/main/resolve/test/src/mill/main/ResolveTests.scala
@@ -1128,21 +1128,18 @@ object ResolveTests extends TestSuite {
         Seq("myA.a.__"),
         isShortError(_, "Cyclic module reference detected")
       )
-      // FIXME: Cannot test like this because it "Cannot find default task to evaluate"
       test - check.checkSeq0(
         Seq("myA.a._"),
         isShortError(_, "Cyclic module reference detected")
       )
-//      // FIXME: Cannot test like this because it "Cannot find default task to evaluate"
-//      test - check(
-//        "myA.a._.a",
-//        Right(Set(_.foo))
-//      )
-//      // FIXME: Cannot test like this because it "Cannot find default task to evaluate"
-//      test - check.checkSeq0(
-//        Seq("myA.a.b.a"),
-//        isShortError(_, "Cyclic module reference detected")
-//      )
+      test - check.checkSeq0(
+        Seq("myA.a._.a"),
+        isShortError(_, "Cyclic module reference detected")
+      )
+      test - check.checkSeq0(
+        Seq("myA.a.b.a"),
+        isShortError(_, "Cyclic module reference detected")
+      )
     }
     test("cyclicModuleRefInitError2") {
       val check = new Checker(TestGraphs.CyclicModuleRefInitError2)
@@ -1193,6 +1190,10 @@ object ResolveTests extends TestSuite {
         "__",
         Right(Set(_.foo))
       )
+      test - check(
+        "__._",
+        Right(Set(_.foo))
+      )
     }
   }
 }

From 235d9f96d0836b21149d47e6c009806e02731a77 Mon Sep 17 00:00:00 2001
From: Li Haoyi <haoyi.sg@gmail.com>
Date: Fri, 1 Nov 2024 03:23:06 -0700
Subject: [PATCH 15/18] .

---
 main/resolve/src/mill/resolve/ResolveCore.scala | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/main/resolve/src/mill/resolve/ResolveCore.scala b/main/resolve/src/mill/resolve/ResolveCore.scala
index c16f558ca67..de312bd8e44 100644
--- a/main/resolve/src/mill/resolve/ResolveCore.scala
+++ b/main/resolve/src/mill/resolve/ResolveCore.scala
@@ -88,7 +88,7 @@ private object ResolveCore {
             checkedSearchModules0 <- checkedSearchModules
             searchModules <- Right(checkedSearchModules0)
           } yield {
-            searchModules.map(r => resolve(rootModule, tail, r, querySoFar ++ Seq(head), seenModules ++ moduleClasses(Set(r))))
+            searchModules.map(r => resolve(rootModule, tail, r, querySoFar ++ Seq(head), seenModules ++ moduleClasses(Set(current))))
               .partitionMap { case s: Success => Right(s.value); case f: Failed => Left(f) }
           }
 

From 15f045935fce4954feb54bcbf19d6d090ca12e30 Mon Sep 17 00:00:00 2001
From: Li Haoyi <haoyi.sg@gmail.com>
Date: Fri, 1 Nov 2024 04:19:16 -0700
Subject: [PATCH 16/18] .

---
 main/resolve/src/mill/resolve/Resolve.scala   |  3 +-
 .../src/mill/resolve/ResolveCore.scala        | 40 +++++++------------
 2 files changed, 17 insertions(+), 26 deletions(-)

diff --git a/main/resolve/src/mill/resolve/Resolve.scala b/main/resolve/src/mill/resolve/Resolve.scala
index 68672972a9b..d686dcfa8e6 100644
--- a/main/resolve/src/mill/resolve/Resolve.scala
+++ b/main/resolve/src/mill/resolve/Resolve.scala
@@ -284,7 +284,8 @@ trait Resolve[T] {
         rootModule = rootModule,
         remainingQuery = sel.value.toList,
         current = rootResolved,
-        querySoFar = Segments()
+        querySoFar = Segments(),
+        seenModules = Set.empty
       ) match {
         case ResolveCore.Success(value) => Right(value)
         case ResolveCore.NotFound(segments, found, next, possibleNexts) =>
diff --git a/main/resolve/src/mill/resolve/ResolveCore.scala b/main/resolve/src/mill/resolve/ResolveCore.scala
index de312bd8e44..3b4e3f62566 100644
--- a/main/resolve/src/mill/resolve/ResolveCore.scala
+++ b/main/resolve/src/mill/resolve/ResolveCore.scala
@@ -80,33 +80,23 @@ private object ResolveCore {
       case head :: tail =>
         def recurse(searchModules: Set[Resolved]): Result = {
           val searchModuleClasses = moduleClasses(searchModules)
-          val checkedSearchModules = if (seenModules.intersect(searchModuleClasses).nonEmpty) {
-            Left(s"Cyclic module reference detected: [${seenModules.intersect(searchModuleClasses).mkString(", ")}], it's required to wrap it in ModuleRef.")
-          } else { Right(searchModules) }
-
-          val errOrResults = for {
-            checkedSearchModules0 <- checkedSearchModules
-            searchModules <- Right(checkedSearchModules0)
-          } yield {
-            searchModules.map(r => resolve(rootModule, tail, r, querySoFar ++ Seq(head), seenModules ++ moduleClasses(Set(current))))
+          if (seenModules.intersect(searchModuleClasses).nonEmpty) {
+            Error(s"Cyclic module reference detected: [${seenModules.intersect(searchModuleClasses).mkString(", ")}], it's required to wrap it in ModuleRef.")
+          } else {
+            val results = searchModules.map(r => resolve(rootModule, tail, r, querySoFar ++ Seq(head), seenModules ++ moduleClasses(Set(current))))
               .partitionMap { case s: Success => Right(s.value); case f: Failed => Left(f) }
-          }
-
-          errOrResults match {
-            case Left(msg) => Error(msg)
-            case Right(results) =>
-              val (failures, successesLists) = results
-              val (errors, notFounds) = failures.partitionMap {
-                case s: NotFound => Right(s)
-                case s: Error => Left(s.msg)
-              }
+            val (failures, successesLists) = results
+            val (errors, notFounds) = failures.partitionMap {
+              case s: NotFound => Right(s)
+              case s: Error => Left(s.msg)
+            }
 
-              if (errors.nonEmpty) Error(errors.mkString("\n"))
-              else if (successesLists.flatten.nonEmpty) Success(successesLists.flatten)
-              else notFounds.size match {
-                case 1 => notFounds.head
-                case _ => notFoundResult(rootModule, querySoFar, current, head)
-              }
+            if (errors.nonEmpty) Error(errors.mkString("\n"))
+            else if (successesLists.flatten.nonEmpty) Success(successesLists.flatten)
+            else notFounds.size match {
+              case 1 => notFounds.head
+              case _ => notFoundResult(rootModule, querySoFar, current, head)
+            }
           }
         }
 

From d052f7686400ede10c9586e0edc7d3ab0a6c2b36 Mon Sep 17 00:00:00 2001
From: Li Haoyi <haoyi.sg@gmail.com>
Date: Fri, 1 Nov 2024 04:33:58 -0700
Subject: [PATCH 17/18] cleanup

---
 .../src/mill/resolve/ResolveCore.scala        | 72 +++++++++++--------
 .../test/src/mill/main/ResolveTests.scala     | 22 +++---
 2 files changed, 53 insertions(+), 41 deletions(-)

diff --git a/main/resolve/src/mill/resolve/ResolveCore.scala b/main/resolve/src/mill/resolve/ResolveCore.scala
index 3b4e3f62566..52e24ee37b3 100644
--- a/main/resolve/src/mill/resolve/ResolveCore.scala
+++ b/main/resolve/src/mill/resolve/ResolveCore.scala
@@ -62,6 +62,10 @@ private object ResolveCore {
   def makeResultException(e: Throwable, base: Exception): Left[String, Nothing] =
     mill.api.Result.makeResultException(e, base)
 
+  def cyclicModuleErrorMsg(segments: Segments) = {
+    s"Cyclic module reference detected at ${segments.render}, " +
+      s"it's required to wrap it in ModuleRef."
+  }
   def resolve(
       rootModule: BaseModule,
       remainingQuery: List[Segment],
@@ -70,34 +74,43 @@ private object ResolveCore {
       seenModules: Set[Class[_]] = Set.empty
   ): Result = {
     def moduleClasses(resolved: Iterable[Resolved]): Set[Class[_]] = {
-      resolved.collect {
-        case Resolved.Module(_, cls) => cls
-      }.toSet
+      resolved.collect { case Resolved.Module(_, cls) => cls }.toSet
     }
 
     remainingQuery match {
       case Nil => Success(Set(current))
       case head :: tail =>
         def recurse(searchModules: Set[Resolved]): Result = {
-          val searchModuleClasses = moduleClasses(searchModules)
-          if (seenModules.intersect(searchModuleClasses).nonEmpty) {
-            Error(s"Cyclic module reference detected: [${seenModules.intersect(searchModuleClasses).mkString(", ")}], it's required to wrap it in ModuleRef.")
-          } else {
-            val results = searchModules.map(r => resolve(rootModule, tail, r, querySoFar ++ Seq(head), seenModules ++ moduleClasses(Set(current))))
-              .partitionMap { case s: Success => Right(s.value); case f: Failed => Left(f) }
-            val (failures, successesLists) = results
-            val (errors, notFounds) = failures.partitionMap {
-              case s: NotFound => Right(s)
-              case s: Error => Left(s.msg)
+          val results = searchModules
+            .map { r =>
+              val rClasses = moduleClasses(Set(r))
+              if (seenModules.intersect(rClasses).nonEmpty) {
+                Error(cyclicModuleErrorMsg(r.segments))
+              } else {
+                resolve(
+                  rootModule,
+                  tail,
+                  r,
+                  querySoFar ++ Seq(head),
+                  seenModules ++ moduleClasses(Set(current))
+                )
+              }
             }
+            .partitionMap { case s: Success => Right(s.value); case f: Failed => Left(f) }
 
-            if (errors.nonEmpty) Error(errors.mkString("\n"))
-            else if (successesLists.flatten.nonEmpty) Success(successesLists.flatten)
-            else notFounds.size match {
-              case 1 => notFounds.head
-              case _ => notFoundResult(rootModule, querySoFar, current, head)
-            }
+          val (failures, successesLists) = results
+          val (errors, notFounds) = failures.partitionMap {
+            case s: NotFound => Right(s)
+            case s: Error => Left(s.msg)
           }
+
+          if (errors.nonEmpty) Error(errors.mkString("\n"))
+          else if (successesLists.flatten.nonEmpty) Success(successesLists.flatten)
+          else notFounds.size match {
+            case 1 => notFounds.head
+            case _ => notFoundResult(rootModule, querySoFar, current, head)
+          }
+
         }
 
         (head, current) match {
@@ -260,19 +273,18 @@ private object ResolveCore {
     typePattern: Seq[String],
     seenModules: Set[Class[_]],
   ): Either[String, Set[Resolved]] = {
-    val errOrDirect = resolveDirectChildren(rootModule, cls, nameOpt, segments, typePattern)
-    val directTraverse = resolveDirectChildren(rootModule, cls, nameOpt, segments, Nil)
-
-    val errOrModules = directTraverse.map { modules =>
-      modules.flatMap {
-        case m: Resolved.Module => Some(m)
-        case _ => None
+    if (seenModules.contains(cls)) Left(cyclicModuleErrorMsg(segments))
+    else {
+      val errOrDirect = resolveDirectChildren(rootModule, cls, nameOpt, segments, typePattern)
+      val directTraverse = resolveDirectChildren(rootModule, cls, nameOpt, segments, Nil)
+
+      val errOrModules = directTraverse.map { modules =>
+        modules.flatMap {
+          case m: Resolved.Module => Some(m)
+          case _ => None
+        }
       }
-    }
 
-    if (seenModules.contains(cls)) {
-      Left(s"Cyclic module reference detected: ${cls.getName}, it's required to wrap it in ModuleRef.")
-    } else {
       val errOrIndirect0 = errOrModules match {
         case Right(modules) =>
           modules.flatMap { m =>
diff --git a/main/resolve/test/src/mill/main/ResolveTests.scala b/main/resolve/test/src/mill/main/ResolveTests.scala
index e15407dca86..19a5d587f4c 100644
--- a/main/resolve/test/src/mill/main/ResolveTests.scala
+++ b/main/resolve/test/src/mill/main/ResolveTests.scala
@@ -1114,7 +1114,7 @@ object ResolveTests extends TestSuite {
       val check = new Checker(TestGraphs.CyclicModuleRefInitError)
       test - check.checkSeq0(
         Seq("__"),
-        isShortError(_, "Cyclic module reference detected")
+        isShortError(_, "Cyclic module reference detected at myA.a,")
       )
       test - check(
         "_",
@@ -1122,52 +1122,52 @@ object ResolveTests extends TestSuite {
       )
       test - check.checkSeq0(
         Seq("myA.__"),
-        isShortError(_, "Cyclic module reference detected")
+        isShortError(_, "Cyclic module reference detected at myA.a,")
       )
       test - check.checkSeq0(
         Seq("myA.a.__"),
-        isShortError(_, "Cyclic module reference detected")
+        isShortError(_, "Cyclic module reference detected at myA.a.a,")
       )
       test - check.checkSeq0(
         Seq("myA.a._"),
-        isShortError(_, "Cyclic module reference detected")
+        isShortError(_, "Cyclic module reference detected at myA.a.a,")
       )
       test - check.checkSeq0(
         Seq("myA.a._.a"),
-        isShortError(_, "Cyclic module reference detected")
+        isShortError(_, "Cyclic module reference detected at myA.a.a,")
       )
       test - check.checkSeq0(
         Seq("myA.a.b.a"),
-        isShortError(_, "Cyclic module reference detected")
+        isShortError(_, "Cyclic module reference detected at myA.a.b.a,")
       )
     }
     test("cyclicModuleRefInitError2") {
       val check = new Checker(TestGraphs.CyclicModuleRefInitError2)
       test - check.checkSeq0(
         Seq("__"),
-        isShortError(_, "Cyclic module reference detected")
+        isShortError(_, "Cyclic module reference detected at A.myA.a,")
       )
     }
     test("cyclicModuleRefInitError3") {
       val check = new Checker(TestGraphs.CyclicModuleRefInitError3)
       test - check.checkSeq0(
         Seq("__"),
-        isShortError(_, "Cyclic module reference detected")
+        isShortError(_, "Cyclic module reference detected at A.b.a,")
       )
       test - check.checkSeq0(
         Seq("A.__"),
-        isShortError(_, "Cyclic module reference detected")
+        isShortError(_, "Cyclic module reference detected at A.b.a,")
       )
       test - check.checkSeq0(
         Seq("A.b.__.a.b"),
-        isShortError(_, "Cyclic module reference detected")
+        isShortError(_, "Cyclic module reference detected at A.b.a.b,")
       )
     }
     test("crossedCyclicModuleRefInitError") {
       val check = new Checker(TestGraphs.CrossedCyclicModuleRefInitError)
       test - check.checkSeq0(
         Seq("__"),
-        isShortError(_, "Cyclic module reference detected")
+        isShortError(_, "Cyclic module reference detected at cross[210].c2[210].c1,")
       )
     }
     test("nonCyclicModules") {

From 736eb1fbe935b575ed0cc0092c18cd043b44b9ce Mon Sep 17 00:00:00 2001
From: Li Haoyi <haoyi.sg@gmail.com>
Date: Fri, 1 Nov 2024 04:50:01 -0700
Subject: [PATCH 18/18] cleanup

---
 main/resolve/src/mill/resolve/Resolve.scala   |  2 +-
 .../src/mill/resolve/ResolveCore.scala        | 35 +++++++++++--------
 .../test/src/mill/main/ResolveTests.scala     |  4 +--
 3 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/main/resolve/src/mill/resolve/Resolve.scala b/main/resolve/src/mill/resolve/Resolve.scala
index d686dcfa8e6..9815d3c906c 100644
--- a/main/resolve/src/mill/resolve/Resolve.scala
+++ b/main/resolve/src/mill/resolve/Resolve.scala
@@ -73,7 +73,7 @@ object Resolve {
                 rootModule,
                 value.getClass,
                 Some(value.defaultCommandName()),
-                value.millModuleSegments,
+                value.millModuleSegments
               )
 
               directChildrenOrErr.flatMap(directChildren =>
diff --git a/main/resolve/src/mill/resolve/ResolveCore.scala b/main/resolve/src/mill/resolve/ResolveCore.scala
index 52e24ee37b3..0840e25f9ca 100644
--- a/main/resolve/src/mill/resolve/ResolveCore.scala
+++ b/main/resolve/src/mill/resolve/ResolveCore.scala
@@ -62,7 +62,7 @@ private object ResolveCore {
   def makeResultException(e: Throwable, base: Exception): Left[String, Nothing] =
     mill.api.Result.makeResultException(e, base)
 
-  def cyclicModuleErrorMsg(segments: Segments) = {
+  def cyclicModuleErrorMsg(segments: Segments): String = {
     s"Cyclic module reference detected at ${segments.render}, " +
       s"it's required to wrap it in ModuleRef."
   }
@@ -125,7 +125,7 @@ private object ResolveCore {
                     None,
                     current.segments,
                     Nil,
-                    Set.empty
+                    seenModules
                   )
 
                 transitiveOrErr.map(transitive => self ++ transitive)
@@ -135,7 +135,7 @@ private object ResolveCore {
                   rootModule,
                   m.cls,
                   None,
-                  current.segments,
+                  current.segments
                 )
 
               case pattern if pattern.startsWith("__:") =>
@@ -148,7 +148,7 @@ private object ResolveCore {
                   None,
                   current.segments,
                   typePattern,
-                  Set.empty
+                  seenModules
                 )
 
                 transitiveOrErr.map(transitive => self ++ transitive)
@@ -168,7 +168,7 @@ private object ResolveCore {
                   rootModule,
                   m.cls,
                   Some(singleLabel),
-                  current.segments,
+                  current.segments
                 )
             }
 
@@ -234,7 +234,7 @@ private object ResolveCore {
           rootModule,
           current.millModuleSegments,
           current.getClass,
-          Some(s),
+          Some(s)
         ).flatMap {
           case Seq((_, Some(f))) =>
             val res = f(current)
@@ -266,12 +266,12 @@ private object ResolveCore {
   }
 
   def resolveTransitiveChildren(
-    rootModule: BaseModule,
-    cls: Class[_],
-    nameOpt: Option[String],
-    segments: Segments,
-    typePattern: Seq[String],
-    seenModules: Set[Class[_]],
+      rootModule: BaseModule,
+      cls: Class[_],
+      nameOpt: Option[String],
+      segments: Segments,
+      typePattern: Seq[String],
+      seenModules: Set[Class[_]]
   ): Either[String, Set[Resolved]] = {
     if (seenModules.contains(cls)) Left(cyclicModuleErrorMsg(segments))
     else {
@@ -288,7 +288,14 @@ private object ResolveCore {
       val errOrIndirect0 = errOrModules match {
         case Right(modules) =>
           modules.flatMap { m =>
-            Some(resolveTransitiveChildren(rootModule, m.cls, nameOpt, m.segments, typePattern, seenModules + cls))
+            Some(resolveTransitiveChildren(
+              rootModule,
+              m.cls,
+              nameOpt,
+              m.segments,
+              typePattern,
+              seenModules + cls
+            ))
           }
         case Left(err) => Seq(Left(err))
       }
@@ -439,7 +446,7 @@ private object ResolveCore {
           rootModule,
           m.cls,
           None,
-          current.segments,
+          current.segments
         ).toOption.get.map(
           _.segments.value.last
         )
diff --git a/main/resolve/test/src/mill/main/ResolveTests.scala b/main/resolve/test/src/mill/main/ResolveTests.scala
index 19a5d587f4c..0316a74152e 100644
--- a/main/resolve/test/src/mill/main/ResolveTests.scala
+++ b/main/resolve/test/src/mill/main/ResolveTests.scala
@@ -1126,7 +1126,7 @@ object ResolveTests extends TestSuite {
       )
       test - check.checkSeq0(
         Seq("myA.a.__"),
-        isShortError(_, "Cyclic module reference detected at myA.a.a,")
+        isShortError(_, "Cyclic module reference detected at myA.a,")
       )
       test - check.checkSeq0(
         Seq("myA.a._"),
@@ -1160,7 +1160,7 @@ object ResolveTests extends TestSuite {
       )
       test - check.checkSeq0(
         Seq("A.b.__.a.b"),
-        isShortError(_, "Cyclic module reference detected at A.b.a.b,")
+        isShortError(_, "Cyclic module reference detected at A.b.a,")
       )
     }
     test("crossedCyclicModuleRefInitError") {