diff --git a/build.sbt b/build.sbt index d26763b4c..50a47303b 100644 --- a/build.sbt +++ b/build.sbt @@ -670,8 +670,12 @@ lazy val coreMacrosProj = (project in file("core-macros")) SettingKey[Boolean]("exportPipelining") := false, mimaSettings, mimaBinaryIssueFilters ++= Seq( - exclude[ReversedMissingMethodProblem]("sbt.internal.util.appmacro.ContextUtil.*"), + // macro-expansion internals; Output's per-call-site var codegen members were removed + ProblemFilters.exclude[DirectMissingMethodProblem]( + "sbt.internal.util.appmacro.ContextUtil#Output.*" + ), exclude[DirectMissingMethodProblem]("sbt.internal.util.appmacro.ContextUtil#Input.*"), + exclude[ReversedMissingMethodProblem]("sbt.internal.util.appmacro.ContextUtil.*"), ), ) diff --git a/core-macros/src/main/scala/sbt/internal/util/appmacro/Cont.scala b/core-macros/src/main/scala/sbt/internal/util/appmacro/Cont.scala index dfdf1dc5b..8393c6d91 100644 --- a/core-macros/src/main/scala/sbt/internal/util/appmacro/Cont.scala +++ b/core-macros/src/main/scala/sbt/internal/util/appmacro/Cont.scala @@ -192,6 +192,16 @@ trait Cont: val inputBuf = ListBuffer[Input]() val outputBuf = ListBuffer[Output]() + lazy val outputAccSym: Symbol = + Symbol.newVal( + Symbol.spliceOwner, + freshName("outputs"), + TypeRepr.of[ListBuffer[VirtualFile]], + Flags.EmptyFlags, + Symbol.noSymbol + ) + def outputAccRef: Expr[ListBuffer[VirtualFile]] = + Ref(outputAccSym).asExprOf[ListBuffer[VirtualFile]] def unitExpr: Expr[Unit] = '{ () } @@ -405,28 +415,33 @@ trait Cont: } // This will generate following code for Def.declareOutput(...): - // var $o1: VirtualFile = null - // ActionCache.ActionResult({ + // val $outputs = ListBuffer.empty[VirtualFile] + // ActionCache.InternalActionResult({ // body... - // $o1 = out // Def.declareOutput(out) + // ActionCache.registerOutput(out, $outputs) // Def.declareOutput(out) // result - // }, List($o1)) + // }, $outputs.toList) def letOutput[A1: Type]( outputs: List[Output], cacheConfigExpr: Expr[BuildWideCacheConfiguration], )(body: Expr[A1]): Expr[ActionCache.InternalActionResult[A1]] = - Block( - outputs.map(_.toVarDef), + if outputs.isEmpty then '{ ActionCache.InternalActionResult( value = $body, - outputs = List(${ - Varargs[VirtualFile](outputs.map: out => - out.toRef.asExprOf[VirtualFile]) - }*), + outputs = Nil, ) - }.asTerm - ).asExprOf[ActionCache.InternalActionResult[A1]] + } + else + Block( + ValDef(outputAccSym, Some('{ ListBuffer.empty[VirtualFile] }.asTerm)) :: Nil, + '{ + ActionCache.InternalActionResult( + value = $body, + outputs = $outputAccRef.toList, + ) + }.asTerm + ).asExprOf[ActionCache.InternalActionResult[A1]] val WrapOutputName = "wrapOutput_\u2603\u2603" val WrapOutputDirectoryName = "wrapOutputDirectory_\u2603\u2603" @@ -442,12 +457,16 @@ trait Cont: val output = Output( tpe = TypeRepr.of[a], term = qual, - name = freshName("o"), - parent = Symbol.spliceOwner, - outputType = OutputType.File + outputType = OutputType.File, ) outputBuf += output - if cacheConfigExprOpt.isDefined then output.toAssign(output.term) + if cacheConfigExprOpt.isDefined then + '{ + ActionCache.registerOutput( + ${ output.term.asExprOf[VirtualFile] }, + $outputAccRef, + ) + }.asTerm else oldTree case WrapOutputDirectoryName => val output = Output( @@ -455,20 +474,21 @@ trait Cont: // which contains hash. tpe = TypeRepr.of[VirtualFile], term = qual, - name = freshName("o"), - parent = Symbol.spliceOwner, outputType = OutputType.Directory, ) outputBuf += output cacheConfigExprOpt match case Some(cacheConfigExpr) => - output.toAssign('{ - ActionCache.packageDirectory( - dir = ${ output.term.asExprOf[VirtualFileRef] }, - conv = $cacheConfigExpr.fileConverter, - outputDirectory = $cacheConfigExpr.outputDirectory, + '{ + ActionCache.registerOutput( + ActionCache.packageDirectory( + dir = ${ output.term.asExprOf[VirtualFileRef] }, + conv = $cacheConfigExpr.fileConverter, + outputDirectory = $cacheConfigExpr.outputDirectory, + ), + $outputAccRef, ) - }.asTerm) + }.asTerm case None => oldTree case _ => inputBuf += Input( diff --git a/core-macros/src/main/scala/sbt/internal/util/appmacro/ContextUtil.scala b/core-macros/src/main/scala/sbt/internal/util/appmacro/ContextUtil.scala index 2b65e4f76..2d1414553 100644 --- a/core-macros/src/main/scala/sbt/internal/util/appmacro/ContextUtil.scala +++ b/core-macros/src/main/scala/sbt/internal/util/appmacro/ContextUtil.scala @@ -157,31 +157,10 @@ trait ContextUtil[C <: Quotes & scala.Singleton](val valStart: Int): final class Output( val tpe: TypeRepr, val term: Term, - val name: String, - val parent: Symbol, val outputType: OutputType, ): override def toString: String = - s"Output($tpe, $term, $name, $outputType)" - val placeholder: Symbol = - tpe.asType match - case '[a] => - Symbol.newVal( - parent, - name, - tpe, - Flags.Mutable, - Symbol.noSymbol - ) - def toVarDef: ValDef = - ValDef(placeholder, rhs = Some('{ null }.asTerm)) - def toAssign(value: Term): Term = - Block( - Assign(toRef, value) :: Nil, - toRef - ) - def toRef: Ref = Ref(placeholder) - def isFile: Boolean = outputType == OutputType.File + s"Output($tpe, $term, $outputType)" end Output def applyTuple(tupleTerm: Term, tpe: TypeRepr, idx: Int): Term = diff --git a/notes/2.0.0/declare-output-in-loop.md b/notes/2.0.0/declare-output-in-loop.md new file mode 100644 index 000000000..fb788352a --- /dev/null +++ b/notes/2.0.0/declare-output-in-loop.md @@ -0,0 +1,13 @@ +### Every `Def.declareOutput` call registers, including inside loops + +The cached-task macro allocated one slot per syntactic `Def.declareOutput` (or +`Def.declareOutputDirectory`) call site, so a call inside a loop or `.map` over a +runtime-determined list of files overwrote the same slot on every iteration and +only the last file was cached and restored. Declared outputs now accumulate per +execution, so a dynamic number of outputs declared from one call site all +survive a cache hit. A `declareOutput` in a conditional branch that is not taken +no longer contributes a null entry to the task's outputs either. + +This addresses the loop half of [#9462][i9462]. + +[i9462]: https://github.com/sbt/sbt/issues/9462 diff --git a/sbt-app/src/sbt-test/cache/declare-output-loop/build.sbt b/sbt-app/src/sbt-test/cache/declare-output-loop/build.sbt new file mode 100644 index 000000000..69248e35a --- /dev/null +++ b/sbt-app/src/sbt-test/cache/declare-output-loop/build.sbt @@ -0,0 +1,66 @@ +import sbt.internal.util.CacheEventSummary +import xsbti.HashedVirtualFileRef + +val declareLoop = taskKey[Seq[HashedVirtualFileRef]]("declares 3 files via .map over a runtime list") +val checkAll = taskKey[Unit]("asserts all 3 files exist") +val delFiles = taskKey[Unit]("deletes the 3 files") +val checkNone = taskKey[Unit]("asserts none of the 3 files exist") +val checkHit = taskKey[Unit]("asserts previous command was a pure cache hit") + +Global / localCacheDirectory := baseDirectory.value / "diskcache" + +lazy val declareOutputLoop = project.in(file(".")) + +declareLoop := { + val log = streams.value.log + val dir = target.value / "gen-multi" + IO.createDirectory(dir) + val files = List(dir / "a.txt", dir / "b.txt", dir / "c.txt") + IO.write(files(0), "AAA") + IO.write(files(1), "BBB") + IO.write(files(2), "CCC") + log.info(s"COMPUTED declareLoop (cache miss)") + if (sys.props.contains("never.set.property")) { + val ghost = fileConverter.value.toVirtualFile((dir / "never.txt").toPath) + val _ = Def.declareOutput(ghost) + } + files.map { f => + val vf = fileConverter.value.toVirtualFile(f.toPath) + Def.declareOutput(vf) + } +} + +def listing(dir: File): String = + if (dir.exists) (dir ** "*").get().mkString(", ") else "" + +checkAll := Def.uncached { + val dir = target.value / "gen-multi" + streams.value.log.info(s"gen-multi listing: ${listing(dir)}") + assert((dir / "a.txt").exists, s"a.txt missing under $dir") + assert((dir / "b.txt").exists, s"b.txt missing under $dir") + assert((dir / "c.txt").exists, s"c.txt missing under $dir") +} + +delFiles := Def.uncached { + val dir = target.value / "gen-multi" + IO.delete(Seq(dir / "a.txt", dir / "b.txt", dir / "c.txt")) + streams.value.log.info(s"deleted files under $dir") +} + +checkNone := Def.uncached { + val dir = target.value / "gen-multi" + assert( + !(dir / "a.txt").exists && !(dir / "b.txt").exists && !(dir / "c.txt").exists, + s"files still present under $dir" + ) +} + +checkHit := Def.uncached { + val config = Def.cacheConfiguration.value + val prev = config.cacheEventLog.previous match + case s: CacheEventSummary.Data => s + case _ => sys.error("empty event log") + streams.value.log.info(s"prev hitCount=${prev.hitCount} missCount=${prev.missCount}") + assert(prev.missCount == 0, s"expected pure hit but missCount=${prev.missCount}") + assert(prev.hitCount >= 1, s"expected a hit but hitCount=${prev.hitCount}") +} diff --git a/sbt-app/src/sbt-test/cache/declare-output-loop/test b/sbt-app/src/sbt-test/cache/declare-output-loop/test new file mode 100644 index 000000000..bba463933 --- /dev/null +++ b/sbt-app/src/sbt-test/cache/declare-output-loop/test @@ -0,0 +1,10 @@ +# Regression for #9462: every execution of a Def.declareOutput call site must register, +# including calls inside a .map over a runtime-determined list. The task also contains a +# declareOutput in a never-taken branch, which must contribute nothing. +> declareLoop +> checkAll +> delFiles +> checkNone +> declareLoop +> checkHit +> checkAll diff --git a/util-cache/src/main/scala/sbt/util/ActionCache.scala b/util-cache/src/main/scala/sbt/util/ActionCache.scala index 143de2904..292fdadff 100644 --- a/util-cache/src/main/scala/sbt/util/ActionCache.scala +++ b/util-cache/src/main/scala/sbt/util/ActionCache.scala @@ -33,6 +33,7 @@ import sbt.nio.file.syntax.* import sbt.util.CacheImplicits import scala.reflect.ClassTag import scala.annotation.{ meta, StaticAnnotation } +import scala.collection.mutable import scala.util.control.NonFatal import sjsonnew.{ HashWriter, JsonFormat } import sjsonnew.support.murmurhash.Hasher @@ -374,6 +375,14 @@ object ActionCache: Files.move(staging, destZip, StandardCopyOption.REPLACE_EXISTING) finally Files.deleteIfExists(staging) + /** Appends a declared output; called from code generated by the cached-task macro. */ + def registerOutput( + vf: VirtualFile, + outputs: mutable.ListBuffer[VirtualFile], + ): VirtualFile = + outputs += vf + vf + def packageDirectory( dir: VirtualFileRef, conv: FileConverter,