Merge pull request #341 from olafurpg/moar-lints

Fix linter issues discovered while writing scala-lang blogpost.

Author: olafurpg

scalafix-core/shared/src/main/scala/org/langmeta/internal/ScalafixLangmetaHacks.scala

@@ -17,9 +17,9 @@ object ScalafixLangmetaHacks {
         val count = end - start
         val eolOffset =
           if (input.chars.lift(start + count - 1).contains('\n')) -1 else 0
-        new String(input.chars, start, count + eolOffset)
+        new String(input.chars, start, math.max(0, count + eolOffset))
       }
-      var caret = " " * pos.startColumn + "^"
+      val caret = " " * pos.startColumn + "^"
       header + EOL + line + EOL + caret
     } else {
       s"$severity: $message"

scalafix-core/shared/src/main/scala/scalafix/internal/rule/NoInfer.scala

@@ -36,7 +36,6 @@ case object NoInfer {
     Symbol("_root_.java.io.Serializable#"),
     Symbol("_root_.scala.Any#"),
     Symbol("_root_.scala.AnyVal#"),
-    Symbol("_root_.scala.AnyVal#"),
     Symbol("_root_.scala.Product#")
   )
 

scalafix-core/shared/src/main/scala/scalafix/lint/LintCategory.scala

@@ -19,6 +19,7 @@ final case class LintCategory(
 ) {
   def key(owner: RuleName): String =
     if (owner.isEmpty) id
+    else if (id.isEmpty) owner.value
     else s"${owner.value}.$id"
   private def noExplanation: LintCategory =
     new LintCategory(id, explanation, severity)

scalafix-core/shared/src/main/scala/scalafix/patch/Patch.scala

@@ -15,6 +15,7 @@ import scalafix.internal.util.Failure
 import scalafix.internal.util.TokenOps
 import scalafix.lint.LintMessage
 import scalafix.patch.TreePatch.ReplaceSymbol
+import scalafix.rule.RuleName
 import org.scalameta.logger
 
 /** A data structure that can produce a .patch file.
@@ -116,13 +117,20 @@ object Patch {
     case _ => throw Failure.TokenPatchMergeError(a, b)
   }
 
-  private[scalafix] def lintMessages(
-      patch: Patch,
-      ctx: RuleCtx,
-      checkMessages: scala.Seq[LintMessage]
-  ): List[LintMessage] = {
+  private[scalafix] def reportLintMessages(
+      patches: Map[RuleName, Patch],
+      ctx: RuleCtx): Unit = {
+    patches.foreach {
+      case (name, patch) =>
+        Patch.lintMessages(patch).foreach { msg =>
+          // Set the lint message owner. This allows us to distinguish
+          // LintCategory with the same id from different rules.
+          ctx.printLintMessage(msg, name)
+        }
+    }
+  }
+  private[scalafix] def lintMessages(patch: Patch): List[LintMessage] = {
     val builder = List.newBuilder[LintMessage]
-    checkMessages.foreach(builder += _)
     foreach(patch) {
       case LintPatch(lint) =>
         builder += lint
@@ -200,6 +208,17 @@ object Patch {
     builder.result()
   }
 
+  private[scalafix] def isOnlyLintMessages(patch: Patch): Boolean = {
+    // TODO(olafur): foreach should really return Stream[Patch] for early termination.
+    var onlyLint = true
+    var hasLintMessage = false
+    foreach(patch) {
+      case _: LintPatch => hasLintMessage = true
+      case _ => onlyLint = false
+    }
+    hasLintMessage && onlyLint
+  }
+
   private def foreach(patch: Patch)(f: Patch => Unit): Unit = {
     def loop(patch: Patch): Unit = patch match {
       case Concat(a, b) =>

scalafix-core/shared/src/main/scala/scalafix/rule/Rule.scala

@@ -9,6 +9,7 @@ import scalafix.syntax._
 import metaconfig.Conf
 import metaconfig.ConfDecoder
 import metaconfig.Configured
+import org.scalameta.logger
 
 /** A Scalafix Rule.
   *
@@ -79,13 +80,11 @@ abstract class Rule(ruleName: RuleName) { self =>
   }
   final def apply(input: String): String = apply(Input.String(input))
   final def apply(ctx: RuleCtx, patch: Patch): String = {
-    val result = Patch(patch, ctx, semanticOption)
-    val checkMessages = check(ctx)
-    Patch.lintMessages(patch, ctx, checkMessages).foreach { msg =>
-      // Set the lint message owner. This allows us to distinguish
-      // LintCategory with the same id from different rules.
-      ctx.printLintMessage(msg, name)
-    }
+    apply(ctx, Map(name -> patch))
+  }
+  final def apply(ctx: RuleCtx, patches: Map[RuleName, Patch]): String = {
+    val result = Patch(patches.values.asPatch, ctx, semanticOption)
+    Patch.reportLintMessages(patches, ctx)
     result
   }
 
@@ -101,6 +100,9 @@ abstract class Rule(ruleName: RuleName) { self =>
 
   private[scalafix] final def allNames: List[String] =
     name.identifiers.map(_.value)
+  protected[scalafix] def fixWithName(ctx: RuleCtx): Map[RuleName, Patch] =
+    Map(name -> (fix(ctx) ++ check(ctx).map(ctx.lint)))
+
   final override def toString: String = name.toString
   final def name: RuleName = ruleName
   // NOTE. This is kind of hacky and hopefully we can find a better workaround.
@@ -132,6 +134,8 @@ object Rule {
     }
     override def check(ctx: RuleCtx): Seq[LintMessage] =
       rules.flatMap(_.check(ctx))
+    override def fixWithName(ctx: RuleCtx): Map[RuleName, Patch] =
+      rules.foldLeft(Map.empty[RuleName, Patch])(_ ++ _.fixWithName(ctx))
     override def fix(ctx: RuleCtx): Patch =
       Patch.empty ++ rules.map(_.fix(ctx))
     override def semanticOption: Option[SemanticdbIndex] =
@@ -181,6 +185,14 @@ object Rule {
     }
 
   /** Combine two rules into a single rule */
-  def merge(a: Rule, b: Rule): Rule =
-    new CompositeRule(a :: b :: Nil)
+  def merge(a: Rule, b: Rule): Rule = (a, b) match {
+    case (c1: CompositeRule, c2: CompositeRule) =>
+      new CompositeRule(c1.rules ::: c2.rules)
+    case (c1: CompositeRule, _) =>
+      new CompositeRule(b :: c1.rules)
+    case (_, c2: CompositeRule) =>
+      new CompositeRule(b :: c2.rules)
+    case (_, _) =>
+      new CompositeRule(a :: b :: Nil)
+  }
 }

scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRuleSuite.scala

@@ -10,6 +10,7 @@ import org.scalatest.BeforeAndAfterAll
 import org.scalatest.FunSuite
 import org.scalatest.exceptions.TestFailedException
 import scala.util.matching.Regex
+import scalafix.rule.RuleName
 import org.langmeta.internal.ScalafixLangmetaHacks
 
 object SemanticRuleSuite {
@@ -65,61 +66,70 @@ abstract class SemanticRuleSuite(
   private def assertLintMessagesAreReported(
       rule: Rule,
       ctx: RuleCtx,
-      lints: List[LintMessage],
+      patches: Map[RuleName, Patch],
       tokens: Tokens): Unit = {
-    val lintMessages = lints.to[mutable.Set]
-    def assertLint(position: Position, key: String): Unit = {
-      val matchingMessage = lintMessages.filter { m =>
-        assert(m.position.input == position.input)
-        m.position.startLine == position.startLine &&
-        m.category.key(rule.name) == key
-      }
-      if (matchingMessage.isEmpty) {
+
+    type Msg = (Position, String)
+
+    def matches(a: Msg)(b: Msg) =
+      a._1.startLine == b._1.startLine &&
+        a._2 == b._2
+
+    def diff(a: Seq[Msg], b: Seq[Msg]) =
+      a.filter(x => !b.exists(matches(x)))
+
+    val lintAssertions = tokens.collect {
+      case tok @ Token.Comment(SemanticRuleSuite.LintAssertion(key)) =>
+        tok.pos -> key
+    }
+    val lintMessages = patches.toSeq.flatMap {
+      case (name, patch) =>
+        Patch
+          .lintMessages(patch)
+          .map(lint => lint.position -> lint.category.key(name))
+    }
+
+    val uncoveredAsserts = diff(lintAssertions, lintMessages)
+    uncoveredAsserts.foreach {
+      case (pos, key) =>
         throw new TestFailedException(
           ScalafixLangmetaHacks.formatMessage(
-            position,
+            pos,
             "error",
             s"Message '$key' was not reported here!"),
           0
         )
-      } else {
-        lintMessages --= matchingMessage
-      }
     }
-    tokens.foreach {
-      case tok @ Token.Comment(SemanticRuleSuite.LintAssertion(key)) =>
-        assertLint(tok.pos, key)
-      case _ =>
-    }
-    if (lintMessages.nonEmpty) {
-      lintMessages.foreach(x => ctx.printLintMessage(x, rule.name))
-      val key = lintMessages.head.category.key(rule.name)
-      val explanation =
-        s"""|To fix this problem, suffix the culprit lines with
-            |   // assert: $key
-            |""".stripMargin
+
+    val uncoveredMessages = diff(lintMessages, lintAssertions)
+    if (uncoveredMessages.nonEmpty) {
+      Patch.reportLintMessages(patches, ctx)
+      val explanation = uncoveredMessages
+        .groupBy(_._2)
+        .map {
+          case (key, positions) =>
+            s"""Append to lines: ${positions.map(_._1.startLine).mkString(", ")}
+               |   // assert: $key""".stripMargin
+        }
+        .mkString("\n\n")
       throw new TestFailedException(
-        s"Uncaught linter messages! $explanation",
+        s"Uncaught linter messages! To fix this problem\n$explanation",
         0)
     }
   }
 
   def runOn(diffTest: DiffTest): Unit = {
     test(diffTest.name) {
       val (rule, config) = diffTest.config.apply()
-      val ctx = RuleCtx(
+      val ctx: RuleCtx = RuleCtx(
         config.dialect(diffTest.original).parse[Source].get,
         config.copy(dialect = diffTest.document.dialect)
       )
-      val patch = rule.fix(ctx)
+      val patches = rule.fixWithName(ctx)
+      assertLintMessagesAreReported(rule, ctx, patches, ctx.tokens)
+      val patch = patches.values.asPatch
       val obtainedWithComment = Patch.apply(patch, ctx, rule.semanticOption)
       val tokens = obtainedWithComment.tokenize.get
-      val checkMessages = rule.check(ctx)
-      assertLintMessagesAreReported(
-        rule,
-        ctx,
-        Patch.lintMessages(patch, ctx, checkMessages),
-        ctx.tokens)
       val obtained = SemanticRuleSuite.stripTestkitComments(tokens)
       val candidateOutputFiles = expectedOutputSourceroot.flatMap { root =>
         val scalaSpecificFilename =
@@ -132,9 +142,7 @@ abstract class SemanticRuleSuite(
         .collectFirst { case f if f.isFile => f.readAllBytes }
         .map(new String(_))
         .getOrElse {
-          // TODO(olafur) come up with more principled check to determine if
-          // rule is linter or rewrite.
-          if (checkMessages.nonEmpty) obtained // linter
+          if (Patch.isOnlyLintMessages(patch)) obtained // linter
           else {
             val tried = candidateOutputFiles.mkString("\n")
             sys.error(

scalafix-tests/input/src/main/scala/test/LintAsserts.scala

@@ -0,0 +1,12 @@
+/*
+rules = [
+  NoInfer
+  "class:scalafix.test.DummyLinter"
+]
+ */
+package test
+
+class LintAsserts {
+  List(1, (1, 2)) // assert: NoInfer.any
+}
+// assert: DummyLinter

scalafix-tests/unit/src/main/scala/scalafix/test/DummyLinter.scala

@@ -0,0 +1,13 @@
+package scalafix.test
+
+import scalafix.LintCategory
+import scalafix.LintMessage
+import scalafix.Rule
+import scalafix.RuleCtx
+
+object DummyLinter extends Rule("DummyLinter") {
+  val error = LintCategory.error("Bam!")
+  override def check(ctx: RuleCtx): Seq[LintMessage] = {
+    error.at(ctx.tokenList.prev(ctx.tokens.last).pos) :: Nil
+  }
+}