Cycle detection in moduleDeps and compileModuleDeps (#2790)

The problem: We currently don't detect cycles in transitive `moduleDeps`
and `compileModuleDeps`. Cycles in these result in a
`StackOverflowError`.

To solve this, we first need to detect cycles. This is the newly added
`ModuleUtils.recursive` method. Instead of `moduleDeps` we use the
`moduleDepsChecked` in some places to ensure we checked for cycles
before accessing module dependencies.

Unfortunately, we can't easily report the detected cycle at evaluation
time, as `moduleDeps` needs to be evaluated before we can create a task
graph (via `T.traverse`), hence we need an alternative way to propagate
the exception. The current approach throws an `BuildScriptException`
which itself derives from `mill.api.MillException`.

In `MillMain` as well as `MillClientMain`, our central entry points to
Mill, we catch the `MillException` and report the error to the user.

Pull request: #2790

Author: lefou

contrib/bloop/src/mill/contrib/bloop/BloopImpl.scala

@@ -201,7 +201,7 @@ class BloopImpl(ev: () => Evaluator, wd: os.Path) extends ExternalModule { outer
     // //////////////////////////////////////////////////////////////////////////
 
     val classpath = T.task {
-      val depModules = (module.compileModuleDeps ++ module.recursiveModuleDeps).distinct
+      val depModules = (module.compileModuleDepsChecked ++ module.recursiveModuleDeps).distinct
       // dep modules ++ ivy deps ++ unmanaged
       depModules.map(classes) ++
         module.resolvedIvyDeps().map(_.path) ++
@@ -408,7 +408,8 @@ class BloopImpl(ev: () => Evaluator, wd: os.Path) extends ExternalModule { outer
         sources = mSources,
         sourcesGlobs = None,
         sourceRoots = None,
-        dependencies = (module.moduleDeps ++ module.compileModuleDeps).map(name).toList,
+        dependencies =
+          (module.moduleDepsChecked ++ module.compileModuleDepsChecked).map(name).toList,
         classpath = classpath().map(_.toNIO).toList,
         out = out(module).toNIO,
         classesDir = classes(module).toNIO,

main/api/src/mill/api/MillException.scala

@@ -5,3 +5,6 @@ package mill.api
  * @param msg The error message, to be displayed to the user.
  */
 class MillException(msg: String) extends Exception(msg)
+
+class BuildScriptException(msg: String)
+    extends MillException("Build script contains errors:\n" + msg)

main/define/src/mill/define/Task.scala

@@ -354,7 +354,7 @@ object Target extends Applicative.Applyer[Task, Task, Result, mill.api.Ctx] {
 
       val taskIsPrivate = isPrivateTargetOption(c)
 
-      val lhs = Applicative.impl0[Task, T, mill.api.Ctx](c)(reify(Result.Success(t.splice)).tree)
+      val lhs = Applicative.impl0[Task, T, mill.api.Ctx](c)(reify(Result.create(t.splice)).tree)
 
       mill.moduledefs.Cacher.impl0[Target[T]](c)(
         reify(

scalalib/src/mill/scalalib/GenIdeaImpl.scala

@@ -554,8 +554,8 @@ case class GenIdeaImpl(
         val libNames = Strict.Agg.from(sanizedDeps).iterator.toSeq
 
         val depNames = Strict.Agg
-          .from(mod.moduleDeps.map((_, None)) ++
-            mod.compileModuleDeps.map((_, Some("PROVIDED"))))
+          .from(mod.moduleDepsChecked.map((_, None)) ++
+            mod.compileModuleDepsChecked.map((_, Some("PROVIDED"))))
           .filter(!_._1.skipIdea)
           .map { case (v, s) => ScopedOrd(moduleName(moduleLabels(v)), s) }
           .iterator

scalalib/src/mill/scalalib/JavaModule.scala

@@ -9,13 +9,14 @@ import coursier.parse.JavaOrScalaModule
 import coursier.parse.ModuleParser
 import coursier.util.ModuleMatcher
 import mainargs.Flag
-import mill.api.Loose.Agg
-import mill.define.ModuleRef
+import mill.Agg
 import mill.api.{JarManifest, PathRef, Result, internal}
-import mill.util.Jvm
+import mill.define.{Command, ModuleRef, Segment, Task, TaskModule}
+import mill.scalalib.internal.ModuleUtils
 import mill.scalalib.api.CompilationResult
 import mill.scalalib.bsp.{BspBuildTarget, BspModule}
 import mill.scalalib.publish.Artifact
+import mill.util.Jvm
 import os.Path
 
 /**
@@ -122,15 +123,49 @@ trait JavaModule
    */
   def javacOptions: T[Seq[String]] = T { Seq.empty[String] }
 
-  /** The direct dependencies of this module */
+  /**
+   *  The direct dependencies of this module.
+   *  @see [[moduleDepschecked]]
+   */
   def moduleDeps: Seq[JavaModule] = Seq.empty
 
+  /** Same as [[moduleDeps]] but checked to not contain cycles. */
+  final def moduleDepsChecked: Seq[JavaModule] = {
+    // trigger initialization to check for cycles
+    recModuleDeps
+    moduleDeps
+  }
+
+  /** Should only be called from [[moduleDepsChecked]] */
+  private lazy val recModuleDeps: Seq[JavaModule] =
+    ModuleUtils.recursive[JavaModule](
+      (millModuleSegments ++ Seq(Segment.Label("moduleDeps"))).render,
+      this,
+      _.moduleDeps
+    )
+
   /** The compile-only direct dependencies of this module. */
   def compileModuleDeps: Seq[JavaModule] = Seq.empty
 
+  /** Same as [[compileModuleDeps]] but checked to not contain cycles. */
+  final def compileModuleDepsChecked: Seq[JavaModule] = {
+    // trigger initialization to check for cycles
+    recCompileModuleDeps
+    compileModuleDeps
+  }
+
+  /** Should only be called from [[compileModuleDeps]] */
+  private lazy val recCompileModuleDeps: Seq[JavaModule] =
+    ModuleUtils.recursive[JavaModule](
+      (millModuleSegments ++ Seq(Segment.Label("compileModuleDeps"))).render,
+      this,
+      _.compileModuleDeps
+    )
+
   /** The direct and indirect dependencies of this module */
   def recursiveModuleDeps: Seq[JavaModule] = {
-    moduleDeps.flatMap(_.transitiveModuleDeps).distinct
+//    moduleDeps.flatMap(_.transitiveModuleDeps).distinct
+    recModuleDeps
   }
 
   /**
@@ -148,32 +183,32 @@ trait JavaModule
    * look at the direct `compileModuleDeps` when assembling this list
    */
   def transitiveModuleCompileModuleDeps: Seq[JavaModule] = {
-    (moduleDeps ++ compileModuleDeps).flatMap(_.transitiveModuleDeps).distinct
+    (moduleDepsChecked ++ compileModuleDepsChecked).flatMap(_.transitiveModuleDeps).distinct
   }
 
   /** The compile-only transitive ivy dependencies of this module and all it's upstream compile-only modules. */
   def transitiveCompileIvyDeps: T[Agg[BoundDep]] = T {
     // We never include compile-only dependencies transitively, but we must include normal transitive dependencies!
     compileIvyDeps().map(bindDependency()) ++
-      T.traverse(compileModuleDeps)(_.transitiveIvyDeps)().flatten
+      T.traverse(compileModuleDepsChecked)(_.transitiveIvyDeps)().flatten
   }
 
   /**
    * Show the module dependencies.
    * @param recursive If `true` include all recursive module dependencies, else only show direct dependencies.
    */
   def showModuleDeps(recursive: Boolean = false): Command[Unit] = T.command {
-    val normalDeps = if (recursive) recursiveModuleDeps else moduleDeps
+    val normalDeps = if (recursive) recursiveModuleDeps else moduleDepsChecked
     val compileDeps =
-      if (recursive) compileModuleDeps.flatMap(_.transitiveModuleDeps).distinct
-      else compileModuleDeps
+      if (recursive) compileModuleDepsChecked.flatMap(_.transitiveModuleDeps).distinct
+      else compileModuleDepsChecked
     val deps = (normalDeps ++ compileDeps).distinct
     val asString =
       s"${if (recursive) "Recursive module"
         else "Module"} dependencies of ${millModuleSegments.render}:\n\t${deps
           .map { dep =>
             dep.millModuleSegments.render ++
-              (if (compileModuleDeps.contains(dep) || !normalDeps.contains(dep)) " (compile)"
+              (if (compileModuleDepsChecked.contains(dep) || !normalDeps.contains(dep)) " (compile)"
                else "")
           }
           .mkString("\n\t")}"
@@ -193,7 +228,7 @@ trait JavaModule
    */
   def transitiveIvyDeps: T[Agg[BoundDep]] = T {
     (ivyDeps() ++ mandatoryIvyDeps()).map(bindDependency()) ++
-      T.traverse(moduleDeps)(_.transitiveIvyDeps)().flatten
+      T.traverse(moduleDepsChecked)(_.transitiveIvyDeps)().flatten
   }
 
   /**

scalalib/src/mill/scalalib/PublishModule.scala

@@ -58,8 +58,10 @@ trait PublishModule extends JavaModule { outer =>
       .filter(!ivyPomDeps.contains(_))
       .map(_.copy(scope = Scope.Provided))
 
-    val modulePomDeps = T.sequence(moduleDeps.map(_.publishSelfDependency))()
-    val compileModulePomDeps = T.sequence(compileModuleDeps.collect {
+    val modulePomDeps = T.sequence(moduleDepsChecked.collect {
+      case m: PublishModule => m.publishSelfDependency
+    })()
+    val compileModulePomDeps = T.sequence(compileModuleDepsChecked.collect {
       case m: PublishModule => m.publishSelfDependency
     })()
 

scalalib/src/mill/scalalib/internal/JavaModuleUtils.scala

@@ -15,7 +15,7 @@ object JavaModuleUtils {
         found
       else {
         val subMods = mod.millModuleDirectChildren ++ (mod match {
-          case jm: JavaModule => jm.moduleDeps ++ jm.compileModuleDeps
+          case jm: JavaModule => jm.moduleDepsChecked ++ jm.compileModuleDepsChecked
           case other => Seq.empty
         })
         subMods.foldLeft(found ++ Seq(mod)) { (all, mod) => loop(mod, all) }

scalalib/src/mill/scalalib/internal/ModuleUtils.scala

@@ -1,7 +1,10 @@
 package mill.scalalib.internal
 
+import mill.api.{BuildScriptException, experimental}
 import mill.define.{Module, Segments}
 
+import scala.annotation.tailrec
+
 @mill.api.internal
 object ModuleUtils {
 
@@ -11,4 +14,38 @@ object ModuleUtils {
   def moduleDisplayName(module: Module): String = {
     (module.millModuleShared.value.getOrElse(Segments()) ++ module.millModuleSegments).render
   }
+
+  def recursive[T <: Module](name: String, start: T, deps: T => Seq[T]): Seq[T] = {
+
+    @tailrec def rec(
+        seenModules: List[T],
+        toAnalyze: List[(List[T], List[T])]
+    ): List[T] = {
+      toAnalyze match {
+        case Nil => seenModules
+        case traces :: rest =>
+          traces match {
+            case (_, Nil) => rec(seenModules, rest)
+            case (trace, cand :: remaining) =>
+              if (trace.contains(cand)) {
+                // cycle!
+                val rendered =
+                  (cand :: (cand :: trace.takeWhile(_ != cand)).reverse).mkString(" -> ")
+                val msg = s"${name}: cycle detected: ${rendered}"
+                println(msg)
+                throw new BuildScriptException(msg)
+              }
+              rec(
+                seenModules ++ Seq(cand),
+                toAnalyze = ((cand :: trace, deps(cand).toList)) :: (trace, remaining) :: rest
+              )
+          }
+      }
+    }
+
+    rec(
+      seenModules = List(),
+      toAnalyze = List((List(start), deps(start).toList))
+    ).reverse
+  }
 }

scalalib/test/src/mill/scalalib/CycleTests.scala

@@ -0,0 +1,65 @@
+package mill.scalalib
+
+import mill.api.BuildScriptException
+import mill.util.{TestEvaluator, TestUtil}
+import utest.framework.TestPath
+import utest.{TestSuite, Tests, compileError, intercept, test, assert}
+
+object CycleTests extends TestSuite {
+
+  object CycleBase extends TestUtil.BaseModule {
+    // See issue: https://github.com/com-lihaoyi/mill/issues/2341
+    object a extends ScalaModule {
+      override def moduleDeps = Seq(a)
+      override def scalaVersion = sys.props.getOrElse("TEST_SCALA_VERSION", ???)
+    }
+    object b extends JavaModule {
+      override def moduleDeps = Seq(c)
+      object c extends JavaModule {
+        override def moduleDeps = Seq(d)
+      }
+      object d extends JavaModule {
+        override def moduleDeps = Seq(b)
+      }
+    }
+    object e extends JavaModule {
+      override def moduleDeps = Seq(b)
+    }
+    object f extends JavaModule {
+      override def compileModuleDeps = Seq(f)
+    }
+  }
+
+  def workspaceTest[T](m: TestUtil.BaseModule)(t: TestEvaluator => T)(implicit tp: TestPath): T = {
+    val eval = new TestEvaluator(m)
+    os.remove.all(m.millSourcePath)
+    os.remove.all(eval.outPath)
+    os.makeDir.all(m.millSourcePath / os.up)
+    t(eval)
+  }
+
+  override def tests: Tests = Tests {
+    test("moduleDeps") {
+      test("self-reference") - workspaceTest(CycleBase) { eval =>
+        val ex = intercept[BuildScriptException] {
+          eval.apply(CycleBase.a.compile)
+        }
+        assert(ex.getMessage.contains("a.moduleDeps: cycle detected: a -> a"))
+      }
+      test("cycle-in-deps") - workspaceTest(CycleBase) { eval =>
+        val ex = intercept[BuildScriptException] {
+          eval.apply(CycleBase.e.compile)
+        }
+        assert(ex.getMessage.contains("e.moduleDeps: cycle detected: b -> b.c -> b.d -> b"))
+      }
+    }
+    test("compileModuleDeps") {
+      test("self-reference") - workspaceTest(CycleBase) { eval =>
+        val ex = intercept[BuildScriptException] {
+          eval.apply(CycleBase.f.compile)
+        }
+        assert(ex.getMessage.contains("f.compileModuleDeps: cycle detected: f -> f"))
+      }
+    }
+  }
+}

scalalib/test/src/mill/scalalib/bsp/BspModuleTests.scala

@@ -31,7 +31,7 @@ object BspModuleTests extends TestSuite {
   }
 
   object InterDeps extends BspBase {
-    val maxCrossCount = 25
+    val maxCrossCount = 15
     val configs = 1.to(maxCrossCount)
     object Mod extends Cross[ModCross](configs)
     trait ModCross extends ScalaModule with Cross.Module[Int] {
@@ -105,7 +105,7 @@ object BspModuleTests extends TestSuite {
         }
         test("interdependencies are fast") {
           test("reference (no BSP)") {
-            def runNoBsp(entry: Int, maxTime: Int) = workspaceTest(MultiBase) { eval =>
+            def runNoBsp(entry: Int, maxTime: Int) = workspaceTest(InterDeps) { eval =>
               val start = System.currentTimeMillis()
               val Right((result, evalCount)) = eval.apply(
                 InterDeps.Mod(entry).compileClasspath
@@ -116,11 +116,10 @@ object BspModuleTests extends TestSuite {
             }
             test("index 1 (no deps)") { runNoBsp(1, 5000) }
             test("index 10") { runNoBsp(10, 30000) }
-            test("index 20") { runNoBsp(20, 30000) }
-            test("index 25") { runNoBsp(25, 100000) }
+            test("index 15") { runNoBsp(15, 30000) }
           }
           def run(entry: Int, maxTime: Int) = retry(3) {
-            workspaceTest(MultiBase) { eval =>
+            workspaceTest(InterDeps) { eval =>
               val start = System.currentTimeMillis()
               val Right((result, evalCount)) = eval.apply(
                 InterDeps.Mod(entry).bspCompileClasspath
@@ -132,8 +131,7 @@ object BspModuleTests extends TestSuite {
           }
           test("index 1 (no deps)") { run(1, 500) }
           test("index 10") { run(10, 5000) }
-          test("index 20") { run(20, 15000) }
-          test("index 25") { run(25, 50000) }
+          test("index 15") { run(15, 15000) }
         }
       }
     }