First pass at managing failures in the evaluation graph now works. Tweaked the `JavaCompileJarTests` to make use of it, but still need to write a proper unit test suite

8 files changed, 101 insertions, 61 deletions

diff --git a/core/src/main/scala/mill/define/Applicative.scala b/core/src/main/scala/mill/define/Applicative.scala
index 95e7e211..512e5a12 100644
--- a/core/src/main/scala/mill/define/Applicative.scala
+++ b/core/src/main/scala/mill/define/Applicative.scala
@@ -20,25 +20,26 @@ object Applicative {
     @compileTimeOnly("Target#apply() can only be used with a T{...} block")
     def apply(): T = ???
   }
-  trait Applyer[W[_], T[_], Ctx]{
+  type Id[+T] = T
+  trait Applyer[W[_], T[_], Z[_], Ctx]{
     @compileTimeOnly("Target.ctx() can only be used with a T{...} block")
     def ctx(): Ctx = ???
     def underlying[A](v: W[A]): T[_]
 
-    def mapCtx[A, B](a: T[A])(f: (A, Ctx) => B): T[B]
-    def zipMap[R]()(cb: Ctx => R) = mapCtx(zip()){ (_, ctx) => cb(ctx)}
-    def zipMap[A, R](a: T[A])(f: (A, Ctx) => R) = mapCtx(a)(f)
-    def zipMap[A, B, R](a: T[A], b: T[B])(cb: (A, B, Ctx) => R) = mapCtx(zip(a, b)){case ((a, b), x) => cb(a, b, x)}
+    def mapCtx[A, B](a: T[A])(f: (A, Ctx) => Z[B]): T[B]
+    def zipMap[R]()(cb: Ctx => Z[R]) = mapCtx(zip()){ (_, ctx) => cb(ctx)}
+    def zipMap[A, R](a: T[A])(f: (A, Ctx) => Z[R]) = mapCtx(a)(f)
+    def zipMap[A, B, R](a: T[A], b: T[B])(cb: (A, B, Ctx) => Z[R]) = mapCtx(zip(a, b)){case ((a, b), x) => cb(a, b, x)}
     def zipMap[A, B, C, R](a: T[A], b: T[B], c: T[C])
-                          (cb: (A, B, C, Ctx) => R) = mapCtx(zip(a, b, c)){case ((a, b, c), x) => cb(a, b, c, x)}
+                          (cb: (A, B, C, Ctx) => Z[R]) = mapCtx(zip(a, b, c)){case ((a, b, c), x) => cb(a, b, c, x)}
     def zipMap[A, B, C, D, R](a: T[A], b: T[B], c: T[C], d: T[D])
-                             (cb: (A, B, C, D, Ctx) => R) = mapCtx(zip(a, b, c, d)){case ((a, b, c, d), x) => cb(a, b, c, d, x)}
+                             (cb: (A, B, C, D, Ctx) => Z[R]) = mapCtx(zip(a, b, c, d)){case ((a, b, c, d), x) => cb(a, b, c, d, x)}
     def zipMap[A, B, C, D, E, R](a: T[A], b: T[B], c: T[C], d: T[D], e: T[E])
-                                (cb: (A, B, C, D, E, Ctx) => R) = mapCtx(zip(a, b, c, d, e)){case ((a, b, c, d, e), x) => cb(a, b, c, d, e, x)}
+                                (cb: (A, B, C, D, E, Ctx) => Z[R]) = mapCtx(zip(a, b, c, d, e)){case ((a, b, c, d, e), x) => cb(a, b, c, d, e, x)}
     def zipMap[A, B, C, D, E, F, R](a: T[A], b: T[B], c: T[C], d: T[D], e: T[E], f: T[F])
-                                   (cb: (A, B, C, D, E, F, Ctx) => R) = mapCtx(zip(a, b, c, d, e, f)){case ((a, b, c, d, e, f), x) => cb(a, b, c, d, e, f, x)}
+                                   (cb: (A, B, C, D, E, F, Ctx) => Z[R]) = mapCtx(zip(a, b, c, d, e, f)){case ((a, b, c, d, e, f), x) => cb(a, b, c, d, e, f, x)}
     def zipMap[A, B, C, D, E, F, G, R](a: T[A], b: T[B], c: T[C], d: T[D], e: T[E], f: T[F], g: T[G])
-                                      (cb: (A, B, C, D, E, F, G, Ctx) => R) = mapCtx(zip(a, b, c, d, e, f, g)){case ((a, b, c, d, e, f, g), x) => cb(a, b, c, d, e, f, g, x)}
+                                      (cb: (A, B, C, D, E, F, G, Ctx) => Z[R]) = mapCtx(zip(a, b, c, d, e, f, g)){case ((a, b, c, d, e, f, g), x) => cb(a, b, c, d, e, f, g, x)}
     def zip(): T[Unit]
     def zip[A](a: T[A]): T[Tuple1[A]]
     def zip[A, B](a: T[A], b: T[B]): T[(A, B)]
diff --git a/core/src/main/scala/mill/define/Task.scala b/core/src/main/scala/mill/define/Task.scala
index 7635a74f..471c530f 100644
--- a/core/src/main/scala/mill/define/Task.scala
+++ b/core/src/main/scala/mill/define/Task.scala
@@ -1,8 +1,7 @@
 package mill.define
 
 import mill.define.Applicative.Applyable
-
-import mill.eval.PathRef
+import mill.eval.{PathRef, Result}
 import mill.util.Args
 
 import scala.language.experimental.macros
@@ -17,7 +16,7 @@ abstract class Task[+T] extends Task.Ops[T] with Applyable[T]{
   /**
     * Evaluate this target
     */
-  def evaluate(args: Args): T
+  def evaluate(args: Args): Result[T]
 
   /**
     * Even if this target's inputs did not change, does it need to re-evaluate
@@ -35,22 +34,22 @@ abstract class Task[+T] extends Task.Ops[T] with Applyable[T]{
 trait Target[+T] extends Task[T]{
   override def asTarget = Some(this)
 }
-object Target extends Applicative.Applyer[Task, Task, Args]{
+object Target extends Applicative.Applyer[Task, Task, Result, Args]{
 
-  implicit def apply[T](t: T): Target[T] = macro targetImpl[T]
+  implicit def apply[T](t: Result[T]): Target[T] = macro targetImpl[T]
 
   def apply[T](t: Task[T]): Target[T] = macro targetTaskImpl[T]
 
-  def command[T](t: T): Command[T] = macro commandImpl[T]
+  def command[T](t: Result[T]): Command[T] = macro commandImpl[T]
 
   def source(path: ammonite.ops.Path) = new Source(path)
 
   def command[T](t: Task[T]): Command[T] = new Command(t)
 
-  def task[T](t: T): Task[T] = macro Applicative.impl[Task, T, Args]
+  def task[T](t: Result[T]): Task[T] = macro Applicative.impl[Task, T, Args]
   def task[T](t: Task[T]): Task[T] = t
 
-  def persistent[T](t: T): Target[T] = macro persistentImpl[T]
+  def persistent[T](t: Result[T]): Target[T] = macro persistentImpl[T]
   def persistentImpl[T: c.WeakTypeTag](c: Context)(t: c.Expr[T]): c.Expr[Persistent[T]] = {
     import c.universe._
 
@@ -86,7 +85,7 @@ object Target extends Applicative.Applyer[Task, Task, Args]{
   }
 
   def underlying[A](v: Task[A]) = v
-  def mapCtx[A, B](t: Task[A])(f: (A, Args) => B) = t.mapDest(f)
+  def mapCtx[A, B](t: Task[A])(f: (A, Args) => Result[B]) = t.mapDest(f)
   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)
@@ -155,7 +154,7 @@ object Task {
 
   abstract class Ops[+T]{ this: Task[T] =>
     def map[V](f: T => V) = new Task.Mapped(this, f)
-    def mapDest[V](f: (T, Args) => V) = new Task.MappedDest(this, f)
+    def mapDest[V](f: (T, Args) => Result[V]) = new Task.MappedDest(this, f)
 
     def filter(f: T => Boolean) = this
     def withFilter(f: T => Boolean) = this
@@ -178,7 +177,7 @@ object Task {
     def evaluate(args: Args) = f(args(0))
     val inputs = List(source)
   }
-  class MappedDest[+T, +V](source: Task[T], f: (T, Args) => V) extends Task[V]{
+  class MappedDest[+T, +V](source: Task[T], f: (T, Args) => Result[V]) extends Task[V]{
     def evaluate(args: Args) = f(args(0), args)
     val inputs = List(source)
   }
diff --git a/core/src/main/scala/mill/eval/Evaluator.scala b/core/src/main/scala/mill/eval/Evaluator.scala
index db364e20..d76f5f4b 100644
--- a/core/src/main/scala/mill/eval/Evaluator.scala
+++ b/core/src/main/scala/mill/eval/Evaluator.scala
@@ -3,6 +3,7 @@ package mill.eval
 import ammonite.ops._
 import mill.define.{Target, Task}
 import mill.discover.Mirror.LabelledTarget
+import mill.util
 import mill.util.{Args, MultiBiMap, OSet}
 
 import scala.collection.mutable
@@ -20,7 +21,7 @@ class Evaluator(workspacePath: Path,
     }
 
     val evaluated = new OSet.Mutable[Task[_]]
-    val results = mutable.LinkedHashMap.empty[Task[_], Any]
+    val results = mutable.LinkedHashMap.empty[Task[_], Result[Any]]
 
     for ((terminal, group)<- sortedGroups.items()){
       val (newResults, newEvaluated) = evaluateGroupCached(
@@ -35,7 +36,11 @@ class Evaluator(workspacePath: Path,
 
     }
 
-    Evaluator.Results(goals.items.map(results), evaluated, transitive)
+    val failing = new util.MultiBiMap.Mutable[Either[Task[_], LabelledTarget[_]], Result.Failing]
+    for((k, vs) <- sortedGroups.items){
+      failing.addAll(k, vs.items.flatMap(results.get(_)).collect{case f: Result.Failing => f})
+    }
+    Evaluator.Results(goals.items.map(results), evaluated, transitive, failing)
   }
 
   def resolveDestPaths(t: LabelledTarget[_]): (Path, Path) = {
@@ -46,7 +51,7 @@ class Evaluator(workspacePath: Path,
 
   def evaluateGroupCached(terminal: Either[Task[_], LabelledTarget[_]],
                           group: OSet[Task[_]],
-                          results: collection.Map[Task[_], Any]): (collection.Map[Task[_], Any], Seq[Task[_]]) = {
+                          results: collection.Map[Task[_], Result[Any]]): (collection.Map[Task[_], Result[Any]], Seq[Task[_]]) = {
 
 
     val externalInputs = group.items.flatMap(_.inputs).filter(!group.contains(_))
@@ -67,7 +72,7 @@ class Evaluator(workspacePath: Path,
 
         cached match{
           case Some(terminalResult) =>
-            val newResults = mutable.LinkedHashMap.empty[Task[_], Any]
+            val newResults = mutable.LinkedHashMap.empty[Task[_], Result[Any]]
             newResults(labelledTarget.target) = labelledTarget.format.read(terminalResult)
             (newResults, Nil)
 
@@ -77,12 +82,19 @@ class Evaluator(workspacePath: Path,
             if (labelledTarget.target.flushDest) rm(destPath)
             val (newResults, newEvaluated) = evaluateGroup(group, results, Some(destPath))
 
-            val terminalResult = labelledTarget
-                .format
-                .asInstanceOf[upickle.default.ReadWriter[Any]]
-                .write(newResults(labelledTarget.target))
+            newResults(labelledTarget.target) match{
+              case Result.Success(v) =>
+                val terminalResult = labelledTarget
+                  .format
+                  .asInstanceOf[upickle.default.ReadWriter[Any]]
+                  .write(v)
+
+                write.over(metadataPath, upickle.default.write(inputsHash -> terminalResult, indent = 4))
+              case _ =>
+            }
+
+
 
-            write.over(metadataPath, upickle.default.write(inputsHash -> terminalResult, indent = 4))
             (newResults, newEvaluated)
         }
     }
@@ -90,21 +102,25 @@ class Evaluator(workspacePath: Path,
 
 
   def evaluateGroup(group: OSet[Task[_]],
-                    results: collection.Map[Task[_], Any],
+                    results: collection.Map[Task[_], Result[Any]],
                     targetDestPath: Option[Path]) = {
 
 
     val newEvaluated = mutable.Buffer.empty[Task[_]]
-    val newResults = mutable.LinkedHashMap.empty[Task[_], Any]
+    val newResults = mutable.LinkedHashMap.empty[Task[_], Result[Any]]
     for (target <- group.items if !results.contains(target)) {
 
       newEvaluated.append(target)
-      val targetInputValues = target.inputs.toVector.map(x =>
-        newResults.getOrElse(x, results(x))
-      )
+      val targetInputValues = target.inputs
+        .map(x => newResults.getOrElse(x, results(x)))
+        .collect{ case Result.Success(v) => v }
 
-      val args = new Args(targetInputValues, targetDestPath.orNull)
-      val res = target.evaluate(args)
+      val res =
+        if (targetInputValues.length != target.inputs.length) Result.Skipped
+        else {
+          val args = new Args(targetInputValues.toArray[Any], targetDestPath.orNull)
+          target.evaluate(args)
+        }
 
       newResults(target) = res
     }
@@ -117,7 +133,12 @@ class Evaluator(workspacePath: Path,
 
 object Evaluator{
   class TopoSorted private[Evaluator](val values: OSet[Task[_]])
-  case class Results(values: Seq[Any], evaluated: OSet[Task[_]], transitive: OSet[Task[_]])
+  case class Results(rawValues: Seq[Result[Any]],
+                     evaluated: OSet[Task[_]],
+                     transitive: OSet[Task[_]],
+                     failing: MultiBiMap[Either[Task[_], LabelledTarget[_]], Result.Failing]){
+    def values = rawValues.collect{case Result.Success(v) => v}
+  }
   def groupAroundImportantTargets[T](topoSortedTargets: TopoSorted)
                                     (important: PartialFunction[Task[_], T]): MultiBiMap[T, Task[_]] = {
 
diff --git a/core/src/main/scala/mill/eval/Result.scala b/core/src/main/scala/mill/eval/Result.scala
new file mode 100644
index 00000000..e0cc18bb
--- /dev/null
+++ b/core/src/main/scala/mill/eval/Result.scala
@@ -0,0 +1,14 @@
+package mill.eval
+
+sealed trait Result[+T]
+object Result{
+  implicit def create[T](t: => T): Result[T] = {
+    try Success(t)
+    catch { case e: Throwable => Exception(e) }
+  }
+  case class Success[T](value: T) extends Result[T]
+  case object Skipped extends Result[Nothing]
+  sealed trait Failing extends Result[Nothing]
+  case class Failure(msg: String) extends Failing
+  case class Exception(throwable: Throwable) extends Failing
+}
\ No newline at end of file
diff --git a/core/src/main/scala/mill/modules/Jvm.scala b/core/src/main/scala/mill/modules/Jvm.scala
index 63f335f5..84ae5c42 100644
--- a/core/src/main/scala/mill/modules/Jvm.scala
+++ b/core/src/main/scala/mill/modules/Jvm.scala
@@ -115,7 +115,7 @@ object Jvm {
   def jarUp(roots: Task[PathRef]*) = new Task[PathRef]{
 
     val inputs = roots
-    def evaluate(args: Args): PathRef = {
+    def evaluate(args: Args) = {
       createJar(args.dest, args.args.map(_.asInstanceOf[PathRef].path))
       PathRef(args.dest)
     }
diff --git a/core/src/test/scala/mill/ApplicativeTests.scala b/core/src/test/scala/mill/ApplicativeTests.scala
index 5bb9945c..bd930f93 100644
--- a/core/src/test/scala/mill/ApplicativeTests.scala
+++ b/core/src/test/scala/mill/ApplicativeTests.scala
@@ -7,7 +7,7 @@ import language.experimental.macros
 object ApplicativeTests extends TestSuite {
   implicit def optionToOpt[T](o: Option[T]): Opt[T] = new Opt(o)
   class Opt[T](val o: Option[T]) extends Applicative.Applyable[T]
-  object Opt extends define.Applicative.Applyer[Opt, Option, String]{
+  object Opt extends define.Applicative.Applyer[Opt, Option, Applicative.Id, String]{
 
     val injectedCtx = "helloooo"
     def underlying[A](v: Opt[A]) = v.o
diff --git a/core/src/test/scala/mill/EvaluationTests.scala b/core/src/test/scala/mill/EvaluationTests.scala
index d6c928e5..03118a29 100644
--- a/core/src/test/scala/mill/EvaluationTests.scala
+++ b/core/src/test/scala/mill/EvaluationTests.scala
@@ -26,23 +26,23 @@ object EvaluationTests extends TestSuite{
               // are directly evaluating tasks which need to re-evaluate every time
               secondRunNoOp: Boolean = true) = {
 
-      val Evaluator.Results(returnedValues, returnedEvaluated, _) = evaluator.evaluate(OSet(target))
+      val evaled = evaluator.evaluate(OSet(target))
 
-      val (matchingReturnedEvaled, extra) = returnedEvaluated.items.partition(expEvaled.contains)
+      val (matchingReturnedEvaled, extra) = evaled.evaluated.items.partition(expEvaled.contains)
 
       assert(
-        returnedValues == Seq(expValue),
+        evaled.values == Seq(expValue),
         matchingReturnedEvaled.toSet == expEvaled.toSet,
         extraEvaled == -1 || extra.length == extraEvaled
       )
 
       // Second time the value is already cached, so no evaluation needed
       if (secondRunNoOp){
-        val Evaluator.Results(returnedValues2, returnedEvaluated2, _) = evaluator.evaluate(OSet(target))
+        val evaled2 = evaluator.evaluate(OSet(target))
         val expecteSecondRunEvaluated = OSet()
         assert(
-          returnedValues2 == returnedValues,
-          returnedEvaluated2 == expecteSecondRunEvaluated
+          evaled2.values == evaled.values,
+          evaled2.evaluated == expecteSecondRunEvaluated
         )
       }
     }
diff --git a/core/src/test/scala/mill/JavaCompileJarTests.scala b/core/src/test/scala/mill/JavaCompileJarTests.scala
index 77add85a..9390d98e 100644
--- a/core/src/test/scala/mill/JavaCompileJarTests.scala
+++ b/core/src/test/scala/mill/JavaCompileJarTests.scala
@@ -5,7 +5,7 @@ import ammonite.ops._
 import ImplicitWd._
 import mill.define.{Target, Task}
 import mill.discover.Discovered
-import mill.eval.Evaluator
+import mill.eval.{Evaluator, Result}
 import mill.modules.Jvm.jarUp
 import mill.util.OSet
 import utest._
@@ -49,16 +49,22 @@ object JavaCompileJarTests extends TestSuite{
       import Build._
       val mapping = Discovered.mapping(Build)
 
-      def eval[T](t: Task[T]): (T, Int) = {
+      def eval[T](t: Task[T]): Either[Result.Failing, (T, Int)] = {
         val evaluator = new Evaluator(workspacePath, mapping)
         val evaluated = evaluator.evaluate(OSet(t))
-        Tuple2(
-          evaluated.values(0).asInstanceOf[T],
-          evaluated.evaluated.collect{
-            case t: Target[_] if mapping.contains(t) => t
-            case t: mill.define.Command[_] => t
-          }.size
-        )
+
+        if (evaluated.failing.keyCount == 0){
+          Right(Tuple2(
+            evaluated.rawValues(0).asInstanceOf[Result.Success[T]].value,
+            evaluated.evaluated.collect{
+              case t: Target[_] if mapping.contains(t) => t
+              case t: mill.define.Command[_] => t
+            }.size
+          ))
+        }else{
+          Left(evaluated.failing.lookupKey(evaluated.failing.keys().next).items.head)
+        }
+
       }
       def check(targets: OSet[Task[_]], expected: OSet[Task[_]]) = {
         val evaluator = new Evaluator(workspacePath, mapping)
@@ -134,16 +140,15 @@ object JavaCompileJarTests extends TestSuite{
       for(i <- 0 until 3){
         // Build.run is not cached, so every time we eval it it has to
         // re-evaluate
-        val (runOutput, evalCount) = eval(Build.run("test.Foo"))
+        val Right((runOutput, evalCount)) = eval(Build.run("test.Foo"))
         assert(
           runOutput.out.string == (31337 + 271828) + "\n",
           evalCount == 1
         )
       }
 
-      val ex = intercept[ammonite.ops.ShelloutException]{
-        eval(Build.run("test.BarFour"))
-      }
+      val Left(Result.Exception(ex)) = eval(Build.run("test.BarFour"))
+
       assert(ex.getMessage.contains("Could not find or load main class"))
 
       append(
@@ -156,12 +161,12 @@ object JavaCompileJarTests extends TestSuite{
         }
         """
       )
-      val (runOutput2, evalCount2) = eval(Build.run("test.BarFour"))
+      val Right((runOutput2, evalCount2)) = eval(Build.run("test.BarFour"))
       assert(
         runOutput2.out.string == "New Cls!\n",
         evalCount2 == 3
       )
-      val (runOutput3, evalCount3) = eval(Build.run("test.BarFour"))
+      val Right((runOutput3, evalCount3)) = eval(Build.run("test.BarFour"))
       assert(
         runOutput3.out.string == "New Cls!\n",
         evalCount3 == 1