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

Introduce scalafixAll InputTask #126

Merged
merged 2 commits into from
Jun 14, 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
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import sbt.complete._
import sbt.complete.DefaultParsers._

class ScalafixCompletions(
workingDirectory: () => Path,
workingDirectory: Path,
loadedRules: () => Seq[ScalafixRule],
terminalWidth: Option[Int]
) {
Expand Down Expand Up @@ -66,9 +66,9 @@ class ScalafixCompletions(
}

string
.examples(new AbsolutePathExamples(workingDirectory()))
.examples(new AbsolutePathExamples(workingDirectory))
.map { f =>
toAbsolutePath(Paths.get(f), workingDirectory())
toAbsolutePath(Paths.get(f), workingDirectory)
}
.filter(f => Files.exists(f), x => x)
}
Expand Down Expand Up @@ -96,7 +96,7 @@ class ScalafixCompletions(
private val namedRule2: Parser[ShellArgs.Rule] =
namedRule.map(s => ShellArgs.Rule(s))
private lazy val gitDiffParser: P = {
val jgitCompletion = new JGitCompletion(workingDirectory())
val jgitCompletion = new JGitCompletion(workingDirectory)
token(
NotQuoted,
TokenCompletions.fixed((seen, _) => {
Expand Down
138 changes: 94 additions & 44 deletions src/main/scala/scalafix/sbt/ScalafixPlugin.scala
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@ import com.geirsson.coursiersmall.Repository
import sbt.KeyRanks.Invisible
import sbt.Keys._
import sbt._
import sbt.complete._
import sbt.internal.sbtscalafix.Caching._
import sbt.internal.sbtscalafix.{Compat, JLineAccess}
import sbt.plugins.JvmPlugin
import sbt.internal.sbtscalafix.Caching._
import scalafix.interfaces.ScalafixError
import scalafix.internal.sbt._

Expand All @@ -29,9 +30,14 @@ object ScalafixPlugin extends AutoPlugin {

val scalafix: InputKey[Unit] =
inputKey[Unit](
"Run scalafix rule in this project and configuration. " +
"Run scalafix rule(s) in this project and configuration. " +
"For example: scalafix RemoveUnusedImports. " +
"To run on test sources use test:scalafix."
"To run on test sources use test:scalafix or scalafixAll."
)
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."
)
val scalafixCaching: SettingKey[Boolean] =
settingKey[Boolean](
Expand Down Expand Up @@ -100,22 +106,12 @@ object ScalafixPlugin extends AutoPlugin {
override lazy val projectSettings: Seq[Def.Setting[_]] =
Seq(Compile, Test).flatMap(c => inConfig(c)(scalafixConfigSettings(c))) ++
inConfig(ScalafixConfig)(Defaults.configSettings) ++
Seq(ivyConfigurations += ScalafixConfig)
Seq(
ivyConfigurations += ScalafixConfig,
scalafixAll := scalafixAllInputTask.evaluated
)

override lazy val globalSettings: Seq[Def.Setting[_]] = Seq(
initialize := {
val _ = initialize.value
// Ideally, we would not resort to storing mutable state in `initialize`.
// The optimal solution would be to run `scalafixDependencies.value`
// inside `scalafixInputTask`.
// However, we can't do that due to an sbt bug:
// https://github.com/sbt/sbt/issues/3572#issuecomment-417582703
workingDirectory = baseDirectory.in(ThisBuild).value.toPath
scalafixInterface = ScalafixInterface.fromToolClasspath(
scalafixDependencies = scalafixDependencies.in(ThisBuild).value,
scalafixCustomResolvers = scalafixResolvers.in(ThisBuild).value
)
},
scalafixConfig := None, // let scalafix-cli try to infer $CWD/.scalafix.conf
scalafixCaching := false,
scalafixResolvers := ScalafixCoursier.defaultResolvers,
Expand All @@ -124,12 +120,28 @@ object ScalafixPlugin extends AutoPlugin {
)

lazy val stdoutLogger = Compat.ConsoleLogger(System.out)
private var workingDirectory = file("").getAbsoluteFile.toPath
private var scalafixInterface: () => ScalafixInterface =
() => throw new UninitializedError

private val scalafixInterface: Def.Initialize[() => ScalafixInterface] =
Copy link
Collaborator Author

@bjaglin bjaglin Jun 13, 2020

Choose a reason for hiding this comment

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

By being a Setting, the function wrapped here is instantiated at sbt load time (capturing other global scalafix* settings) as it's referenced by scalafix[All]InputTask through parser, but evaluated at task execution time, and since it's backed by a LazyValue, it's memoized.

Def.setting {
ScalafixInterface.fromToolClasspath(
scalafixDependencies = scalafixDependencies.in(ThisBuild).value,
scalafixCustomResolvers = scalafixResolvers.in(ThisBuild).value
)
}

private val parser: Def.Initialize[Parser[ShellArgs]] = Def.setting {
new ScalafixCompletions(
workingDirectory = baseDirectory.in(ThisBuild).value.toPath,
// Unfortunately, local rules will not show up as completions in the parser, as that parser can only
// depend on settings, while local rules classpath must be looked up via tasks
loadedRules = () => scalafixInterface.value().availableRules(),
terminalWidth = Some(JLineAccess.terminalWidth)
).parser
}

private def scalafixArgsFromShell(
shell: ShellArgs,
scalafixInterface: () => ScalafixInterface,
projectDepsExternal: Seq[ModuleID],
baseResolvers: Seq[Repository],
projectDepsInternal: Seq[File]
Expand Down Expand Up @@ -158,20 +170,49 @@ object ScalafixPlugin extends AutoPlugin {

}

private def scalafixAllInputTask(): Def.Initialize[InputTask[Unit]] =
// workaround https://github.com/sbt/sbt/issues/3572 by invoking directly what Def.inputTaskDyn would via macro
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome to see a workaround for this issue, nice job tracking down the issue!

InputTask
.createDyn(InputTask.initParserAsInput(parser))(
Def.task(shellArgs => scalafixAllTask(shellArgs, thisProject.value))
)

private def scalafixAllTask(
shellArgs: ShellArgs,
project: ResolvedProject
): Def.Initialize[Task[Unit]] =
Def.taskDyn {
val configsWithScalafixInputKey = project.settings
.map(_.key)
.filter(_.key == scalafix.key)
.flatMap(_.scope.config.toOption)

configsWithScalafixInputKey
.map(config => scalafixTask(shellArgs, config))
.joinWith(_.join.map(_ => ()))
}

private def scalafixInputTask(
config: Configuration
): Def.Initialize[InputTask[Unit]] =
Def.inputTaskDyn {
val shell0 = new ScalafixCompletions(
workingDirectory = () => workingDirectory,
// Unfortunately, local rules will not show up as completions in the parser, as that parser can only depend
// on global settings (see note in initialize), while local rules classpath must
// be looked up via tasks on projects
loadedRules = () => scalafixInterface().availableRules(),
terminalWidth = Some(JLineAccess.terminalWidth)
).parser.parsed
// workaround https://github.com/sbt/sbt/issues/3572 by invoking directly what Def.inputTaskDyn would via macro
InputTask
.createDyn(InputTask.initParserAsInput(parser))(
Def.task(shellArgs => scalafixTask(shellArgs, config))
)

private def scalafixTask(
shellArgs: ShellArgs,
config: ConfigKey
): Def.Initialize[Task[Unit]] =
Def.taskDyn {
val errorLogger =
new PrintStream(LoggingOutputStream(streams.value.log, Level.Error))
new PrintStream(
LoggingOutputStream(
streams.in(config, scalafix).value.log,
Level.Error
)
)
val projectDepsInternal = products.in(ScalafixConfig).value ++
internalDependencyClasspath.in(ScalafixConfig).value.map(_.data)
val projectDepsExternal =
Expand All @@ -180,18 +221,21 @@ object ScalafixPlugin extends AutoPlugin {
.value
.flatMap(_.get(moduleID.key))

if (shell0.rules.isEmpty && shell0.extra == List("--help")) {
if (shellArgs.rules.isEmpty && shellArgs.extra == List("--help")) {
scalafixHelp
} else {
val scalafixConf = scalafixConfig.value.map(_.toPath)
val scalafixConf = scalafixConfig.in(config).value.map(_.toPath)
val (shell, mainInterface0) = scalafixArgsFromShell(
shell0,
shellArgs,
scalafixInterface.value,
projectDepsExternal,
scalafixResolvers.in(ThisBuild).value,
projectDepsInternal
)
val maybeNoCache =
if (shell.noCache || !scalafixCaching.value) Seq(Arg.NoCache) else Nil
if (shell.noCache || !scalafixCaching.in(config).value)
Seq(Arg.NoCache)
else Nil
val mainInterface = mainInterface0
.withArgs(maybeNoCache: _*)
.withArgs(
Expand All @@ -212,32 +256,35 @@ object ScalafixPlugin extends AutoPlugin {
}
private def scalafixHelp: Def.Initialize[Task[Unit]] =
Def.task {
scalafixInterface().withArgs(Arg.ParsedArgs(List("--help"))).run()
scalafixInterface.value().withArgs(Arg.ParsedArgs(List("--help"))).run()
()
}

private def scalafixSyntactic(
mainInterface: ScalafixInterface,
shellArgs: ShellArgs,
config: Configuration
config: ConfigKey
): Def.Initialize[Task[Unit]] =
Def.task {
val files = filesToFix(shellArgs, config).value
runArgs(mainInterface.withArgs(Arg.Paths(files)), streams.value)
runArgs(
mainInterface.withArgs(Arg.Paths(files)),
streams.in(config, scalafix).value
)
}

private def scalafixSemantic(
ruleNames: Seq[String],
mainArgs: ScalafixInterface,
shellArgs: ShellArgs,
config: Configuration
config: ConfigKey
): Def.Initialize[Task[Unit]] =
Def.taskDyn {
val dependencies = allDependencies.value
val dependencies = allDependencies.in(config).value
val files = filesToFix(shellArgs, config).value
val withScalaInterface = mainArgs.withArgs(
Arg.ScalaVersion(scalaVersion.value),
Arg.ScalacOptions(scalacOptions.value)
Arg.ScalacOptions(scalacOptions.in(config).value)
)
val errors = new SemanticRuleValidator(
new SemanticdbNotFound(ruleNames, scalaVersion.value, sbtVersion.value)
Expand All @@ -246,9 +293,12 @@ object ScalafixPlugin extends AutoPlugin {
Def.task {
val semanticInterface = withScalaInterface.withArgs(
Arg.Paths(files),
Arg.Classpath(fullClasspath.value.map(_.data.toPath))
Arg.Classpath(fullClasspath.in(config).value.map(_.data.toPath))
)
runArgs(
semanticInterface,
streams.in(config, scalafix).value
)
runArgs(semanticInterface, streams.value)
}
} else {
Def.task {
Expand Down Expand Up @@ -389,7 +439,7 @@ object ScalafixPlugin extends AutoPlugin {
}
private def filesToFix(
shellArgs: ShellArgs,
config: Configuration
config: ConfigKey
): Def.Initialize[Task[Seq[Path]]] =
Def.taskDyn {
// Dynamic task to avoid redundantly computing `unmanagedSources.value`
Expand All @@ -400,7 +450,7 @@ object ScalafixPlugin extends AutoPlugin {
} else {
Def.task {
for {
source <- unmanagedSources.in(config).in(scalafix).value
source <- unmanagedSources.in(config, scalafix).value
if source.exists()
if isScalaFile(source)
} yield source.toPath
Expand Down
3 changes: 2 additions & 1 deletion src/sbt-test/sbt-scalafix/cross-build/test
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# compile should not affect scalafix
> compile

-> scalafixAll --test ProcedureSyntax
-> scalafix --test ProcedureSyntax
-> compile:scalafix --test ProcedureSyntax
-> test:scalafix --test ProcedureSyntax
Expand All @@ -17,7 +18,7 @@
> test:scalafix --test ProcedureSyntax
-> it:scalafix --test ProcedureSyntax

> it:scalafix ProcedureSyntax
> scalafixAll ProcedureSyntax
> it:scalafix --test ProcedureSyntax

> javaProject/compile:scalafix ProcedureSyntax
Expand Down
4 changes: 4 additions & 0 deletions src/sbt-test/sbt-scalafix/inconfig/test
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
-> ; example/scalafix --test ; example/it:scalafix --test
-> example/scalafixAll --test
> example/it:scalafix
> example/it:scalafix --test
-> example/scalafix --test
> tests/test
> example/scalafixAll
> example/scalafixAll --test
> example/scalafix --test
18 changes: 18 additions & 0 deletions src/sbt-test/skip-windows/caching/test
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,24 @@ $ exec chmod 000 src/test/scala/Valid.scala
$ delete src/main/scala
$ delete src/test/scala

# scalafixAll and scalafix should share the same cache per configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

👏

> set scalafixConfig := None
$ mkdir src/main/scala
$ mkdir src/test/scala
$ copy-file files/UnusedImports.scala src/main/scala/Valid.scala
$ copy-file files/ProcedureSyntax.scala src/test/scala/InitiallyInvalid.scala
-> scalafixAll --check ProcedureSyntax
$ exec chmod 000 src/main/scala/Valid.scala
> scalafix --check ProcedureSyntax
-> test:scalafix --check ProcedureSyntax
> test:scalafix ProcedureSyntax
$ exec chmod 000 src/test/scala/InitiallyInvalid.scala
> scalafix --check ProcedureSyntax
> test:scalafix --check ProcedureSyntax
> scalafixAll --check ProcedureSyntax
$ delete src/main/scala
$ delete src/test/scala

# make sure the cache is disabled when using `--stdout`
> set scalafixConfig := None
$ mkdir src/main/scala
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class SbtCompletionsSuite extends AnyFunSuite {
val loadedRules = mainArgs.availableRules.toList

val parser = new ScalafixCompletions(
workingDirectory = () => fs.workingDirectory.toAbsolutePath,
workingDirectory = fs.workingDirectory.toAbsolutePath,
loadedRules = () => loadedRules,
terminalWidth = None
).parser
Expand Down