Ensure `Evaluator` can handle the case where you have more than one terminal nodes on the boundary between two groups, *and* one of the terminal nodes depends on another

2 files changed, 53 insertions, 19 deletions

diff --git a/core/src/main/scala/mill/eval/Evaluator.scala b/core/src/main/scala/mill/eval/Evaluator.scala
index 186a7869..54339591 100644
--- a/core/src/main/scala/mill/eval/Evaluator.scala
+++ b/core/src/main/scala/mill/eval/Evaluator.scala
@@ -90,7 +90,7 @@ class Evaluator(workspacePath: Path,
     val allInputs = group.items.flatMap(_.inputs)
     val (internalInputs, externalInputs) = allInputs.partition(group.contains)
     val internalInputSet = internalInputs.toSet
-    val terminals = group.filter(!internalInputSet(_))
+    val terminals = group.filter(x => !internalInputSet(x) || labeling.contains(x))
     (OSet.from(externalInputs.distinct), terminals)
   }
 
diff --git a/core/src/test/scala/mill/EvaluationTests.scala b/core/src/test/scala/mill/EvaluationTests.scala
index 9834bb12..0face75e 100644
--- a/core/src/test/scala/mill/EvaluationTests.scala
+++ b/core/src/test/scala/mill/EvaluationTests.scala
@@ -19,8 +19,13 @@ object EvaluationTests extends TestSuite{
 
     def apply(target: Task[_], expValue: Any,
               expEvaled: OSet[Task[_]],
+              // How many "other" tasks were evaluated other than those listed above.
+              // Pass in -1 to skip the check entirely
               extraEvaled: Int = 0,
-              secondRun: Boolean = true) = {
+              // Perform a second evaluation of the same tasks, and make sure the
+              // outputs are the same but nothing was evaluated. Disable this if you
+              // are directly evaluating tasks which need to re-evaluate every time
+              secondRunNoOp: Boolean = true) = {
 
       val Evaluator.Results(returnedValues, returnedEvaluated) = evaluator.evaluate(OSet(target))
 
@@ -29,11 +34,11 @@ object EvaluationTests extends TestSuite{
       assert(
         returnedValues == Seq(expValue),
         matchingReturnedEvaled.toSet == expEvaled.toSet,
-        extra.length == extraEvaled
+        extraEvaled == -1 || extra.length == extraEvaled
       )
 
       // Second time the value is already cached, so no evaluation needed
-      if (secondRun){
+      if (secondRunNoOp){
         val Evaluator.Results(returnedValues2, returnedEvaluated2) = evaluator.evaluate(OSet(target))
         val expecteSecondRunEvaluated = OSet()
         assert(
@@ -43,7 +48,12 @@ object EvaluationTests extends TestSuite{
       }
     }
   }
-
+  def countGroups[T: Discovered](t: T, terminals: Task[_]*) = {
+    val labeling = Discovered.mapping(t)
+    val topoSorted = Evaluator.topoSortedTransitiveTargets(OSet.from(terminals))
+    val grouped = Evaluator.groupAroundNamedTargets(topoSorted, labeling)
+    grouped.keyCount
+  }
 
   val tests = Tests{
     val graphs = new TestGraphs()
@@ -158,13 +168,15 @@ object EvaluationTests extends TestSuite{
         object taskTriangle extends Cacher{
           val task = T.task{ 1 }
           def left = T{ task() }
-          def right = T{ task() + left() }
+          def right = T{ task() + left() + 1 }
         }
-        val labeling = Discovered.mapping(taskTriangle)
-        val topoSorted = Evaluator.topoSortedTransitiveTargets(OSet(taskTriangle.right, taskTriangle.left))
-        val grouped = Evaluator.groupAroundNamedTargets(topoSorted, labeling)
-        val groupCount = grouped.keyCount
+
+        val groupCount = countGroups(taskTriangle, taskTriangle.right, taskTriangle.left)
         assert(groupCount == 1)
+        val checker = new Checker(taskTriangle)
+        checker(taskTriangle.right, 3, OSet(taskTriangle.right), extraEvaled = -1)
+        checker(taskTriangle.left, 1, OSet(taskTriangle.left), extraEvaled = -1)
+
       }
       'multiTerminalGroup - {
         // Make sure the following graph ends up as a single group
@@ -177,12 +189,34 @@ object EvaluationTests extends TestSuite{
           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
-
+        val groupCount = countGroups(taskTriangle, taskTriangle.right, taskTriangle.left)
         assert(groupCount == 1)
+
+        val checker = new Checker(taskTriangle)
+        checker(taskTriangle.right, 1, OSet(taskTriangle.right), extraEvaled = -1)
+        checker(taskTriangle.left, 1, OSet(taskTriangle.left), extraEvaled = -1)
+      }
+
+      'multiTerminalBoundary - {
+        // Make sure the following graph ends up as a single group
+        //
+        //       _ left _____________
+        //      /        \           \
+        // task1 -------- right ----- task2
+        object multiTerminalBoundary extends Cacher{
+          val task1 = T.task{ 1 }
+          def left = T{ task1() }
+          def right = T{ task1() + left() + 1 }
+          val task2 = T.task{ left() + right() }
+        }
+        import multiTerminalBoundary._
+        val groupCount = countGroups(multiTerminalBoundary, task2)
+        assert(groupCount == 2)
+
+
+        val checker = new Checker(multiTerminalBoundary)
+        checker(task2, 4, OSet(right, left), extraEvaled = -1, secondRunNoOp = false)
+        checker(task2, 4, OSet(), extraEvaled = -1, secondRunNoOp = false)
       }
 
       'tasksAreUncached - {
@@ -238,14 +272,14 @@ object EvaluationTests extends TestSuite{
 
         // Running the tasks themselves results in them being recomputed every
         // single time, even if nothing changes
-        check(left, expValue = 2, expEvaled = OSet(), extraEvaled = 1, secondRun = false)
+        check(left, expValue = 2, expEvaled = OSet(), extraEvaled = 1, secondRunNoOp = false)
         assert(leftCount == 3, middleCount == 2, rightCount == 1)
-        check(left, expValue = 2, expEvaled = OSet(), extraEvaled = 1, secondRun = false)
+        check(left, expValue = 2, expEvaled = OSet(), extraEvaled = 1, secondRunNoOp = false)
         assert(leftCount == 4, middleCount == 2, rightCount == 1)
 
-        check(middle, expValue = 100, expEvaled = OSet(), extraEvaled = 2, secondRun = false)
+        check(middle, expValue = 100, expEvaled = OSet(), extraEvaled = 2, secondRunNoOp = false)
         assert(leftCount == 4, middleCount == 3, rightCount == 1)
-        check(middle, expValue = 100, expEvaled = OSet(), extraEvaled = 2, secondRun = false)
+        check(middle, expValue = 100, expEvaled = OSet(), extraEvaled = 2, secondRunNoOp = false)
         assert(leftCount == 4, middleCount == 4, rightCount == 1)
       }