Skip to content

Commit

Permalink
improve --files CLI UX
Browse files Browse the repository at this point in the history
To avoid getting false positive errors (missing SemanticDB), scalafix
executions with `--files` should not aggregate as they are by nature
specific to a given project / Configuration.

- prevent usage of scalafixAll --files
- validate --files values against unmanagedSourceDirectories
  - allow precise completions
  - avoid aggregated executions: since requested files are not valid
    outside the project on which the task input was crafted, sbt simply
    ignores the run
  • Loading branch information
bjaglin committed Nov 12, 2023
1 parent 8d817d7 commit 371152c
Show file tree
Hide file tree
Showing 10 changed files with 187 additions and 56 deletions.
42 changes: 35 additions & 7 deletions src/main/scala/scalafix/internal/sbt/ScalafixCompletions.scala
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,11 @@ import sbt.complete.DefaultParsers._

class ScalafixCompletions(
workingDirectory: Path,
// 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: () => Seq[ScalafixRule],
terminalWidth: Option[Int]
terminalWidth: Option[Int],
allowedTargetFilesPrefixes: Seq[Path] // Nil means all values are valid
) {

private type P = Parser[String]
Expand All @@ -28,14 +31,18 @@ class ScalafixCompletions(
valueParser: ArgP
): KVArgP = {
val keyParser = Parser.oneOf((key +: keyAliases).map(literal)).examples(key)
val sepValueParser = (sep ~> valueParser).!!!("missing or invalid value")
val sepValueParser =
if (valueParser.toString.contains("!!!")) sep ~> valueParser
else (sep ~> valueParser).!!!("missing or invalid value")
keyParser.^^^(sepValueParser)
}

private def uri(protocol: String): Parser[ShellArgs.Rule] =
token(protocol + ":") ~> NotQuoted.map(x => ShellArgs.Rule(s"$protocol:$x"))

private val filepathParser: Parser[Path] = {
private def filepathParser(
filterPrefixes: Option[Seq[Path]] = None
): Parser[Path] = {
def toAbsolutePath(path: Path, cwd: Path): Path = {
if (path.isAbsolute) path
else cwd.resolve(path)
Expand Down Expand Up @@ -65,12 +72,25 @@ class ScalafixCompletions(
}
}

def isAllowedPrefix(path: Path): Boolean = filterPrefixes match {
case Some(prefixes) =>
val absolutePath = path.toAbsolutePath.toString
prefixes.map(_.toAbsolutePath.toString).exists { prefix =>
absolutePath.startsWith(prefix) || prefix.startsWith(absolutePath)
}
case None => true
}

string
.examples(new AbsolutePathExamples(workingDirectory))
.examples(
exampleSource = new AbsolutePathExamples(workingDirectory),
maxNumberOfExamples = 25,
removeInvalidExamples = true
)
.map { f =>
toAbsolutePath(Paths.get(f), workingDirectory)
}
.filter(f => Files.exists(f), x => x)
.filter(f => Files.exists(f) && isAllowedPrefix(f), x => x)
}

private val namedRule: P = {
Expand Down Expand Up @@ -130,7 +150,7 @@ class ScalafixCompletions(
def hide(p: P): P = p.examples()

def parser: Parser[ShellArgs] = {
val pathParser: Parser[Path] = token(filepathParser)
val pathParser: Parser[Path] = token(filepathParser())
val fileRule: Parser[ShellArgs.Rule] =
(token("file:") ~> pathParser)
.map(path => ShellArgs.Rule(path.toUri.toString))
Expand Down Expand Up @@ -166,7 +186,15 @@ class ScalafixCompletions(
gitDiffParser.map(v => ShellArgs.Extra("--diff-base", Some(v)))
}
val files: KVArgP = valueAfterKey("--files", "-f") {
pathParser.map(v => ShellArgs.File(v.toString))
val errorMessage =
if (allowedTargetFilesPrefixes.nonEmpty)
"--files value(s) must reference existing files or directories in unmanagedSourceDirectories; " +
"are you running scalafix on the right project / Configuration?"
else
"--files can only be used on project-level invocations (i.e. myproject / scalafix --files=f.scala)"
token(filepathParser(Some(allowedTargetFilesPrefixes)))
.map(v => ShellArgs.File(v.toString))
.!!!(errorMessage)
}
val rules: KVArgP = valueAfterKey("--rules", "-r")(rule)

Expand Down
49 changes: 27 additions & 22 deletions src/main/scala/scalafix/sbt/ScalafixPlugin.scala
Original file line number Diff line number Diff line change
Expand Up @@ -127,13 +127,6 @@ object ScalafixPlugin extends AutoPlugin {
Invisible
)

private val scalafixCompletions: SettingKey[ScalafixCompletions] =
SettingKey(
"scalafixCompletions",
"Implementation detail - do not use",
Invisible
)

// Memoize ScalafixInterface instances initialized with a custom tool classpath across projects & configurations
// during task execution, to amortize the classloading cost when invoking scalafix concurrently on many targets
private val scalafixInterfaceCache
Expand Down Expand Up @@ -223,13 +216,6 @@ object ScalafixPlugin extends AutoPlugin {
scalafixDependencies = (ThisBuild / scalafixDependencies).value,
scalafixCustomResolvers = (ThisBuild / scalafixResolvers).value
),
scalafixCompletions := new ScalafixCompletions(
workingDirectory = (ThisBuild / baseDirectory).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 = () => scalafixInterfaceProvider.value().availableRules(),
terminalWidth = Some(JLineAccess.terminalWidth)
),
scalafixInterfaceCache := new BlockingCache[
ToolClasspath,
ScalafixInterface
Expand Down Expand Up @@ -299,14 +285,22 @@ object ScalafixPlugin extends AutoPlugin {

}

private def scalafixAllInputTask(): Def.Initialize[InputTask[Unit]] =
private def scalafixAllInputTask(): Def.Initialize[InputTask[Unit]] = {
val parser = Def.setting(
new ScalafixCompletions(
workingDirectory = (ThisBuild / baseDirectory).value.toPath,
loadedRules = () => scalafixInterfaceProvider.value().availableRules(),
terminalWidth = Some(JLineAccess.terminalWidth),
allowedTargetFilesPrefixes = Nil
)
)(_.parser)
// workaround https://github.com/sbt/sbt/issues/3572 by invoking directly what Def.inputTaskDyn would via macro
InputTask
.createDyn(InputTask.initParserAsInput(scalafixCompletions(_.parser)))(
Def.task(shellArgs =>
scalafixAllTask(shellArgs, thisProject.value, resolvedScoped.value)
)
InputTask.createDyn(InputTask.initParserAsInput(parser))(
Def.task(shellArgs =>
scalafixAllTask(shellArgs, thisProject.value, resolvedScoped.value)
)
)
}

private def scalafixAllTask(
shellArgs: ShellArgs,
Expand Down Expand Up @@ -343,12 +337,23 @@ object ScalafixPlugin extends AutoPlugin {

private def scalafixInputTask(
config: Configuration
): Def.Initialize[InputTask[Unit]] =
): Def.Initialize[InputTask[Unit]] = {
val parser = Def.setting(
new ScalafixCompletions(
workingDirectory = (ThisBuild / baseDirectory).value.toPath,
loadedRules = () => scalafixInterfaceProvider.value().availableRules(),
terminalWidth = Some(JLineAccess.terminalWidth),
allowedTargetFilesPrefixes =
(scalafix / unmanagedSourceDirectories).value.map(_.toPath)
)
)(_.parser)

// workaround https://github.com/sbt/sbt/issues/3572 by invoking directly what Def.inputTaskDyn would via macro
InputTask
.createDyn(InputTask.initParserAsInput(scalafixCompletions(_.parser)))(
.createDyn(InputTask.initParserAsInput(parser))(
Def.task(shellArgs => scalafixTask(shellArgs, config))
)
}

private def scalafixTask(
shellArgs: ShellArgs,
Expand Down
2 changes: 1 addition & 1 deletion src/sbt-test/sbt-scalafix/explicit-files/.scalafix.conf
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
rules = [DisableSyntax]
rules = [DisableSyntax, RemoveUnused]
DisableSyntax.keywords = [null]
12 changes: 10 additions & 2 deletions src/sbt-test/sbt-scalafix/explicit-files/build.sbt
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
import _root_.scalafix.sbt.{BuildInfo => Versions}

val explicit = project.settings(
scalaVersion := Versions.scala212
inThisBuild(
List(
scalaVersion := Versions.scala212,
scalacOptions += "-Ywarn-unused",
semanticdbEnabled := true,
semanticdbVersion := scalafixSemanticdb.revision
)
)

val ok_and_ko = project
val ok_only = project
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
class OK {}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
class OK {}
44 changes: 34 additions & 10 deletions src/sbt-test/sbt-scalafix/explicit-files/test
Original file line number Diff line number Diff line change
@@ -1,14 +1,38 @@
-> scalafix
> ok_only / scalafix
-> ok_and_ko / scalafix

-> scalafix -f=explicit/src/main/scala/KO.scala
-> scalafix --files=explicit/src/main/scala/KO.scala

-> scalafix -f=explicit/src/main/scala/OK.scala --files=explicit/src/main/scala/KO.scala
-> scalafix --files=explicit/src/main/scala/OK.scala -f=explicit/src/main/scala/KO.scala
-> scalafix -f=explicit/src/main/scala/KO.scala -f=explicit/src/main/scala/OK.scala
# CORRECT USAGES
# --------------

> scalafix -f=explicit/src/main/scala/OK.scala
> scalafix --files=explicit/src/main/scala/OK.scala
# Targeting valid files within a project containing invalid files succeeds, with or without a key/value separator
> ok_and_ko / scalafix -f=ok_and_ko/src/main/scala/OK.scala
> ok_and_ko / scalafix --files=ok_and_ko/src/main/scala/OK.scala
> ok_and_ko / scalafix -f ok_and_ko/src/main/scala/OK.scala
> ok_and_ko / scalafix --files ok_and_ko/src/main/scala/OK.scala

> scalafix -f explicit/src/main/scala/OK.scala
> scalafix --files explicit/src/main/scala/OK.scala
# Targeting invalid files fails
-> ok_and_ko / scalafix -f=ok_and_ko/src/main/scala/KO.scala

# Targeting valid and invalid files fails
-> ok_and_ko / scalafix -f=ok_and_ko/src/main/scala/OK.scala -f=ok_and_ko/src/main/scala/KO.scala
-> ok_and_ko / scalafix -f=ok_and_ko/src/main/scala/KO.scala -f=ok_and_ko/src/main/scala/OK.scala

# Targeting valid files from the root project succeeds
> scalafix -f=src/main/scala/OK.scala


# INCORRECT USAGES
# ----------------

# Targeting non-existing files fails
-> ok_only / scalafix -f=ok_only/src/main/scala/NotHere.scala

# Targeting with scalafixAll valid files from the correct project fails
-> ok_only / scalafixAll -f=ok_only/src/main/scala/OK.scala

# Targeting valid files from a different project fails
-> ok_only / scalafix -f=ok_and_ko/src/main/scala/OK.scala

# Targeting valid files from a different configuration fails
-> ok_only / Test / scalafix -f=ok_only/src/main/scala/OK.scala
Loading

0 comments on commit 371152c

Please sign in to comment.