Include ExternalModule and ScriptModule prefixes when rendering task selectors (#6102)

Fixes #6095

We preserve the distinction of `ExternalModule`/`ScriptModule`
information in the first `Segment.Label` in `Segments`:

- If it starts with a `./`, it must be an `ScriptModule`, with the rest
of the token being the filesystem path to the script
- If it ends with a `/`, it must be an `ExternalModule`, with the rest
of the token being the JVM package path to the singleton

This PR also tweaks the `out/` folder layout from external modules,
preserving the `.`s rather than replacing them with `/`s. This should
help avoid filesystem collisions between external modules and
`build.mill` modules with similar JVM paths, at least for most external
modules which have at least one `.`

Covered by an integration test

Author: lihaoyi

core/api/daemon/src/mill/api/daemon/Segments.scala

@@ -5,8 +5,10 @@ import scala.math.Ordering.Implicits.seqOrdering
 /**
  * Models a path with the Mill build hierarchy, e.g. `amm.util[2.11].test.compile`.
  *
- * `.`-separated segments are [[Segment.Label]]s,
- * while `[]`-delimited segments are [[Segment.Cross]]s
+ * - `.`-separated segments are [[Segment.Label]]s,
+ * - `[]`-delimited segments are [[Segment.Cross]]s
+ * - If the first segment starts with `./`, it refers to a single-file script
+ * - If the first segment ends with `/`, it refers to an external module
  */
 final case class Segments private (value: Seq[Segment]) {
 
@@ -30,7 +32,7 @@ final case class Segments private (value: Seq[Segment]) {
 
   def render: String = {
     def renderCross(cross: Segment.Cross): String = "[" + cross.value.mkString(",") + "]"
-    value.toList match {
+    def renderValue(valueList: List[Segment]) = valueList match {
       case Nil => ""
       case head :: rest =>
         val headSegment = head match
@@ -42,6 +44,15 @@ final case class Segments private (value: Seq[Segment]) {
         }
         headSegment + stringSegments.mkString
     }
+
+    value.toList match {
+      // ScriptModule segments always starts with `./`
+      case Segment.Label(s"./$first") :: Nil => s"./$first"
+      case Segment.Label(s"./$first") :: next :: rest => s"./$first:${renderValue(next :: rest)}"
+      // ExternalModule segments always ends with '/'
+      case Segment.Label(s"$first/") :: next :: rest => s"$first/${renderValue(next :: rest)}"
+      case valueList => renderValue(valueList)
+    }
   }
   override lazy val hashCode: Int = value.hashCode()
 }

core/api/src/mill/api/Evaluator.scala

@@ -48,12 +48,6 @@ trait Evaluator extends AutoCloseable with EvaluatorApi {
       allowPositionalCommandArgs: Boolean = false,
       resolveToModuleTasks: Boolean = false
   ): mill.api.Result[List[Resolved]] = {
-    // These are used in the overrides.
-    val _ = scriptArgs
-    val _ = selectMode
-    val _ = allowPositionalCommandArgs
-    val _ = resolveToModuleTasks
-
     mill.api.Result.Success(Nil)
   }
 

core/api/src/mill/api/ExecutionPaths.scala

@@ -35,7 +35,6 @@ object ExecutionPaths {
   // Dollar sign `$` is our masking-character
   private val Dollar = "[$]".r
   // Forward-slashed are reserved for directory delimiters
-  private val Slash = "/".r
 
   private val steps: Seq[String => String] = Seq(
     // Step 1: mask all existing dollar signs, so we can use the dollar as masking character
@@ -46,13 +45,11 @@ object ExecutionPaths {
       case s => s
     },
     // Step 3: Replace colon (:) with $colon
-    s => Colon.replaceAllIn(s, Matcher.quoteReplacement("$colon")),
-    // Step 4: Replace slash (/) with $slash
-    s => Slash.replaceAllIn(s, Matcher.quoteReplacement("$slash"))
+    s => Colon.replaceAllIn(s, Matcher.quoteReplacement("$colon"))
   )
 
   def sanitizePathSegment(segment: String): os.PathChunk = {
     // sanitize and implicitly convert to PathChunk
-    steps.foldLeft(segment) { (segment, f) => f(segment) }
+    os.SubPath(steps.foldLeft(segment) { (segment, f) => f(segment) })
   }
 }

core/api/src/mill/api/ExternalModule.scala

@@ -27,9 +27,10 @@ abstract class ExternalModule(using
     !" #".exists(millModuleEnclosing0.value.contains(_)),
     "External modules must be at a top-level static path, not " + millModuleEnclosing0.value
   )
-  override def moduleSegments: Segments = {
-    Segments(millModuleEnclosing0.value.split('.').map(Segment.Label(_)).toIndexedSeq)
-  }
+
+  // Include a trailing `/` in the external module first segment to distinguish it
+  // from script modules or normal build.mill modules
+  override def moduleSegments: Segments = Segments.labels(millModuleEnclosing0.value + "/")
 }
 
 object ExternalModule {

core/api/src/mill/api/internal/Resolved.scala

@@ -3,8 +3,10 @@ package mill.api.internal
 import mill.api.daemon.Segments
 
 private[mill] sealed trait Resolved {
+  def rootModule: RootModule0
   def segments: Segments
   def cls: Class[?]
+  def fullSegments: Segments = rootModule.moduleSegments ++ segments
 }
 
 private[mill] object Resolved {
@@ -23,7 +25,7 @@ private[mill] object Resolved {
     }
   }
 
-  case class Module(segments: Segments, cls: Class[?]) extends Resolved
-  case class Command(segments: Segments, cls: Class[?]) extends Resolved
-  case class NamedTask(segments: Segments, cls: Class[?]) extends Resolved
+  case class Module(rootModule: RootModule0, segments: Segments, cls: Class[?]) extends Resolved
+  case class Command(rootModule: RootModule0, segments: Segments, cls: Class[?]) extends Resolved
+  case class NamedTask(rootModule: RootModule0, segments: Segments, cls: Class[?]) extends Resolved
 }

core/api/test/src/mill/api/ExecutionPathsTests.scala

@@ -16,9 +16,7 @@ object ExecutionPathsTests extends TestSuite {
           // do not collide with the applied `$`-masking character
           "a$colonb" -> "a$$colonb",
           // replace not just the first $
-          "a$$b" -> "a$$$$b",
-          // replace a forward slash,
-          "a/b" -> "a$slashb"
+          "a$$b" -> "a$$$$b"
         )
         val noReplace = Seq(
           "con10.json"

core/exec/test/src/mill/exec/ModuleTests.scala

@@ -55,10 +55,10 @@ object ModuleTests extends TestSuite {
           zresult == Right(Result(30, 1)),
           os.read(check.execution.outPath / "z.json").contains("30"),
           os.read(
-            check.outPath / "mill/exec/TestExternalModule/x.json"
+            check.outPath / "mill.exec.TestExternalModule/x.json"
           ).contains("13"),
           os.read(
-            check.outPath / "mill/exec/TestExternalModule/inner/y.json"
+            check.outPath / "mill.exec.TestExternalModule/inner/y.json"
           ).contains("17")
         )
       }

core/resolve/src/mill/resolve/Resolve.scala

@@ -27,7 +27,7 @@ private[mill] object Resolve {
         resolveToModuleTasks: Boolean,
         cache: ResolveCore.Cache
     ) = {
-      Result.Success(resolved.map(_.segments))
+      Result.Success(resolved.map(_.fullSegments))
     }
 
     private[mill] override def deduplicate(items: List[Segments]): List[Segments] = items.distinct
@@ -400,7 +400,7 @@ private[mill] trait Resolve[T] {
       sel: Segments,
       cache: ResolveCore.Cache
   ): ResolveCore.Result = {
-    val rootResolved = Resolved.Module(Segments(), rootModule.getClass)
+    val rootResolved = Resolved.Module(rootModule, Segments(), rootModule.getClass)
     ResolveCore.resolve(
       rootModule = rootModule,
       remainingQuery = sel.value.toList,

core/resolve/src/mill/resolve/ResolveCore.scala

@@ -91,7 +91,7 @@ private object ResolveCore {
   ): 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 {
@@ -139,7 +139,7 @@ private object ResolveCore {
           case (Segment.Label(singleLabel), m: Resolved.Module) =>
             val resOrErr: mill.api.Result[Seq[Resolved]] = singleLabel match {
               case "__" =>
-                val self = Seq(Resolved.Module(m.segments, m.cls))
+                val self = Seq(Resolved.Module(rootModule, m.segments, m.cls))
                 val transitiveOrErr =
                   resolveTransitiveChildren(
                     rootModule,
@@ -163,7 +163,7 @@ private object ResolveCore {
 
               case pattern if pattern.startsWith("__:") =>
                 val typePattern = pattern.split(":").drop(1)
-                val self = Seq(Resolved.Module(m.segments, m.cls))
+                val self = Seq(Resolved.Module(rootModule, m.segments, m.cls))
 
                 val transitiveOrErr = resolveTransitiveChildren(
                   rootModule,
@@ -176,7 +176,7 @@ private object ResolveCore {
 
                 transitiveOrErr.map(transitive =>
                   (self ++ transitive).collect {
-                    case r @ Resolved.Module(segments, cls)
+                    case r @ Resolved.Module(_, segments, cls)
                         if classMatchesTypePred(typePattern)(cls) =>
                       r
                   }
@@ -192,7 +192,7 @@ private object ResolveCore {
                   cache
                 ).map {
                   _.collect {
-                    case r @ Resolved.Module(segments, cls)
+                    case r @ Resolved.Module(_, segments, cls)
                         if classMatchesTypePred(typePattern)(cls) => r
                   }
                 }
@@ -237,7 +237,7 @@ private object ResolveCore {
                 case mill.api.Result.Success(searchModules) =>
                   recurse(
                     searchModules
-                      .map(m => Resolved.Module(m.moduleSegments, m.getClass))
+                      .map(m => Resolved.Module(rootModule, m.moduleSegments, m.getClass))
                   )
               }
 
@@ -376,7 +376,7 @@ private object ResolveCore {
       instantiateModule(rootModule, segments, cache).map {
         case cross: Cross[_] =>
           for (item <- cross.items) yield {
-            Resolved.Module(segments ++ Segment.Cross(item.crossSegments), item.cls)
+            Resolved.Module(rootModule, segments ++ Segment.Cross(item.crossSegments), item.cls)
           }
 
         case _ => Nil
@@ -385,9 +385,12 @@ private object ResolveCore {
 
     def expandSegments(direct: Seq[(Resolved, Option[Module => mill.api.Result[Module]])]) = {
       direct.map {
-        case (Resolved.Module(s, cls), _) => Resolved.Module(segments ++ s, cls)
-        case (Resolved.NamedTask(s, enclosing), _) => Resolved.NamedTask(segments ++ s, enclosing)
-        case (Resolved.Command(s, enclosing), _) => Resolved.Command(segments ++ s, enclosing)
+        case (Resolved.Module(rootModule, s, cls), _) =>
+          Resolved.Module(rootModule, segments ++ s, cls)
+        case (Resolved.NamedTask(rootModule, s, enclosing), _) =>
+          Resolved.NamedTask(rootModule, segments ++ s, enclosing)
+        case (Resolved.Command(rootModule, s, enclosing), _) =>
+          Resolved.Command(rootModule, segments ++ s, enclosing)
       }
     }
 
@@ -417,6 +420,7 @@ private object ResolveCore {
               .map(c =>
                 (
                   Resolved.Module(
+                    rootModule,
                     Segments.labels(c.moduleSegments.last.value),
                     c.getClass
                   ),
@@ -429,7 +433,8 @@ private object ResolveCore {
           .reflectNestedObjects02[Module](cls, namePred, cache.getMethods)
           .collect {
             case (name, memberCls, getter) =>
-              val resolved = Resolved.Module(Segments.labels(cache.decode(name)), memberCls)
+              val resolved =
+                Resolved.Module(rootModule, Segments.labels(cache.decode(name)), memberCls)
               val getter2 =
                 Some((mod: Module) => mill.api.ExecResult.catchWrapException(getter(mod)))
               (resolved, getter2)
@@ -441,14 +446,14 @@ private object ResolveCore {
     val namedTasks = Reflect
       .reflect(cls, classOf[Task.Named[?]], namePred, noParams = true, cache.getMethods)
       .map { m =>
-        Resolved.NamedTask(Segments.labels(cache.decode(m.getName)), cls) ->
+        Resolved.NamedTask(rootModule, Segments.labels(cache.decode(m.getName)), cls) ->
           None
       }
 
     val commands = Reflect
       .reflect(cls, classOf[Task.Command[?]], namePred, noParams = false, cache.getMethods)
       .map(m => cache.decode(m.getName))
-      .map { name => Resolved.Command(Segments.labels(name), cls) -> None }
+      .map { name => Resolved.Command(rootModule, Segments.labels(name), cls) -> None }
 
     modulesOrErr.map(_ ++ namedTasks ++ commands)
   }

example/fundamentals/out-dir/1-out-files/build.mill

@@ -125,7 +125,7 @@ out/mill-daemon
 > cat out/mill-profile.json
 [
   {
-    "label": "mill.javalib.JvmWorkerModule.internalWorker",
+    "label": "mill.javalib.JvmWorkerModule/internalWorker",
     "millis": 0,
     "cached": true,
     "valueHashChanged": false,