Override zinc compile analysis for source changes

Zinc records all of the compile source file hashes when compilation
completes. This is problematic because its possible that a source file
was changed during compilation. From the user perspective, this may mean
that their source change will not be recompiled even if a build is
triggered by the change.

To overcome this, I add logic in the sbt provided external hooks to
override the zinc analysis stamps. This is done by writing the source
file stamps to the previous cache after compilation completes. This
allows us to see the source differences from sbt's perspective, rather
than zinc's perspective. We then merge the combined differences in the
actual implementation of ExternalHooks. In some cases this may result in
over-compilation but generally over-compilation is preferred to under
compilation. Most of the time, the results should be the same.

The scripted test that I added modifies a file during compilation by
invoking a macro. It then effectively asserts that the file is
recompiled during the next test run by validating the compilation result
in the test. The test fails on the latest develop hash.

main/src/main/scala/sbt/Defaults.scala

@@ -410,6 +410,7 @@ object Defaults extends BuildCommon {
     },
     unmanagedSources := (unmanagedSources / inputFileStamps).value.map(_._1.toFile),
     managedSourceDirectories := Seq(sourceManaged.value),
+    managedSources / outputFileStamper := sbt.nio.FileStamper.Hash,
     managedSources := {
       val stamper = inputFileStamper.value
       val cache = managedFileStampCache.value
@@ -603,7 +604,18 @@ object Defaults extends BuildCommon {
         else ""
       s"inc_compile$extra.zip"
     },
-    incOptions := { incOptions.value.withExternalHooks(ExternalHooks.default.value) },
+    externalHooks := {
+      val current =
+        (unmanagedSources / inputFileStamps).value ++ (managedSources / outputFileStamps).value
+      val previous = (externalHooks / inputFileStamps).previous
+      ExternalHooks.default.value(previous.flatMap(sbt.nio.Settings.changedFiles(_, current)))
+    },
+    externalHooks / inputFileStamps := {
+      compile.value // ensures the inputFileStamps previous value is only set if compile succeeds.
+      (unmanagedSources / inputFileStamps).value ++ (managedSources / outputFileStamps).value
+    },
+    externalHooks / inputFileStamps := (externalHooks / inputFileStamps).triggeredBy(compile).value,
+    incOptions := { incOptions.value.withExternalHooks(externalHooks.value) },
     compileIncSetup := compileIncSetupTask.value,
     console := consoleTask.value,
     collectAnalyses := Definition.collectAnalysesTask.map(_ => ()).value,

main/src/main/scala/sbt/Keys.scala

@@ -212,6 +212,7 @@ object Keys {
   val copyResources = taskKey[Seq[(File, File)]]("Copies resources to the output directory.").withRank(AMinusTask)
   val aggregate = settingKey[Boolean]("Configures task aggregation.").withRank(BMinusSetting)
   val sourcePositionMappers = taskKey[Seq[xsbti.Position => Option[xsbti.Position]]]("Maps positions in generated source files to the original source it was generated from").withRank(DTask)
+  private[sbt] val externalHooks = taskKey[ExternalHooks]("The external hooks used by zinc.")
 
   // package keys
   val packageBin = taskKey[File]("Produces a main artifact, such as a binary jar.").withRank(ATask)

main/src/main/scala/sbt/internal/ExternalHooks.scala

@@ -7,7 +7,7 @@
 
 package sbt.internal
 
-import java.nio.file.Paths
+import java.nio.file.{ Path, Paths }
 import java.util.Optional
 
 import sbt.Def
@@ -16,18 +16,17 @@ import sbt.internal.inc.ExternalLookup
 import sbt.internal.inc.Stamp.equivStamp.equiv
 import sbt.io.syntax._
 import sbt.nio.Keys._
-import sbt.nio.file.RecursiveGlob
 import sbt.nio.file.syntax._
+import sbt.nio.file.{ ChangedFiles, RecursiveGlob }
 import sbt.nio.{ FileStamp, FileStamper }
 import xsbti.compile._
 import xsbti.compile.analysis.Stamp
 
 import scala.collection.JavaConverters._
-import scala.collection.mutable
 
 private[sbt] object ExternalHooks {
   private val javaHome = Option(System.getProperty("java.home")).map(Paths.get(_))
-  def default: Def.Initialize[sbt.Task[ExternalHooks]] = Def.task {
+  def default: Def.Initialize[sbt.Task[Option[ChangedFiles] => ExternalHooks]] = Def.task {
     val unmanagedCache = unmanagedFileStampCache.value
     val managedCache = managedFileStampCache.value
     val cp = dependencyClasspath.value.map(_.data)
@@ -39,9 +38,11 @@ private[sbt] object ExternalHooks {
     fileTreeView.value.list(classGlob).foreach {
       case (path, _) => managedCache.update(path, FileStamper.LastModified)
     }
-    apply((compileOptions in compile).value, unmanagedCache, managedCache)
+    val options = (compileOptions in compile).value
+    apply(_, options, unmanagedCache, managedCache)
   }
   private def apply(
+      changedFiles: Option[ChangedFiles],
       options: CompileOptions,
       unmanagedCache: FileStamp.Cache,
       managedCache: FileStamp.Cache
@@ -53,22 +54,34 @@ private[sbt] object ExternalHooks {
           val getRemoved: java.util.Set[File] = new java.util.HashSet[File]
           val getChanged: java.util.Set[File] = new java.util.HashSet[File]
           val getUnmodified: java.util.Set[File] = new java.util.HashSet[File]
+          private def add(p: Path, sets: java.util.Set[File]*): Unit = {
+            sets.foreach(add(p.toFile, _))
+          }
+          private def add(f: File, set: java.util.Set[File]): Unit = { set.add(f); () }
+          val allChanges = new java.util.HashSet[File]
+          changedFiles foreach {
+            case ChangedFiles(c, d, u) =>
+              c.foreach(add(_, getAdded, allChanges))
+              d.foreach(add(_, getRemoved, allChanges))
+              u.foreach(add(_, getChanged, allChanges))
+          }
           override def isEmpty: java.lang.Boolean =
             getAdded.isEmpty && getRemoved.isEmpty && getChanged.isEmpty
-          val prevSources: mutable.Map[File, Stamp] =
+          private val prevSources = previousAnalysis.readStamps().getAllSourceStamps
-            previousAnalysis.readStamps().getAllSourceStamps.asScala
+          prevSources.forEach { (file: File, s: Stamp) =>
-          prevSources.foreach {
+            if (!allChanges.contains(file)) {
-            case (file: File, s: Stamp) =>
               val path = file.toPath
               unmanagedCache
                 .get(path)
                 .orElse(managedCache.getOrElseUpdate(file.toPath, FileStamper.Hash)) match {
-                case None => getRemoved.add(file)
+                case None => add(file, getRemoved)
                 case Some(stamp) =>
-                  if (equiv(stamp.stamp, s)) getUnmodified.add(file) else getChanged.add(file)
+                  if (equiv(stamp.stamp, s)) add(file, getUnmodified)
+                  else add(file, getChanged)
               }
+            }
           }
-          options.sources.foreach(file => if (!prevSources.contains(file)) getAdded.add(file))
+          options.sources.foreach(file => if (!prevSources.containsKey(file)) getAdded.add(file))
         }
       }
 

main/src/main/scala/sbt/nio/Settings.scala

@@ -251,36 +251,38 @@ private[sbt] object Settings {
   ): Def.Setting[_] =
     addTaskDefinition(changeKey in scopedKey.scope := {
       val current = (stampKey in scopedKey.scope).value
-      (stampKey in scopedKey.scope).previous match {
+      (stampKey in scopedKey.scope).previous.flatMap(changedFiles(_, current))
-        case Some(previous) =>
-          val createdBuilder = new VectorBuilder[Path]
-          val deletedBuilder = new VectorBuilder[Path]
-          val updatedBuilder = new VectorBuilder[Path]
-          val currentMap = current.toMap
-          val prevMap = previous.toMap
-          current.foreach {
-            case (path, currentStamp) =>
-              prevMap.get(path) match {
-                case Some(oldStamp) => if (oldStamp != currentStamp) updatedBuilder += path
-                case None           => createdBuilder += path
-              }
-          }
-          previous.foreach {
-            case (path, _) =>
-              if (currentMap.get(path).isEmpty) deletedBuilder += path
-          }
-          val created = createdBuilder.result()
-          val deleted = deletedBuilder.result()
-          val updated = updatedBuilder.result()
-          if (created.isEmpty && deleted.isEmpty && updated.isEmpty) {
-            None
-          } else {
-            val cf = ChangedFiles(created = created, deleted = deleted, updated = updated)
-            Some(cf)
-          }
-        case None => None
-      }
     })
+  private[sbt] def changedFiles(
+      previous: Seq[(Path, FileStamp)],
+      current: Seq[(Path, FileStamp)]
+  ): Option[ChangedFiles] = {
+    val createdBuilder = new VectorBuilder[Path]
+    val deletedBuilder = new VectorBuilder[Path]
+    val updatedBuilder = new VectorBuilder[Path]
+    val currentMap = current.toMap
+    val prevMap = previous.toMap
+    current.foreach {
+      case (path, currentStamp) =>
+        prevMap.get(path) match {
+          case Some(oldStamp) => if (oldStamp != currentStamp) updatedBuilder += path
+          case None           => createdBuilder += path
+        }
+    }
+    previous.foreach {
+      case (path, _) =>
+        if (currentMap.get(path).isEmpty) deletedBuilder += path
+    }
+    val created = createdBuilder.result()
+    val deleted = deletedBuilder.result()
+    val updated = updatedBuilder.result()
+    if (created.isEmpty && deleted.isEmpty && updated.isEmpty) {
+      None
+    } else {
+      val cf = ChangedFiles(created = created, deleted = deleted, updated = updated)
+      Some(cf)
+    }
+  }
 
   /**
    * Provides an automatically generated clean method for a task that provides fileOutputs.

sbt/src/sbt-test/nio/external-hooks/build.sbt

@@ -0,0 +1,23 @@
+val generateSourceFile = taskKey[Unit]("generate source file")
+generateSourceFile := {
+  val testDir = ((Test / scalaSource).value.toPath / "Foo.scala").toString
+  val content = s"object Foo { val x = 2 }"
+  val src =
+    s"""
+       |import scala.language.experimental.macros
+       |import scala.reflect.macros.blackbox
+       |import java.nio.file.{ Files, Paths }
+       |
+       |object Generate {
+       |  def gen: Unit = macro genImpl
+       |  def genImpl(c: blackbox.Context): c.Expr[Unit] = {
+       |    Files.write(Paths.get("${testDir.replace("\\", "\\\\")}"), "$content".getBytes)
+       |    c.universe.reify(())
+       |  }
+       |}
+       |""".stripMargin
+  IO.write((Compile / scalaSource).value / "Generate.scala", src)
+}
+
+libraryDependencies += "org.scala-lang" % "scala-reflect" % scalaVersion.value
+libraryDependencies += "org.scalatest" %% "scalatest" % "3.0.5" % "test"

sbt/src/sbt-test/nio/external-hooks/src/test/scala/Foo.scala

@@ -0,0 +1,3 @@
+object Foo {
+  def x = 1
+}

sbt/src/sbt-test/nio/external-hooks/src/test/scala/FooTest.scala

@@ -0,0 +1,8 @@
+import org.scalatest.FlatSpec
+
+class FooTest extends FlatSpec {
+  Generate.gen
+  it should "work" in {
+    assert(Foo.x == 2)
+  }
+}

sbt/src/sbt-test/nio/external-hooks/test

@@ -0,0 +1,5 @@
+> generateSourceFile
+
+-> test
+
+> test

sbt/src/sbt-test/source-dependencies/restore-classes/build.sbt

@@ -14,6 +14,7 @@ recordPreviousIterations := {
         log.info("No previous analysis detected")
         0
       case Some(a: Analysis) => a.compilations.allCompilations.size
+      case Some(_) => -1 // should be unreachable but causes warnings
     }
   }
 }