Merge pull request #411 from bjaglin/store-analysis

avoid full (re)compilation after cold scalafix invocations

build.sbt

@@ -56,7 +56,7 @@ scriptedSbt := {
  if (jdk >= 21)
  "1.9.0" // first release that supports JDK21
  else
- (pluginCrossBuild / sbtVersion).value
+ "1.3.3" // get https://github.com/sbt/sbt/issues/1673 to avoid race conditions
 }
 
 libraryDependencies += compilerPlugin(scalafixSemanticdb)

src/main/scala/scalafix/sbt/ScalafixPlugin.scala

@@ -102,9 +102,20 @@ object ScalafixPlugin extends AutoPlugin {
  val oldCompile =
  compile.value // evaluated first, before the potential scalafix evaluation
  if (scalafixOnCompile.value)
- scalafix
- .toTask(" --triggered")
- .map(_ => oldCompile)
+ Def.taskDyn {
+ // evaluate scalafixInvoked only when scalafixOnCompile is set
+ // to reduce risk of side effects on the build
+ if (scalafixInvoked.value)
+ // don't trigger scalafix if it was explicitly invoked,
+ // as it would be not only inefficient, but incorrect since
+ // scalafix might run with different CLI args, potentially
+ // rewriting files while `--check` was used
+ Def.task(oldCompile)
+ else
+ scalafix
+ .toTask(" --triggered")
+ .map(_ => oldCompile)
+ }
  else Def.task(oldCompile)
  }.value,
  // In some cases (I haven't been able to understand when/why, but this also happens for bgRunMain while
@@ -505,8 +516,13 @@ object ScalafixPlugin extends AutoPlugin {
  (config / scalafix / streams).value
  )
  }
- // sub-task of compile after which bytecode should not be modified
- task.dependsOn(config / manipulateBytecode)
+ // we should not and cannot (circular dependency) trigger compilation
+ // if this is an non-explicit invocation - we use the presence of a
+ // flag rather than checking !scalafixInvoked, in order to trigger
+ // compile in case scalafix is wrapped in another task that
+ // scalafixInvoked does not know about
+ if (shellArgs.extra.contains("--triggered")) task
+ else task.dependsOn(config / compile)
  } else {
  Def.task {
  if (errors.length == 1) {
@@ -583,7 +599,7 @@ object ScalafixPlugin extends AutoPlugin {
  case "--stdout" =>
  // --stdout cannot be cached as we don't capture the output to replay it
  throw StampingImpossible
- case "--tool-classpath" =>
+ case tcp if tcp.startsWith("--tool-classpath") =>
  // custom tool classpaths might contain directories for which we would need to stamp all files, so
  // just disable caching for now to keep it simple and to be safe
  throw StampingImpossible
@@ -743,16 +759,6 @@ object ScalafixPlugin extends AutoPlugin {
  options.ignoredScalacOptions() ++
  scalacOptionsToRelax.map(_.pattern())
  )
- },
- manipulateBytecode := {
- val analysis = manipulateBytecode.value
- if (!scalafixInvoked.value) analysis
- else {
- // prevent storage of the analysis with relaxed scalacOptions - despite not depending explicitly on compile,
- // it is being triggered for parent configs/projects through evaluation of dependencyClasspath (TrackAlways)
- // in the scope where scalafix is invoked
- analysis.withHasModified(false)
- }
  }
  )
 

src/sbt-test/sbt-scalafix/relax-scalacOptions/build.sbt

@@ -1,14 +1,8 @@
 val V = _root_.scalafix.sbt.BuildInfo
 
 addCompilerPlugin(scalafixSemanticdb)
-scalacOptions ++= Seq("-Yrangepos")
-
+scalacOptions ++= Seq("-Yrangepos", "-Ywarn-unused")
 scalaVersion := V.scala212
-scalacOptions ++= Seq(
- // generate errors on unused imports
- "-Xfatal-warnings",
- "-Ywarn-unused"
-)
 Compile / compile / scalacOptions ++= Seq(
  // generate errors on procedure syntax
  "-Wconf:cat=deprecation:e",
@@ -38,11 +32,6 @@ TaskKey[Unit]("checkLastCompilationCached") := {
  }
 }
 
-TaskKey[Unit]("checkZincAnalysisPresent") := {
- val isPresent = (Compile / previousCompile).value.analysis.isPresent()
- assert(isPresent, "zinc analysis not found")
-}
-
 // https://github.com/sbt/sbt/commit/dbb47b3ce822ff7ec25881dadd71a3b29e202273
 // must be outside a macro to workaround "Illegal dynamic reference: Def"
 def streamScopedKey(scope: Scope) = Def.ScopedKey(scope, Keys.streams.key)

src/sbt-test/sbt-scalafix/relax-scalacOptions/test

@@ -1,22 +1,45 @@
-# we have 2 fatal warnings preventing compilation
+# when the build does not contain any flag relaxed on scalafix invocation
+# check that regular compilation benefits froma prior direct scalafix invocation ...
+-> scalafix --check RemoveUnused
+-> checkLastCompilationCached
+> compile
+> checkLastCompilationCached
+
+# ... and indirect scalafix invocation
+> clean
+> test:scalafix --check RemoveUnused
+-> checkLastCompilationCached
+> compile
+> checkLastCompilationCached
+
+# we now have 2 fatal warnings preventing compilation
+> set scalacOptions ++= Seq("-Xfatal-warnings")
 -> compile
 
 # ensure that a semantic rule can be ran despite warnings, to fix one of the warning
-> scalafixAll RemoveUnused
+> scalafix RemoveUnused
 
-# but that the zinc analysis file has not been persisted, otherwise the run with relax scalacOptions would
-# cause the next compile to be full (non-incremental) no matter if there was a previous successful compilation
--> checkZincAnalysisPresent
+# confirm that the rule was applied and that triggered compilation is cached once the updated file recompiled
+> scalafix --check RemoveUnused
+-> checkLastCompilationCached
+> scalafix --check RemoveUnused
+> checkLastCompilationCached
 
 # and that -Xfatal-warnings remains honored for regular compilation
 -> compile
 
-# confirm that compilation succeeds after fixing the last warning
+# confirm that regular compilation succeeds after fixing the last warning
 > scalafix ProcedureSyntax
 > compile
-> checkZincAnalysisPresent
 
-# check that there is no recompilation when running a semantic rule after a successful compilation
--> checkLastCompilationCached
-> scalafixAll RemoveUnused
+# check that regular compilation is cached as usual
+> compile
 > checkLastCompilationCached
+
+# check that there is no recompilation when running a semantic rule after a regular compilation ...
+> scalafix RemoveUnused
+> checkLastCompilationCached
+
+# nor when compiling after running a semantic rule
+> compile
+> checkLastCompilationCached