From 31f232c84ae58174213e2a8920fa7ab36a72bd33 Mon Sep 17 00:00:00 2001 From: BrianHotopp Date: Fri, 24 Jul 2026 02:55:33 -0400 Subject: [PATCH] [2.x] fix: Register every Def.declareOutput execution, not one per call site (#9492) The cached-task macro allocated one mutable slot per syntactic Def.declareOutput / Def.declareOutputDirectory call site and snapshotted the slots into the task's outputs after the body ran. A call inside a loop or .map over a runtime-determined list is a single syntactic site executed many times, so each iteration overwrote the same slot and only the last file was cached and restored on a cache hit. There was also no way for a conditional call site that did not execute to stay out of the outputs: its slot remained null. Declared outputs now accumulate in a per-task ListBuffer: the macro emits one buffer at the top of the cached body and rewrites each call site to ActionCache.registerOutput(vf, buffer), which appends and returns the value. Every execution registers, an unexecuted site contributes nothing, and the static multi-site shape is unchanged. Refs #9462 (the declareOutput-in-a-loop half) Co-authored-by: Claude Opus 4.8 (1M context) Co-authored-by: eugene yokota --- build.sbt | 6 +- .../sbt/internal/util/appmacro/Cont.scala | 68 ++++++++++++------- .../internal/util/appmacro/ContextUtil.scala | 23 +------ notes/2.0.0/declare-output-in-loop.md | 13 ++++ .../cache/declare-output-loop/build.sbt | 66 ++++++++++++++++++ .../sbt-test/cache/declare-output-loop/test | 10 +++ .../src/main/scala/sbt/util/ActionCache.scala | 9 +++ 7 files changed, 148 insertions(+), 47 deletions(-) create mode 100644 notes/2.0.0/declare-output-in-loop.md create mode 100644 sbt-app/src/sbt-test/cache/declare-output-loop/build.sbt create mode 100644 sbt-app/src/sbt-test/cache/declare-output-loop/test 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,