From 655b5634532ad87aa60c13b1569bb4769e051228 Mon Sep 17 00:00:00 2001 From: eugene yokota Date: Tue, 27 Aug 2019 05:59:25 -0400 Subject: [PATCH] Fix false positive and false negative cache hits (take 2) (#46) --- .../org/scalafmt/sbt/ScalafmtPlugin.scala | 246 +++++++++++------- .../src/sbt-test/scalafmt-sbt/sbt/build.sbt | 3 + .../sbt/changes/Dependencies.scala | 5 + .../scalafmt-sbt/sbt/changes/bad.scala | 6 + .../scalafmt-sbt/sbt/changes/good.scala | 6 + .../sbt/p17/src/main/scala/Test.scala | 7 + plugin/src/sbt-test/scalafmt-sbt/sbt/test | 35 ++- 7 files changed, 209 insertions(+), 99 deletions(-) create mode 100644 plugin/src/sbt-test/scalafmt-sbt/sbt/changes/Dependencies.scala create mode 100644 plugin/src/sbt-test/scalafmt-sbt/sbt/changes/bad.scala create mode 100644 plugin/src/sbt-test/scalafmt-sbt/sbt/changes/good.scala create mode 100644 plugin/src/sbt-test/scalafmt-sbt/sbt/p17/src/main/scala/Test.scala diff --git a/plugin/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala b/plugin/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala index e15fb23..9b679c1 100644 --- a/plugin/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala +++ b/plugin/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala @@ -7,6 +7,7 @@ import sbt.Keys._ import sbt.Def import sbt._ import complete.DefaultParsers._ +import sbt.util.CacheImplicits._ import sbt.util.CacheStoreFactory import sbt.util.FileInfo import sbt.util.FilesInfo @@ -57,8 +58,19 @@ object ScalafmtPlugin extends AutoPlugin { "(By default this means the Compile and Test configurations.)" ) } + import autoImport._ + case class ScalafmtAnalysis(failedScalafmtCheck: Set[File]) + object ScalafmtAnalysis { + import sjsonnew.{:*:, LList, LNil} + implicit val analysisIso = LList.iso({ a: ScalafmtAnalysis => + ("failedScalafmtCheck", a.failedScalafmtCheck) :*: LNil + }, { in: Set[File] :*: LNil => + ScalafmtAnalysis(in.head) + }) + } + private val scalafmtDoFormatOnCompile = taskKey[Unit]("Format Scala source files if scalafmtOnCompile is on.") @@ -84,7 +96,6 @@ object ScalafmtPlugin extends AutoPlugin { log: Logger, writer: PrintWriter )( - onError: (File, Throwable) => T, onFormat: (File, Input, Output) => T ): Seq[Option[T]] = { val reporter = new ScalafmtSbtReporter(log, writer) @@ -103,32 +114,38 @@ object ScalafmtPlugin extends AutoPlugin { } private def formatSources( - cacheDirectory: File, + cacheStoreFactory: CacheStoreFactory, sources: Seq[File], config: Path, log: Logger, writer: PrintWriter ): Unit = { - cached(cacheDirectory, FilesInfo.lastModified, config) { cacheMisses => - val changed = cacheMisses.filter(_.exists) - if (changed.size > 0) { - log.info(s"Formatting ${changed.size} Scala sources...") - formatSources(changed.toSeq, config, log, writer) - } - }(sources.toSet).getOrElse(()) + trackSourcesAndConfig(cacheStoreFactory, sources, config) { + (outDiff, configChanged, prev) => + log.debug(outDiff.toString) + val updatedOrAdded = outDiff.modified & outDiff.checked + val filesToFormat: Set[File] = + if (configChanged) sources.toSet + else { + // in addition to the detected changes, process files that failed scalafmtCheck + // we can ignore the succeeded files because, they don't require reformatting + updatedOrAdded | prev.failedScalafmtCheck + } + if (filesToFormat.nonEmpty) { + log.info(s"Formatting ${filesToFormat.size} Scala sources...") + formatSources(filesToFormat, config, log, writer) + } + ScalafmtAnalysis(Set.empty) + } } private def formatSources( - sources: Seq[File], + sources: Set[File], config: Path, log: Logger, writer: PrintWriter ): Unit = { - val cnt = withFormattedSources(sources, config, log, writer)( - (file, e) => { - log.err(e.toString) - 0 - }, + val cnt = withFormattedSources(sources.toSeq, config, log, writer)( (file, input, output) => { if (input != output) { IO.write(file, output) @@ -146,21 +163,42 @@ object ScalafmtPlugin extends AutoPlugin { } private def checkSources( - cacheDirectory: File, + cacheStoreFactory: CacheStoreFactory, sources: Seq[File], config: Path, log: Logger, writer: PrintWriter - ): Boolean = { - cached(cacheDirectory, FilesInfo.lastModified, config) { cacheMisses => - val changed = cacheMisses.filter(_.exists) - if (changed.size > 0) { - log.info(s"Checking ${changed.size} Scala sources...") - checkSources(changed.toSeq, config, log, writer) - } else { - true - } - }(sources.toSet).getOrElse(true) + ): ScalafmtAnalysis = { + trackSourcesAndConfig(cacheStoreFactory, sources, config) { + (outDiff, configChanged, prev) => + log.debug(outDiff.toString) + val updatedOrAdded = outDiff.modified & outDiff.checked + val filesToCheck: Set[File] = + if (configChanged) sources.toSet + else updatedOrAdded + val prevFailed: Set[File] = + if (configChanged) Set.empty + else prev.failedScalafmtCheck & outDiff.unmodified + prevFailed foreach { warnBadFormat(_, log) } + val result = checkSources(filesToCheck.toSeq, config, log, writer) + prev.copy( + failedScalafmtCheck = result.failedScalafmtCheck | prevFailed + ) + } + } + + private def trueOrBoom(analysis: ScalafmtAnalysis): Boolean = { + val failureCount = analysis.failedScalafmtCheck.size + if (failureCount > 0) { + throw new MessageOnlyException( + s"${failureCount} files must be formatted" + ) + } + true + } + + private def warnBadFormat(file: File, log: Logger): Unit = { + log.warn(s"${file.toString} isn't formatted properly!") } private def checkSources( @@ -168,113 +206,126 @@ object ScalafmtPlugin extends AutoPlugin { config: Path, log: Logger, writer: PrintWriter - ): Boolean = { - val unformattedCount = withFormattedSources(sources, config, log, writer)( - (file, e) => { - log.err(e.toString) - false - }, + ): ScalafmtAnalysis = { + if (sources.nonEmpty) { + log.info(s"Checking ${sources.size} Scala sources...") + } + val unformatted = withFormattedSources(sources, config, log, writer)( (file, input, output) => { val diff = input != output if (diff) { - log.warn(s"${file.toString} isn't formatted properly!") - } - !diff + warnBadFormat(file, log) + Some(file) + } else None } - ).flatten.count(x => !x) - if (unformattedCount > 0) { - throw new MessageOnlyException( - s"${unformattedCount} files must be formatted" - ) - } - unformattedCount == 0 + ).flatten.flatten.toSet + ScalafmtAnalysis(failedScalafmtCheck = unformatted) } - private def cached[T]( - cacheBaseDirectory: File, - outStyle: FileInfo.Style, + // This tracks + // 1. previous value + // 2. changes to the config file + // 3. changes to source and their last modified dates after the operation + // The tracking is shared between scalafmt and scalafmtCheck + private def trackSourcesAndConfig( + cacheStoreFactory: CacheStoreFactory, + sources: Seq[File], config: Path )( - action: Set[File] => T - ): Set[File] => Option[T] = { - lazy val outCache = Difference.outputs( - CacheStoreFactory(cacheBaseDirectory).make("out-cache"), - outStyle - ) - inputs => { - outCache(inputs + config.toFile) { outReport => - val updatedOrAdded = outReport.modified -- outReport.removed - if (!updatedOrAdded.isEmpty) { - val cacheMisses = updatedOrAdded.intersect(inputs) - if (!cacheMisses.isEmpty) { - // incremental run - Some(action(cacheMisses)) - } else { - // config file must have changed, rerun everything - Some(action(inputs)) + f: (ChangeReport[File], Boolean, ScalafmtAnalysis) => ScalafmtAnalysis + ): ScalafmtAnalysis = { + // use prevTracker to share previous values between tasks + val prevTracker = Tracked.lastOutput[Unit, ScalafmtAnalysis]( + cacheStoreFactory.make("last") + ) { (_, prev0) => + val prev = prev0.getOrElse(ScalafmtAnalysis(Set.empty)) + val tracker = Tracked.inputChanged[HashFileInfo, ScalafmtAnalysis]( + cacheStoreFactory.make("config") + ) { + case (configChanged, configHash) => + Tracked.diffOutputs( + cacheStoreFactory.make("output-diff"), + FileInfo.lastModified + )(sources.toSet) { (outDiff: ChangeReport[File]) => + f(outDiff, configChanged, prev) } - } else { - None - } } + tracker(FileInfo.hash(config.toFile)) } + prevTracker(()) } - private lazy val sbtSources = thisProject.map { proj => + private lazy val sbtSources = Def.task { + val rootBase = (LocalRootProject / baseDirectory).value + val thisBase = (thisProject.value).base val rootSbt = - BuildPaths.configurationSources(proj.base).filterNot(_.isHidden) - val projectSbt = - (BuildPaths.projectStandard(proj.base) * GlobFilter("*.sbt")).get - .filterNot(_.isHidden) - rootSbt ++ projectSbt + BuildPaths.configurationSources(thisBase).filterNot(_.isHidden) + val metabuildSbt = + if (rootBase == thisBase) + (BuildPaths.projectStandard(thisBase) ** GlobFilter("*.sbt")).get + else Nil + rootSbt ++ metabuildSbt } - private lazy val projectSources = thisProject.map { proj => - (BuildPaths.projectStandard(proj.base) * GlobFilter("*.scala")).get + + private lazy val metabuildSources = Def.task { + val rootBase = (LocalRootProject / baseDirectory).value + val thisBase = (thisProject.value).base + if (rootBase == thisBase) + (BuildPaths.projectStandard(thisBase) ** GlobFilter("*.scala")).get + else Nil } lazy val scalafmtConfigSettings: Seq[Def.Setting[_]] = Seq( - scalafmt := formatSources( - streams.value.cacheDirectory, - (unmanagedSources in scalafmt).value, - scalaConfig.value, - streams.value.log, - streams.value.text() - ), + scalafmt := { + formatSources( + streams.value.cacheStoreFactory, + (unmanagedSources in scalafmt).value, + scalaConfig.value, + streams.value.log, + streams.value.text() + ) + }, scalafmtIncremental := scalafmt.value, scalafmtSbt := { formatSources( - sbtSources.value, + sbtSources.value.toSet, sbtConfig.value, streams.value.log, streams.value.text() ) formatSources( - projectSources.value, + metabuildSources.value.toSet, scalaConfig.value, streams.value.log, streams.value.text() ) }, - scalafmtCheck := - checkSources( - (streams in scalafmt).value.cacheDirectory, + scalafmtCheck := { + val analysis = checkSources( + (scalafmt / streams).value.cacheStoreFactory, (unmanagedSources in scalafmt).value, scalaConfig.value, streams.value.log, streams.value.text() - ), + ) + trueOrBoom(analysis) + }, scalafmtSbtCheck := { - checkSources( - sbtSources.value, - sbtConfig.value, - streams.value.log, - streams.value.text() + trueOrBoom( + checkSources( + sbtSources.value, + sbtConfig.value, + streams.value.log, + streams.value.text() + ) ) - checkSources( - projectSources.value, - scalaConfig.value, - streams.value.log, - streams.value.text() + trueOrBoom( + checkSources( + metabuildSources.value, + scalaConfig.value, + streams.value.log, + streams.value.text() + ) ) }, scalafmtDoFormatOnCompile := Def.settingDyn { @@ -300,7 +351,7 @@ object ScalafmtPlugin extends AutoPlugin { // scalaConfig formatSources( - absFiles, + absFiles.toSet, scalaConfig.value, streams.value.log, streams.value.text() @@ -328,4 +379,5 @@ object ScalafmtPlugin extends AutoPlugin { Seq( scalafmtOnCompile := false ) + } diff --git a/plugin/src/sbt-test/scalafmt-sbt/sbt/build.sbt b/plugin/src/sbt-test/scalafmt-sbt/sbt/build.sbt index 5a7e168..8b1b82d 100644 --- a/plugin/src/sbt-test/scalafmt-sbt/sbt/build.sbt +++ b/plugin/src/sbt-test/scalafmt-sbt/sbt/build.sbt @@ -65,6 +65,9 @@ lazy val p16 = project.settings( scalaVersion := "2.12.1", scalafmtConfig := file(".scalafmt16.conf") ) +lazy val p17 = project.settings( + scalaVersion := "2.12.1", +) def assertContentsEqual(file: File, expected: String): Unit = { val obtained = diff --git a/plugin/src/sbt-test/scalafmt-sbt/sbt/changes/Dependencies.scala b/plugin/src/sbt-test/scalafmt-sbt/sbt/changes/Dependencies.scala new file mode 100644 index 0000000..6f11c06 --- /dev/null +++ b/plugin/src/sbt-test/scalafmt-sbt/sbt/changes/Dependencies.scala @@ -0,0 +1,5 @@ +object +Dependencies +{ + +} diff --git a/plugin/src/sbt-test/scalafmt-sbt/sbt/changes/bad.scala b/plugin/src/sbt-test/scalafmt-sbt/sbt/changes/bad.scala new file mode 100644 index 0000000..d3fdd4f --- /dev/null +++ b/plugin/src/sbt-test/scalafmt-sbt/sbt/changes/bad.scala @@ -0,0 +1,6 @@ +object +Test2 +{ + def foo2(a: Int, // comment + b: Double) = ??? +} diff --git a/plugin/src/sbt-test/scalafmt-sbt/sbt/changes/good.scala b/plugin/src/sbt-test/scalafmt-sbt/sbt/changes/good.scala new file mode 100644 index 0000000..b10f4cf --- /dev/null +++ b/plugin/src/sbt-test/scalafmt-sbt/sbt/changes/good.scala @@ -0,0 +1,6 @@ +object Test2 { + def foo2( + a: Int, // comment + b: Double + ) = ??? +} diff --git a/plugin/src/sbt-test/scalafmt-sbt/sbt/p17/src/main/scala/Test.scala b/plugin/src/sbt-test/scalafmt-sbt/sbt/p17/src/main/scala/Test.scala new file mode 100644 index 0000000..23472be --- /dev/null +++ b/plugin/src/sbt-test/scalafmt-sbt/sbt/p17/src/main/scala/Test.scala @@ -0,0 +1,7 @@ +// already formatted! +object Test { + foo( + a, // comment + b + ) +} diff --git a/plugin/src/sbt-test/scalafmt-sbt/sbt/test b/plugin/src/sbt-test/scalafmt-sbt/sbt/test index 235ee76..93cfca3 100644 --- a/plugin/src/sbt-test/scalafmt-sbt/sbt/test +++ b/plugin/src/sbt-test/scalafmt-sbt/sbt/test @@ -5,6 +5,14 @@ > p123/compile:scalafmtCheck > p123/test:scalafmtCheck +# expect failure because of bad build.sbt +-> scalafmtSbtCheck +> scalafmtSbt +> scalafmtSbtCheck + +$ copy-file changes/Dependencies.scala project/Dependencies.scala +# expect failure because of bad project/Dependencies.scala +-> scalafmtSbtCheck > scalafmtSbt > scalafmtSbtCheck @@ -94,16 +102,39 @@ $ touch p14/src/main/scala/Test.scala > p15/scalafmt > p15/scalafmtCheck -$ copy-file .scalafmt.conf .scalafmt15.conf # prevent reading the source (without changing the mtime) to detect actual/uncached invocation of scalafmt $ exec chmod 000 p15/src/main/scala/Test.scala +######## full run expected when config was updated and a source file added since last run +$ copy-file .scalafmt.conf .scalafmt15.conf +$ copy-file changes/bad.scala p15/src/main/scala/Test2.scala +-> p15/scalafmt +-> p15/scalafmtCheck +######## full run expected when only config was updated since last run +$ delete p15/src/main/scala/Test2.scala -> p15/scalafmt -> p15/scalafmtCheck > p16/scalafmt > p16/scalafmtCheck -> set p16/scalafmtConfig := file(".scalafmt.conf") # prevent reading the source (without changing the mtime) to detect actual/uncached invocation of scalafmt $ exec chmod 000 p16/src/main/scala/Test.scala +######## full run expected when config was updated and a source file added since last run +> set p16/scalafmtConfig := file(".scalafmt.conf") +$ copy-file changes/bad.scala p16/src/main/scala/Test2.scala +-> p16/scalafmt +-> p16/scalafmtCheck +######## full run expected when only config was updated since last run +$ delete p16/src/main/scala/Test2.scala -> p16/scalafmt -> p16/scalafmtCheck + +$ copy-file changes/bad.scala p17/src/main/scala/Test2.scala +-> p17/scalafmtCheck +# prevent reading the source (without changing the mtime) to detect actual/uncached invocation of scalafmt +$ exec chmod 000 p17/src/main/scala/Test.scala +######## incremetnal checking should carry over failure without actually processing the file +-> p17/scalafmtCheck +######## incremental checking expected when all previous validations failed on some sources only +$ copy-file changes/good.scala p17/src/main/scala/Test2.scala +> p17/scalafmtCheck +> p17/scalafmt