Fix selective execution when multiple changes are made to one module under --watch (main branch) (#5033)

Forward port to `main` of #5032

If you have two modules and run `--watch
{moduleA.fooCommand,moduleB.fooCommand}`, mill will run
`moduleB.fooCommand` every 2nd change to `moduleA`, even if there is no
changes to `moduleB`.

The problem stems from the fact that when we save the changed inputs in
selective execution, we did not store the inputs from `moduleB` and when
selective execution runs again (the 2nd time), it thinks that
*everything* in `moduleB` changed.

This fix correctly saves the state of all modules when storing the
selective execution state.

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>

Author: arturaz

core/define/src/mill/define/SelectiveExecution.scala

@@ -1,8 +1,8 @@
 package mill.define
 
-import mill.api.{Result, Val, ExecResult}
+import mill.api.{ExecResult, Result, Val}
 private[mill] trait SelectiveExecution {
-  import SelectiveExecution._
+  import SelectiveExecution.*
   def computeHashCodeSignatures(
       transitiveNamed: Seq[NamedTask[?]],
       codeSignatures: Map[String, Int]
@@ -20,7 +20,10 @@ private[mill] trait SelectiveExecution {
       tasks: Seq[String]
   ): Result[ChangedTasks]
 
-  def computeChangedTasks0(tasks: Seq[NamedTask[?]]): ChangedTasks
+  def computeChangedTasks0(
+      tasks: Seq[NamedTask[?]],
+      computedMetadata: SelectiveExecution.Metadata.Computed
+  ): Option[ChangedTasks]
 
   def resolve0(tasks: Seq[String]): Result[Array[String]]
 
@@ -30,17 +33,27 @@ private[mill] trait SelectiveExecution {
 
   def computeMetadata(
       tasks: Seq[NamedTask[?]]
-  ): (SelectiveExecution.Metadata, Map[Task[?], ExecResult[Val]])
+  ): SelectiveExecution.Metadata.Computed
 }
 object SelectiveExecution {
   case class Metadata(inputHashes: Map[String, Int], codeSignatures: Map[String, Int])
+  object Metadata {
+    case class Computed(
+        metadata: Metadata,
+        results: Map[Task[?], ExecResult[Val]]
+    )
+  }
 
   implicit val rw: upickle.default.ReadWriter[Metadata] = upickle.default.macroRW
 
   case class ChangedTasks(
       resolved: Seq[NamedTask[?]],
       changedRootTasks: Set[NamedTask[?]],
-      downstreamTasks: Seq[NamedTask[?]],
-      results: Map[Task[?], ExecResult[Val]]
+      downstreamTasks: Seq[NamedTask[?]]
   )
+  object ChangedTasks {
+
+    /** Indicates that all of the passed in tasks were changed. */
+    def all(tasks: Seq[NamedTask[?]]): ChangedTasks = ChangedTasks(tasks, tasks.toSet, tasks)
+  }
 }

core/eval/src/mill/eval/EvaluatorImpl.scala

@@ -1,14 +1,12 @@
 package mill.eval
 
 import mill.api.*
-import mill.api.internal.{ExecutionResultsApi, TestReporter, CompileProblemReporter}
-import mill.define.PathRef
+import mill.api.internal.{CompileProblemReporter, ExecutionResultsApi, TestReporter}
 import mill.constants.OutFiles
-import mill.define.*
+import mill.constants.OutFiles.*
+import mill.define.{PathRef, *}
+import mill.define.internal.{ResolveChecker, TopoSorted, Watchable}
 import mill.exec.{Execution, PlanImpl}
-import mill.define.internal.{ResolveChecker, Watchable}
-import OutFiles.*
-import mill.define.internal.TopoSorted
 import mill.resolve.Resolve
 
 /**
@@ -144,21 +142,37 @@ final class EvaluatorImpl private[mill] (
     val selectiveExecutionEnabled = selectiveExecution && !targets.exists(_.isExclusiveCommand)
 
     val selectedTasksOrErr =
-      if (selectiveExecutionEnabled && os.exists(outPath / OutFiles.millSelectiveExecution)) {
+      if (!selectiveExecutionEnabled) (targets, Map.empty, None)
+      else {
         val (named, unnamed) =
           targets.partitionMap { case n: NamedTask[?] => Left(n); case t => Right(t) }
-        val changedTasks = this.selective.computeChangedTasks0(named)
+        val newComputedMetadata = SelectiveExecutionImpl.Metadata.compute(this, named)
 
-        val selectedSet = changedTasks.downstreamTasks.map(_.ctx.segments.render).toSet
+        val selectiveExecutionStoredData = for {
+          _ <- Option.when(os.exists(outPath / OutFiles.millSelectiveExecution))(())
+          changedTasks <- this.selective.computeChangedTasks0(named, newComputedMetadata)
+        } yield changedTasks
 
-        (
-          unnamed ++ named.filter(t => t.isExclusiveCommand || selectedSet(t.ctx.segments.render)),
-          changedTasks.results
-        )
-      } else (targets, Map.empty)
+        selectiveExecutionStoredData match {
+          case None =>
+            // Ran when previous selective execution metadata is not available, which happens the first time you run
+            // selective execution.
+            (targets, Map.empty, Some(newComputedMetadata.metadata))
+          case Some(changedTasks) =>
+            val selectedSet = changedTasks.downstreamTasks.map(_.ctx.segments.render).toSet
+
+            (
+              unnamed ++ named.filter(t =>
+                t.isExclusiveCommand || selectedSet(t.ctx.segments.render)
+              ),
+              newComputedMetadata.results,
+              Some(newComputedMetadata.metadata)
+            )
+        }
+      }
 
     selectedTasksOrErr match {
-      case (selectedTasks, selectiveResults) =>
+      case (selectedTasks, selectiveResults, maybeNewMetadata) =>
         val evaluated: ExecutionResults =
           execution.executeTasks(
             selectedTasks,
@@ -199,15 +213,8 @@ final class EvaluatorImpl private[mill] (
           .flatten
           .toSeq
 
-        val allInputHashes = evaluated.transitiveResults
-          .iterator
-          .collect {
-            case (t: InputImpl[_], ExecResult.Success(Val(value))) =>
-              (t.ctx.segments.render, value.##)
-          }
-          .toMap
-
-        if (selectiveExecutionEnabled) {
+        maybeNewMetadata.foreach { newMetadata =>
+          val allInputHashes = newMetadata.inputHashes
           this.selective.saveMetadata(
             SelectiveExecution.Metadata(allInputHashes, codeSignatures)
           )

core/eval/src/mill/eval/SelectiveExecutionImpl.scala

@@ -1,10 +1,10 @@
 package mill.eval
 
-import mill.api.{ExecResult, Result, Val}
 import mill.api.internal.TestReporter
+import mill.api.{ExecResult, Result, Val}
 import mill.constants.OutFiles
-import mill.define.{Evaluator, InputImpl, NamedTask, SelectMode, Task, SelectiveExecution}
 import mill.define.SelectiveExecution.ChangedTasks
+import mill.define.*
 import mill.exec.{CodeSigUtils, Execution, PlanImpl}
 import mill.internal.SpanningForest
 import mill.internal.SpanningForest.breadthFirst
@@ -87,26 +87,42 @@ private[mill] class SelectiveExecutionImpl(evaluator: Evaluator)
       tasks,
       SelectMode.Separated,
       evaluator.allowPositionalCommandArgs
-    ).map(computeChangedTasks0(_))
+    ).map { tasks =>
+      computeChangedTasks0(tasks, SelectiveExecutionImpl.Metadata.compute(evaluator, tasks))
+        // If we did not have the metadata, presume everything was changed.
+        .getOrElse(ChangedTasks.all(tasks))
+    }
   }
 
-  def computeChangedTasks0(tasks: Seq[NamedTask[?]]): ChangedTasks = {
+  /**
+   * @return [[None]] when the metadata file is empty.
+   * @note throws if the metadata file does not exist.
+   */
+  def computeChangedTasks0(
+      tasks: Seq[NamedTask[?]],
+      computedMetadata: SelectiveExecution.Metadata.Computed
+  ): Option[ChangedTasks] = {
     val oldMetadataTxt = os.read(evaluator.outPath / OutFiles.millSelectiveExecution)
 
-    if (oldMetadataTxt == "") ChangedTasks(tasks, tasks.toSet, tasks, Map.empty)
-    else {
+    // We allow to clear the selective execution metadata to rerun all tasks.
+    //
+    // You would think that removing the file achieves the same result, however, blanking the file indicates that
+    // this was intentional and you did not simply forgot to run `selective.prepare` beforehand.
+    if (oldMetadataTxt == "") None
+    else Some {
       val transitiveNamed = PlanImpl.transitiveNamed(tasks)
       val oldMetadata = upickle.default.read[SelectiveExecution.Metadata](oldMetadataTxt)
-      val (newMetadata, results) =
-        SelectiveExecutionImpl.Metadata.compute0(evaluator, transitiveNamed)
       val (changedRootTasks, downstreamTasks) =
-        evaluator.selective.computeDownstream(transitiveNamed, oldMetadata, newMetadata)
+        evaluator.selective.computeDownstream(
+          transitiveNamed,
+          oldMetadata,
+          computedMetadata.metadata
+        )
 
       ChangedTasks(
         tasks,
         changedRootTasks.collect { case n: NamedTask[_] => n },
-        downstreamTasks.collect { case n: NamedTask[_] => n },
-        results
+        downstreamTasks.collect { case n: NamedTask[_] => n }
       )
     }
   }
@@ -174,22 +190,22 @@ private[mill] class SelectiveExecutionImpl(evaluator: Evaluator)
 
   def computeMetadata(
       tasks: Seq[NamedTask[?]]
-  ): (SelectiveExecution.Metadata, Map[Task[?], ExecResult[Val]]) =
+  ): SelectiveExecution.Metadata.Computed =
     SelectiveExecutionImpl.Metadata.compute(evaluator, tasks)
 }
 object SelectiveExecutionImpl {
   object Metadata {
     def compute(
         evaluator: Evaluator,
         tasks: Seq[NamedTask[?]]
-    ): (SelectiveExecution.Metadata, Map[Task[?], ExecResult[Val]]) = {
+    ): SelectiveExecution.Metadata.Computed = {
       compute0(evaluator, PlanImpl.transitiveNamed(tasks))
     }
 
     def compute0(
         evaluator: Evaluator,
         transitiveNamed: Seq[NamedTask[?]]
-    ): (SelectiveExecution.Metadata, Map[Task[?], ExecResult[Val]]) = {
+    ): SelectiveExecution.Metadata.Computed = {
       val results: Map[NamedTask[?], mill.api.Result[Val]] = transitiveNamed
         .collect { case task: InputImpl[_] =>
           val ctx = new mill.define.TaskCtx.Impl(
@@ -212,10 +228,13 @@ object SelectiveExecutionImpl {
       val inputHashes = results.map {
         case (task, execResultVal) => (task.ctx.segments.render, execResultVal.get.value.hashCode)
       }
-      new SelectiveExecution.Metadata(
-        inputHashes,
-        evaluator.codeSignatures
-      ) -> results.map { case (k, v) => (k, ExecResult.Success(v.get)) }
+      SelectiveExecution.Metadata.Computed(
+        new SelectiveExecution.Metadata(
+          inputHashes,
+          evaluator.codeSignatures
+        ),
+        results.map { case (k, v) => (k, ExecResult.Success(v.get)) }
+      )
     }
   }
 

integration/invalidation/selective-execution/src/SelectiveExecutionTests.scala

@@ -223,6 +223,13 @@ object SelectiveExecutionWatchTests extends UtestIntegrationTestSuite {
             !output.contains("Computing fooCommand") && output.contains("Computing barCommand")
           }
 
+          // Test for a bug where modifying the sources 2nd time would run tasks from both modules.
+          output0 = Nil
+          modifyFile(workspacePath / "bar/bar.txt", _ + "!")
+          eventually {
+            !output.contains("Computing fooCommand") && output.contains("Computing barCommand")
+          }
+
           output0 = Nil
           modifyFile(workspacePath / "foo/foo.txt", _ + "!")
           eventually {

libs/main/src/mill/main/SelectiveExecutionModule.scala

@@ -39,9 +39,9 @@ trait SelectiveExecutionModule extends mill.define.Module {
         if (tasks.isEmpty) Seq("__") else tasks,
         SelectMode.Multi
       ).map { resolvedTasks =>
-        val (metadata, _) = evaluator.selective.computeMetadata(resolvedTasks)
+        val computed = evaluator.selective.computeMetadata(resolvedTasks)
 
-        evaluator.selective.saveMetadata(metadata)
+        evaluator.selective.saveMetadata(computed.metadata)
       }
     }