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

fix usage of scalafix via another task #152

Merged
merged 2 commits into from
Jul 15, 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
20 changes: 4 additions & 16 deletions src/main/scala/scalafix/sbt/ScalafixPlugin.scala
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,7 @@ object ScalafixPlugin extends AutoPlugin {
compile := Def.taskDyn {
val oldCompile =
compile.value // evaluated first, before the potential scalafix evaluation
val runScalafixAfterCompile =
scalafixOnCompile.value && !scalafixRunExplicitly.value
if (runScalafixAfterCompile)
if (scalafixOnCompile.value)
scalafix
.toTask("")
.map(_ => oldCompile)
Expand Down Expand Up @@ -415,7 +413,7 @@ object ScalafixPlugin extends AutoPlugin {
).findErrors(files, dependencies, withScalaInterface)
if (errors.isEmpty) {
val task = Def.task {
// passively consume compilation output without triggering compile as it can result in a cyclic dependency
// don't use fullClasspath as it results in a cyclic dependency via compile when scalafixOnCompile := true
val classpath =
dependencyClasspath.in(config).value.map(_.data.toPath) :+
classDirectory.in(config).value.toPath
Expand All @@ -428,8 +426,8 @@ object ScalafixPlugin extends AutoPlugin {
streams.in(config, scalafix).value
)
}
if (scalafixRunExplicitly.value) task.dependsOn(compile.in(config))
else task
// sub-task of compile after which bytecode should not be modified
task.dependsOn(manipulateBytecode.in(config))
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

} else {
Def.task {
if (errors.length == 1) {
Expand Down Expand Up @@ -563,16 +561,6 @@ 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
1 change: 1 addition & 0 deletions src/sbt-test/sbt-scalafix/wrapper/.scalafix.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
rules = [RemoveUnused]
5 changes: 5 additions & 0 deletions src/sbt-test/sbt-scalafix/wrapper/build.sbt
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
val V = _root_.scalafix.sbt.BuildInfo

scalaVersion := V.scala212
addCompilerPlugin(scalafixSemanticdb)
scalacOptions ++= Seq("-Yrangepos", "-Ywarn-unused")
24 changes: 24 additions & 0 deletions src/sbt-test/sbt-scalafix/wrapper/project/LintAllPlugin.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import sbt._
import scalafix.sbt.ScalafixPlugin
import scalafix.sbt.ScalafixPlugin.autoImport._

object LintAllPlugin extends AutoPlugin {

override def trigger: PluginTrigger = allRequirements

override def requires: Plugins = ScalafixPlugin

object autoImport {
val lintAll = taskKey[Unit]("run all linters")
}

import autoImport._

override def projectSettings: Seq[Def.Setting[_]] =
Seq(
lintAll := {
scalafix.in(Compile).toTask(" --check").value
// & other linters...
}
)
}
2 changes: 2 additions & 0 deletions src/sbt-test/sbt-scalafix/wrapper/project/plugins.sbt
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,5 @@
package example

import imported.Imported

object Example
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
package imported

object Imported
14 changes: 14 additions & 0 deletions src/sbt-test/sbt-scalafix/wrapper/test
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# check that missing rewrites are detected, with or without a warm compilation cache
> set scalafixOnCompile := false
-> lintAll
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

on master

[info] [info] Non-compiled module 'compiler-bridge_2.12' for Scala 2.12.12. Compiling...
[info] [info] Running scalafix on 2 Scala sources
[error] /tmp/sbt_4021633e/wrapper/src/main/scala/example/Example.scala:3: error: not found: object imported
[error] import imported.Imported
[error]        ^
[info] [info]   Compilation completed in 14.904s.
[info] [warn] /tmp/sbt_4021633e/wrapper/src/main/scala/example/Example.scala:3:17: Unused import
[info] [warn] import imported.Imported
[info] [warn]                 ^
[info] [warn] one warning found
[info] [info] Done compiling.
[info] [success] Total time: 20 s, completed Jul 15, 2020, 11:52:59 AM
[error] x sbt-scalafix / wrapper 
[error]  Cause of test exception: {line 3}  Command succeeded but failure was expected

Copy link
Collaborator Author

@bjaglin bjaglin Jul 15, 2020

Choose a reason for hiding this comment

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

I investigated where the concurrent Non-compiled module 'compiler-bridge_2.12' for Scala 2.12.12. Compiling.../Done compiling was coming from as it looked like it could race with the bytecode consumption in scalafix. It turns out to be from Test / scalafix, so I added bjaglin@3a0a4c8 to strengthen the test.

Without the fix, the test logs now shows:

[info] [info] Running scalafix on 2 Scala sources
[error] /tmp/sbt_93ed664/wrapper/src/main/scala/example/Example.scala:3: error: not found: object imported
[error] import imported.Imported
[error]        ^
[info] [success] Total time: 5 s, completed Jul 15, 2020, 5:29:15 PM
[error] x sbt-scalafix / wrapper 
[error]  Cause of test exception: {line 3}  Command succeeded but failure was expected

> compile
-> lintAll

# apply the rewrite via an explicit call
> scalafix

# confirms that `lintAll` sees that rewrite, with or without a warm compilation cache
> clean
> lintAll
> compile
> lintAll