Improve overriden target detection to let it handle stackable-traits-…

…style overrides (#1600)

Fixes #554

This PR swaps out our detection of overriden targets and commands: 

- Previously, we tried to count how many overriden methods there are upstream of a particular target, statically at its definition site. Any target that has the max number of overriden methods is considered the "final" one, while others are considered overriden. This fails for stackable traits, e.g. the example in the test suite below, as multiple traits can have the same number of static definiton-site overrides but when stacked together on a module one of them ends up winning

- Now, we simply do a reverse walk of the targets list after they have been topo sorted. Since the directionality of overrides is exactly opposite that of the topo sort, we can assume that the first time we see a target of a particular label in the reverse walk it's the "final" one, while any subsequent targets we see with the same label must be the overriden versions

The new mechanism is simpler, and possibly more correct. Hopefully CI would catch any issues...

Added a test case that fails on master and passes on this PR. Also verified manually that the issue reported above can be reproduced on master, and does not reproduce (i.e. it behaves correctly) on this PR.

ci/mill-bootstrap.patch

@@ -0,0 +1,142 @@
+diff --git a/build.sc b/build.sc
+index cf9b2036..335e5ec0 100755
+--- a/build.sc
++++ b/build.sc
+@@ -1,16 +1,16 @@
+ import $file.ci.shared
+ import $file.ci.upload
+ import $ivy.`org.scalaj::scalaj-http:2.4.2`
+-import $ivy.`de.tototec::de.tobiasroeser.mill.vcs.version::0.1.2-4-dcde72`
+-import $ivy.`com.github.lolgab::mill-mima::0.0.6`
++//import $ivy.`de.tototec::de.tobiasroeser.mill.vcs.version::0.1.2-4-dcde72`
++//import $ivy.`com.github.lolgab::mill-mima::0.0.6`
+ import $ivy.`net.sourceforge.htmlcleaner:htmlcleaner:2.24`
+ import java.nio.file.attribute.PosixFilePermission
+
+-import com.github.lolgab.mill.mima
+-import com.github.lolgab.mill.mima.ProblemFilter
+-import com.typesafe.tools.mima.core.{DirectMissingMethodProblem, IncompatibleSignatureProblem}
++//import com.github.lolgab.mill.mima
++//import com.github.lolgab.mill.mima.ProblemFilter
++//import com.typesafe.tools.mima.core.{DirectMissingMethodProblem, IncompatibleSignatureProblem}
+ import coursier.maven.MavenRepository
+-import de.tobiasroeser.mill.vcs.version.VcsVersion
++//import de.tobiasroeser.mill.vcs.version.VcsVersion
+ import mill._
+ import mill.define.{Command, Source, Sources, Target, Task}
+ import mill.eval.Evaluator
+@@ -127,12 +127,8 @@ object Deps {
+   val jarjarabrams = ivy"com.eed3si9n.jarjarabrams::jarjar-abrams-core:1.8.0"
+ }
+
+-def millVersion: T[String] = T { VcsVersion.vcsState().format() }
+-def millLastTag: T[String] = T {
+-  VcsVersion.vcsState().lastTag.getOrElse(
+-    sys.error("No (last) git tag found. Your git history seems incomplete!")
+-  )
+-}
++def millVersion: T[String] = T { "0.0.0.test" }
++def millLastTag: T[String] = T { "0.0.0.test" }
+ def millBinPlatform: T[String] = T {
+   val tag = millLastTag()
+   if (tag.contains("-M")) tag
+@@ -170,44 +166,44 @@ trait MillCoursierModule extends CoursierModule {
+   }
+ }
+
+-trait MillMimaConfig extends mima.Mima {
+-  override def mimaPreviousVersions: T[Seq[String]] = Settings.mimaBaseVersions
+-  override def mimaPreviousArtifacts =
+-    if (Settings.mimaBaseVersions.isEmpty) T { Agg[Dep]() }
+-    else super.mimaPreviousArtifacts
+-  override def mimaExcludeAnnotations: T[Seq[String]] = Seq(
+-    "mill.api.internal",
+-    "mill.api.experimental"
+-  )
+-  override def mimaBinaryIssueFilters: Target[Seq[ProblemFilter]] = T {
+-    issueFilterByModule.getOrElse(this, Seq())
+-  }
+-  lazy val issueFilterByModule: Map[MillMimaConfig, Seq[ProblemFilter]] = Map(
+-    main.api -> Seq(
+-      ProblemFilter.exclude[IncompatibleSignatureProblem]("mill.api.Ctx.args"),
+-      ProblemFilter.exclude[IncompatibleSignatureProblem]("mill.api.Ctx.this"),
+-      ProblemFilter.exclude[IncompatibleSignatureProblem]("mill.api.Ctx#Args.args")
+-    ),
+-    main.core -> Seq(
+-      ProblemFilter.exclude[IncompatibleSignatureProblem]("mill.define.Target.makeT"),
+-      ProblemFilter.exclude[IncompatibleSignatureProblem]("mill.define.Target.args"),
+-      ProblemFilter.exclude[IncompatibleSignatureProblem]("mill.define.ParseArgs.standaloneIdent"),
+-      ProblemFilter.exclude[IncompatibleSignatureProblem](
+-        "mill.define.ParseArgs#BraceExpansionParser.plainChars"
+-      ),
+-      ProblemFilter.exclude[IncompatibleSignatureProblem](
+-        "mill.define.ParseArgs#BraceExpansionParser.braceParser"
+-      ),
+-      ProblemFilter.exclude[IncompatibleSignatureProblem](
+-        "mill.define.ParseArgs#BraceExpansionParser.parser"
+-      ),
+-      ProblemFilter.exclude[IncompatibleSignatureProblem](
+-        "mill.define.ParseArgs#BraceExpansionParser.toExpand"
+-      ),
+-      ProblemFilter.exclude[IncompatibleSignatureProblem]("mill.eval.EvaluatorPaths.*"),
+-      ProblemFilter.exclude[DirectMissingMethodProblem]("mill.eval.EvaluatorPaths.*")
+-    )
+-  )
++trait MillMimaConfig {
++//  override def mimaPreviousVersions: T[Seq[String]] = Settings.mimaBaseVersions
++//  override def mimaPreviousArtifacts =
++//    if (Settings.mimaBaseVersions.isEmpty) T { Agg[Dep]() }
++//    else super.mimaPreviousArtifacts
++//  override def mimaExcludeAnnotations: T[Seq[String]] = Seq(
++//    "mill.api.internal",
++//    "mill.api.experimental"
++//  )
++//  def mimaBinaryIssueFilters: Target[Seq[ProblemFilter]] = T {
++//    issueFilterByModule.getOrElse(this, Seq())
++//  }
++//  lazy val issueFilterByModule: Map[MillMimaConfig, Seq[ProblemFilter]] = Map(
++//    main.api -> Seq(
++//      ProblemFilter.exclude[IncompatibleSignatureProblem]("mill.api.Ctx.args"),
++//      ProblemFilter.exclude[IncompatibleSignatureProblem]("mill.api.Ctx.this"),
++//      ProblemFilter.exclude[IncompatibleSignatureProblem]("mill.api.Ctx#Args.args")
++//    ),
++//    main.core -> Seq(
++//      ProblemFilter.exclude[IncompatibleSignatureProblem]("mill.define.Target.makeT"),
++//      ProblemFilter.exclude[IncompatibleSignatureProblem]("mill.define.Target.args"),
++//      ProblemFilter.exclude[IncompatibleSignatureProblem]("mill.define.ParseArgs.standaloneIdent"),
++//      ProblemFilter.exclude[IncompatibleSignatureProblem](
++//        "mill.define.ParseArgs#BraceExpansionParser.plainChars"
++//      ),
++//      ProblemFilter.exclude[IncompatibleSignatureProblem](
++//        "mill.define.ParseArgs#BraceExpansionParser.braceParser"
++//      ),
++//      ProblemFilter.exclude[IncompatibleSignatureProblem](
++//        "mill.define.ParseArgs#BraceExpansionParser.parser"
++//      ),
++//      ProblemFilter.exclude[IncompatibleSignatureProblem](
++//        "mill.define.ParseArgs#BraceExpansionParser.toExpand"
++//      ),
++//      ProblemFilter.exclude[IncompatibleSignatureProblem]("mill.eval.EvaluatorPaths.*"),
++//      ProblemFilter.exclude[DirectMissingMethodProblem]("mill.eval.EvaluatorPaths.*")
++//    )
++//  )
+ }
+
+ trait MillApiModule
+@@ -1255,12 +1251,11 @@ def launcher = T {
+ }
+
+ def uploadToGithub(authKey: String) = T.command {
+-  val vcsState = VcsVersion.vcsState()
+-  val label = vcsState.format()
++//  val vcsState = VcsVersion.vcsState()
++//  val label = vcsState.format()
++  val label = millVersion()
+   if (label != millVersion()) sys.error("Modified mill version detected, aborting upload")
+-  val releaseTag = vcsState.lastTag.getOrElse(sys.error(
+-    "Incomplete git history. No tag found.\nIf on CI, make sure your git checkout job includes enough history."
+-  ))
++  val releaseTag = millLastTag()
+
+   if (releaseTag == label) {
+     scalaj.http.Http(

main/core/src/mill/define/BaseModule.scala

@@ -21,7 +21,6 @@ abstract class BaseModule(
         implicitly,
         BasePath(millSourcePath0),
         Segments(),
-        mill.define.Overrides(0),
         Ctx.External(external0),
         Ctx.Foreign(foreign0),
         millFile0,

main/core/src/mill/define/Ctx.scala

@@ -64,7 +64,6 @@ case class Ctx(
     segment: Segment,
     millSourcePath: os.Path,
     segments: Segments,
-    overrides: Int,
     external: Boolean,
     foreign: Option[Segments],
     fileName: String,
@@ -81,7 +80,6 @@ object Ctx {
       millName0: sourcecode.Name,
       millModuleBasePath0: BasePath,
       segments0: Segments,
-      overrides0: mill.define.Overrides,
       external0: External,
       foreign0: Foreign,
       fileName: sourcecode.File,
@@ -93,7 +91,6 @@ object Ctx {
       Segment.Label(millName0.value),
       millModuleBasePath0.value,
       segments0,
-      overrides0.value,
       external0.value,
       foreign0.value,
       fileName.value,

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

@@ -209,23 +209,20 @@ object Target extends TargetGenerated with Applicative.Applyer[Task, Task, Resul
       ctx: mill.define.Ctx,
       w: W[T],
       cls: EnclosingClass,
-      overrides: Overrides
   ): Command[T] = {
-    new Command(t, ctx, w, cls.value, overrides.value)
+    new Command(t, ctx, w, cls.value)
   }
 
   def command[T](t: Result[T])(implicit
       w: W[T],
       ctx: mill.define.Ctx,
       cls: EnclosingClass,
-      overrides: Overrides
   ): Command[T] = macro commandImpl[T]
 
   def commandImpl[T: c.WeakTypeTag](c: Context)(t: c.Expr[T])(
       w: c.Expr[W[T]],
       ctx: c.Expr[mill.define.Ctx],
       cls: c.Expr[EnclosingClass],
-      overrides: c.Expr[Overrides]
   ): c.Expr[Command[T]] = {
     import c.universe._
     reify(
@@ -234,7 +231,6 @@ object Target extends TargetGenerated with Applicative.Applyer[Task, Task, Resul
         ctx.splice,
         w.splice,
         cls.splice.value,
-        overrides.splice.value
       )
     )
   }
@@ -317,7 +313,6 @@ class Command[+T](
     ctx0: mill.define.Ctx,
     val writer: W[_],
     val cls: Class[_],
-    val overrides: Int
 ) extends NamedTaskImpl[T](ctx0, t) {
   override def asCommand = Some(this)
 }

main/core/src/mill/eval/Evaluator.scala

@@ -93,7 +93,7 @@ case class Evaluator(
         (int: Int) => Option.empty[CompileProblemReporter],
       testReporter: TestReporter = DummyTestReporter
   ): Evaluator.Results = {
-    val (sortedGroups, transitive) = Evaluator.plan(rootModule, goals)
+    val (sortedGroups, transitive) = Evaluator.plan(goals)
 
     val evaluated = new Agg.Mutable[Task[_]]
     val results = mutable.LinkedHashMap.empty[Task[_], mill.api.Result[(Any, Int)]]
@@ -175,7 +175,7 @@ case class Evaluator(
     os.makeDir.all(outPath)
     val timeLog = new ParallelProfileLogger(outPath, System.currentTimeMillis())
 
-    val (sortedGroups, transitive) = Evaluator.plan(rootModule, goals)
+    val (sortedGroups, transitive) = Evaluator.plan(goals)
 
     val interGroupDeps = findInterGroupDeps(sortedGroups)
     import scala.concurrent._
@@ -656,47 +656,30 @@ object Evaluator {
     def values = rawValues.collect { case mill.api.Result.Success(v) => v }
   }
 
-  def plan(rootModule: BaseModule, goals: Agg[Task[_]]) = {
+  def plan(goals: Agg[Task[_]]) = {
     val transitive = Graph.transitiveTargets(goals)
     val topoSorted = Graph.topoSorted(transitive)
+    val seen = collection.mutable.Set.empty[Segments]
+    val overriden = collection.mutable.Set.empty[Task[_]]
+    topoSorted.values.reverse.foreach{
+      case x: NamedTask[_] =>
+        if (!seen.contains(x.ctx.segments)) seen.add(x.ctx.segments)
+        else overriden.add(x)
+      case _ => //donothing
+    }
+
     val sortedGroups = Graph.groupAroundImportantTargets(topoSorted) {
       case t: NamedTask[Any] =>
         val segments = t.ctx.segments
-        val finalTaskOverrides = t match {
-          case t: Target[_] =>
-            rootModule.millInternal.segmentsToTargets.get(segments).fold(0)(_.ctx.overrides)
-
-          case c: mill.define.Command[_] =>
-            def findMatching(cls: Class[_]): Option[Seq[(Int, MainData[_, _])]] = {
-              rootModule.millDiscover.value.get(cls) match {
-                case Some(v) => Some(v)
-                case None =>
-                  cls.getSuperclass match {
-                    case null => None
-                    case superCls => findMatching(superCls)
-                  }
-              }
-            }
-
-            findMatching(c.cls) match {
-              case Some(v) =>
-                v.find(_._2.name == c.ctx.segment.pathSegments.head).get._1
-              // For now we don't properly support overrides for external modules
-              // that do not appear in the Evaluator's main Discovered listing
-              case None => 0
-            }
-
-          case c: mill.define.Worker[_] => 0
-        }
 
         val additional =
-          if (finalTaskOverrides == t.ctx.overrides) Nil
-          else
-            Seq(Segment.Label("overriden")) ++ t.ctx.enclosing.split("\\.|#| ").map(Segment.Label)
+          if (!overriden(t)) Nil
+          else Seq(Segment.Label("overriden")) ++ t.ctx.enclosing.split("\\.|#| ").map(Segment.Label)
 
         Right(Labelled(t, segments ++ additional))
       case t if goals.contains(t) => Left(t)
     }
+
     (sortedGroups, transitive)
   }
 

main/src/mill/main/MainModule.scala

@@ -117,7 +117,7 @@ trait MainModule extends mill.Module {
     ) match {
       case Left(err) => Left(err)
       case Right(rs) =>
-        val (sortedGroups, _) = Evaluator.plan(evaluator.rootModule, rs)
+        val (sortedGroups, _) = Evaluator.plan(rs)
         Right(sortedGroups.keys().collect { case Right(r) => r }.toArray)
     }
   }

main/test/src/eval/EvaluationTests.scala

@@ -356,5 +356,32 @@ class EvaluationTests(threadCount: Option[Int]) extends TestSuite {
         assert(leftCount == 4, middleCount == 4, rightCount == 1)
       }
     }
+    "stackableOverrides" - {
+      // Make sure you can override commands, call their supers, and have the
+      // overriden command be allocated a spot within the overriden/ folder of
+      // the main publicly-available command
+      import StackableOverrides._
+
+      val checker = new Checker(canOverrideSuper)
+      checker(
+        m.f,
+        6,
+        Agg(m.f),
+        extraEvaled = -1
+      )
+
+      val overridePrefix =
+        os.sub /  "overriden" / "mill" / "util" / "TestGraphs" / "StackableOverrides"
+
+      assert(
+        os.read(checker.evaluator.outPath / "m" / "f" / overridePrefix / "X" / "f.json")
+          .contains(" 1,")
+      )
+      assert(
+        os.read(checker.evaluator.outPath / "m" / "f" / overridePrefix / "A" / "f.json")
+          .contains(" 3,")
+      )
+      assert(os.read(checker.evaluator.outPath / "m" / "f.json").contains(" 6,"))
+    }
   }
 }

main/test/src/util/TestGraphs.scala

@@ -263,4 +263,19 @@ object TestGraphs {
       }
     }
   }
+
+  object StackableOverrides extends TestUtil.BaseModule {
+    trait X extends Module{
+      def f = T{ 1 }
+    }
+    trait A extends X{
+      override def f = T{ super.f() +  2 }
+    }
+
+    trait B extends X{
+      override def f = T{ super.f() + 3 }
+    }
+    object m extends A with B{
+    }
+  }
 }

main/test/src/util/TestUtil.scala

@@ -28,7 +28,6 @@ object TestUtil {
       millModuleEnclosing0: sourcecode.Enclosing,
       millModuleLine0: sourcecode.Line,
       millName0: sourcecode.Name,
-      overrides: Overrides
   ) extends mill.define.BaseModule(getSrcPathBase() / millModuleEnclosing0.value.split("\\.| |#"))(
         implicitly,
         implicitly,