From 07993b4f781735a0e8a0f5cf19e33c6eea370409 Mon Sep 17 00:00:00 2001 From: Brice Jaglin Date: Fri, 10 Jul 2020 15:23:27 +0200 Subject: [PATCH] caching: don't forget previous invocations This allows cache accumulation after successive runs of scalafix with different rules, which is common as it's currently the only way to safely run several semantic rules in order. Note that the cache remains incremental only for files, so if a file is stale for a single rule, all rules will be executed on this file. --- .../scala/scalafix/sbt/ScalafixPlugin.scala | 62 ++++++++++++++----- src/sbt-test/skip-windows/caching/test | 13 ++++ 2 files changed, 58 insertions(+), 17 deletions(-) diff --git a/src/main/scala/scalafix/sbt/ScalafixPlugin.scala b/src/main/scala/scalafix/sbt/ScalafixPlugin.scala index 84c37df5..d6ff126e 100644 --- a/src/main/scala/scalafix/sbt/ScalafixPlugin.scala +++ b/src/main/scala/scalafix/sbt/ScalafixPlugin.scala @@ -457,8 +457,7 @@ object ScalafixPlugin extends AutoPlugin { throw StampingImpossible case _ => } - // don't stamp rules explicitly requested on the CLI but those which will actually run - write(interface.rulesThatWillRun().map(_.name())) + // don't stamp rules as inputs: outputs are stamped by rule case Arg.Config(maybeFile) => maybeFile match { case Some(path) => @@ -490,38 +489,67 @@ object ScalafixPlugin extends AutoPlugin { } } - def diffWithPreviousRun[T](f: (Boolean, ChangeReport[File]) => T): T = { + def diffWithPreviousRuns[T](f: (Boolean, Set[File]) => T): T = { val tracker = Tracked.inputChanged(streams.cacheDirectory / "args") { (argsChanged: Boolean, _: Seq[Arg.CacheKey]) => - val diffInputs: Difference = Tracked.diffInputs( - streams.cacheDirectory / "targets", - lastModifiedStyle - ) - diffInputs(paths.map(_.toFile).toSet) { - diffTargets: ChangeReport[File] => - f(argsChanged, diffTargets) - } + val targets = paths.map(_.toFile).toSet + + // Stamp targets before execution of the provided function, and persist + // the result only on successful execution of the provided function + def diffTargets( + rule: String + )(doForStaleTargets: Set[File] => T): T = + Tracked.diffInputs( + streams.cacheDirectory / "rules" / rule, + lastModifiedStyle + )(targets) { changeReport: ChangeReport[File] => + doForStaleTargets(changeReport.modified -- changeReport.removed) + } + + // Execute the callback function against the accumulated files that cache-miss for at least one rule + def accumulateAndRunForStaleTargets( + ruleTargetsDiffs: List[(Set[File] => T) => T], + acc: Set[File] = Set.empty + ): T = + ruleTargetsDiffs match { + case Nil => f(argsChanged, acc) + case targetsDiff :: tail => + targetsDiff { ruleStaleTargets => + // This cannot be @tailrec as it must nest function calls, so that nothing is persisted + // in case of exception during evaluation of the one and only effect (the terminal callback + // above). Stack depth is limited to the number of requested rules to run, so it should + // be fine... + accumulateAndRunForStaleTargets( + tail, + acc ++ ruleStaleTargets + ) + } + } + + val ruleTargetDiffs = interface.rulesThatWillRun + .map(rule => diffTargets(rule.name) _) + .toList + accumulateAndRunForStaleTargets(ruleTargetDiffs) } Try(tracker(cacheKeyArgs)).recover { // in sbt 1.x, this is not necessary as any exception thrown during stamping is already silently ignored, // but having this here helps keeping code as common as possible // https://github.com/sbt/util/blob/v1.0.0/util-tracking/src/main/scala/sbt/util/Tracked.scala#L180 - case _ @StampingImpossible => f(true, new EmptyChangeReport()) + case _ @StampingImpossible => f(true, Set.empty) }.get } - diffWithPreviousRun { (cacheKeyArgsChanged, diffTargets) => + diffWithPreviousRuns { (cacheKeyArgsChanged, staleTargets) => val errors = if (cacheKeyArgsChanged) { streams.log.info(s"Running scalafix on ${paths.size} Scala sources") interface.run() } else { - val dirtyTargets = diffTargets.modified -- diffTargets.removed - if (dirtyTargets.nonEmpty) { + if (staleTargets.nonEmpty) { streams.log.info( - s"Running scalafix on ${dirtyTargets.size} Scala sources (incremental)" + s"Running scalafix on ${staleTargets.size} Scala sources (incremental)" ) interface - .withArgs(Arg.Paths(dirtyTargets.map(_.toPath).toSeq)) + .withArgs(Arg.Paths(staleTargets.map(_.toPath).toSeq)) .run() } else { streams.log.debug(s"already ran on ${paths.length} files") diff --git a/src/sbt-test/skip-windows/caching/test b/src/sbt-test/skip-windows/caching/test index 0ea7b658..d2a07ada 100644 --- a/src/sbt-test/skip-windows/caching/test +++ b/src/sbt-test/skip-windows/caching/test @@ -106,6 +106,19 @@ $ exec chmod 000 src/main/scala/InitiallyValid.scala -> scalafix --check ProcedureSyntax $ delete src/main/scala +# files should not be re-checked if all requested rules have previously been run across invocations +> set scalafixConfig := Some(file("files/DisableSyntaxVar.scalafix.conf")) +$ mkdir src/test/scala +$ copy-file files/Valid.scala src/test/scala/Valid.scala +> test:scalafix ProcedureSyntax +> test:scalafix --check +$ exec chmod 000 src/test/scala/Valid.scala +> test:scalafix --check ProcedureSyntax DisableSyntax +> test:scalafix --syntactic ProcedureSyntax +> test:scalafix DisableSyntax ProcedureSyntax +> test:scalafix +$ delete src/test/scala + # files should be re-checked after updating the configuration (even if the rule is the same) > set scalafixConfig := Some(file("files/DisableSyntaxVar.scalafix.conf")) $ mkdir src/main/scala