From a9ff6a08170c369cb2b19e8fb7e6d2e4ffaff7b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93lafur=20P=C3=A1ll=20Geirsson?= Date: Wed, 16 Aug 2017 12:20:41 +0200 Subject: [PATCH 01/18] Replace sourcecode.Name with scalafix.rewrite.RewriteName. RewriteName is a wrapper around a non-empty list of strings instead of being a single string. Original names are preserved when composing multiple RewriteNames. --- .../main/scala/scalafix/rewrite/Rewrite.scala | 12 ++++------ .../scala/scalafix/rewrite/RewriteName.scala | 24 +++++++++++++++++++ 2 files changed, 28 insertions(+), 8 deletions(-) create mode 100644 scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteName.scala diff --git a/scalafix-core/shared/src/main/scala/scalafix/rewrite/Rewrite.scala b/scalafix-core/shared/src/main/scala/scalafix/rewrite/Rewrite.scala index 4b1a5ac2c..b0ff86426 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/rewrite/Rewrite.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/rewrite/Rewrite.scala @@ -11,7 +11,7 @@ import metaconfig.Configured import sourcecode.Name /** A Rewrite is a program that produces a Patch from a scala.meta.Tree. */ -abstract class Rewrite(implicit rewriteName: Name) { self => +abstract class Rewrite(implicit val rewriteName: RewriteName) { self => /** Build patch for a single tree/compilation unit. * @@ -45,8 +45,8 @@ abstract class Rewrite(implicit rewriteName: Name) { self => } - final def name: String = rewriteName.value - final override def toString: String = name + final def name: String = rewriteName.toString + final override def toString: String = name.toString // NOTE. This is kind of hacky and hopefully we can find a better workaround. // The challenge is the following: @@ -97,11 +97,7 @@ object Rewrite { /** Combine two rewrites into a single rewrite */ def merge(a: Rewrite, b: Rewrite): Rewrite = { - val newName = - if (a.name == "empty") b.name - else if (b.name == "empty") a.name - else s"${a.name}+${b.name}" - new Rewrite()(Name(newName)) { + new Rewrite()(a.rewriteName + b.rewriteName) { override def rewrite(ctx: RewriteCtx): Patch = a.rewrite(ctx) + b.rewrite(ctx) override def semanticOption: Option[SemanticCtx] = diff --git a/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteName.scala b/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteName.scala new file mode 100644 index 000000000..e599a2a6d --- /dev/null +++ b/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteName.scala @@ -0,0 +1,24 @@ +package scalafix.rewrite + +final class RewriteName(val head: String, val tail: List[String]) { + require( + head != RewriteName.emptyHead || tail.isEmpty, + "Empty rewrite cannot have underlying names!" + ) + def name: String = + (head :: tail).mkString("+") + def isEmpty: Boolean = head == RewriteName.emptyHead + def +(other: RewriteName): RewriteName = + if (isEmpty) other + else if (other.isEmpty) this + else new RewriteName(head, head :: tail ::: other.tail) + override def toString: String = name +} + +object RewriteName { + private[scalafix] val emptyHead = "empty" + final val empty = new RewriteName(emptyHead, Nil) + def apply(name: String) = new RewriteName(name, Nil) + implicit def generate(implicit name: sourcecode.Name): RewriteName = + new RewriteName(name.value, Nil) +} From ecab46127cfacf4fbc3868bd26f6b32d6780dacf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93lafur=20P=C3=A1ll=20Geirsson?= Date: Wed, 16 Aug 2017 16:29:05 +0200 Subject: [PATCH 02/18] Add infrastructure for linters, towards #92. New package scalafix.lint with data structures for emitting messages from linters: - LintID: uniquely identifies a certain kind of linter message, owned by a rewrite. A LintID is attached to a default warning/error severity but can be configured to another category with `lint.ignore/warning/error = [ LintID ]` - LintMessage: an instance of LintID with a customized message at a particular position. A LintMessage can be created wit LintID.at(String/Position). - A LintMessage can be turned into a Patch with ctx.lint(LintMessage). LintMessages don't report to the console until Rewrite.apply is called. To accommodate linter development, scalafix-testkit now enforces that expected warnings/errors are documented in the input sources with a trailing `// scalafix: error/warning` comment. scalafix-cli returns a non-zero exit code of kind "LinterError" when a linter reports an error. --- .../main/scala/scalafix/cli/CliRunner.scala | 4 +- .../main/scala/scalafix/cli/ExitStatus.scala | 3 +- .../internal/config/FilterMatcher.scala | 3 +- .../scalafix/internal/config/LintConfig.scala | 24 +++++++ .../internal/config/PrintStreamReporter.scala | 6 ++ .../internal/config/ScalafixConfig.scala | 11 +-- .../internal/config/ScalafixReporter.scala | 5 +- .../internal/rewrite/RemoveXmlLiterals.scala | 18 ++--- .../scala/scalafix/lint/LintMessage.scala | 68 +++++++++++++++++++ .../src/main/scala/scalafix/package.scala | 5 ++ .../src/main/scala/scalafix/patch/Patch.scala | 40 +++++++++-- .../main/scala/scalafix/patch/PatchOps.scala | 1 + .../main/scala/scalafix/rewrite/Rewrite.scala | 7 +- .../scala/scalafix/rewrite/RewriteCtx.scala | 20 ++++++ .../testkit/SemanticRewriteSuite.scala | 66 ++++++++++++++++-- .../main/scala/test/RemoveXmlLiterals.scala | 4 +- 16 files changed, 254 insertions(+), 31 deletions(-) create mode 100644 scalafix-core/shared/src/main/scala/scalafix/internal/config/LintConfig.scala create mode 100644 scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala diff --git a/scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala b/scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala index 6a4d414ff..00933ba54 100644 --- a/scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala +++ b/scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala @@ -80,7 +80,9 @@ sealed abstract case class CliRunner( code } display.stop() - exitCode.get() + val exit = exitCode.get() + if (!config.reporter.hasErrors) ExitStatus.merge(ExitStatus.LinterError, exit) + else exit } // safeguard to verify that the original file contents have not changed since the diff --git a/scalafix-cli/src/main/scala/scalafix/cli/ExitStatus.scala b/scalafix-cli/src/main/scala/scalafix/cli/ExitStatus.scala index 3b4c48d12..7cce62a46 100644 --- a/scalafix-cli/src/main/scala/scalafix/cli/ExitStatus.scala +++ b/scalafix-cli/src/main/scala/scalafix/cli/ExitStatus.scala @@ -30,7 +30,8 @@ object ExitStatus { InvalidCommandLineOption, MissingSemanticApi, StaleSemanticDB, - TestFailed + TestFailed, + LinterError : ExitStatus = generateExitStatus // format: on lazy val all: List[ExitStatus] = allInternal.toList diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/config/FilterMatcher.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/config/FilterMatcher.scala index 13745b399..65b76cd20 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/internal/config/FilterMatcher.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/config/FilterMatcher.scala @@ -22,7 +22,8 @@ case class FilterMatcher( } object FilterMatcher { - val matchEverything = new FilterMatcher(".*".r, mkRegexp(Nil)) + lazy val matchEverything = new FilterMatcher(".*".r, mkRegexp(Nil)) + lazy val matchNothing = new FilterMatcher(mkRegexp(Nil), mkRegexp(Nil)) implicit val reader: ConfDecoder[FilterMatcher] = matchEverything.reader def mkRegexp(filters: Seq[String]): Regex = diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/config/LintConfig.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/config/LintConfig.scala new file mode 100644 index 000000000..50afa7195 --- /dev/null +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/config/LintConfig.scala @@ -0,0 +1,24 @@ +package scalafix.internal.config + +import metaconfig.ConfDecoder + +case class LintConfig( + explain: Boolean = false, + ignore: FilterMatcher = FilterMatcher.matchNothing, + warning: FilterMatcher = FilterMatcher.matchNothing, + error: FilterMatcher = FilterMatcher.matchNothing +) { + val reader: ConfDecoder[LintConfig] = + ConfDecoder.instanceF[LintConfig] { c => + ( + c.getOrElse("explain")(explain) |@| + c.getOrElse("ignore")(ignore) |@| + c.getOrElse("warning")(warning) |@| + c.getOrElse("error")(error) + ).map { case (((a, b), c), d) => LintConfig(a, b, c, d) } + } +} + +object LintConfig { + lazy val default = LintConfig() +} diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/config/PrintStreamReporter.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/config/PrintStreamReporter.scala index 9ddd7a523..f8ff2e94d 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/internal/config/PrintStreamReporter.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/config/PrintStreamReporter.scala @@ -3,6 +3,7 @@ package scalafix.internal.config import scala.meta.Position import scala.meta.internal.inputs.XtensionPositionFormatMessage import java.io.PrintStream +import java.util.concurrent.atomic.AtomicReference import scalafix.internal.util.Severity import metaconfig._ @@ -28,12 +29,17 @@ case class PrintStreamReporter( ) } } + private val _hasError = new AtomicReference(false) override def report(message: String, position: Position, severity: Severity)( implicit ctx: LogContext): Unit = { + _hasError.compareAndSet(false, severity == Severity.Error) val enclosing = if (includeLoggerName) s"(${ctx.enclosing.value}) " else "" outStream.println( position.formatMessage(enclosing + severity.toString, message)) } + + /** Returns true if this reporter has seen an error */ + override def hasErrors: Boolean = _hasError.get() } diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/config/ScalafixConfig.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/config/ScalafixConfig.scala index 336372f0c..0b43d02f1 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/internal/config/ScalafixConfig.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/config/ScalafixConfig.scala @@ -19,7 +19,8 @@ case class ScalafixConfig( // Feel free to read data from here if your custom rewrite needs // configuration from the user. x: Conf = Conf.Obj(), - explicitReturnTypes: ExplicitReturnTypesConfig = ExplicitReturnTypesConfig() + explicitReturnTypes: ExplicitReturnTypesConfig = ExplicitReturnTypesConfig(), + lint: LintConfig = LintConfig.default ) { def getRewriteConfig[T: ConfDecoder](key: String, default: T): T = { x.getOrElse[T](key)(default).get @@ -34,16 +35,18 @@ case class ScalafixConfig( getOrElse("patches")(patches)(patches.reader) |@| getOrElse("dialect")(dialect) |@| getOrElse("x")(x) |@| - getOrElse("explicitReturnTypes")(explicitReturnTypes) + getOrElse("explicitReturnTypes")(explicitReturnTypes) |@| + getOrElse("lint")(lint)(lint.reader) ).map { - case (((((a, b), c), d), e), f) => + case ((((((a, b), c), d), e), f), g) => copy( fatalWarnings = a, reporter = b, patches = c, dialect = d, x = e, - explicitReturnTypes = f + explicitReturnTypes = f, + lint = g ) } diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/config/ScalafixReporter.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/config/ScalafixReporter.scala index aaccda1a5..14914ed76 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/internal/config/ScalafixReporter.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/config/ScalafixReporter.scala @@ -6,6 +6,9 @@ import metaconfig.ConfDecoder trait ScalafixReporter { + /** Returns true if this reporter has seen an error */ + def hasErrors: Boolean + /** Messages with severity < minSeverity are skipped. */ def minSeverity: Severity @@ -20,7 +23,7 @@ trait ScalafixReporter { protected def report(message: String, position: Position, severity: Severity)( implicit ctx: LogContext): Unit - protected def handleMessage( + private[scalafix] def handleMessage( message: String, position: Position, severity: Severity)(implicit ctx: LogContext): Unit = diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/rewrite/RemoveXmlLiterals.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/rewrite/RemoveXmlLiterals.scala index 8b9a6e4ff..476384c44 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/internal/rewrite/RemoveXmlLiterals.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/rewrite/RemoveXmlLiterals.scala @@ -1,5 +1,6 @@ package scalafix.internal.rewrite +import scalafix._ import scala.meta._ import scalafix.Patch import scalafix.rewrite.Rewrite @@ -22,6 +23,13 @@ import scalafix.rewrite.RewriteCtx */ case object RemoveXmlLiterals extends Rewrite { + val singleBracesEscape = LintId.warning( + """Single braces don't need be escaped with {{ and }} inside xml interpolators, unlike xml literals. + |For example {{ is identical to xml"{". This Rewrite will replace all occurrences of + |{{ and }}. Make sure this is intended. + |""".stripMargin + ) + override def rewrite(ctx: RewriteCtx): Patch = { def isMultiLine(xml: Term.Xml) = @@ -54,18 +62,12 @@ case object RemoveXmlLiterals extends Rewrite { /** Substitute {{ by { and }} by } */ def patchEscapedBraces(tok: Token.Xml.Part) = { - ctx.reporter.warn( - """Single braces don't need be escaped with {{ and }} inside the xml interpolator, unlike xml literals. - |For example {{ is identical to xml"{". - |This Rewrite will replace all occurrences of {{ and }} . Make sure this is intended. - """.stripMargin, - tok.pos - ) val patched = tok.value .replaceAllLiterally("{{", "{") .replaceAllLiterally("}}", "}") - ctx.replaceToken(tok, patched) + ctx.replaceToken(tok, patched) + + ctx.lint(singleBracesEscape.at(tok.pos)) } removeSplices(xml.tokens).collect { diff --git a/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala b/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala new file mode 100644 index 000000000..69303a6a8 --- /dev/null +++ b/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala @@ -0,0 +1,68 @@ +package scalafix.lint + +import scala.meta.Position +import scalafix.internal.util.Severity +import scalafix.rewrite.RewriteName + +final class LintID( + val id: String, + val owner: RewriteName, + val explanation: String, + val category: LintCategory +) { + override def toString: String = key + lazy val key = s"$owner.$id" + private def noExplanation: LintID = + new LintID(id, owner, explanation, category) + def at(message: String, position: Position): LintMessage = + new LintMessage(message, position, this) + def at(message: String): LintMessage = + new LintMessage(message, Position.None, this) + def at(position: Position): LintMessage = + new LintMessage(explanation, position, noExplanation) +} + +object LintID { + def error(explain: String)( + implicit alias: sourcecode.Name, + rewrite: RewriteName): LintID = + new LintID(alias.value, rewrite, explain, LintCategory.Error) + def warning(explain: String)( + implicit alias: sourcecode.Name, + rewrite: RewriteName): LintID = + new LintID(alias.value, rewrite, explain, LintCategory.Warning) +} + +class LintMessage( + val message: String, + val position: Position, + val id: LintID +) { + def format(explain: Boolean): String = { + val explanation = + if (explain) + s""" + |Explanation: + |${id.explanation} + |""".stripMargin + else "" + s"[${id.owner.name}.${id.id}] $message$explanation" + } +} + +sealed abstract class LintCategory { + private[scalafix] def toSeverity: Severity = this match { + case LintCategory.Error => Severity.Error + case LintCategory.Warning => Severity.Warn + } + def isError: Boolean = this == LintCategory.Error + + override def toString: String = this match { + case LintCategory.Error => "error" + case LintCategory.Warning => "warning" + } +} +object LintCategory { + case object Warning extends LintCategory + case object Error extends LintCategory +} diff --git a/scalafix-core/shared/src/main/scala/scalafix/package.scala b/scalafix-core/shared/src/main/scala/scalafix/package.scala index 424288839..f85ac34fb 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/package.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/package.scala @@ -29,6 +29,11 @@ package object scalafix { type Patch = patch.Patch val Patch = patch.Patch + type LintId = scalafix.lint.LintID + val LintId = scalafix.lint.LintID + + type LintMessage = scalafix.lint.LintMessage + implicit class XtensionSeqPatch(patches: Iterable[Patch]) { def asPatch: Patch = Patch.fromIterable(patches) } diff --git a/scalafix-core/shared/src/main/scala/scalafix/patch/Patch.scala b/scalafix-core/shared/src/main/scala/scalafix/patch/Patch.scala index fddf3353c..678386991 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/patch/Patch.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/patch/Patch.scala @@ -12,6 +12,7 @@ import scalafix.diff.DiffUtils import scalafix.internal.patch.ImportPatchOps import scalafix.internal.patch.ReplaceSymbolOps import scalafix.internal.util.TokenOps +import scalafix.lint.LintMessage import scalafix.patch.TreePatch.ReplaceSymbol import org.scalameta.logger @@ -88,6 +89,9 @@ private[scalafix] object TreePatch { } // implementation detail +private[scalafix] case class LintPatch( + message: LintMessage +) extends Patch private[scalafix] case class Concat(a: Patch, b: Patch) extends Patch private[scalafix] case object EmptyPatch extends Patch with LowLevelPatch @@ -115,10 +119,29 @@ object Patch { private[scalafix] def apply( p: Patch, ctx: RewriteCtx, - semanticCtx: Option[SemanticCtx]): String = { + semanticCtx: Option[SemanticCtx]): String = + applyInternal(p, ctx, semanticCtx) + + private[scalafix] def lintMessages( + patch: Patch, + ctx: RewriteCtx): List[LintMessage] = { + val builder = List.newBuilder[LintMessage] + foreach(patch) { + case LintPatch(lint) => + builder += lint + case _ => + } + builder.result() + } + + private[scalafix] def applyInternal( + p: Patch, + ctx: RewriteCtx, + semanticCtx: Option[SemanticCtx] + ): String = { val patches = underlying(p) val semanticPatches = patches.collect { case tp: TreePatch => tp } - semanticCtx match { + val result = semanticCtx match { case Some(x: SemanticCtx) => semanticApply(p)(ctx, x) case _ => @@ -129,6 +152,7 @@ object Patch { case tp: TokenPatch => tp }) } + result } private def syntaxApply( @@ -171,16 +195,24 @@ object Patch { private def underlying(patch: Patch): Seq[Patch] = { val builder = Seq.newBuilder[Patch] + foreach(patch) { + case _: LintPatch => + case els => + builder += els + } + builder.result() + } + + private def foreach(patch: Patch)(f: Patch => Unit): Unit = { def loop(patch: Patch): Unit = patch match { case Concat(a, b) => loop(a) loop(b) case EmptyPatch => // do nothing case els => - builder += els + f(els) } loop(patch) - builder.result() } def unifiedDiff(original: Input, revised: Input): String = { diff --git a/scalafix-core/shared/src/main/scala/scalafix/patch/PatchOps.scala b/scalafix-core/shared/src/main/scala/scalafix/patch/PatchOps.scala index 565cc5a53..d4f58a120 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/patch/PatchOps.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/patch/PatchOps.scala @@ -14,6 +14,7 @@ trait PatchOps { def rename(from: Name, to: String): Patch def addRight(tok: Token, toAdd: String): Patch def addLeft(tok: Token, toAdd: String): Patch + def lint(msg: LintMessage): Patch def removeGlobalImport(symbol: Symbol)(implicit semanticCtx: SemanticCtx): Patch def addGlobalImport(symbol: Symbol)(implicit semanticCtx: SemanticCtx): Patch diff --git a/scalafix-core/shared/src/main/scala/scalafix/rewrite/Rewrite.scala b/scalafix-core/shared/src/main/scala/scalafix/rewrite/Rewrite.scala index b0ff86426..197d7d497 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/rewrite/Rewrite.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/rewrite/Rewrite.scala @@ -28,10 +28,13 @@ abstract class Rewrite(implicit val rewriteName: RewriteName) { self => input: Input, config: ScalafixConfig = ScalafixConfig.default): String = { val ctx = RewriteCtx(config.dialect(input).parse[Source].get, config) - apply(ctx, rewrite(ctx)) + val patch = rewrite(ctx) + val result = apply(ctx, patch) + Patch.lintMessages(patch, ctx).foreach(ctx.printLintMessage) + result } final def apply(input: String): String = apply(Input.String(input)) - final protected def apply(ctx: RewriteCtx, patch: Patch): String = + final def apply(ctx: RewriteCtx, patch: Patch): String = Patch(patch, ctx, semanticOption) /** Returns unified diff from applying this patch */ diff --git a/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala b/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala index 30e1849f7..0b3876edd 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala @@ -8,6 +8,8 @@ import scalafix.internal.config.ScalafixConfig import scalafix.internal.config.ScalafixMetaconfigReaders import scalafix.internal.config.ScalafixReporter import scalafix.internal.util.SymbolOps.BottomSymbol +import scalafix.lint.LintCategory +import scalafix.patch.LintPatch import scalafix.patch.PatchOps import scalafix.patch.TokenPatch import scalafix.patch.TokenPatch.Add @@ -48,6 +50,24 @@ case class RewriteCtx(tree: Tree, config: ScalafixConfig) extends PatchOps { logger.elem(values: _*) } + def printLintMessage(msg: LintMessage): Unit = { + if (config.lint.ignore.matches(msg.id.key)) () + else { + val category = + if (config.lint.error.matches(msg.id.key)) LintCategory.Error + else if (config.lint.warning.matches(msg.id.key)) LintCategory.Warning + else msg.id.category + reporter.handleMessage( + msg.format(config.lint.explain), + msg.position, + category.toSeverity + ) + } + } + + def lint(msg: LintMessage): Patch = + LintPatch(msg) + // Syntactic patch ops. def removeImportee(importee: Importee): Patch = TreePatch.RemoveImportee(importee) diff --git a/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRewriteSuite.scala b/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRewriteSuite.scala index 8bcc11892..ab2bf15ac 100644 --- a/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRewriteSuite.scala +++ b/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRewriteSuite.scala @@ -1,12 +1,17 @@ package scalafix package testkit +import scala.collection.JavaConverters._ +import scala.collection.mutable import scalafix.syntax._ import scala.meta._ import scalafix.internal.util.SemanticCtxImpl import org.scalameta.logger import org.scalatest.BeforeAndAfterAll import org.scalatest.FunSuite +import org.scalatest.exceptions.TestFailedException +import scala.meta.internal.inputs.XtensionPositionFormatMessage +import scalafix.lint.LintCategory abstract class SemanticRewriteSuite( val semanticCtx: SemanticCtx, @@ -44,15 +49,62 @@ abstract class SemanticRewriteSuite( def runOn(diffTest: DiffTest): Unit = { test(diffTest.name) { val (rewrite, config) = diffTest.config.apply() - val obtainedWithComment = - rewrite.apply( - diffTest.original, - config.copy(dialect = diffTest.attributes.dialect)) + val ctx = RewriteCtx( + config.dialect(diffTest.original).parse[Source].get, + config.copy(dialect = diffTest.attributes.dialect) + ) + val patch = rewrite.rewrite(ctx) + val obtainedWithComment = rewrite.apply(ctx, patch) + val lintMessages = Patch.lintMessages(patch, ctx).to[mutable.Set] + def assertLint(position: Position, category: LintCategory): Unit = { + val matchingMessage = lintMessages.find { m => + // NOTE(olafur) I have no idea why -1 is necessary. + m.position.startLine == (position.startLine - 1) && + m.id.category == category + } + matchingMessage match { + case Some(x) => + lintMessages -= x + case None => + throw new TestFailedException( + position.formatMessage("error", s"No $category reported!"), + 0 + ) + } + } val obtained = { val tokens = obtainedWithComment.tokenize.get - val comment = tokens - .find(x => x.is[Token.Comment] && x.syntax.startsWith("/*")) - tokens.filterNot(comment.contains).mkString + val configComment = tokens.find { x => + x.is[Token.Comment] && x.syntax.startsWith("/*") + }.get + val LintAssertion = " scalafix: (.*)".r + tokens.filter { + case `configComment` => false + case tok @ Token.Comment(LintAssertion(severity)) => + severity match { + case "warning" => + assertLint(tok.pos, LintCategory.Warning) + case "error" => + assertLint(tok.pos, LintCategory.Error) + case els => + throw new TestFailedException( + tok.pos.formatMessage("error", s"Unknown severity '$els'"), + 0) + } + false + case _ => true + }.mkString + } + if (lintMessages.nonEmpty) { + Patch.lintMessages(patch, ctx).foreach(ctx.printLintMessage) + val explanation = + """To fix this problem, suffix the culprit lines with + | // scalafix: warning + | // scalafix: error + |""".stripMargin + throw new TestFailedException( + s"Uncaught linter messages! $explanation", + 0) } val candidateOutputFiles = expectedOutputSourceroot.flatMap { root => val scalaSpecificFilename = diff --git a/scalafix-tests/input/src/main/scala/test/RemoveXmlLiterals.scala b/scalafix-tests/input/src/main/scala/test/RemoveXmlLiterals.scala index 5823a2524..f4b662771 100644 --- a/scalafix-tests/input/src/main/scala/test/RemoveXmlLiterals.scala +++ b/scalafix-tests/input/src/main/scala/test/RemoveXmlLiterals.scala @@ -45,8 +45,8 @@ class RemoveXmlLiterals { } object I { - val a =
{{
- val b =
}}
+ val a =
{{
// scalafix: warning + val b =
}}
// scalafix: warning //
>>> xml"""
""" } From a014ff4cfd6ec24e387bc4724d2d3d9001cd2129 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93lafur=20P=C3=A1ll=20Geirsson?= Date: Wed, 16 Aug 2017 17:20:13 +0200 Subject: [PATCH 03/18] Fix failing tests and compilation errors. --- scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala | 3 ++- .../scalafix/internal/config/PrintStreamReporter.scala | 9 ++++++--- .../scalafix/internal/config/ScalafixReporter.scala | 5 ++++- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala b/scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala index 00933ba54..020a23ace 100644 --- a/scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala +++ b/scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala @@ -81,7 +81,8 @@ sealed abstract case class CliRunner( } display.stop() val exit = exitCode.get() - if (!config.reporter.hasErrors) ExitStatus.merge(ExitStatus.LinterError, exit) + if (!config.reporter.hasErrors) + ExitStatus.merge(ExitStatus.LinterError, exit) else exit } diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/config/PrintStreamReporter.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/config/PrintStreamReporter.scala index f8ff2e94d..29f203738 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/internal/config/PrintStreamReporter.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/config/PrintStreamReporter.scala @@ -3,6 +3,7 @@ package scalafix.internal.config import scala.meta.Position import scala.meta.internal.inputs.XtensionPositionFormatMessage import java.io.PrintStream +import java.util.concurrent.atomic.AtomicInteger import java.util.concurrent.atomic.AtomicReference import scalafix.internal.util.Severity import metaconfig._ @@ -29,11 +30,13 @@ case class PrintStreamReporter( ) } } - private val _hasError = new AtomicReference(false) + private val _errorCount = new AtomicInteger() override def report(message: String, position: Position, severity: Severity)( implicit ctx: LogContext): Unit = { - _hasError.compareAndSet(false, severity == Severity.Error) + if (severity == Severity.Error) { + _errorCount.incrementAndGet() + } val enclosing = if (includeLoggerName) s"(${ctx.enclosing.value}) " else "" outStream.println( @@ -41,5 +44,5 @@ case class PrintStreamReporter( } /** Returns true if this reporter has seen an error */ - override def hasErrors: Boolean = _hasError.get() + override def errorCount: Int = _errorCount.get() } diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/config/ScalafixReporter.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/config/ScalafixReporter.scala index 14914ed76..97c8b7379 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/internal/config/ScalafixReporter.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/config/ScalafixReporter.scala @@ -7,7 +7,10 @@ import metaconfig.ConfDecoder trait ScalafixReporter { /** Returns true if this reporter has seen an error */ - def hasErrors: Boolean + def hasErrors: Boolean = errorCount > 0 + + /** Returns the number of reported errors */ + def errorCount: Int /** Messages with severity < minSeverity are skipped. */ def minSeverity: Severity From 6e5b915a39c30b71e0d70c6ed9469097f1f97ca6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93lafur=20P=C3=A1ll=20Geirsson?= Date: Thu, 17 Aug 2017 11:03:03 +0200 Subject: [PATCH 04/18] Fix bug where cli always returned LinterError. --- scalafix-cli/src/main/scala/scalafix/cli/Cli.scala | 9 ++++++++- scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala | 6 +++--- .../scala/scalafix/internal/cli/ScalafixOptions.scala | 4 ---- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/scalafix-cli/src/main/scala/scalafix/cli/Cli.scala b/scalafix-cli/src/main/scala/scalafix/cli/Cli.scala index ba0ad6829..4059e14f4 100644 --- a/scalafix-cli/src/main/scala/scalafix/cli/Cli.scala +++ b/scalafix-cli/src/main/scala/scalafix/cli/Cli.scala @@ -10,6 +10,9 @@ import scalafix.internal.cli.CommonOptions import scalafix.internal.cli.ScalafixOptions import scalafix.rewrite.ScalafixRewrites import scala.meta.io.AbsolutePath +import scalafix.internal.config.PrintStreamReporter +import scalafix.internal.config.ScalafixReporter +import scalafix.internal.util.Severity import caseapp.Name import caseapp.core.Arg import caseapp.core.Messages @@ -222,7 +225,11 @@ object ScalafixRewriteNames { val result = cliCommand match { case CliCommand.PrintAndExit(msg, exit) => if (exit.isOk) commonOptions.out.println(msg) - else commonOptions.reporter.error(msg) + else { + ScalafixReporter.default + .copy(outStream = commonOptions.err) + .error(msg) + } exit case CliCommand.RunScalafix(runner) => val exit = runner.run() diff --git a/scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala b/scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala index 020a23ace..b3302197c 100644 --- a/scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala +++ b/scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala @@ -81,9 +81,9 @@ sealed abstract case class CliRunner( } display.stop() val exit = exitCode.get() - if (!config.reporter.hasErrors) + if (config.reporter.hasErrors) { ExitStatus.merge(ExitStatus.LinterError, exit) - else exit + } else exit } // safeguard to verify that the original file contents have not changed since the @@ -173,7 +173,7 @@ sealed abstract case class CliRunner( path: AbsolutePath, cause: Throwable, options: ScalafixOptions): Unit = { - options.common.reporter.error(s"Failed to fix $path") + config.reporter.error(s"Failed to fix $path") cause.setStackTrace(cause.getStackTrace.take(options.common.stackVerbosity)) cause.printStackTrace(options.common.err) } diff --git a/scalafix-cli/src/main/scala/scalafix/internal/cli/ScalafixOptions.scala b/scalafix-cli/src/main/scala/scalafix/internal/cli/ScalafixOptions.scala index 1743c1f84..4cb52fcf3 100644 --- a/scalafix-cli/src/main/scala/scalafix/internal/cli/ScalafixOptions.scala +++ b/scalafix-cli/src/main/scala/scalafix/internal/cli/ScalafixOptions.scala @@ -5,8 +5,6 @@ import java.io.InputStream import java.io.PrintStream import scala.meta._ import scala.meta.io.AbsolutePath -import scalafix.internal.config.PrintStreamReporter -import scalafix.internal.config.ScalafixReporter import scalafix.internal.rewrite.ProcedureSyntax import scalafix.rewrite.ScalafixRewrites import caseapp._ @@ -18,8 +16,6 @@ case class CommonOptions( @Hidden err: PrintStream = System.err, @Hidden stackVerbosity: Int = 20 ) { - def reporter: PrintStreamReporter = - ScalafixReporter.default.copy(outStream = err) def workingPath = AbsolutePath(workingDirectory) def workingDirectoryFile = new File(workingDirectory) } From 9f11786cf9ed607c07f67382d23c47387cd16c9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93lafur=20P=C3=A1ll=20Geirsson?= Date: Thu, 17 Aug 2017 11:09:16 +0200 Subject: [PATCH 05/18] Add LintCategory.Info --- .../scalafix/internal/config/FilterMatcher.scala | 2 ++ .../scala/scalafix/internal/config/LintConfig.scala | 13 ++++++++++++- .../src/main/scala/scalafix/lint/LintMessage.scala | 4 +++- .../main/scala/scalafix/rewrite/RewriteCtx.scala | 6 +----- 4 files changed, 18 insertions(+), 7 deletions(-) diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/config/FilterMatcher.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/config/FilterMatcher.scala index 65b76cd20..5ce778397 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/internal/config/FilterMatcher.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/config/FilterMatcher.scala @@ -19,6 +19,8 @@ case class FilterMatcher( def matches(input: String): Boolean = includeFilters.findFirstIn(input).isDefined && excludeFilters.findFirstIn(input).isEmpty + def unapply(arg: String): Boolean = + matches(arg) } object FilterMatcher { diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/config/LintConfig.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/config/LintConfig.scala index 50afa7195..ce6308c05 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/internal/config/LintConfig.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/config/LintConfig.scala @@ -1,10 +1,13 @@ package scalafix.internal.config +import scalafix.lint.LintCategory +import scalafix.lint.LintID import metaconfig.ConfDecoder case class LintConfig( explain: Boolean = false, ignore: FilterMatcher = FilterMatcher.matchNothing, + info: FilterMatcher = FilterMatcher.matchNothing, warning: FilterMatcher = FilterMatcher.matchNothing, error: FilterMatcher = FilterMatcher.matchNothing ) { @@ -13,10 +16,18 @@ case class LintConfig( ( c.getOrElse("explain")(explain) |@| c.getOrElse("ignore")(ignore) |@| + c.getOrElse("info")(info) |@| c.getOrElse("warning")(warning) |@| c.getOrElse("error")(error) - ).map { case (((a, b), c), d) => LintConfig(a, b, c, d) } + ).map { case ((((a, b), c), d), e) => LintConfig(a, b, c, d, e) } } + + def getCategory(id: LintID): LintCategory = id.key match { + case error() => LintCategory.Error + case warning() => LintCategory.Warning + case info() => LintCategory.Info + case _ => id.category + } } object LintConfig { diff --git a/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala b/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala index 69303a6a8..1051439b9 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala @@ -54,15 +54,17 @@ sealed abstract class LintCategory { private[scalafix] def toSeverity: Severity = this match { case LintCategory.Error => Severity.Error case LintCategory.Warning => Severity.Warn + case LintCategory.Info => Severity.Info } def isError: Boolean = this == LintCategory.Error - override def toString: String = this match { case LintCategory.Error => "error" case LintCategory.Warning => "warning" + case LintCategory.Info => "info" } } object LintCategory { + case object Info extends LintCategory case object Warning extends LintCategory case object Error extends LintCategory } diff --git a/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala b/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala index 0b3876edd..233a05d39 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala @@ -53,14 +53,10 @@ case class RewriteCtx(tree: Tree, config: ScalafixConfig) extends PatchOps { def printLintMessage(msg: LintMessage): Unit = { if (config.lint.ignore.matches(msg.id.key)) () else { - val category = - if (config.lint.error.matches(msg.id.key)) LintCategory.Error - else if (config.lint.warning.matches(msg.id.key)) LintCategory.Warning - else msg.id.category reporter.handleMessage( msg.format(config.lint.explain), msg.position, - category.toSeverity + config.lint.getCategory(msg.id).toSeverity ) } } From 33ec950d3327c7fef910367d0728c7587e72f3b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93lafur=20P=C3=A1ll=20Geirsson?= Date: Thu, 17 Aug 2017 11:17:34 +0200 Subject: [PATCH 06/18] Add docstrings for new scalafix.lint data structures. --- .../internal/rewrite/RemoveXmlLiterals.scala | 2 +- .../scala/scalafix/lint/LintCategory.scala | 23 ++++++++ .../src/main/scala/scalafix/lint/LintID.scala | 43 ++++++++++++++ .../scala/scalafix/lint/LintMessage.scala | 58 +++---------------- .../src/main/scala/scalafix/package.scala | 4 +- 5 files changed, 77 insertions(+), 53 deletions(-) create mode 100644 scalafix-core/shared/src/main/scala/scalafix/lint/LintCategory.scala create mode 100644 scalafix-core/shared/src/main/scala/scalafix/lint/LintID.scala diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/rewrite/RemoveXmlLiterals.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/rewrite/RemoveXmlLiterals.scala index 476384c44..5e75eb033 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/internal/rewrite/RemoveXmlLiterals.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/rewrite/RemoveXmlLiterals.scala @@ -23,7 +23,7 @@ import scalafix.rewrite.RewriteCtx */ case object RemoveXmlLiterals extends Rewrite { - val singleBracesEscape = LintId.warning( + val singleBracesEscape = LintID.warning( """Single braces don't need be escaped with {{ and }} inside xml interpolators, unlike xml literals. |For example {{ is identical to xml"{". This Rewrite will replace all occurrences of |{{ and }}. Make sure this is intended. diff --git a/scalafix-core/shared/src/main/scala/scalafix/lint/LintCategory.scala b/scalafix-core/shared/src/main/scala/scalafix/lint/LintCategory.scala new file mode 100644 index 000000000..d7b8abf8a --- /dev/null +++ b/scalafix-core/shared/src/main/scala/scalafix/lint/LintCategory.scala @@ -0,0 +1,23 @@ +package scalafix.lint + +import scalafix.internal.util.Severity + +sealed abstract class LintCategory { + private[scalafix] def toSeverity: Severity = this match { + case LintCategory.Error => Severity.Error + case LintCategory.Warning => Severity.Warn + case LintCategory.Info => Severity.Info + } + def isError: Boolean = this == LintCategory.Error + override def toString: String = this match { + case LintCategory.Error => "error" + case LintCategory.Warning => "warning" + case LintCategory.Info => "info" + } +} + +object LintCategory { + case object Info extends LintCategory + case object Warning extends LintCategory + case object Error extends LintCategory +} \ No newline at end of file diff --git a/scalafix-core/shared/src/main/scala/scalafix/lint/LintID.scala b/scalafix-core/shared/src/main/scala/scalafix/lint/LintID.scala new file mode 100644 index 000000000..03e84147e --- /dev/null +++ b/scalafix-core/shared/src/main/scala/scalafix/lint/LintID.scala @@ -0,0 +1,43 @@ +package scalafix.lint + +import scalafix.rewrite.RewriteName +import scala.meta.inputs.Position + +/** A unique identifier for one kind of a linter message. + * + * @param id a string ID for this message, typically the name of the + * assigned variable. + * @param owner The rewrite that owns this lint message, to distinguish + * lint messages with conflicting IDs from different rewrites. + * @param explanation An optional explanation for this kind of message. + * @param category The default category this message should get reported to. + * Note that users can configure/override the default category. + */ +final class LintID( + val id: String, + val owner: RewriteName, + val explanation: String, + val category: LintCategory +) { + override def toString: String = key + lazy val key = s"$owner.$id" + private def noExplanation: LintID = + new LintID(id, owner, explanation, category) + def at(message: String, position: Position): LintMessage = + new LintMessage(message, position, this) + def at(message: String): LintMessage = + new LintMessage(message, Position.None, this) + def at(position: Position): LintMessage = + new LintMessage(explanation, position, noExplanation) +} + +object LintID { + def error(explain: String)( + implicit alias: sourcecode.Name, + rewrite: RewriteName): LintID = + new LintID(alias.value, rewrite, explain, LintCategory.Error) + def warning(explain: String)( + implicit alias: sourcecode.Name, + rewrite: RewriteName): LintID = + new LintID(alias.value, rewrite, explain, LintCategory.Warning) +} diff --git a/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala b/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala index 1051439b9..7d0ad9869 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala @@ -1,38 +1,15 @@ package scalafix.lint import scala.meta.Position -import scalafix.internal.util.Severity -import scalafix.rewrite.RewriteName - -final class LintID( - val id: String, - val owner: RewriteName, - val explanation: String, - val category: LintCategory -) { - override def toString: String = key - lazy val key = s"$owner.$id" - private def noExplanation: LintID = - new LintID(id, owner, explanation, category) - def at(message: String, position: Position): LintMessage = - new LintMessage(message, position, this) - def at(message: String): LintMessage = - new LintMessage(message, Position.None, this) - def at(position: Position): LintMessage = - new LintMessage(explanation, position, noExplanation) -} - -object LintID { - def error(explain: String)( - implicit alias: sourcecode.Name, - rewrite: RewriteName): LintID = - new LintID(alias.value, rewrite, explain, LintCategory.Error) - def warning(explain: String)( - implicit alias: sourcecode.Name, - rewrite: RewriteName): LintID = - new LintID(alias.value, rewrite, explain, LintCategory.Warning) -} +/** An instance of a LintID with a custom message at a particular position + * + * @param message The message to display to the user. If empty, LintID.explanation + * is used instead. + * @param position Optionally place a caret under a location in a source file. + * For an empty position use Position.None. + * @param id the LintID associated with this message. + */ class LintMessage( val message: String, val position: Position, @@ -49,22 +26,3 @@ class LintMessage( s"[${id.owner.name}.${id.id}] $message$explanation" } } - -sealed abstract class LintCategory { - private[scalafix] def toSeverity: Severity = this match { - case LintCategory.Error => Severity.Error - case LintCategory.Warning => Severity.Warn - case LintCategory.Info => Severity.Info - } - def isError: Boolean = this == LintCategory.Error - override def toString: String = this match { - case LintCategory.Error => "error" - case LintCategory.Warning => "warning" - case LintCategory.Info => "info" - } -} -object LintCategory { - case object Info extends LintCategory - case object Warning extends LintCategory - case object Error extends LintCategory -} diff --git a/scalafix-core/shared/src/main/scala/scalafix/package.scala b/scalafix-core/shared/src/main/scala/scalafix/package.scala index f85ac34fb..0be65babb 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/package.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/package.scala @@ -29,8 +29,8 @@ package object scalafix { type Patch = patch.Patch val Patch = patch.Patch - type LintId = scalafix.lint.LintID - val LintId = scalafix.lint.LintID + type LintID = scalafix.lint.LintID + val LintID = scalafix.lint.LintID type LintMessage = scalafix.lint.LintMessage From c95f1b249e9fcd8334531b2ef8cf685c9afba2cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93lafur=20P=C3=A1ll=20Geirsson?= Date: Thu, 17 Aug 2017 11:35:07 +0200 Subject: [PATCH 07/18] Refactor RewriteName. - RewriteName is a list of RewriteIdentifier. - RewriteIdentifier is a name + optional deprecation warning. This makes it possible to rename a rewrite without breaking peoples config. - Rewrite.empty has name RewriteName.empty, which is an empty list of RewriteIdentifier. --- .../main/scala/scalafix/rewrite/Rewrite.scala | 23 ++++++------ .../scala/scalafix/rewrite/RewriteName.scala | 36 +++++++++++-------- .../scalafix/rewrite/ScalafixRewrites.scala | 7 ++-- 3 files changed, 38 insertions(+), 28 deletions(-) diff --git a/scalafix-core/shared/src/main/scala/scalafix/rewrite/Rewrite.scala b/scalafix-core/shared/src/main/scala/scalafix/rewrite/Rewrite.scala index 197d7d497..5f7ee016b 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/rewrite/Rewrite.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/rewrite/Rewrite.scala @@ -8,7 +8,6 @@ import scalafix.internal.config.ScalafixConfig import scalafix.syntax._ import metaconfig.ConfDecoder import metaconfig.Configured -import sourcecode.Name /** A Rewrite is a program that produces a Patch from a scala.meta.Tree. */ abstract class Rewrite(implicit val rewriteName: RewriteName) { self => @@ -49,6 +48,7 @@ abstract class Rewrite(implicit val rewriteName: RewriteName) { self => } final def name: String = rewriteName.toString + final def names: List[String] = rewriteName.identifiers.map(_.value) final override def toString: String = name.toString // NOTE. This is kind of hacky and hopefully we can find a better workaround. @@ -58,7 +58,8 @@ abstract class Rewrite(implicit val rewriteName: RewriteName) { self => protected[scalafix] def semanticOption: Option[SemanticCtx] = None } -abstract class SemanticRewrite(semanticCtx: SemanticCtx)(implicit name: Name) +abstract class SemanticRewrite(semanticCtx: SemanticCtx)( + implicit name: RewriteName) extends Rewrite { implicit val ImplicitSemanticCtx: SemanticCtx = semanticCtx override def semanticOption: Option[SemanticCtx] = Some(semanticCtx) @@ -68,33 +69,33 @@ object Rewrite { val syntaxRewriteConfDecoder: ConfDecoder[Rewrite] = ScalafixMetaconfigReaders.rewriteConfDecoderSyntactic( ScalafixMetaconfigReaders.baseSyntacticRewriteDecoder) + lazy val empty: Rewrite = syntactic(_ => Patch.empty)(RewriteName.empty) def emptyConfigured: Configured[Rewrite] = Configured.Ok(empty) - def empty: Rewrite = syntactic(_ => Patch.empty) def emptyFromSemanticCtxOpt(semanticCtx: Option[SemanticCtx]): Rewrite = semanticCtx.fold(empty)(emptySemantic) def combine(rewrites: Seq[Rewrite]): Rewrite = rewrites.foldLeft(empty)(_ andThen _) - // into an actual rewrite instead of handling it specially inside Patch.applied. private[scalafix] def emptySemantic(semanticCtx: SemanticCtx): Rewrite = - semantic(x => y => Patch.empty)(Name("empty"))(semanticCtx) + semantic(_ => _ => Patch.empty)(RewriteName.empty)(semanticCtx) /** Creates a syntactic rewrite. */ - def syntactic(f: RewriteCtx => Patch)(implicit name: Name): Rewrite = + def syntactic(f: RewriteCtx => Patch)(implicit name: RewriteName): Rewrite = new Rewrite() { override def rewrite(ctx: RewriteCtx): Patch = f(ctx) } /** Creates a semantic rewrite. */ def semantic(f: SemanticCtx => RewriteCtx => Patch)( - implicit name: Name): SemanticCtx => Rewrite = { semanticCtx => - new SemanticRewrite(semanticCtx) { - override def rewrite(ctx: RewriteCtx): Patch = f(semanticCtx)(ctx) - } + implicit rewriteName: RewriteName): SemanticCtx => Rewrite = { + semanticCtx => + new SemanticRewrite(semanticCtx) { + override def rewrite(ctx: RewriteCtx): Patch = f(semanticCtx)(ctx) + } } /** Creates a rewrite that always returns the same patch. */ def constant(name: String, patch: Patch, semanticCtx: SemanticCtx): Rewrite = - new SemanticRewrite(semanticCtx)(Name(name)) { + new SemanticRewrite(semanticCtx)(RewriteName(name)) { override def rewrite(ctx: RewriteCtx): Patch = patch } diff --git a/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteName.scala b/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteName.scala index e599a2a6d..09f364bee 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteName.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteName.scala @@ -1,24 +1,32 @@ package scalafix.rewrite -final class RewriteName(val head: String, val tail: List[String]) { - require( - head != RewriteName.emptyHead || tail.isEmpty, - "Empty rewrite cannot have underlying names!" - ) +/** A thin wrapper around a string name and optional deprecation warning. */ +final case class RewriteIdentifier( + value: String, + deprecated: Option[scala.deprecated] +) { + override def toString: String = value +} + +object RewriteIdentifier { + def apply(value: String) = + new RewriteIdentifier(value, None) +} + +/** A thin wrapper around a list of RewriteIdentifier. */ +final case class RewriteName(identifiers: List[RewriteIdentifier]) { def name: String = - (head :: tail).mkString("+") - def isEmpty: Boolean = head == RewriteName.emptyHead + if (identifiers.isEmpty) "empty" + else identifiers.mkString("+") + def isEmpty: Boolean = identifiers.isEmpty def +(other: RewriteName): RewriteName = - if (isEmpty) other - else if (other.isEmpty) this - else new RewriteName(head, head :: tail ::: other.tail) + new RewriteName((identifiers :: other.identifiers :: Nil).flatten) override def toString: String = name } object RewriteName { - private[scalafix] val emptyHead = "empty" - final val empty = new RewriteName(emptyHead, Nil) - def apply(name: String) = new RewriteName(name, Nil) + final val empty = new RewriteName(Nil) + def apply(name: String) = new RewriteName(RewriteIdentifier(name) :: Nil) implicit def generate(implicit name: sourcecode.Name): RewriteName = - new RewriteName(name.value, Nil) + RewriteName(name.value) } diff --git a/scalafix-core/shared/src/main/scala/scalafix/rewrite/ScalafixRewrites.scala b/scalafix-core/shared/src/main/scala/scalafix/rewrite/ScalafixRewrites.scala index 04ad4db5f..2954a99f6 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/rewrite/ScalafixRewrites.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/rewrite/ScalafixRewrites.scala @@ -22,11 +22,12 @@ object ScalafixRewrites { def all(semanticCtx: SemanticCtx): List[Rewrite] = syntax ++ semantic(semanticCtx) def name2rewrite(semanticCtx: SemanticCtx): Map[String, Rewrite] = - all(semanticCtx).map(x => x.name -> x).toMap + all(semanticCtx).flatMap(x => x.names.map(_ -> x)).toMap lazy val syntaxName2rewrite: Map[String, Rewrite] = - syntax.map(x => x.name -> x).toMap + syntax.flatMap(x => x.names.map(_ -> x)).toMap val emptyDatabase = SemanticCtx(Nil) lazy val syntacticNames: List[String] = syntaxName2rewrite.keys.toList - lazy val semanticNames: List[String] = semantic(emptyDatabase).map(_.name) + lazy val semanticNames: List[String] = + semantic(emptyDatabase).flatMap(_.names) def allNames: List[String] = syntaxName2rewrite.keys.toList ++ semanticNames } From 9aec7a0ca20928db309187a6d876b4a680de44a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93lafur=20P=C3=A1ll=20Geirsson?= Date: Thu, 17 Aug 2017 12:26:04 +0200 Subject: [PATCH 08/18] Emit deprecation warnings when referencing a rewrite by old name. This change allows rewrites to change their names without breaking compatibility. Users using the old name will receive deprecation warnings and have opportunity to switch to the new name. --- .../src/main/scala/scalafix/cli/Cli.scala | 6 +---- .../main/scala/scalafix/cli/CliRunner.scala | 3 ++- .../internal/cli/ScalafixOptions.scala | 4 ++++ .../main/scala/scalafix/config/package.scala | 2 +- .../internal/config/LazySemanticCtx.scala | 24 +++++++++++++++++++ .../config/ScalafixMetaconfigReaders.scala | 13 ++++++---- .../scalafix/internal/config/package.scala | 8 ------- .../scala/scalafix/rewrite/RewriteName.scala | 14 ++++++++++- .../main/scala/scalafix/util/Deprecated.scala | 4 ++++ .../scalafix/reflect/ScalafixReflect.scala | 4 ++-- .../scala/scalafix/testkit/DiffTest.scala | 6 +++-- 11 files changed, 64 insertions(+), 24 deletions(-) create mode 100644 scalafix-core/shared/src/main/scala/scalafix/internal/config/LazySemanticCtx.scala create mode 100644 scalafix-core/shared/src/main/scala/scalafix/util/Deprecated.scala diff --git a/scalafix-cli/src/main/scala/scalafix/cli/Cli.scala b/scalafix-cli/src/main/scala/scalafix/cli/Cli.scala index 4059e14f4..a8ae1b5f4 100644 --- a/scalafix-cli/src/main/scala/scalafix/cli/Cli.scala +++ b/scalafix-cli/src/main/scala/scalafix/cli/Cli.scala @@ -225,11 +225,7 @@ object ScalafixRewriteNames { val result = cliCommand match { case CliCommand.PrintAndExit(msg, exit) => if (exit.isOk) commonOptions.out.println(msg) - else { - ScalafixReporter.default - .copy(outStream = commonOptions.err) - .error(msg) - } + else commonOptions.reporter.error(msg) exit case CliCommand.RunScalafix(runner) => val exit = runner.run() diff --git a/scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala b/scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala index b3302197c..fc83a3133 100644 --- a/scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala +++ b/scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala @@ -333,7 +333,8 @@ object CliRunner { if (kind.isSyntactic) None else computeAndCacheDatabase() } - private val lazySemanticCtx: LazySemanticCtx = resolveDatabase + private val lazySemanticCtx: LazySemanticCtx = + new LazySemanticCtx(resolveDatabase, common.reporter) // expands a single file into a list of files. def expand(matcher: FilterMatcher)(path: AbsolutePath): Seq[FixFile] = { diff --git a/scalafix-cli/src/main/scala/scalafix/internal/cli/ScalafixOptions.scala b/scalafix-cli/src/main/scala/scalafix/internal/cli/ScalafixOptions.scala index 4cb52fcf3..7841d5793 100644 --- a/scalafix-cli/src/main/scala/scalafix/internal/cli/ScalafixOptions.scala +++ b/scalafix-cli/src/main/scala/scalafix/internal/cli/ScalafixOptions.scala @@ -5,6 +5,8 @@ import java.io.InputStream import java.io.PrintStream import scala.meta._ import scala.meta.io.AbsolutePath +import scalafix.internal.config.PrintStreamReporter +import scalafix.internal.config.ScalafixReporter import scalafix.internal.rewrite.ProcedureSyntax import scalafix.rewrite.ScalafixRewrites import caseapp._ @@ -16,6 +18,8 @@ case class CommonOptions( @Hidden err: PrintStream = System.err, @Hidden stackVerbosity: Int = 20 ) { + lazy val reporter: PrintStreamReporter = + ScalafixReporter.default.copy(outStream = out) def workingPath = AbsolutePath(workingDirectory) def workingDirectoryFile = new File(workingDirectory) } diff --git a/scalafix-core/shared/src/main/scala/scalafix/config/package.scala b/scalafix-core/shared/src/main/scala/scalafix/config/package.scala index bf9c411c2..f278cef21 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/config/package.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/config/package.scala @@ -18,7 +18,7 @@ package object config { configuration: String, decoder: ConfDecoder[Rewrite] ): Configured[(Rewrite, internal.config.ScalafixConfig)] = - fromInput(Input.String(configuration), _ => None, Nil, decoder) + fromInput(Input.String(configuration), LazySemanticCtx.empty, Nil, decoder) /** Load configuration from an input. * diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/config/LazySemanticCtx.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/config/LazySemanticCtx.scala new file mode 100644 index 000000000..37e081801 --- /dev/null +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/config/LazySemanticCtx.scala @@ -0,0 +1,24 @@ +package scalafix.internal.config + +import scalafix.SemanticCtx + +// The challenge when loading a rewrite is that 1) if it's semantic it needs a +// semanticCtx constructor argument and 2) we don't know upfront if it's semantic. +// For example, to know if a classloaded rewrites is semantic or syntactic +// we have to test against it's Class[_]. For default rewrites, the interface +// to detect if a rewrite is semantic is different. +// LazySemanticCtx allows us to delay the computation of a semanticCtx right up until +// the moment we instantiate the rewrite. +//type LazySemanticCtx = RewriteKind => Option[SemanticCtx] +class LazySemanticCtx( + f: RewriteKind => Option[SemanticCtx], + val reporter: ScalafixReporter) + extends Function[RewriteKind, Option[SemanticCtx]] { + override def apply(v1: RewriteKind): Option[SemanticCtx] = f(v1) +} + +object LazySemanticCtx { + lazy val empty = new LazySemanticCtx(_ => None, ScalafixReporter.default) + def apply(f: RewriteKind => Option[SemanticCtx]): LazySemanticCtx = + new LazySemanticCtx(f, ScalafixReporter.default) +} diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/config/ScalafixMetaconfigReaders.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/config/ScalafixMetaconfigReaders.scala index 9ed4726d9..a0f4eed7c 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/internal/config/ScalafixMetaconfigReaders.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/config/ScalafixMetaconfigReaders.scala @@ -102,15 +102,20 @@ trait ScalafixMetaconfigReaders { ScalafixRewrites.syntaxName2rewrite ++ semanticCtx.fold(Map.empty[String, Rewrite])( ScalafixRewrites.name2rewrite) - ReaderUtil.fromMap(names).read(conf) + val result = ReaderUtil.fromMap(names).read(conf) + result match { + case Ok(rewrite) => + rewrite.rewriteName + .reportDeprecationWarning(value, getSemanticCtx.reporter) + case _ => + } + result } private lazy val semanticRewriteClass = classOf[SemanticRewrite] def classloadRewrite( semanticCtx: LazySemanticCtx): Class[_] => Seq[SemanticCtx] = { cls => - val semanticRewrite = - cls.getClassLoader.loadClass("scalafix.rewrite.SemanticRewrite") val kind = if (semanticRewriteClass.isAssignableFrom(cls)) RewriteKind.Semantic else RewriteKind.Syntactic @@ -146,7 +151,7 @@ trait ScalafixMetaconfigReaders { } def baseSyntacticRewriteDecoder: ConfDecoder[Rewrite] = - baseRewriteDecoders(_ => None) + baseRewriteDecoders(LazySemanticCtx.empty) def baseRewriteDecoders(semanticCtx: LazySemanticCtx): ConfDecoder[Rewrite] = { MetaconfigPendingUpstream.orElse( defaultRewriteDecoder(semanticCtx), diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/config/package.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/config/package.scala index af212fc09..ad884fbdc 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/internal/config/package.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/config/package.scala @@ -6,12 +6,4 @@ import scalafix.SemanticCtx package object config extends ScalafixMetaconfigReaders { type MetaParser = Parse[_ <: Tree] - // The challenge when loading a rewrite is that 1) if it's semantic it needs a - // semanticCtx constructor argument and 2) we don't know upfront if it's semantic. - // For example, to know if a classloaded rewrites is semantic or syntactic - // we have to test against it's Class[_]. For default rewrites, the interface - // to detect if a rewrite is semantic is different. - // LazySemanticCtx allows us to delay the computation of a semanticCtx right up until - // the moment we instantiate the rewrite. - type LazySemanticCtx = RewriteKind => Option[SemanticCtx] } diff --git a/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteName.scala b/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteName.scala index 09f364bee..d8a39754f 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteName.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteName.scala @@ -1,9 +1,11 @@ package scalafix.rewrite +import scalafix.internal.config.ScalafixReporter + /** A thin wrapper around a string name and optional deprecation warning. */ final case class RewriteIdentifier( value: String, - deprecated: Option[scala.deprecated] + deprecated: Option[scalafix.util.Deprecated] ) { override def toString: String = value } @@ -22,6 +24,16 @@ final case class RewriteName(identifiers: List[RewriteIdentifier]) { def +(other: RewriteName): RewriteName = new RewriteName((identifiers :: other.identifiers :: Nil).flatten) override def toString: String = name + def reportDeprecationWarning(name: String, reporter: ScalafixReporter): Unit = { + identifiers.foreach { ident => + if (ident.value == name) { + ident.deprecated.foreach { d => + reporter.warn( + s"Name $name is deprecated. ${d.message} (since ${d.since})") + } + } + } + } } object RewriteName { diff --git a/scalafix-core/shared/src/main/scala/scalafix/util/Deprecated.scala b/scalafix-core/shared/src/main/scala/scalafix/util/Deprecated.scala new file mode 100644 index 000000000..90bd2b083 --- /dev/null +++ b/scalafix-core/shared/src/main/scala/scalafix/util/Deprecated.scala @@ -0,0 +1,4 @@ +package scalafix.util + +/** Identical to scala.deprecated except it's a case class. */ +final case class Deprecated(message: String, since: String) diff --git a/scalafix-reflect/src/main/scala/scalafix/reflect/ScalafixReflect.scala b/scalafix-reflect/src/main/scala/scalafix/reflect/ScalafixReflect.scala index 32be25ed6..01aa220fb 100644 --- a/scalafix-reflect/src/main/scala/scalafix/reflect/ScalafixReflect.scala +++ b/scalafix-reflect/src/main/scala/scalafix/reflect/ScalafixReflect.scala @@ -8,10 +8,10 @@ import metaconfig.ConfDecoder object ScalafixReflect { def syntactic: ConfDecoder[Rewrite] = - fromLazySemanticCtx(_ => None) + fromLazySemanticCtx(LazySemanticCtx.empty) def semantic(semanticCtx: SemanticCtx): ConfDecoder[Rewrite] = - fromLazySemanticCtx(_ => Some(semanticCtx)) + fromLazySemanticCtx(LazySemanticCtx(_ => Some(semanticCtx))) def fromLazySemanticCtx(semanticCtx: LazySemanticCtx): ConfDecoder[Rewrite] = rewriteConfDecoder( diff --git a/scalafix-testkit/src/main/scala/scalafix/testkit/DiffTest.scala b/scalafix-testkit/src/main/scala/scalafix/testkit/DiffTest.scala index 9dfd9dd8e..b431f08b7 100644 --- a/scalafix-testkit/src/main/scala/scalafix/testkit/DiffTest.scala +++ b/scalafix-testkit/src/main/scala/scalafix/testkit/DiffTest.scala @@ -3,6 +3,7 @@ package scalafix.testkit import scala.meta._ import scalafix.SemanticCtx import scalafix.Rewrite +import scalafix.internal.config.LazySemanticCtx import scalafix.internal.config.ScalafixConfig import scalafix.reflect.ScalafixReflect import org.scalatest.exceptions.TestFailedException @@ -32,11 +33,12 @@ object DiffTest { .collectFirst { case Token.Comment(comment) => val decoder = - ScalafixReflect.fromLazySemanticCtx(_ => Some(semanticCtx)) + ScalafixReflect.fromLazySemanticCtx( + LazySemanticCtx(_ => Some(semanticCtx))) ScalafixConfig .fromInput( Input.VirtualFile(label, stripPrefix(comment)), - _ => Some(semanticCtx))(decoder) + LazySemanticCtx(_ => Some(semanticCtx)))(decoder) .get } .getOrElse(throw new TestFailedException( From dcacac584d243921349557526e1143c73c2cecbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93lafur=20P=C3=A1ll=20Geirsson?= Date: Thu, 17 Aug 2017 13:23:57 +0200 Subject: [PATCH 09/18] Move cli/test to unit/testOnly *cli*. unit/test has fixtures to build SemanticCtx, which will come in handy to test the cli. --- .../main/scala/scalafix/test/StringFS.scala | 51 +++++++++++++++++++ .../scalafix/cli/AutoClasspathSuite.scala | 0 .../src/test/scala/scalafix/cli/CliTest.scala | 30 +++++------ 3 files changed, 65 insertions(+), 16 deletions(-) create mode 100644 scalafix-tests/unit/src/main/scala/scalafix/test/StringFS.scala rename {scalafix-cli => scalafix-tests/unit}/src/test/scala/scalafix/cli/AutoClasspathSuite.scala (100%) rename {scalafix-cli => scalafix-tests/unit}/src/test/scala/scalafix/cli/CliTest.scala (92%) diff --git a/scalafix-tests/unit/src/main/scala/scalafix/test/StringFS.scala b/scalafix-tests/unit/src/main/scala/scalafix/test/StringFS.scala new file mode 100644 index 000000000..aa301da8d --- /dev/null +++ b/scalafix-tests/unit/src/main/scala/scalafix/test/StringFS.scala @@ -0,0 +1,51 @@ +package scalafix.test + +import java.io.File +import java.nio.file.Files +import scala.meta.AbsolutePath + +object StringFS { + + /** + * The inverse of [[dir2string]]. Given a string representation creates the + * necessary files/directories with respective file contents. + */ + def string2dir(layout: String): AbsolutePath = { + val root = Files.createTempDirectory("root") + layout.split("(?=\n/)").foreach { row => + val path :: contents :: Nil = + row.stripPrefix("\n").split("\n", 2).toList + val file = root.resolve(path) + file.getParent.toFile.mkdirs() + Files.write(file, contents.getBytes) + } + AbsolutePath(root) + } + + /** Gives a string representation of a directory. For example + * + * /build.sbt + * val x = project + * /src/main/scala/Main.scala + * object A { def main = Unit } + * /target/scala-2.11/foo.class + * ^!*@#@!*#&@*!&#^ + */ + def dir2string(file: AbsolutePath): String = { + import scala.collection.JavaConverters._ + Files + .list(file.toNIO) + .iterator() + .asScala + .toArray + .sorted + .map { path => + val contents = new String(Files.readAllBytes(path)) + s"""|${file.toNIO.relativize(path)} + |$contents""".stripMargin + } + .mkString("\n") + .replace(File.separator, "/") // ensure original separators + } + +} diff --git a/scalafix-cli/src/test/scala/scalafix/cli/AutoClasspathSuite.scala b/scalafix-tests/unit/src/test/scala/scalafix/cli/AutoClasspathSuite.scala similarity index 100% rename from scalafix-cli/src/test/scala/scalafix/cli/AutoClasspathSuite.scala rename to scalafix-tests/unit/src/test/scala/scalafix/cli/AutoClasspathSuite.scala diff --git a/scalafix-cli/src/test/scala/scalafix/cli/CliTest.scala b/scalafix-tests/unit/src/test/scala/scalafix/cli/CliTest.scala similarity index 92% rename from scalafix-cli/src/test/scala/scalafix/cli/CliTest.scala rename to scalafix-tests/unit/src/test/scala/scalafix/cli/CliTest.scala index 96e9c4c7a..6d23e334d 100644 --- a/scalafix-cli/src/test/scala/scalafix/cli/CliTest.scala +++ b/scalafix-tests/unit/src/test/scala/scalafix/cli/CliTest.scala @@ -16,19 +16,21 @@ import scalafix.internal.util.FileOps import scalafix.testkit.DiffAssertions import org.scalatest.FunSuite -trait ScalafixCliTest extends FunSuite with DiffAssertions { - val original = """|object Main { - | def foo() { - | println(1) - | } - |} - """.stripMargin - val expected = """|object Main { - | def foo(): Unit = { - | println(1) - | } - |} +class CliTest extends FunSuite with DiffAssertions { + val original: String = + """|object Main { + | def foo() { + | println(1) + | } + |} """.stripMargin + val expected: String = + """|object Main { + | def foo(): Unit = { + | println(1) + | } + |} + |""".stripMargin implicit val cwd: Path = Files.createTempDirectory("scalafix-cli") val devNull = CommonOptions( @@ -38,10 +40,6 @@ trait ScalafixCliTest extends FunSuite with DiffAssertions { val default = ScalafixOptions(common = devNull) -} - -class CliTest extends ScalafixCliTest { - test("Cli.parse") { val RunScalafix(runner) = Cli.parse( Seq( From 6433616cb2c21ddb35980dd629e13e53576f0c7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93lafur=20P=C3=A1ll=20Geirsson?= Date: Thu, 17 Aug 2017 15:24:06 +0200 Subject: [PATCH 10/18] Rewrite CliTest with StringFS. The cli tests have in need for a rewrite for a long time. This commits creates a helper method to easily construct a fixture and run assertions against the expected output. --- .../src/main/scala/scalafix/cli/Cli.scala | 18 +- .../main/scala/scalafix/cli/CliRunner.scala | 16 +- .../internal/cli/ScalafixOptions.scala | 4 + .../internal/config/FilterMatcher.scala | 17 +- .../config/ScalafixMetaconfigReaders.scala | 3 +- .../src/main/scala/scalafix/lint/LintID.scala | 24 +- .../scala/scalafix/lint/LintMessage.scala | 8 +- .../main/scala/scalafix/rewrite/Rewrite.scala | 13 +- .../scala/scalafix/rewrite/RewriteCtx.scala | 3 +- .../main/scala/scalafix/test/StringFS.scala | 20 +- .../test/scala/scalafix/cli/BaseCliTest.scala | 58 +++ .../src/test/scala/scalafix/cli/CliTest.scala | 348 +++++++++--------- .../scala/scalafix/cli/TestRewrites.scala | 15 + 13 files changed, 337 insertions(+), 210 deletions(-) create mode 100644 scalafix-tests/unit/src/test/scala/scalafix/cli/BaseCliTest.scala create mode 100644 scalafix-tests/unit/src/test/scala/scalafix/cli/TestRewrites.scala diff --git a/scalafix-cli/src/main/scala/scalafix/cli/Cli.scala b/scalafix-cli/src/main/scala/scalafix/cli/Cli.scala index a8ae1b5f4..f3b646d32 100644 --- a/scalafix-cli/src/main/scala/scalafix/cli/Cli.scala +++ b/scalafix-cli/src/main/scala/scalafix/cli/Cli.scala @@ -189,7 +189,7 @@ object ScalafixRewriteNames { filename.endsWith(".scala") || filename.endsWith(".sbt") } - def parse(args: Seq[String]): CliCommand = { + def parse(args: Seq[String], common: CommonOptions): CliCommand = { import CliCommand._ OptionsParser.withHelp.detailedParse(args) match { case Left(err) => @@ -212,12 +212,16 @@ object ScalafixRewriteNames { Files.write(path, sbtCompletions.getBytes) PrintAndExit(s"Sbt completions installed in $path", ExitStatus.Ok) case Right((WithHelp(_, _, options), extraFiles, _)) => - parseOptions(options.copy(files = options.files ++ extraFiles)) + parseOptions( + options.copy( + common = common, + files = options.files ++ extraFiles + )) } } - def runMain(args: Seq[String], commonOptions: CommonOptions): ExitStatus = - runMain(parse(args), commonOptions) + def runMain(args: Seq[String], common: CommonOptions): ExitStatus = + runMain(parse(args, common), common) def runMain( cliCommand: CliCommand, @@ -233,7 +237,11 @@ object ScalafixRewriteNames { } // This one accummulates a lot of garbage, scalameta needs to get rid of it. PlatformTokenizerCache.megaCache.clear() - result + if (commonOptions.reporter.hasErrors) { + ExitStatus.merge(ExitStatus.InvalidCommandLineOption, result) + } else { + result + } } def nailMain(nGContext: NGContext): Unit = { diff --git a/scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala b/scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala index fc83a3133..5c0f7d114 100644 --- a/scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala +++ b/scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala @@ -37,6 +37,7 @@ import scalafix.reflect.ScalafixReflect import scalafix.syntax._ import metaconfig.Configured.Ok import metaconfig._ +import org.scalameta.logger sealed abstract case class CliRunner( sourceroot: AbsolutePath, @@ -117,7 +118,7 @@ sealed abstract case class CliRunner( } case parsers.Parsed.Success(tree) => val ctx = RewriteCtx(tree, config) - val fixed = rewrite(ctx) + val fixed = rewrite.apply(ctx) writeMode match { case WriteMode.Stdout => common.out.write(fixed.getBytes) @@ -339,7 +340,8 @@ object CliRunner { // expands a single file into a list of files. def expand(matcher: FilterMatcher)(path: AbsolutePath): Seq[FixFile] = { if (!path.toFile.exists()) { - common.err.println(s"$path does not exist.") + common.reporter.error( + s"$path does not exist. ${common.workingDirectory}") Nil } else if (path.isDirectory) { val builder = Seq.newBuilder[FixFile] @@ -430,7 +432,15 @@ object CliRunner { } } val resolvedRewrite: Configured[Rewrite] = - resolvedRewriteAndConfig.map(_._1) + resolvedRewriteAndConfig.andThen { + case (rewrite, _) => + if (rewrite.rewriteName.isEmpty) + ConfError + .msg( + "No rewrite was provided! Use --rewrite to specify a rewrite.") + .notOk + else Ok(rewrite) + } val resolvedConfig: Configured[ScalafixConfig] = resolvedRewriteAndConfig.map(_._2) diff --git a/scalafix-cli/src/main/scala/scalafix/internal/cli/ScalafixOptions.scala b/scalafix-cli/src/main/scala/scalafix/internal/cli/ScalafixOptions.scala index 7841d5793..9de5d85be 100644 --- a/scalafix-cli/src/main/scala/scalafix/internal/cli/ScalafixOptions.scala +++ b/scalafix-cli/src/main/scala/scalafix/internal/cli/ScalafixOptions.scala @@ -24,6 +24,10 @@ case class CommonOptions( def workingDirectoryFile = new File(workingDirectory) } +object CommonOptions { + lazy val default = CommonOptions() +} + // NOTE: Do not depend on this class as a library, the interface will change between // patch versions. @AppName("scalafix") diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/config/FilterMatcher.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/config/FilterMatcher.scala index 5ce778397..30628d9a8 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/internal/config/FilterMatcher.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/config/FilterMatcher.scala @@ -9,11 +9,18 @@ case class FilterMatcher( excludeFilters: Regex ) { val reader: ConfDecoder[FilterMatcher] = - ConfDecoder.instanceF[FilterMatcher] { c => - ( - c.getOrElse("include")(includeFilters) |@| - c.getOrElse("exclude")(excludeFilters) - ).map { case (a, b) => FilterMatcher(a, b) } + ConfDecoder.instanceF[FilterMatcher] { + case c @ Conf.Str(_) => + c.as[Regex].map(FilterMatcher(_, FilterMatcher.mkRegexp(Nil))) + case c @ Conf.Lst(_) => + c.as[List[String]].map { x => + FilterMatcher(FilterMatcher.mkRegexp(x), FilterMatcher.mkRegexp(Nil)) + } + case c => + ( + c.getOrElse("include")(includeFilters) |@| + c.getOrElse("exclude")(excludeFilters) + ).map { case (a, b) => FilterMatcher(a, b) } } def matches(file: AbsolutePath): Boolean = matches(file.toString()) def matches(input: String): Boolean = diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/config/ScalafixMetaconfigReaders.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/config/ScalafixMetaconfigReaders.scala index a0f4eed7c..e5ed5280f 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/internal/config/ScalafixMetaconfigReaders.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/config/ScalafixMetaconfigReaders.scala @@ -25,6 +25,7 @@ import metaconfig.Configured.Ok import scalafix.internal.config.MetaconfigParser.{parser => hoconParser} import scalafix.patch.TreePatch import scalafix.rewrite.ConfigRewrite +import org.scalameta.logger object ScalafixMetaconfigReaders extends ScalafixMetaconfigReaders // A collection of metaconfig.Reader instances that are shared across @@ -94,7 +95,7 @@ trait ScalafixMetaconfigReaders { def defaultRewriteDecoder( getSemanticCtx: LazySemanticCtx): ConfDecoder[Rewrite] = ConfDecoder.instance[Rewrite] { - case conf @ Conf.Str(value) => + case conf @ Conf.Str(value) if !value.contains(":") => val isSyntactic = ScalafixRewrites.syntacticNames.contains(value) val kind = RewriteKind(syntactic = isSyntactic) val semanticCtx = getSemanticCtx(kind) diff --git a/scalafix-core/shared/src/main/scala/scalafix/lint/LintID.scala b/scalafix-core/shared/src/main/scala/scalafix/lint/LintID.scala index 03e84147e..fb91ec27a 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/lint/LintID.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/lint/LintID.scala @@ -13,31 +13,29 @@ import scala.meta.inputs.Position * @param category The default category this message should get reported to. * Note that users can configure/override the default category. */ -final class LintID( - val id: String, - val owner: RewriteName, - val explanation: String, - val category: LintCategory +final case class LintID( + id: String, + owner: RewriteName, + explanation: String, + category: LintCategory ) { override def toString: String = key lazy val key = s"$owner.$id" private def noExplanation: LintID = new LintID(id, owner, explanation, category) def at(message: String, position: Position): LintMessage = - new LintMessage(message, position, this) + LintMessage(message, position, this) def at(message: String): LintMessage = - new LintMessage(message, Position.None, this) + LintMessage(message, Position.None, this) def at(position: Position): LintMessage = - new LintMessage(explanation, position, noExplanation) + LintMessage(explanation, position, noExplanation) } object LintID { - def error(explain: String)( - implicit alias: sourcecode.Name, - rewrite: RewriteName): LintID = - new LintID(alias.value, rewrite, explain, LintCategory.Error) + def error(explain: String)(implicit alias: sourcecode.Name): LintID = + new LintID(alias.value, RewriteName.empty, explain, LintCategory.Error) def warning(explain: String)( implicit alias: sourcecode.Name, rewrite: RewriteName): LintID = - new LintID(alias.value, rewrite, explain, LintCategory.Warning) + new LintID(alias.value, RewriteName.empty, explain, LintCategory.Warning) } diff --git a/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala b/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala index 7d0ad9869..a75966a54 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala @@ -10,10 +10,10 @@ import scala.meta.Position * For an empty position use Position.None. * @param id the LintID associated with this message. */ -class LintMessage( - val message: String, - val position: Position, - val id: LintID +final case class LintMessage( + message: String, + position: Position, + id: LintID ) { def format(explain: Boolean): String = { val explanation = diff --git a/scalafix-core/shared/src/main/scala/scalafix/rewrite/Rewrite.scala b/scalafix-core/shared/src/main/scala/scalafix/rewrite/Rewrite.scala index 5f7ee016b..9afbdd150 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/rewrite/Rewrite.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/rewrite/Rewrite.scala @@ -28,13 +28,16 @@ abstract class Rewrite(implicit val rewriteName: RewriteName) { self => config: ScalafixConfig = ScalafixConfig.default): String = { val ctx = RewriteCtx(config.dialect(input).parse[Source].get, config) val patch = rewrite(ctx) - val result = apply(ctx, patch) - Patch.lintMessages(patch, ctx).foreach(ctx.printLintMessage) - result + apply(ctx, patch) } final def apply(input: String): String = apply(Input.String(input)) - final def apply(ctx: RewriteCtx, patch: Patch): String = - Patch(patch, ctx, semanticOption) + final def apply(ctx: RewriteCtx, patch: Patch): String = { + val result = Patch(patch, ctx, semanticOption) + Patch.lintMessages(patch, ctx).foreach { msg => + ctx.printLintMessage(msg.copy(id = msg.id.copy(owner = rewriteName))) + } + result + } /** Returns unified diff from applying this patch */ final def diff(ctx: RewriteCtx): String = diff --git a/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala b/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala index 233a05d39..c0f5c664e 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala @@ -53,10 +53,11 @@ case class RewriteCtx(tree: Tree, config: ScalafixConfig) extends PatchOps { def printLintMessage(msg: LintMessage): Unit = { if (config.lint.ignore.matches(msg.id.key)) () else { + val category = config.lint.getCategory(msg.id) reporter.handleMessage( msg.format(config.lint.explain), msg.position, - config.lint.getCategory(msg.id).toSeverity + category.toSeverity ) } } diff --git a/scalafix-tests/unit/src/main/scala/scalafix/test/StringFS.scala b/scalafix-tests/unit/src/main/scala/scalafix/test/StringFS.scala index aa301da8d..27d78b086 100644 --- a/scalafix-tests/unit/src/main/scala/scalafix/test/StringFS.scala +++ b/scalafix-tests/unit/src/main/scala/scalafix/test/StringFS.scala @@ -13,11 +13,16 @@ object StringFS { def string2dir(layout: String): AbsolutePath = { val root = Files.createTempDirectory("root") layout.split("(?=\n/)").foreach { row => - val path :: contents :: Nil = - row.stripPrefix("\n").split("\n", 2).toList - val file = root.resolve(path) - file.getParent.toFile.mkdirs() - Files.write(file, contents.getBytes) + row.stripPrefix("\n").split("\n", 2).toList match { + case path :: contents :: Nil => + val file = root.resolve(path.stripPrefix("/")) + file.getParent.toFile.mkdirs() + Files.write(file, contents.getBytes) + case els => + throw new IllegalArgumentException( + s"Unable to split argument info path/contents! \n$els") + + } } AbsolutePath(root) } @@ -34,14 +39,15 @@ object StringFS { def dir2string(file: AbsolutePath): String = { import scala.collection.JavaConverters._ Files - .list(file.toNIO) + .walk(file.toNIO) .iterator() .asScala + .filter(_.toFile.isFile) .toArray .sorted .map { path => val contents = new String(Files.readAllBytes(path)) - s"""|${file.toNIO.relativize(path)} + s"""|/${file.toNIO.relativize(path)} |$contents""".stripMargin } .mkString("\n") diff --git a/scalafix-tests/unit/src/test/scala/scalafix/cli/BaseCliTest.scala b/scalafix-tests/unit/src/test/scala/scalafix/cli/BaseCliTest.scala new file mode 100644 index 000000000..3d4855f8d --- /dev/null +++ b/scalafix-tests/unit/src/test/scala/scalafix/cli/BaseCliTest.scala @@ -0,0 +1,58 @@ +package scalafix.cli + +import java.io.ByteArrayOutputStream +import java.io.PrintStream +import java.nio.file.Files +import java.nio.file.Path +import scala.collection.immutable.Seq +import scalafix.internal.cli.CommonOptions +import scalafix.internal.cli.ScalafixOptions +import scalafix.test.StringFS +import scalafix.testkit.DiffAssertions +import org.scalatest.FunSuite + +// extend this class to run custom cli tests. +trait BaseCliTest extends FunSuite with DiffAssertions { + val original: String = + """|object Main { + | def foo() { + | } + |} + |""".stripMargin + val expected: String = + """|object Main { + | def foo(): Unit = { + | } + |} + |""".stripMargin + val cwd: Path = Files.createTempDirectory("scalafix-cli") + val ps = new PrintStream(new ByteArrayOutputStream()) + val devNull = CommonOptions( + // Uncomment these lines to diagnose errors from tests. + out = ps, + err = ps, + workingDirectory = cwd.toString + ) + + val default = ScalafixOptions(common = devNull) + + def check( + name: String, + originalLayout: String, + args: Seq[String], + expectedLayout: String, + expectedExit: ExitStatus, + common: CommonOptions = devNull + ): Unit = { + test(name) { + val root = StringFS.string2dir(originalLayout) + val exit = + Cli.runMain(args, common.copy(workingDirectory = root.toString())) + assert(exit == expectedExit) + val obtained = StringFS.dir2string(root) + assertNoDiff(obtained, expectedLayout) + } + } + def parse(args: Seq[String]): CliCommand = + Cli.parse(args, CommonOptions.default) +} diff --git a/scalafix-tests/unit/src/test/scala/scalafix/cli/CliTest.scala b/scalafix-tests/unit/src/test/scala/scalafix/cli/CliTest.scala index 6d23e334d..24dd89919 100644 --- a/scalafix-tests/unit/src/test/scala/scalafix/cli/CliTest.scala +++ b/scalafix-tests/unit/src/test/scala/scalafix/cli/CliTest.scala @@ -1,58 +1,28 @@ package scalafix.cli import java.io.ByteArrayOutputStream -import java.io.File import java.io.PrintStream -import java.nio.file.Files -import java.nio.file.Path import scala.collection.immutable.Seq import scalafix.cli.CliCommand.PrintAndExit import scalafix.cli.CliCommand.RunScalafix -import scalafix.internal.cli.CommonOptions -import scalafix.internal.cli.ScalafixOptions -import scalafix.internal.rewrite.ExplicitReturnTypes import scalafix.internal.rewrite.ProcedureSyntax -import scalafix.internal.util.FileOps -import scalafix.testkit.DiffAssertions -import org.scalatest.FunSuite - -class CliTest extends FunSuite with DiffAssertions { - val original: String = - """|object Main { - | def foo() { - | println(1) - | } - |} - """.stripMargin - val expected: String = - """|object Main { - | def foo(): Unit = { - | println(1) - | } - |} - |""".stripMargin - implicit val cwd: Path = Files.createTempDirectory("scalafix-cli") - - val devNull = CommonOptions( - err = new PrintStream(new ByteArrayOutputStream()), - workingDirectory = cwd.toString - ) - val default = ScalafixOptions(common = devNull) +class CliTest extends BaseCliTest { - test("Cli.parse") { - val RunScalafix(runner) = Cli.parse( + test("parse") { + val RunScalafix(runner) = parse( Seq( "--verbose", "--config-str", "fatalWarnings=true", "--single-thread", + "-r", + ProcedureSyntax.toString, "--files", - "a.scala", - "b.scala", + "build.sbt", + "project/Mima.scala", "--stdout", - "foo.scala", - "bar.scala" + "project/Dependencies.scala" )) val obtained = runner.cli assert(!runner.writeMode.isWriteFile) @@ -60,156 +30,202 @@ class CliTest extends FunSuite with DiffAssertions { assert(obtained.verbose) assert(obtained.singleThread) assert( - obtained.files == List("a.scala", "b.scala", "foo.scala", "bar.scala")) + obtained.files == List( + "build.sbt", + "project/Mima.scala", + "project/Dependencies.scala" + ) + ) } - def withFile(contents: String)(f: File => Unit): Unit = { - val file = File.createTempFile("prefix", ".scala") - FileOps.writeFile(file, contents) - f(file) - } + check( + name = "fix file", + originalLayout = s"""/hello.scala + |$original + |""".stripMargin, + args = Seq("-r", ProcedureSyntax.toString, "hello.scala"), + expectedLayout = s"""/hello.scala + |$expected + |""".stripMargin, + expectedExit = ExitStatus.Ok + ) - test("write fix to file") { - withFile(original) { file => - Cli.runOn( - default.copy( - rewrites = List(ProcedureSyntax.toString), - files = List(file.getAbsolutePath) - )) - assertNoDiff(FileOps.readFile(file), expected) - } - } + check( + name = "fix directory", + originalLayout = s"""|/dir/a.scala + |$original + |/dir/b.scala + |$original""".stripMargin, + args = Seq( + "-r", + ProcedureSyntax.toString, + "dir" + ), + expectedLayout = s"""|/dir/a.scala + |$expected + |/dir/b.scala + |$expected""".stripMargin, + expectedExit = ExitStatus.Ok + ) - test("--test") { - val config = default.copy( - test = true, - rewrites = List(ProcedureSyntax.toString) - ) - withFile(original) { file => - val exit = Cli.runOn(config.copy(files = List(file.getAbsolutePath))) - assert(exit == ExitStatus.TestFailed) - assertNoDiff(FileOps.readFile(file), original) - } - withFile(expected) { file => - val exit = Cli.runOn(config.copy(files = List(file.getAbsolutePath))) - assert(exit == ExitStatus.Ok) - assertNoDiff(FileOps.readFile(file), expected) - } - } + check( + name = "file not found", + originalLayout = s"/foobar.scala\n", + args = Seq("unknown-file.scala"), + expectedLayout = "/foobar.scala", + expectedExit = ExitStatus.InvalidCommandLineOption + ) - test("--include/--exclude is respected") { - val ignore = "Ignore" - val exclude = File.createTempFile("prefix", s"$ignore.scala") - val include = File.createTempFile("prefix", "Fixme.scala") - FileOps.writeFile(exclude, original) - FileOps.writeFile(include, original) - Cli.runOn( - default.copy( - rewrites = List(ProcedureSyntax.toString), - files = List(exclude.getAbsolutePath, include.getAbsolutePath), - exclude = List(ignore) - )) - assertNoDiff(FileOps.readFile(exclude), original) - assertNoDiff(FileOps.readFile(include), expected) - } + check( + name = "empty rewrite", + originalLayout = s"/foobar.scala\n", + args = Seq("foobar.scala"), + expectedLayout = "/foobar.scala", + expectedExit = ExitStatus.InvalidCommandLineOption + ) - test("print to stdout does not write to file") { - val file = File.createTempFile("prefix", ".scala") - FileOps.writeFile(file, original) - val baos = new ByteArrayOutputStream() - val exit = Cli.runOn( - default.copy( - common = CommonOptions( - out = new PrintStream(baos) - ), - rewrites = List(ProcedureSyntax.toString), - files = List(file.getAbsolutePath), - stdout = true - )) - assert(exit == ExitStatus.Ok) - assertNoDiff(FileOps.readFile(file), original) - assertNoDiff(new String(baos.toByteArray), expected) - } + check( + name = "--test error", + originalLayout = s"""/foobar.scala + |$original""".stripMargin, + args = Seq("--test", "-r", ProcedureSyntax.toString, "foobar.scala"), + expectedLayout = s"""/foobar.scala + |$original""".stripMargin, + expectedExit = ExitStatus.TestFailed + ) + + check( + name = "--test OK", + originalLayout = s"""/foobar.scala + |$expected""".stripMargin, + args = Seq("--test", "-r", ProcedureSyntax.toString, "foobar.scala"), + expectedLayout = s"""/foobar.scala + |$expected""".stripMargin, + expectedExit = ExitStatus.Ok + ) - test("write fix to directory") { - val dir = File.createTempFile("project/src/main/scala", "sbt") - dir.delete() - dir.mkdirs() - assert(dir.isDirectory) - - def createFile(): File = { - val file = File.createTempFile("file", ".scala", dir) - FileOps.writeFile(file, original) - file - } - val file1, file2 = createFile() - - Cli.runOn( - default.copy( - rewrites = List(ProcedureSyntax.toString), - files = List(dir.getAbsolutePath))) - assertNoDiff(FileOps.readFile(file1), expected) - assertNoDiff(FileOps.readFile(file2), expected) + check( + name = "linter error", + originalLayout = s"""/foobar.scala + |$original""".stripMargin, + args = Seq( + "-r", + "scala:scalafix.cli.TestRewrites.LintError", + "foobar.scala" + ), + expectedLayout = s"""/foobar.scala + |$original""".stripMargin, + expectedExit = ExitStatus.LinterError + ) + + check( + name = "linter warning promoted to error", + originalLayout = s"""/foobar.scala + |$original""".stripMargin, + args = Seq( + "--config-str", + "lint.error=LintWarning.warning", + "-r", + "scala:scalafix.cli.TestRewrites.LintWarning", + "foobar.scala" + ), + expectedLayout = s"""/foobar.scala + |$original""".stripMargin, + expectedExit = ExitStatus.LinterError + ) + + check( + name = "--exclude is respected", + originalLayout = s"""|/ignoreme.scala + |$original + |/fixme.scala + |$original""".stripMargin, + args = Seq( + "--exclude", + "ignoreme.scala", + "-r", + ProcedureSyntax.toString, + "ignoreme.scala", + "fixme.scala" + ), + expectedLayout = s"""|/fixme.scala + |$expected + |/ignoreme.scala + |$original""".stripMargin, + expectedExit = ExitStatus.Ok + ) + + val stdout = new ByteArrayOutputStream() + check( + name = "--stdout does not write to file", + originalLayout = s"""|/a.scala + |$original + |""".stripMargin, + args = Seq( + "--stdout", + "-r", + ProcedureSyntax.toString, + "a.scala" + ), + expectedLayout = s"""|/a.scala + |$original""".stripMargin, + expectedExit = ExitStatus.Ok, + common = default.common.copy(out = new PrintStream(stdout)) + ) + + test("--stdout prints to stdout") { + val obtained = stdout.toString + assertNoDiff(obtained, expected) } test("--rewrites") { val RunScalafix(runner) = - Cli.parse(Seq("--rewrites", "VolatileLazyVal")) + parse(Seq("--rewrites", "VolatileLazyVal")) assert(runner.rewrite.name == "VolatileLazyVal") - assert(Cli.parse(Seq("--rewrites", "Foobar")).isError) + assert(parse(Seq("--rewrites", "Foobar")).isError) } - test("--sourceroot --classpath") { - // NOTE: This assertion should fail by default, but scalafix-cli % Test - // depends on testkit, which has semanticdb-scalac as a dependency. - assert( - Cli - .parse(List("--sourceroot", "/foo.scala", "--classpath", "/bar")) - .isOk - ) - assert( - Cli - .parse( - List( - "--sourceroot", - "/foo.scala", - "--classpath", - "/bar" - )) - .isOk) - } - - test("error returns failure exit code") { - val file = File.createTempFile("prefix", ".scala") - FileOps.writeFile(file, "object a { implicit val x = ??? }") - val code = Cli.runOn( - default.copy( - rewrites = List(ExplicitReturnTypes.toString), - files = List(file.getAbsolutePath), - common = devNull)) - assert(code == ExitStatus.InvalidCommandLineOption) - } + check( + name = "parse errors return exit code", + originalLayout = s"""|/a.scala + |objec bar + |""".stripMargin, + args = Seq( + "-r", + ProcedureSyntax.toString, + "a.scala" + ), + expectedLayout = s"""|/a.scala + |objec bar""".stripMargin, + expectedExit = ExitStatus.ParseError + ) - test(".sbt files get fixed with sbt dialect") { - val file = File.createTempFile("prefix", ".sbt") - FileOps.writeFile(file, "def foo { println(1) }\n") - val code = Cli.runOn( - default.copy( - rewrites = List(ProcedureSyntax.toString), - files = List(file.getAbsolutePath), - common = devNull)) - assert(code == ExitStatus.Ok) - assert(FileOps.readFile(file) == "def foo: Unit = { println(1) }\n") - } + check( + name = "fix sbt files", + originalLayout = s"""|/a.sbt + |def foo { println(1) } + |lazy val bar = project + |""".stripMargin, + args = Seq( + "-r", + ProcedureSyntax.toString, + "a.sbt" + ), + expectedLayout = s"""|/a.sbt + |def foo: Unit = { println(1) } + |lazy val bar = project + |""".stripMargin, + expectedExit = ExitStatus.Ok + ) test("--zsh") { - val obtained = Cli.parse(Seq("--zsh")) + val obtained = parse(Seq("--zsh")) assert(obtained.isOk) assert(obtained.isInstanceOf[PrintAndExit]) } test("--bash") { - val obtained = Cli.parse(Seq("--bash")) + val obtained = parse(Seq("--bash")) assert(obtained.isOk) assert(obtained.isInstanceOf[PrintAndExit]) } diff --git a/scalafix-tests/unit/src/test/scala/scalafix/cli/TestRewrites.scala b/scalafix-tests/unit/src/test/scala/scalafix/cli/TestRewrites.scala new file mode 100644 index 000000000..8f6c1424f --- /dev/null +++ b/scalafix-tests/unit/src/test/scala/scalafix/cli/TestRewrites.scala @@ -0,0 +1,15 @@ +package scalafix.cli + +import scalafix.lint.LintID +import scalafix.rewrite.Rewrite + +object TestRewrites { + val LintError: Rewrite = Rewrite.syntactic { ctx => + val failure = LintID.error("Error!") + ctx.lint(failure.at(ctx.tree.pos)) + } + val LintWarning: Rewrite = Rewrite.syntactic { ctx => + val warning = LintID.warning("Warning!") + ctx.lint(warning.at(ctx.tree.pos)) + } +} From e43b4b0c5065dbf0efd916e4f5003aac9f738558 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93lafur=20P=C3=A1ll=20Geirsson?= Date: Thu, 17 Aug 2017 15:37:40 +0200 Subject: [PATCH 11/18] Remove LintID.owner. Instead of using LintID.owner, the RewriteCtx.printMessages takes an `owner: RewriteName` argument. This results in a simpler LintID data structure. --- .../scalafix/internal/config/LintConfig.scala | 12 ++++++------ .../main/scala/scalafix/lint/LintCategory.scala | 2 +- .../src/main/scala/scalafix/lint/LintID.scala | 14 ++++++-------- .../src/main/scala/scalafix/lint/LintMessage.scala | 5 +++-- .../src/main/scala/scalafix/patch/Patch.scala | 7 ++++--- .../src/main/scala/scalafix/rewrite/Rewrite.scala | 4 +++- .../main/scala/scalafix/rewrite/RewriteCtx.scala | 11 +++++++---- .../scalafix/testkit/SemanticRewriteSuite.scala | 4 +++- 8 files changed, 33 insertions(+), 26 deletions(-) diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/config/LintConfig.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/config/LintConfig.scala index ce6308c05..71c722f70 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/internal/config/LintConfig.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/config/LintConfig.scala @@ -22,12 +22,12 @@ case class LintConfig( ).map { case ((((a, b), c), d), e) => LintConfig(a, b, c, d, e) } } - def getCategory(id: LintID): LintCategory = id.key match { - case error() => LintCategory.Error - case warning() => LintCategory.Warning - case info() => LintCategory.Info - case _ => id.category - } + def getConfiguredCategory(key: String): Option[LintCategory] = + Option(key).collect { + case error() => LintCategory.Error + case warning() => LintCategory.Warning + case info() => LintCategory.Info + } } object LintConfig { diff --git a/scalafix-core/shared/src/main/scala/scalafix/lint/LintCategory.scala b/scalafix-core/shared/src/main/scala/scalafix/lint/LintCategory.scala index d7b8abf8a..9e4b4e844 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/lint/LintCategory.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/lint/LintCategory.scala @@ -20,4 +20,4 @@ object LintCategory { case object Info extends LintCategory case object Warning extends LintCategory case object Error extends LintCategory -} \ No newline at end of file +} diff --git a/scalafix-core/shared/src/main/scala/scalafix/lint/LintID.scala b/scalafix-core/shared/src/main/scala/scalafix/lint/LintID.scala index fb91ec27a..ff960dabe 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/lint/LintID.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/lint/LintID.scala @@ -7,22 +7,20 @@ import scala.meta.inputs.Position * * @param id a string ID for this message, typically the name of the * assigned variable. - * @param owner The rewrite that owns this lint message, to distinguish - * lint messages with conflicting IDs from different rewrites. * @param explanation An optional explanation for this kind of message. * @param category The default category this message should get reported to. * Note that users can configure/override the default category. */ final case class LintID( id: String, - owner: RewriteName, explanation: String, category: LintCategory ) { - override def toString: String = key - lazy val key = s"$owner.$id" + def key(owner: RewriteName): String = + if (owner.isEmpty) id + else s"$owner.$id" private def noExplanation: LintID = - new LintID(id, owner, explanation, category) + new LintID(id, explanation, category) def at(message: String, position: Position): LintMessage = LintMessage(message, position, this) def at(message: String): LintMessage = @@ -33,9 +31,9 @@ final case class LintID( object LintID { def error(explain: String)(implicit alias: sourcecode.Name): LintID = - new LintID(alias.value, RewriteName.empty, explain, LintCategory.Error) + new LintID(alias.value, explain, LintCategory.Error) def warning(explain: String)( implicit alias: sourcecode.Name, rewrite: RewriteName): LintID = - new LintID(alias.value, RewriteName.empty, explain, LintCategory.Warning) + new LintID(alias.value, explain, LintCategory.Warning) } diff --git a/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala b/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala index a75966a54..dffbb1620 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala @@ -1,6 +1,7 @@ package scalafix.lint import scala.meta.Position +import scalafix.rewrite.RewriteName /** An instance of a LintID with a custom message at a particular position * @@ -15,7 +16,7 @@ final case class LintMessage( position: Position, id: LintID ) { - def format(explain: Boolean): String = { + def format(owner: RewriteName, explain: Boolean): String = { val explanation = if (explain) s""" @@ -23,6 +24,6 @@ final case class LintMessage( |${id.explanation} |""".stripMargin else "" - s"[${id.owner.name}.${id.id}] $message$explanation" + s"[${owner.name}.${id.id}] $message$explanation" } } diff --git a/scalafix-core/shared/src/main/scala/scalafix/patch/Patch.scala b/scalafix-core/shared/src/main/scala/scalafix/patch/Patch.scala index 678386991..4f4ca2707 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/patch/Patch.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/patch/Patch.scala @@ -89,9 +89,7 @@ private[scalafix] object TreePatch { } // implementation detail -private[scalafix] case class LintPatch( - message: LintMessage -) extends Patch +private[scalafix] case class LintPatch(message: LintMessage) extends Patch private[scalafix] case class Concat(a: Patch, b: Patch) extends Patch private[scalafix] case object EmptyPatch extends Patch with LowLevelPatch @@ -116,6 +114,9 @@ object Patch { case (rem: Remove, rem2: Remove) => rem case _ => throw Failure.TokenPatchMergeError(a, b) } + // Patch.apply and Patch.lintMessages package private. Feel free to use them + // for your application, but please ask on the Gitter channel to see if we + // can expose a better api for your use case. private[scalafix] def apply( p: Patch, ctx: RewriteCtx, diff --git a/scalafix-core/shared/src/main/scala/scalafix/rewrite/Rewrite.scala b/scalafix-core/shared/src/main/scala/scalafix/rewrite/Rewrite.scala index 9afbdd150..16636df63 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/rewrite/Rewrite.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/rewrite/Rewrite.scala @@ -34,7 +34,9 @@ abstract class Rewrite(implicit val rewriteName: RewriteName) { self => final def apply(ctx: RewriteCtx, patch: Patch): String = { val result = Patch(patch, ctx, semanticOption) Patch.lintMessages(patch, ctx).foreach { msg => - ctx.printLintMessage(msg.copy(id = msg.id.copy(owner = rewriteName))) + // Set the lint message owner. This allows us to distinguish + // LintID with the same id from different rewrites. + ctx.printLintMessage(msg, rewriteName) } result } diff --git a/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala b/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala index c0f5c664e..5391bae73 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala @@ -50,12 +50,15 @@ case class RewriteCtx(tree: Tree, config: ScalafixConfig) extends PatchOps { logger.elem(values: _*) } - def printLintMessage(msg: LintMessage): Unit = { - if (config.lint.ignore.matches(msg.id.key)) () + def printLintMessage(msg: LintMessage, owner: RewriteName): Unit = { + val key = msg.id.key(owner) + if (config.lint.ignore.matches(key)) () else { - val category = config.lint.getCategory(msg.id) + val category = config.lint + .getConfiguredCategory(key) + .getOrElse(msg.id.category) reporter.handleMessage( - msg.format(config.lint.explain), + msg.format(owner, config.lint.explain), msg.position, category.toSeverity ) diff --git a/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRewriteSuite.scala b/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRewriteSuite.scala index ab2bf15ac..fe188d23e 100644 --- a/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRewriteSuite.scala +++ b/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRewriteSuite.scala @@ -96,7 +96,9 @@ abstract class SemanticRewriteSuite( }.mkString } if (lintMessages.nonEmpty) { - Patch.lintMessages(patch, ctx).foreach(ctx.printLintMessage) + Patch + .lintMessages(patch, ctx) + .foreach(x => ctx.printLintMessage(x, rewrite.rewriteName)) val explanation = """To fix this problem, suffix the culprit lines with | // scalafix: warning From c0bd9f48718f0267a8254ac53c47e71764873421 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93lafur=20P=C3=A1ll=20Geirsson?= Date: Thu, 17 Aug 2017 16:09:38 +0200 Subject: [PATCH 12/18] Document lint messages on the website --- readme/Changelog05.scalatex | 7 +++++++ readme/Configuration.scalatex | 8 ++++++++ readme/ImplementingRewrites.scalatex | 20 ++++++++++++++++++++ 3 files changed, 35 insertions(+) diff --git a/readme/Changelog05.scalatex b/readme/Changelog05.scalatex index 18c147fa5..9c1d73512 100644 --- a/readme/Changelog05.scalatex +++ b/readme/Changelog05.scalatex @@ -23,6 +23,8 @@ Scala.js support for @sect.ref{scalafix-core}, by @user{gabro}. @li Configurable @sect.ref{ExplicitReturnTypes}, by @user{taisuke}. + @li + Configurable @sect.ref{lint} reporting. @li @sect.ref{sbt-scalafix} rewrite tab completion. @li @@ -42,6 +44,11 @@ Ability to implement rewrites for sources of sbt builds, including @code{*.sbt} files. The API to write sbt rewrites is identical to regular Scala rewrites. + @li + Rewrites can now emit lint messages with @code{ctx.lint}, see + @sect.ref{LintMessage}. + @li + Rewrites can have multiple names with optional deprecation warnings. @li Thanks to upstream improvements in the @lnk("Scalameta v2.0 Semantic API", "http://scalameta.org/tutorial/#SemanticAPI"), diff --git a/readme/Configuration.scalatex b/readme/Configuration.scalatex index eea05e738..947d0c287 100644 --- a/readme/Configuration.scalatex +++ b/readme/Configuration.scalatex @@ -65,6 +65,14 @@ @config rewrite = "https://gist.githubusercontent.com/olafurpg/fc6f43a695ac996bd02000f45ed02e63/raw/80218434edb85120a9c6fd6533a4418118de8ba7/ExampleRewrite.scala" + @sect{lint} + Override the default severity level of a lint message with @code{lint} + @config + lint.error = [ Foo.warningID ] // promote Foo.warnigID to an error + lint.warning = [ Foo.errorID ] // demote Foo.errorID to a warning + lint.info = [ Foo.errorID ] // demote Foo.errorID to info + lint.ignore = [ Foo.errorID ] // don't report Foo.errorID + lint.explain = true // print out detailed explanation for lint messages. @sect{patches} For simple use-cases, it's possible to write custom rewrites directly in .scalafix.conf. diff --git a/readme/ImplementingRewrites.scalatex b/readme/ImplementingRewrites.scalatex index 9d6352c66..aa3957833 100644 --- a/readme/ImplementingRewrites.scalatex +++ b/readme/ImplementingRewrites.scalatex @@ -96,6 +96,11 @@ @li Test failures are reported as unified diffs from the obtained output of the rewrite and the expected output in the @code{output} project. + @li + Assert a @sect.ref{LintMessage} is reported at a particular line + by suffixing the line with the comment @code{// scalafix: warning}. + The test fails if there exist reported lint messages that have no + associated assertion in the input file. @sect{Example rewrites} The Scalafix repository contains several example rewrites and tests, @@ -140,6 +145,21 @@ If you experience that it's difficult to implement something that seems simple then don't hesitate to ask on @gitter. + @sect{LintMessage} + Rewrites are able to emit "lint messages" with info/warn/error severity + using @code{ctx.lint(lintID.at(String/Position))}. + To report a lint message, first create a @sect.ref{LintID} and then + report as a Patch + @hl.scala + val divisionByZero = LintID.error("Division by zero is unsafe!") + def rewrite(ctx: RewriteCtx): Patch = { + val tree: Tree = // ... + ctx.lint(divisionByZero.at(tree.pos)) + } + + @sect{LintID} + A LintID is a unique identifier for a category for lint messages. + @sect{Scalameta} Scalafix uses @lnk("Scalameta", "http://scalameta.org/") to implement rewrites. From 275bdf01bf3891a6057393c6e09a8e9877f8049a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93lafur=20P=C3=A1ll=20Geirsson?= Date: Thu, 17 Aug 2017 16:10:08 +0200 Subject: [PATCH 13/18] s/LintCategory/LintSeverity/ "Category" is misleading. --- .../scalafix/internal/config/LintConfig.scala | 10 ++++---- .../scala/scalafix/lint/LintCategory.scala | 23 ------------------- .../src/main/scala/scalafix/lint/LintID.scala | 10 ++++---- .../scala/scalafix/lint/LintSeverity.scala | 23 +++++++++++++++++++ .../scala/scalafix/rewrite/RewriteCtx.scala | 6 ++--- .../testkit/SemanticRewriteSuite.scala | 10 ++++---- 6 files changed, 41 insertions(+), 41 deletions(-) delete mode 100644 scalafix-core/shared/src/main/scala/scalafix/lint/LintCategory.scala create mode 100644 scalafix-core/shared/src/main/scala/scalafix/lint/LintSeverity.scala diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/config/LintConfig.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/config/LintConfig.scala index 71c722f70..cd26c3587 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/internal/config/LintConfig.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/config/LintConfig.scala @@ -1,6 +1,6 @@ package scalafix.internal.config -import scalafix.lint.LintCategory +import scalafix.lint.LintSeverity import scalafix.lint.LintID import metaconfig.ConfDecoder @@ -22,11 +22,11 @@ case class LintConfig( ).map { case ((((a, b), c), d), e) => LintConfig(a, b, c, d, e) } } - def getConfiguredCategory(key: String): Option[LintCategory] = + def getConfiguredSeverity(key: String): Option[LintSeverity] = Option(key).collect { - case error() => LintCategory.Error - case warning() => LintCategory.Warning - case info() => LintCategory.Info + case error() => LintSeverity.Error + case warning() => LintSeverity.Warning + case info() => LintSeverity.Info } } diff --git a/scalafix-core/shared/src/main/scala/scalafix/lint/LintCategory.scala b/scalafix-core/shared/src/main/scala/scalafix/lint/LintCategory.scala deleted file mode 100644 index 9e4b4e844..000000000 --- a/scalafix-core/shared/src/main/scala/scalafix/lint/LintCategory.scala +++ /dev/null @@ -1,23 +0,0 @@ -package scalafix.lint - -import scalafix.internal.util.Severity - -sealed abstract class LintCategory { - private[scalafix] def toSeverity: Severity = this match { - case LintCategory.Error => Severity.Error - case LintCategory.Warning => Severity.Warn - case LintCategory.Info => Severity.Info - } - def isError: Boolean = this == LintCategory.Error - override def toString: String = this match { - case LintCategory.Error => "error" - case LintCategory.Warning => "warning" - case LintCategory.Info => "info" - } -} - -object LintCategory { - case object Info extends LintCategory - case object Warning extends LintCategory - case object Error extends LintCategory -} diff --git a/scalafix-core/shared/src/main/scala/scalafix/lint/LintID.scala b/scalafix-core/shared/src/main/scala/scalafix/lint/LintID.scala index ff960dabe..d891fad92 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/lint/LintID.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/lint/LintID.scala @@ -8,19 +8,19 @@ import scala.meta.inputs.Position * @param id a string ID for this message, typically the name of the * assigned variable. * @param explanation An optional explanation for this kind of message. - * @param category The default category this message should get reported to. + * @param severity The default category this message should get reported to. * Note that users can configure/override the default category. */ final case class LintID( id: String, explanation: String, - category: LintCategory + severity: LintSeverity ) { def key(owner: RewriteName): String = if (owner.isEmpty) id else s"$owner.$id" private def noExplanation: LintID = - new LintID(id, explanation, category) + new LintID(id, explanation, severity) def at(message: String, position: Position): LintMessage = LintMessage(message, position, this) def at(message: String): LintMessage = @@ -31,9 +31,9 @@ final case class LintID( object LintID { def error(explain: String)(implicit alias: sourcecode.Name): LintID = - new LintID(alias.value, explain, LintCategory.Error) + new LintID(alias.value, explain, LintSeverity.Error) def warning(explain: String)( implicit alias: sourcecode.Name, rewrite: RewriteName): LintID = - new LintID(alias.value, explain, LintCategory.Warning) + new LintID(alias.value, explain, LintSeverity.Warning) } diff --git a/scalafix-core/shared/src/main/scala/scalafix/lint/LintSeverity.scala b/scalafix-core/shared/src/main/scala/scalafix/lint/LintSeverity.scala new file mode 100644 index 000000000..6380ef4f7 --- /dev/null +++ b/scalafix-core/shared/src/main/scala/scalafix/lint/LintSeverity.scala @@ -0,0 +1,23 @@ +package scalafix.lint + +import scalafix.internal.util.Severity + +sealed abstract class LintSeverity { + private[scalafix] def toSeverity: Severity = this match { + case LintSeverity.Error => Severity.Error + case LintSeverity.Warning => Severity.Warn + case LintSeverity.Info => Severity.Info + } + def isError: Boolean = this == LintSeverity.Error + override def toString: String = this match { + case LintSeverity.Error => "error" + case LintSeverity.Warning => "warning" + case LintSeverity.Info => "info" + } +} + +object LintSeverity { + case object Info extends LintSeverity + case object Warning extends LintSeverity + case object Error extends LintSeverity +} diff --git a/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala b/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala index 5391bae73..bbc3978dd 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala @@ -8,7 +8,7 @@ import scalafix.internal.config.ScalafixConfig import scalafix.internal.config.ScalafixMetaconfigReaders import scalafix.internal.config.ScalafixReporter import scalafix.internal.util.SymbolOps.BottomSymbol -import scalafix.lint.LintCategory +import scalafix.lint.LintSeverity import scalafix.patch.LintPatch import scalafix.patch.PatchOps import scalafix.patch.TokenPatch @@ -55,8 +55,8 @@ case class RewriteCtx(tree: Tree, config: ScalafixConfig) extends PatchOps { if (config.lint.ignore.matches(key)) () else { val category = config.lint - .getConfiguredCategory(key) - .getOrElse(msg.id.category) + .getConfiguredSeverity(key) + .getOrElse(msg.id.severity) reporter.handleMessage( msg.format(owner, config.lint.explain), msg.position, diff --git a/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRewriteSuite.scala b/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRewriteSuite.scala index fe188d23e..c58303121 100644 --- a/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRewriteSuite.scala +++ b/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRewriteSuite.scala @@ -11,7 +11,7 @@ import org.scalatest.BeforeAndAfterAll import org.scalatest.FunSuite import org.scalatest.exceptions.TestFailedException import scala.meta.internal.inputs.XtensionPositionFormatMessage -import scalafix.lint.LintCategory +import scalafix.lint.LintSeverity abstract class SemanticRewriteSuite( val semanticCtx: SemanticCtx, @@ -56,11 +56,11 @@ abstract class SemanticRewriteSuite( val patch = rewrite.rewrite(ctx) val obtainedWithComment = rewrite.apply(ctx, patch) val lintMessages = Patch.lintMessages(patch, ctx).to[mutable.Set] - def assertLint(position: Position, category: LintCategory): Unit = { + def assertLint(position: Position, category: LintSeverity): Unit = { val matchingMessage = lintMessages.find { m => // NOTE(olafur) I have no idea why -1 is necessary. m.position.startLine == (position.startLine - 1) && - m.id.category == category + m.id.severity == category } matchingMessage match { case Some(x) => @@ -83,9 +83,9 @@ abstract class SemanticRewriteSuite( case tok @ Token.Comment(LintAssertion(severity)) => severity match { case "warning" => - assertLint(tok.pos, LintCategory.Warning) + assertLint(tok.pos, LintSeverity.Warning) case "error" => - assertLint(tok.pos, LintCategory.Error) + assertLint(tok.pos, LintSeverity.Error) case els => throw new TestFailedException( tok.pos.formatMessage("error", s"Unknown severity '$els'"), From 5d38cf67e86c75a63da944c5fd465fb5605f93e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93lafur=20P=C3=A1ll=20Geirsson?= Date: Thu, 17 Aug 2017 16:13:18 +0200 Subject: [PATCH 14/18] s/LintID/LintCategory/ LintCategory is more suitable since it is a category of LintMessages. --- readme/Configuration.scalatex | 4 +++- readme/ImplementingRewrites.scalatex | 9 +++++---- .../scalafix/internal/config/LintConfig.scala | 2 +- .../internal/rewrite/RemoveXmlLiterals.scala | 2 +- .../lint/{LintID.scala => LintCategory.scala} | 16 ++++++++-------- .../main/scala/scalafix/lint/LintMessage.scala | 10 +++++----- .../shared/src/main/scala/scalafix/package.scala | 4 ++-- .../main/scala/scalafix/rewrite/Rewrite.scala | 2 +- .../main/scala/scalafix/rewrite/RewriteCtx.scala | 4 ++-- .../scalafix/testkit/SemanticRewriteSuite.scala | 2 +- .../test/scala/scalafix/cli/TestRewrites.scala | 6 +++--- 11 files changed, 32 insertions(+), 29 deletions(-) rename scalafix-core/shared/src/main/scala/scalafix/lint/{LintID.scala => LintCategory.scala} (76%) diff --git a/readme/Configuration.scalatex b/readme/Configuration.scalatex index 947d0c287..460174d36 100644 --- a/readme/Configuration.scalatex +++ b/readme/Configuration.scalatex @@ -66,13 +66,15 @@ rewrite = "https://gist.githubusercontent.com/olafurpg/fc6f43a695ac996bd02000f45ed02e63/raw/80218434edb85120a9c6fd6533a4418118de8ba7/ExampleRewrite.scala" @sect{lint} - Override the default severity level of a lint message with @code{lint} + Override the default severity level of a @sect.ref{LintMessage} with @code{lint} @config + // Assuming 'Foo' is a rewrite and 'warningID'/'errorID' are LintCategory IDs. lint.error = [ Foo.warningID ] // promote Foo.warnigID to an error lint.warning = [ Foo.errorID ] // demote Foo.errorID to a warning lint.info = [ Foo.errorID ] // demote Foo.errorID to info lint.ignore = [ Foo.errorID ] // don't report Foo.errorID lint.explain = true // print out detailed explanation for lint messages. + @sect{patches} For simple use-cases, it's possible to write custom rewrites directly in .scalafix.conf. diff --git a/readme/ImplementingRewrites.scalatex b/readme/ImplementingRewrites.scalatex index aa3957833..3b3e0672b 100644 --- a/readme/ImplementingRewrites.scalatex +++ b/readme/ImplementingRewrites.scalatex @@ -148,17 +148,18 @@ @sect{LintMessage} Rewrites are able to emit "lint messages" with info/warn/error severity using @code{ctx.lint(lintID.at(String/Position))}. - To report a lint message, first create a @sect.ref{LintID} and then + To report a lint message, first create a @sect.ref{LintCategory} and then report as a Patch @hl.scala - val divisionByZero = LintID.error("Division by zero is unsafe!") + val divisionByZero = LintCategory.error("Division by zero is unsafe!") def rewrite(ctx: RewriteCtx): Patch = { val tree: Tree = // ... ctx.lint(divisionByZero.at(tree.pos)) } - @sect{LintID} - A LintID is a unique identifier for a category for lint messages. + @sect{LintCategory} + A LintCategory is group of lint messages of the same kind. + The severity of a LintCategory can be customized in @sect.ref{lint}. @sect{Scalameta} Scalafix uses @lnk("Scalameta", "http://scalameta.org/") to implement diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/config/LintConfig.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/config/LintConfig.scala index cd26c3587..c7eb69d0f 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/internal/config/LintConfig.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/config/LintConfig.scala @@ -1,7 +1,7 @@ package scalafix.internal.config import scalafix.lint.LintSeverity -import scalafix.lint.LintID +import scalafix.lint.LintCategory import metaconfig.ConfDecoder case class LintConfig( diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/rewrite/RemoveXmlLiterals.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/rewrite/RemoveXmlLiterals.scala index 5e75eb033..205930ffe 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/internal/rewrite/RemoveXmlLiterals.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/rewrite/RemoveXmlLiterals.scala @@ -23,7 +23,7 @@ import scalafix.rewrite.RewriteCtx */ case object RemoveXmlLiterals extends Rewrite { - val singleBracesEscape = LintID.warning( + val singleBracesEscape = LintCategory.warning( """Single braces don't need be escaped with {{ and }} inside xml interpolators, unlike xml literals. |For example {{ is identical to xml"{". This Rewrite will replace all occurrences of |{{ and }}. Make sure this is intended. diff --git a/scalafix-core/shared/src/main/scala/scalafix/lint/LintID.scala b/scalafix-core/shared/src/main/scala/scalafix/lint/LintCategory.scala similarity index 76% rename from scalafix-core/shared/src/main/scala/scalafix/lint/LintID.scala rename to scalafix-core/shared/src/main/scala/scalafix/lint/LintCategory.scala index d891fad92..eec656b1b 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/lint/LintID.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/lint/LintCategory.scala @@ -11,7 +11,7 @@ import scala.meta.inputs.Position * @param severity The default category this message should get reported to. * Note that users can configure/override the default category. */ -final case class LintID( +final case class LintCategory( id: String, explanation: String, severity: LintSeverity @@ -19,8 +19,8 @@ final case class LintID( def key(owner: RewriteName): String = if (owner.isEmpty) id else s"$owner.$id" - private def noExplanation: LintID = - new LintID(id, explanation, severity) + private def noExplanation: LintCategory = + new LintCategory(id, explanation, severity) def at(message: String, position: Position): LintMessage = LintMessage(message, position, this) def at(message: String): LintMessage = @@ -29,11 +29,11 @@ final case class LintID( LintMessage(explanation, position, noExplanation) } -object LintID { - def error(explain: String)(implicit alias: sourcecode.Name): LintID = - new LintID(alias.value, explain, LintSeverity.Error) +object LintCategory { + def error(explain: String)(implicit alias: sourcecode.Name): LintCategory = + new LintCategory(alias.value, explain, LintSeverity.Error) def warning(explain: String)( implicit alias: sourcecode.Name, - rewrite: RewriteName): LintID = - new LintID(alias.value, explain, LintSeverity.Warning) + rewrite: RewriteName): LintCategory = + new LintCategory(alias.value, explain, LintSeverity.Warning) } diff --git a/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala b/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala index dffbb1620..5e99937c4 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala @@ -3,27 +3,27 @@ package scalafix.lint import scala.meta.Position import scalafix.rewrite.RewriteName -/** An instance of a LintID with a custom message at a particular position +/** An observation of a LintCategory at a particular position * * @param message The message to display to the user. If empty, LintID.explanation * is used instead. * @param position Optionally place a caret under a location in a source file. * For an empty position use Position.None. - * @param id the LintID associated with this message. + * @param category the LintCategory associated with this message. */ final case class LintMessage( message: String, position: Position, - id: LintID + category: LintCategory ) { def format(owner: RewriteName, explain: Boolean): String = { val explanation = if (explain) s""" |Explanation: - |${id.explanation} + |${category.explanation} |""".stripMargin else "" - s"[${owner.name}.${id.id}] $message$explanation" + s"[${owner.name}.${category.id}] $message$explanation" } } diff --git a/scalafix-core/shared/src/main/scala/scalafix/package.scala b/scalafix-core/shared/src/main/scala/scalafix/package.scala index 0be65babb..8450b6666 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/package.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/package.scala @@ -29,8 +29,8 @@ package object scalafix { type Patch = patch.Patch val Patch = patch.Patch - type LintID = scalafix.lint.LintID - val LintID = scalafix.lint.LintID + type LintCategory = scalafix.lint.LintCategory + val LintCategory = scalafix.lint.LintCategory type LintMessage = scalafix.lint.LintMessage diff --git a/scalafix-core/shared/src/main/scala/scalafix/rewrite/Rewrite.scala b/scalafix-core/shared/src/main/scala/scalafix/rewrite/Rewrite.scala index 16636df63..b34d91c03 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/rewrite/Rewrite.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/rewrite/Rewrite.scala @@ -35,7 +35,7 @@ abstract class Rewrite(implicit val rewriteName: RewriteName) { self => val result = Patch(patch, ctx, semanticOption) Patch.lintMessages(patch, ctx).foreach { msg => // Set the lint message owner. This allows us to distinguish - // LintID with the same id from different rewrites. + // LintCategory with the same id from different rewrites. ctx.printLintMessage(msg, rewriteName) } result diff --git a/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala b/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala index bbc3978dd..7dcb13762 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala @@ -51,12 +51,12 @@ case class RewriteCtx(tree: Tree, config: ScalafixConfig) extends PatchOps { } def printLintMessage(msg: LintMessage, owner: RewriteName): Unit = { - val key = msg.id.key(owner) + val key = msg.category.key(owner) if (config.lint.ignore.matches(key)) () else { val category = config.lint .getConfiguredSeverity(key) - .getOrElse(msg.id.severity) + .getOrElse(msg.category.severity) reporter.handleMessage( msg.format(owner, config.lint.explain), msg.position, diff --git a/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRewriteSuite.scala b/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRewriteSuite.scala index c58303121..9aa26a7fe 100644 --- a/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRewriteSuite.scala +++ b/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRewriteSuite.scala @@ -60,7 +60,7 @@ abstract class SemanticRewriteSuite( val matchingMessage = lintMessages.find { m => // NOTE(olafur) I have no idea why -1 is necessary. m.position.startLine == (position.startLine - 1) && - m.id.severity == category + m.category.severity == category } matchingMessage match { case Some(x) => diff --git a/scalafix-tests/unit/src/test/scala/scalafix/cli/TestRewrites.scala b/scalafix-tests/unit/src/test/scala/scalafix/cli/TestRewrites.scala index 8f6c1424f..717519a7f 100644 --- a/scalafix-tests/unit/src/test/scala/scalafix/cli/TestRewrites.scala +++ b/scalafix-tests/unit/src/test/scala/scalafix/cli/TestRewrites.scala @@ -1,15 +1,15 @@ package scalafix.cli -import scalafix.lint.LintID +import scalafix.lint.LintCategory import scalafix.rewrite.Rewrite object TestRewrites { val LintError: Rewrite = Rewrite.syntactic { ctx => - val failure = LintID.error("Error!") + val failure = LintCategory.error("Error!") ctx.lint(failure.at(ctx.tree.pos)) } val LintWarning: Rewrite = Rewrite.syntactic { ctx => - val warning = LintID.warning("Warning!") + val warning = LintCategory.warning("Warning!") ctx.lint(warning.at(ctx.tree.pos)) } } From 6de3ac816f6add9ec1b16756c732d87348c461ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93lafur=20P=C3=A1ll=20Geirsson?= Date: Thu, 17 Aug 2017 16:39:03 +0200 Subject: [PATCH 15/18] Add test for deprecated names, VolatileLazyVal => DottyVolatileLazyVal. I've been meaning to rename this rewrite for a while but wasn't satisfied that this would break people's configuraiton. I wanted to solve this potentially common problem in the future more elegantly. Now that rewrites can have multiple names and names can have deprecation warnings, this commits tests that it indeed works as expected. --- readme/Changelog03.scalatex | 2 +- readme/Rewrites.scalatex | 2 +- ...zyVal.scala => DottyVolatileLazyVal.scala} | 9 +++++- .../scala/scalafix/rewrite/RewriteName.scala | 12 ++++++-- .../scalafix/rewrite/ScalafixRewrites.scala | 2 +- .../src/test/scala/scalafix/cli/CliTest.scala | 30 +++++++++++++++++-- 6 files changed, 48 insertions(+), 9 deletions(-) rename scalafix-core/shared/src/main/scala/scalafix/internal/rewrite/{VolatileLazyVal.scala => DottyVolatileLazyVal.scala} (69%) diff --git a/readme/Changelog03.scalatex b/readme/Changelog03.scalatex index 46de62473..aeca01275 100644 --- a/readme/Changelog03.scalatex +++ b/readme/Changelog03.scalatex @@ -142,4 +142,4 @@ Two rewrite rules: @sect.ref{ProcedureSyntax} and - @sect.ref{VolatileLazyVal}. + @sect.ref{DottyVolatileLazyVal}. diff --git a/readme/Rewrites.scalatex b/readme/Rewrites.scalatex index 20db2ff83..ebeff7311 100644 --- a/readme/Rewrites.scalatex +++ b/readme/Rewrites.scalatex @@ -132,7 +132,7 @@ println("Hello world!") } - @sect(VolatileLazyVal.toString) + @sect(DottyVolatileLazyVal.toString) @p Adds a @code{@@volatile} annotation to lazy vals. The @code{@@volatile} annotation is needed to maintain thread-safe diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/rewrite/VolatileLazyVal.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/rewrite/DottyVolatileLazyVal.scala similarity index 69% rename from scalafix-core/shared/src/main/scala/scalafix/internal/rewrite/VolatileLazyVal.scala rename to scalafix-core/shared/src/main/scala/scalafix/internal/rewrite/DottyVolatileLazyVal.scala index 940de6131..b20611dcd 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/internal/rewrite/VolatileLazyVal.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/rewrite/DottyVolatileLazyVal.scala @@ -4,8 +4,15 @@ import scala.meta._ import scalafix.Patch import scalafix.rewrite.Rewrite import scalafix.rewrite.RewriteCtx +import scalafix.rewrite.RewriteName -case object VolatileLazyVal extends Rewrite { +case object DottyVolatileLazyVal + extends Rewrite()( + RewriteName("DottyVolatileLazyVal").withOldName( + name = "VolatileLazyVal", + message = "Use DottyVolatileLazyVal instead.", + since = "0.5.0") + ) { private object NonVolatileLazyVal { def unapply(defn: Defn.Val): Option[Token] = { defn.mods.collectFirst { diff --git a/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteName.scala b/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteName.scala index d8a39754f..1514ea668 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteName.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteName.scala @@ -1,11 +1,12 @@ package scalafix.rewrite import scalafix.internal.config.ScalafixReporter +import scalafix.util.Deprecated /** A thin wrapper around a string name and optional deprecation warning. */ final case class RewriteIdentifier( value: String, - deprecated: Option[scalafix.util.Deprecated] + deprecated: Option[Deprecated] ) { override def toString: String = value } @@ -17,9 +18,14 @@ object RewriteIdentifier { /** A thin wrapper around a list of RewriteIdentifier. */ final case class RewriteName(identifiers: List[RewriteIdentifier]) { + private def nonDeprecated = identifiers.filter(_.deprecated.isEmpty) + def withOldName(name: String, message: String, since: String): RewriteName = + this.+( + RewriteName( + RewriteIdentifier(name, Some(Deprecated(message, since))) :: Nil)) def name: String = - if (identifiers.isEmpty) "empty" - else identifiers.mkString("+") + if (nonDeprecated.isEmpty) "empty" + else nonDeprecated.mkString("+") def isEmpty: Boolean = identifiers.isEmpty def +(other: RewriteName): RewriteName = new RewriteName((identifiers :: other.identifiers :: Nil).flatten) diff --git a/scalafix-core/shared/src/main/scala/scalafix/rewrite/ScalafixRewrites.scala b/scalafix-core/shared/src/main/scala/scalafix/rewrite/ScalafixRewrites.scala index 2954a99f6..95e2c8c12 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/rewrite/ScalafixRewrites.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/rewrite/ScalafixRewrites.scala @@ -6,7 +6,7 @@ import scalafix.internal.rewrite._ object ScalafixRewrites { val syntax: List[Rewrite] = List( ProcedureSyntax, - VolatileLazyVal, + DottyVolatileLazyVal, RemoveXmlLiterals, ExplicitUnit, NoValInForComprehension, diff --git a/scalafix-tests/unit/src/test/scala/scalafix/cli/CliTest.scala b/scalafix-tests/unit/src/test/scala/scalafix/cli/CliTest.scala index 24dd89919..ab0477cba 100644 --- a/scalafix-tests/unit/src/test/scala/scalafix/cli/CliTest.scala +++ b/scalafix-tests/unit/src/test/scala/scalafix/cli/CliTest.scala @@ -180,8 +180,8 @@ class CliTest extends BaseCliTest { test("--rewrites") { val RunScalafix(runner) = - parse(Seq("--rewrites", "VolatileLazyVal")) - assert(runner.rewrite.name == "VolatileLazyVal") + parse(Seq("--rewrites", "DottyVolatileLazyVal")) + assert(runner.rewrite.name == "DottyVolatileLazyVal") assert(parse(Seq("--rewrites", "Foobar")).isError) } @@ -218,6 +218,32 @@ class CliTest extends BaseCliTest { expectedExit = ExitStatus.Ok ) + val deprecatedOut = new ByteArrayOutputStream() + check( + name = "deprecated name emits warning", + originalLayout = s"""|/a.scala + |object a { + | @volatile lazy val x = 2 + |} + |""".stripMargin, + args = Seq( + "-r", + "VolatileLazyVal", + "a.scala" + ), + expectedLayout = s"""|/a.scala + |object a { + | @volatile lazy val x = 2 + |} + |""".stripMargin, + expectedExit = ExitStatus.Ok, + default.common.copy(out = new PrintStream(deprecatedOut)) + ) + test("RewriteIdentifier.deprecated is reported") { + val obtained = deprecatedOut.toString + assert(obtained.contains("Use DottyVolatileLazyVal instead")) + } + test("--zsh") { val obtained = parse(Seq("--zsh")) assert(obtained.isOk) From 0bbaf08edc9d6e4c00607b6d0efac5ead999c34c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93lafur=20P=C3=A1ll=20Geirsson?= Date: Thu, 17 Aug 2017 17:07:41 +0200 Subject: [PATCH 16/18] Last minute changes. - doc polish - Require `// scalafix: Rewrite.LintCategory` assertions instead of warning/error. This is more robust. - Rename VolatileLazyVal to DottyVolatileLazyVal in tests, I noticed because of deprecation warnings getting printed out. - Don't print out lint messages in scalafix-testkit by default, if you have a lot of assertions then the console gets flooded with too many messages. --- readme/Changelog05.scalatex | 4 ++- readme/ImplementingRewrites.scalatex | 4 +-- .../testkit/SemanticRewriteSuite.scala | 35 +++++++------------ ...zyVal.scala => DottyVolatileLazyVal.scala} | 4 +-- .../main/scala/test/RemoveXmlLiterals.scala | 4 +-- ...zyVal.scala => DottyVolatileLazyVal.scala} | 2 +- 6 files changed, 23 insertions(+), 30 deletions(-) rename scalafix-tests/input/src/main/scala/test/{VolatileLazyVal.scala => DottyVolatileLazyVal.scala} (72%) rename scalafix-tests/output/src/main/scala/test/{VolatileLazyVal.scala => DottyVolatileLazyVal.scala} (86%) diff --git a/readme/Changelog05.scalatex b/readme/Changelog05.scalatex index 9c1d73512..0e658cb19 100644 --- a/readme/Changelog05.scalatex +++ b/readme/Changelog05.scalatex @@ -81,7 +81,7 @@ @code{Predef.intArrayOps} and now it resolves to @code{IndexedSeqOptimized.head}. - @h4{Breaking changes for rewrite authors} + @h4{Breaking changes} @p From 0.5 onwards, our CI will check binary breaking changes in the public API on every pull request. @@ -103,4 +103,6 @@ @li upgraded to Scalameta 2.0, which has several breaking changes in the Tree api. + @li + The @code{VolatileLazyVal} rewrite is now named @sect.ref{DottyVolatileLazyVal}. diff --git a/readme/ImplementingRewrites.scalatex b/readme/ImplementingRewrites.scalatex index 3b3e0672b..d1dc93d09 100644 --- a/readme/ImplementingRewrites.scalatex +++ b/readme/ImplementingRewrites.scalatex @@ -97,7 +97,7 @@ Test failures are reported as unified diffs from the obtained output of the rewrite and the expected output in the @code{output} project. @li - Assert a @sect.ref{LintMessage} is reported at a particular line + Assert that a @sect.ref{LintMessage} is expected at a particular line by suffixing the line with the comment @code{// scalafix: warning}. The test fails if there exist reported lint messages that have no associated assertion in the input file. @@ -147,7 +147,7 @@ @sect{LintMessage} Rewrites are able to emit "lint messages" with info/warn/error severity - using @code{ctx.lint(lintID.at(String/Position))}. + using @code{ctx.lint(lintCategory.at(String/Position)): Patch}. To report a lint message, first create a @sect.ref{LintCategory} and then report as a Patch @hl.scala diff --git a/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRewriteSuite.scala b/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRewriteSuite.scala index 9aa26a7fe..c1ced4bd0 100644 --- a/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRewriteSuite.scala +++ b/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRewriteSuite.scala @@ -54,20 +54,22 @@ abstract class SemanticRewriteSuite( config.copy(dialect = diffTest.attributes.dialect) ) val patch = rewrite.rewrite(ctx) - val obtainedWithComment = rewrite.apply(ctx, patch) + val obtainedWithComment = Patch.apply(patch, ctx, rewrite.semanticOption) val lintMessages = Patch.lintMessages(patch, ctx).to[mutable.Set] - def assertLint(position: Position, category: LintSeverity): Unit = { + def assertLint(position: Position, key: String): Unit = { val matchingMessage = lintMessages.find { m => // NOTE(olafur) I have no idea why -1 is necessary. m.position.startLine == (position.startLine - 1) && - m.category.severity == category + m.category.key(rewrite.rewriteName) == key } matchingMessage match { case Some(x) => lintMessages -= x case None => throw new TestFailedException( - position.formatMessage("error", s"No $category reported!"), + position.formatMessage( + "error", + s"Message '$key' was not reported here!"), 0 ) } @@ -80,30 +82,19 @@ abstract class SemanticRewriteSuite( val LintAssertion = " scalafix: (.*)".r tokens.filter { case `configComment` => false - case tok @ Token.Comment(LintAssertion(severity)) => - severity match { - case "warning" => - assertLint(tok.pos, LintSeverity.Warning) - case "error" => - assertLint(tok.pos, LintSeverity.Error) - case els => - throw new TestFailedException( - tok.pos.formatMessage("error", s"Unknown severity '$els'"), - 0) - } + case tok @ Token.Comment(LintAssertion(key)) => + assertLint(tok.pos, key) false case _ => true }.mkString } if (lintMessages.nonEmpty) { - Patch - .lintMessages(patch, ctx) - .foreach(x => ctx.printLintMessage(x, rewrite.rewriteName)) + lintMessages.foreach(x => ctx.printLintMessage(x, rewrite.rewriteName)) + val key = lintMessages.head.category.key(rewrite.rewriteName) val explanation = - """To fix this problem, suffix the culprit lines with - | // scalafix: warning - | // scalafix: error - |""".stripMargin + s"""|To fix this problem, suffix the culprit lines with + | // scalafix: $key + |""".stripMargin throw new TestFailedException( s"Uncaught linter messages! $explanation", 0) diff --git a/scalafix-tests/input/src/main/scala/test/VolatileLazyVal.scala b/scalafix-tests/input/src/main/scala/test/DottyVolatileLazyVal.scala similarity index 72% rename from scalafix-tests/input/src/main/scala/test/VolatileLazyVal.scala rename to scalafix-tests/input/src/main/scala/test/DottyVolatileLazyVal.scala index 4a943ee09..ebfa9c18a 100644 --- a/scalafix-tests/input/src/main/scala/test/VolatileLazyVal.scala +++ b/scalafix-tests/input/src/main/scala/test/DottyVolatileLazyVal.scala @@ -1,9 +1,9 @@ /* -rewrites = VolatileLazyVal +rewrites = DottyVolatileLazyVal */ package test -class VolatileLazyVal { +class DottyVolatileLazyVal { lazy val x = 2 @volatile lazy val dontChangeMe = 2 private lazy val y = 2 diff --git a/scalafix-tests/input/src/main/scala/test/RemoveXmlLiterals.scala b/scalafix-tests/input/src/main/scala/test/RemoveXmlLiterals.scala index f4b662771..0e468ae50 100644 --- a/scalafix-tests/input/src/main/scala/test/RemoveXmlLiterals.scala +++ b/scalafix-tests/input/src/main/scala/test/RemoveXmlLiterals.scala @@ -45,8 +45,8 @@ class RemoveXmlLiterals { } object I { - val a =
{{
// scalafix: warning - val b =
}}
// scalafix: warning + val a =
{{
// scalafix: RemoveXmlLiterals.singleBracesEscape + val b =
}}
// scalafix: RemoveXmlLiterals.singleBracesEscape //
>>> xml"""
""" } diff --git a/scalafix-tests/output/src/main/scala/test/VolatileLazyVal.scala b/scalafix-tests/output/src/main/scala/test/DottyVolatileLazyVal.scala similarity index 86% rename from scalafix-tests/output/src/main/scala/test/VolatileLazyVal.scala rename to scalafix-tests/output/src/main/scala/test/DottyVolatileLazyVal.scala index f0cd237db..f8952be53 100644 --- a/scalafix-tests/output/src/main/scala/test/VolatileLazyVal.scala +++ b/scalafix-tests/output/src/main/scala/test/DottyVolatileLazyVal.scala @@ -1,6 +1,6 @@ package test -class VolatileLazyVal { +class DottyVolatileLazyVal { @volatile lazy val x = 2 @volatile lazy val dontChangeMe = 2 @volatile private lazy val y = 2 From 9798839741cb4c6180ea191c94f2738e12dbf88d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93lafur=20P=C3=A1ll=20Geirsson?= Date: Fri, 18 Aug 2017 10:10:10 +0200 Subject: [PATCH 17/18] Clean up before merge - remove unused imports with scalafix (hurray!) - refactor build.sbt a bit - polish new doc entries - Remove unused Patch.applyInternal --- build.sbt | 38 ++++++++++++------- project/plugins.sbt | 1 + readme/ImplementingRewrites.scalatex | 8 ++-- readme/src/main/scala/scalafix/Readme.scala | 1 - .../src/main/scala/scalafix/cli/Cli.scala | 3 -- .../main/scala/scalafix/cli/CliRunner.scala | 2 - .../src/main/scala/scalafix/Failure.scala | 2 - .../scalafix/internal/config/LintConfig.scala | 1 - .../internal/config/PrintStreamReporter.scala | 1 - .../config/ScalafixMetaconfigReaders.scala | 1 - .../scalafix/internal/config/package.scala | 1 - .../src/main/scala/scalafix/patch/Patch.scala | 13 ++----- .../scala/scalafix/rewrite/RewriteCtx.scala | 1 - .../testkit/SemanticRewriteSuite.scala | 2 - 14 files changed, 34 insertions(+), 41 deletions(-) diff --git a/build.sbt b/build.sbt index 78ca56151..eb08df0b7 100644 --- a/build.sbt +++ b/build.sbt @@ -97,8 +97,8 @@ lazy val allSettings = List( stableVersion := version.value.replaceAll("\\+.*", ""), resolvers += Resolver.sonatypeRepo("releases"), triggeredMessage in ThisBuild := Watched.clearWhenTriggered, - scalacOptions ++= compilerOptions, - scalacOptions in (Compile, console) := compilerOptions :+ "-Yrepl-class-based", + scalacOptions ++= compilerOptions.value, + scalacOptions in (Compile, console) := compilerOptions.value :+ "-Yrepl-class-based", libraryDependencies += scalatest.value % Test, testOptions in Test += Tests.Argument("-oD"), scalaVersion := ciScalaVersion.getOrElse(scala212), @@ -197,6 +197,7 @@ lazy val `scalafix-sbt` = project .configs(IntegrationTest) .settings( allSettings, + is210Only, publishSettings, buildInfoSettings, Defaults.itSettings, @@ -220,8 +221,6 @@ lazy val `scalafix-sbt` = project "; very scalafix-sbt/scripted" )(state.value) }, - scalaVersion := scala210, - crossScalaVersions := Seq(scala210), moduleName := "sbt-scalafix", scriptedLaunchOpts ++= Seq( "-Dplugin.version=" + version.value, @@ -293,6 +292,7 @@ lazy val testsInput = project resolvers += Resolver.bintrayRepo("allanrenucci", "maven"), libraryDependencies ++= testsDeps ) + .disablePlugins(ScalafixPlugin) .dependsOn(testsShared) lazy val testsOutput = project @@ -301,6 +301,7 @@ lazy val testsOutput = project allSettings, noPublish, semanticdbSettings, + scalacOptions -= warnUnusedImports, resolvers := resolvers.in(testsInput).value, libraryDependencies := libraryDependencies.in(testsInput).value ) @@ -324,7 +325,7 @@ lazy val testsInputSbt = project .settings( logLevel := Level.Error, // avoid flood of deprecation warnings. scalacOptions += "-Xplugin-require:sbthost", - scalaVersion := scala210, + is210Only, sbtPlugin := true, scalacOptions += s"-P:sbthost:sourceroot:${sourceDirectory.in(Compile).value}", addCompilerPlugin( @@ -336,8 +337,7 @@ lazy val testsOutputSbt = project .settings( allSettings, noPublish, - scalaVersion := scala210, - crossScalaVersions := Seq(scala210), + is210Only, sbtPlugin := true ) @@ -437,17 +437,27 @@ lazy val readme = scalatex .dependsOn(coreJVM, cli) .enablePlugins(GhpagesPlugin) +lazy val is210Only = Seq( + scalaVersion := scala210, + crossScalaVersions := Seq(scala210), + scalacOptions -= warnUnusedImports +) + lazy val isFullCrossVersion = Seq( crossVersion := CrossVersion.full ) -lazy val compilerOptions = Seq( - "-deprecation", - "-encoding", - "UTF-8", - "-feature", - "-unchecked" -) +lazy val warnUnusedImports = "-Ywarn-unused-import" +lazy val compilerOptions = Def.setting { + Seq( + "-deprecation", + "-encoding", + "UTF-8", + "-feature", + warnUnusedImports, + "-unchecked" + ) +} lazy val gitPushTag = taskKey[Unit]("Push to git tag") diff --git a/project/plugins.sbt b/project/plugins.sbt index fac6e0a44..20226ea74 100644 --- a/project/plugins.sbt +++ b/project/plugins.sbt @@ -15,5 +15,6 @@ addSbtPlugin( addSbtPlugin("org.scala-js" % "sbt-scalajs" % "0.6.18") addSbtPlugin("ch.epfl.scala" % "sbt-scalajs-bundler" % "0.7.0") addSbtPlugin("com.typesafe" % "sbt-mima-plugin" % "0.1.14") +addSbtPlugin("ch.epfl.scala" % "sbt-scalafix" % "0.5.0-M4") libraryDependencies += "org.scala-sbt" % "scripted-plugin" % sbtVersion.value diff --git a/readme/ImplementingRewrites.scalatex b/readme/ImplementingRewrites.scalatex index d1dc93d09..1d8b3274d 100644 --- a/readme/ImplementingRewrites.scalatex +++ b/readme/ImplementingRewrites.scalatex @@ -98,7 +98,7 @@ of the rewrite and the expected output in the @code{output} project. @li Assert that a @sect.ref{LintMessage} is expected at a particular line - by suffixing the line with the comment @code{// scalafix: warning}. + by suffixing the line with the comment @code{// scalafix: }. The test fails if there exist reported lint messages that have no associated assertion in the input file. @@ -149,7 +149,7 @@ Rewrites are able to emit "lint messages" with info/warn/error severity using @code{ctx.lint(lintCategory.at(String/Position)): Patch}. To report a lint message, first create a @sect.ref{LintCategory} and then - report as a Patch + report it as a @code{Patch} @hl.scala val divisionByZero = LintCategory.error("Division by zero is unsafe!") def rewrite(ctx: RewriteCtx): Patch = { @@ -159,7 +159,9 @@ @sect{LintCategory} A LintCategory is group of lint messages of the same kind. - The severity of a LintCategory can be customized in @sect.ref{lint}. + A LintCategory has a default severity level (info/warn/error) at which + it will be reported. Scalafix users can override the default severity + with @sect.ref{lint}. @sect{Scalameta} Scalafix uses @lnk("Scalameta", "http://scalameta.org/") to implement diff --git a/readme/src/main/scala/scalafix/Readme.scala b/readme/src/main/scala/scalafix/Readme.scala index 5960a4714..3fdb09c55 100644 --- a/readme/src/main/scala/scalafix/Readme.scala +++ b/readme/src/main/scala/scalafix/Readme.scala @@ -7,7 +7,6 @@ import scalafix.internal.config.ScalafixMetaconfigReaders import scalafix.reflect.ScalafixReflect import scalatags.Text.TypedTag import scalatags.Text.all._ -import scalatex.Main import scalatex.site.Highlighter object Readme { diff --git a/scalafix-cli/src/main/scala/scalafix/cli/Cli.scala b/scalafix-cli/src/main/scala/scalafix/cli/Cli.scala index f3b646d32..bca0a8ab5 100644 --- a/scalafix-cli/src/main/scala/scalafix/cli/Cli.scala +++ b/scalafix-cli/src/main/scala/scalafix/cli/Cli.scala @@ -10,9 +10,6 @@ import scalafix.internal.cli.CommonOptions import scalafix.internal.cli.ScalafixOptions import scalafix.rewrite.ScalafixRewrites import scala.meta.io.AbsolutePath -import scalafix.internal.config.PrintStreamReporter -import scalafix.internal.config.ScalafixReporter -import scalafix.internal.util.Severity import caseapp.Name import caseapp.core.Arg import caseapp.core.Messages diff --git a/scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala b/scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala index 5c0f7d114..13f1f893b 100644 --- a/scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala +++ b/scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala @@ -17,7 +17,6 @@ import java.util.regex.PatternSyntaxException import scala.meta._ import scala.meta.inputs.Input import scala.meta.internal.inputs._ -import scala.meta.internal.tokenizers.PlatformTokenizerCache import scala.meta.io.AbsolutePath import scala.meta.sbthost.Sbthost import scala.util.Try @@ -37,7 +36,6 @@ import scalafix.reflect.ScalafixReflect import scalafix.syntax._ import metaconfig.Configured.Ok import metaconfig._ -import org.scalameta.logger sealed abstract case class CliRunner( sourceroot: AbsolutePath, diff --git a/scalafix-core/shared/src/main/scala/scalafix/Failure.scala b/scalafix-core/shared/src/main/scala/scalafix/Failure.scala index 5118b1e0d..64475b175 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/Failure.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/Failure.scala @@ -1,7 +1,5 @@ package scalafix -import scala.meta._ - // TODO(olafur) this signature makes no sense. We should try to avoid exception // try/catch dodgeball court whenever possible. sealed abstract class Failure(val ex: Throwable) diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/config/LintConfig.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/config/LintConfig.scala index c7eb69d0f..3b14b1940 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/internal/config/LintConfig.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/config/LintConfig.scala @@ -1,7 +1,6 @@ package scalafix.internal.config import scalafix.lint.LintSeverity -import scalafix.lint.LintCategory import metaconfig.ConfDecoder case class LintConfig( diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/config/PrintStreamReporter.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/config/PrintStreamReporter.scala index 29f203738..1e936ee53 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/internal/config/PrintStreamReporter.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/config/PrintStreamReporter.scala @@ -4,7 +4,6 @@ import scala.meta.Position import scala.meta.internal.inputs.XtensionPositionFormatMessage import java.io.PrintStream import java.util.concurrent.atomic.AtomicInteger -import java.util.concurrent.atomic.AtomicReference import scalafix.internal.util.Severity import metaconfig._ diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/config/ScalafixMetaconfigReaders.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/config/ScalafixMetaconfigReaders.scala index e5ed5280f..01883c667 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/internal/config/ScalafixMetaconfigReaders.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/config/ScalafixMetaconfigReaders.scala @@ -25,7 +25,6 @@ import metaconfig.Configured.Ok import scalafix.internal.config.MetaconfigParser.{parser => hoconParser} import scalafix.patch.TreePatch import scalafix.rewrite.ConfigRewrite -import org.scalameta.logger object ScalafixMetaconfigReaders extends ScalafixMetaconfigReaders // A collection of metaconfig.Reader instances that are shared across diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/config/package.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/config/package.scala index ad884fbdc..22348ed5b 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/internal/config/package.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/config/package.scala @@ -2,7 +2,6 @@ package scalafix.internal import scala.meta.Tree import scala.meta.parsers.Parse -import scalafix.SemanticCtx package object config extends ScalafixMetaconfigReaders { type MetaParser = Parse[_ <: Tree] diff --git a/scalafix-core/shared/src/main/scala/scalafix/patch/Patch.scala b/scalafix-core/shared/src/main/scala/scalafix/patch/Patch.scala index 4f4ca2707..9e2c61813 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/patch/Patch.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/patch/Patch.scala @@ -114,14 +114,6 @@ object Patch { case (rem: Remove, rem2: Remove) => rem case _ => throw Failure.TokenPatchMergeError(a, b) } - // Patch.apply and Patch.lintMessages package private. Feel free to use them - // for your application, but please ask on the Gitter channel to see if we - // can expose a better api for your use case. - private[scalafix] def apply( - p: Patch, - ctx: RewriteCtx, - semanticCtx: Option[SemanticCtx]): String = - applyInternal(p, ctx, semanticCtx) private[scalafix] def lintMessages( patch: Patch, @@ -135,7 +127,10 @@ object Patch { builder.result() } - private[scalafix] def applyInternal( + // Patch.apply and Patch.lintMessages package private. Feel free to use them + // for your application, but please ask on the Gitter channel to see if we + // can expose a better api for your use case. + private[scalafix] def apply( p: Patch, ctx: RewriteCtx, semanticCtx: Option[SemanticCtx] diff --git a/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala b/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala index 7dcb13762..91ea7c382 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/rewrite/RewriteCtx.scala @@ -8,7 +8,6 @@ import scalafix.internal.config.ScalafixConfig import scalafix.internal.config.ScalafixMetaconfigReaders import scalafix.internal.config.ScalafixReporter import scalafix.internal.util.SymbolOps.BottomSymbol -import scalafix.lint.LintSeverity import scalafix.patch.LintPatch import scalafix.patch.PatchOps import scalafix.patch.TokenPatch diff --git a/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRewriteSuite.scala b/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRewriteSuite.scala index c1ced4bd0..ca9453e0c 100644 --- a/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRewriteSuite.scala +++ b/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRewriteSuite.scala @@ -1,7 +1,6 @@ package scalafix package testkit -import scala.collection.JavaConverters._ import scala.collection.mutable import scalafix.syntax._ import scala.meta._ @@ -11,7 +10,6 @@ import org.scalatest.BeforeAndAfterAll import org.scalatest.FunSuite import org.scalatest.exceptions.TestFailedException import scala.meta.internal.inputs.XtensionPositionFormatMessage -import scalafix.lint.LintSeverity abstract class SemanticRewriteSuite( val semanticCtx: SemanticCtx, From b8e85db8ecc24f8744d82e8284e14f46403196fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93lafur=20P=C3=A1ll=20Geirsson?= Date: Fri, 18 Aug 2017 10:39:13 +0200 Subject: [PATCH 18/18] Disable sbt-scalafix due to scaladoc problems Reported at https://github.com/scalameta/scalameta/issues/1072 --- build.sbt | 1 - project/plugins.sbt | 1 - 2 files changed, 2 deletions(-) diff --git a/build.sbt b/build.sbt index eb08df0b7..eeb6f6294 100644 --- a/build.sbt +++ b/build.sbt @@ -292,7 +292,6 @@ lazy val testsInput = project resolvers += Resolver.bintrayRepo("allanrenucci", "maven"), libraryDependencies ++= testsDeps ) - .disablePlugins(ScalafixPlugin) .dependsOn(testsShared) lazy val testsOutput = project diff --git a/project/plugins.sbt b/project/plugins.sbt index 20226ea74..fac6e0a44 100644 --- a/project/plugins.sbt +++ b/project/plugins.sbt @@ -15,6 +15,5 @@ addSbtPlugin( addSbtPlugin("org.scala-js" % "sbt-scalajs" % "0.6.18") addSbtPlugin("ch.epfl.scala" % "sbt-scalajs-bundler" % "0.7.0") addSbtPlugin("com.typesafe" % "sbt-mima-plugin" % "0.1.14") -addSbtPlugin("ch.epfl.scala" % "sbt-scalafix" % "0.5.0-M4") libraryDependencies += "org.scala-sbt" % "scripted-plugin" % sbtVersion.value