Properly merge groups in `groupAroundNamedTargets` to handle cases where a group has multiple terminals

4 files changed, 28 insertions, 5 deletions

diff --git a/core/src/main/scala/mill/define/Task.scala b/core/src/main/scala/mill/define/Task.scala
index 9d0972eb..ad6248d9 100644
--- a/core/src/main/scala/mill/define/Task.scala
+++ b/core/src/main/scala/mill/define/Task.scala
@@ -41,7 +41,7 @@ object Task extends Applicative.Applyer[Task, Task, Args]{
   trait Cacher extends mill.define.Cacher[Task, Target]{
     def wrapCached[T](t: Task[T], enclosing: String): Target[T] = new TargetImpl(t, enclosing)
   }
-  class Target0[T](t: T) extends Task[T]{
+  class Task0[T](t: T) extends Task[T]{
     lazy val t0 = t
     val inputs = Nil
     def evaluate(args: Args)  = t0
@@ -116,7 +116,7 @@ object Task extends Applicative.Applyer[Task, Task, Args]{
 
 
   def mapCtx[A, B](t: Task[A])(f: (A, Args) => B) = t.mapDest(f)
-  def zip() =  new Task.Target0(())
+  def zip() =  new Task.Task0(())
   def zip[A](a: Task[A]) = a.map(Tuple1(_))
   def zip[A, B](a: Task[A], b: Task[B]) = a.zip(b)
   def zip[A, B, C](a: Task[A], b: Task[B], c: Task[C]) = new Task[(A, B, C)]{
diff --git a/core/src/main/scala/mill/eval/Evaluator.scala b/core/src/main/scala/mill/eval/Evaluator.scala
index 947a2bca..186a7869 100644
--- a/core/src/main/scala/mill/eval/Evaluator.scala
+++ b/core/src/main/scala/mill/eval/Evaluator.scala
@@ -147,9 +147,9 @@ object Evaluator{
         grouping.lookupValueOpt(upstream) match{
           case None if !labeling.contains(upstream) =>
             grouping.add(targetGroup, upstream)
-          case Some(upstreamGroup) if upstreamGroup == targetGroup =>
+          case Some(upstreamGroup)
+            if !labeling.contains(upstream) && upstreamGroup != targetGroup =>
             val upstreamTargets = grouping.removeAll(upstreamGroup)
-
             grouping.addAll(targetGroup, upstreamTargets)
           case _ => //donothing
         }
@@ -171,7 +171,9 @@ object Evaluator{
     val targetOrdering = topoSortedTargets.values.items.zipWithIndex.toMap
     val output = new MultiBiMap.Mutable[Int, Task[_]]
     for((groupIndices, i) <- groupOrdering.zipWithIndex){
-      val sortedGroup = OSet.from(groupIndices.flatMap(grouping.lookupKey).sortBy(targetOrdering))
+      val sortedGroup = OSet.from(
+        groupIndices.flatMap(grouping.lookupKeyOpt).flatten.sortBy(targetOrdering)
+      )
       output.addAll(i, sortedGroup)
     }
     output
diff --git a/core/src/main/scala/mill/util/MultiBiMap.scala b/core/src/main/scala/mill/util/MultiBiMap.scala
index 88bb172c..2052ef90 100644
--- a/core/src/main/scala/mill/util/MultiBiMap.scala
+++ b/core/src/main/scala/mill/util/MultiBiMap.scala
@@ -25,6 +25,7 @@ object MultiBiMap{
     private[this] val keyToValues = mutable.LinkedHashMap.empty[K, OSet.Mutable[V]]
     def containsValue(v: V) = valueToKey.contains(v)
     def lookupKey(k: K) = keyToValues(k)
+    def lookupKeyOpt(k: K) = keyToValues.get(k)
     def lookupValue(v: V) = valueToKey(v)
     def lookupValueOpt(v: V) = valueToKey.get(v)
     def add(k: K, v: V): Unit = {
diff --git a/core/src/test/scala/mill/EvaluationTests.scala b/core/src/test/scala/mill/EvaluationTests.scala
index ee7b081e..c39c9a50 100644
--- a/core/src/test/scala/mill/EvaluationTests.scala
+++ b/core/src/test/scala/mill/EvaluationTests.scala
@@ -165,8 +165,28 @@ object EvaluationTests extends TestSuite{
         val grouped = Evaluator.groupAroundNamedTargets(topoSorted, labeling)
         val groupCount = grouped.keyCount
         assert(groupCount == 1)
+      }
+      'multiTerminalGroup - {
+        // Make sure the following graph ends up as a single group, since although
+        // `right` depends on `left`, both of them depend on the un-cached `task`
+        // which would force them both to re-compute every time `task` changes
+        //
+        //      _ left
+        //     /
+        // task -------- right
+        object taskTriangle extends Cacher{
+          val task = T.task{ 1 }
+          def left = T{ task() }
+          def right = T{ task() }
+        }
+        val labeling = Discovered.mapping(taskTriangle)
+        val topoSorted = Evaluator.topoSortedTransitiveTargets(OSet(taskTriangle.right, taskTriangle.left))
+        val grouped = Evaluator.groupAroundNamedTargets(topoSorted, labeling)
+        val groupCount = grouped.keyCount
 
+        assert(groupCount == 1)
       }
+
       'tasksAreUncached - {
         // Make sure the tasks `left` and `middle` re-compute every time, while
         // the target `right` does not