Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

scalafixOnCompile setting key #140

Merged
merged 3 commits into from
Jul 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 46 additions & 10 deletions src/main/scala/scalafix/sbt/ScalafixPlugin.scala
Original file line number Diff line number Diff line change
Expand Up @@ -32,17 +32,27 @@ object ScalafixPlugin extends AutoPlugin {
val scalafix: InputKey[Unit] =
inputKey[Unit](
"Run scalafix rule(s) in this project and configuration. " +
"For example: scalafix RemoveUnusedImports. " +
"To run on test sources use test:scalafix or scalafixAll."
"For example: scalafix RemoveUnused. " +
"To run on test sources use test:scalafix or scalafixAll. " +
"When invoked directly, prior compilation will be triggered for semantic rules."
Copy link
Collaborator Author

@bjaglin bjaglin Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not very proud of the "When invoked directly" wording but I am trying to get the attention of power users that would call/use scalafix input keys from another context (like another input meta input key that would aggregate scalafmt & scalafix for example). In that case, scalafixRunExplicitly will be false, so Scalafix might run on stale semanticdb/class files. I haven't found a way to work around that, but I think it's such a corner case and the current implementation is quite readable that it's not worth trying to address it.

)
val scalafixAll: InputKey[Unit] =
inputKey[Unit](
"Run scalafix rule(s) in this project, for all configurations where scalafix is enabled. " +
"Compile and Test are enabled by default, other configurations can be enabled via scalafixConfigSettings."
"Compile and Test are enabled by default, other configurations can be enabled via scalafixConfigSettings. " +
"When invoked directly, prior compilation will be triggered for semantic rules."
)

val scalafixOnCompile: SettingKey[Boolean] =
settingKey[Boolean](
"Run Scalafix rule(s) declared in .scalafix.conf on compilation and fail on lint errors. " +
"Off by default. When enabled, caching will be automatically activated, " +
"but can be disabled with `scalafixCaching := false`."
)

val scalafixCaching: SettingKey[Boolean] =
settingKey[Boolean](
"Cache scalafix invocations (off by default, still experimental)."
"Cache scalafix invocations (off by default, on if scalafixOnCompile := true)."
)

import scala.language.implicitConversions
Expand Down Expand Up @@ -87,6 +97,17 @@ object ScalafixPlugin extends AutoPlugin {
def scalafixConfigSettings(config: Configuration): Seq[Def.Setting[_]] =
Seq(
scalafix := scalafixInputTask(config).evaluated,
compile := Def.taskDyn {
val oldCompile =
compile.value // evaluated first, before the potential scalafix evaluation
val runScalafixAfterCompile =
scalafixOnCompile.value && !scalafixRunExplicitly.value
if (runScalafixAfterCompile)
scalafix
.toTask("")
.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
// fgRunMain is fine), there is no specific streams attached to InputTasks, so we they end up sharing the
// global streams, causing issues for cache storage. This does not happen for Tasks, so we define a dummy one
Expand Down Expand Up @@ -159,7 +180,7 @@ object ScalafixPlugin extends AutoPlugin {

override lazy val globalSettings: Seq[Def.Setting[_]] = Seq(
scalafixConfig := None, // let scalafix-cli try to infer $CWD/.scalafix.conf
scalafixCaching := false,
scalafixOnCompile := false,
scalafixResolvers := Seq(
Repository.ivy2Local(),
Repository.central(),
Expand Down Expand Up @@ -333,10 +354,9 @@ object ScalafixPlugin extends AutoPlugin {
scalafixResolvers.in(ThisBuild).value,
projectDepsInternal
)
val cachingRequested = scalafixCaching.or(scalafixOnCompile).value
val maybeNoCache =
if (shell.noCache || !scalafixCaching.in(config).value)
Seq(Arg.NoCache)
else Nil
if (shell.noCache || !cachingRequested) Seq(Arg.NoCache) else Nil
val mainInterface = mainInterface0
.withArgs(maybeNoCache: _*)
.withArgs(
Expand Down Expand Up @@ -394,16 +414,22 @@ object ScalafixPlugin extends AutoPlugin {
new SemanticdbNotFound(ruleNames, scalaVersion.value, sbtVersion.value)
).findErrors(files, dependencies, withScalaInterface)
if (errors.isEmpty) {
Def.task {
val task = Def.task {
// passively consume compilation output without triggering compile as it can result in a cyclic dependency
val classpath =
dependencyClasspath.in(config).value.map(_.data.toPath) :+
classDirectory.in(config).value.toPath
Comment on lines +418 to +421
Copy link
Collaborator Author

@bjaglin bjaglin Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have left out copyResources since I don't see why Scalafix would need it but it might be a wrong assumption.

val semanticInterface = withScalaInterface.withArgs(
Arg.Paths(files),
Arg.Classpath(fullClasspath.in(config).value.map(_.data.toPath))
Arg.Classpath(classpath)
)
runArgs(
semanticInterface,
streams.in(config, scalafix).value
)
}
if (scalafixRunExplicitly.value) task.dependsOn(compile.in(config))
else task
} else {
Def.task {
if (errors.length == 1) {
Expand Down Expand Up @@ -537,6 +563,16 @@ object ScalafixPlugin extends AutoPlugin {
}
}

// Controls whether scalafix should depend on compile (true) & whether compile may depend on
// scalafix (false), to avoid cyclic dependencies causing deadlocks during executions (as
// dependencies come from dynamic tasks).
private val scalafixRunExplicitly: Def.Initialize[Task[Boolean]] =
Def.task {
executionRoots.value.exists { root =>
Seq(scalafix.key, scalafixAll.key).contains(root.key)
}
}

private def isScalaFile(file: File): Boolean = {
val path = file.getPath
path.endsWith(".scala") ||
Expand Down
2 changes: 2 additions & 0 deletions src/sbt-test/sbt-scalafix/scalafixOnCompile/.scalafix.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
rules = [DisableSyntax, RemoveUnused]
DisableSyntax.noNulls = true
23 changes: 23 additions & 0 deletions src/sbt-test/sbt-scalafix/scalafixOnCompile/build.sbt
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import _root_.scalafix.sbt.{BuildInfo => Versions}

inThisBuild(
Seq(
scalaVersion := Versions.scala212,
scalacOptions ++= List(
"-Yrangepos",
"-Ywarn-unused-import"
)
)
)
lazy val lint = project
.settings(
addCompilerPlugin(scalafixSemanticdb)
)

lazy val rewrite = project
.configs(IntegrationTest)
.settings(
Defaults.itSettings,
inConfig(IntegrationTest)(scalafixConfigSettings(IntegrationTest)),
addCompilerPlugin(scalafixSemanticdb)
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
object Null {
println(null)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
resolvers += Resolver.sonatypeRepo("public")
addSbtPlugin("ch.epfl.scala" % "sbt-scalafix" % sys.props("plugin.version"))
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import java.time.Instant

object UnusedImports
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import java.time.Instant

object UnusedImports
26 changes: 26 additions & 0 deletions src/sbt-test/sbt-scalafix/scalafixOnCompile/test
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# check implicit rewrite of rewrite/src/main/scala/UnusedImports.scala via `compile`
> compile
-> scalafix --check
> set scalafixOnCompile.in(ThisBuild) := true
-> scalafix --check
> compile
> scalafix --check

# check explicit rewrite of rewrite/src/it/scala/UnusedImports.scala via `scalafix`
-> it:scalafix --check
> it:scalafix
> it:scalafix --check

# check lint for lint/src/test/scala/Null.scala
-> lint/test:scalafix --check
-> lint/test:scalafix
-> lint/test:compile

# check that default rules are ignored when rules are passed explicitly
-> lint/test:scalafix --check
> lint/test:scalafix --check RemoveUnused
> lint/test:scalafix RemoveUnused

# check configuration granularity for scalafixOnCompile
> set scalafixOnCompile.in(lint, Test) := false
> lint/test:compile