Escape hatches on fix (fix scalacenter#368)

project/Mima.scala

@@ -13,6 +13,10 @@ object Mima {
       // marked private[scalafix]
       ProblemFilters.exclude[ReversedMissingMethodProblem](
         "scalafix.rule.RuleCtx.printLintMessage"),
+      ProblemFilters.exclude[ReversedMissingMethodProblem](
+        "scalafix.rule.RuleCtx.filter"),
+      ProblemFilters.exclude[IncompatibleMethTypeProblem](
+        "scalafix.patch.Patch.apply"),
       ProblemFilters.exclude[DirectMissingMethodProblem](
         "scalafix.patch.Patch.reportLintMessages"),
       ProblemFilters.exclude[DirectMissingMethodProblem](

scalafix-core/shared/src/main/scala/scalafix/internal/patch/EscapeHatch.scala

@@ -4,6 +4,7 @@ package internal.patch
 import scalafix.internal.config.FilterMatcher
 import scalafix.lint.LintMessage
 import scalafix.rule.RuleName
+import scalafix.patch._
 
 import scala.meta._
 import scala.meta.tokens.Token
@@ -36,23 +37,23 @@ class EscapeHatch(
   // comment disabling r at position p1 < p
   // and there is no comment enabling r in position p2 where p1 < p2 < p.
   private def isEnabled(
-      message: LintMessage): (Boolean, Option[EscapeFilter]) = {
-
+      name: String,
+      start: Int): (Boolean, Option[EscapeFilter]) = {
     var culprit = Option.empty[EscapeFilter]
 
     val isDisabled =
-      disableRules.to(EscapeOffset(message.position.start)).exists {
+      disableRules.to(EscapeOffset(start)).exists {
         case (disableOffset, disableFilter) => {
-          val isDisabled = disableFilter.matches(message.id)
+          val isDisabled = disableFilter.matches(name)
           if (isDisabled) {
             culprit = Some(disableFilter)
           }
           isDisabled && {
             !enableRules
-              .range(disableOffset, EscapeOffset(message.position.start))
+              .range(disableOffset, EscapeOffset(start))
               .values
               .exists { enableFilter =>
-                val isEnabled = enableFilter.matches(message.id)
+                val isEnabled = enableFilter.matches(name)
                 if (isEnabled) {
                   culprit = Some(enableFilter)
                 }
@@ -65,23 +66,50 @@ class EscapeHatch(
     (!isDisabled, culprit)
   }
 
-  /* This method disable rules based on the escape hatch comments
-   * and report any unused escapes
-   */
-  def filter(lints: List[LintMessage]): List[LintMessage] = {
+  def filter(
+      patchesByName: Map[RuleName, Patch],
+      ctx: RuleCtx,
+      index: SemanticdbIndex): (Patch, List[LintMessage]) = {
     val usedEscapes = mutable.Set.empty[EscapeOffset]
     val filteredLints = List.newBuilder[LintMessage]
 
-    lints.foreach { lint =>
-      val (isLintEnabled, culprit) = isEnabled(lint)
+    def loop(name: RuleName, patch: Patch): Patch = patch match {
+      case AtomicPatch(underlying) =>
+        val hasEscape =
+          Patch.treePatchApply(underlying)(ctx, index).exists { tp =>
+            val (isPatchEnabled, culprit) =
+              isEnabled(name.toString, tp.tok.pos.start)
+            culprit.foreach(escape => usedEscapes += escape.offset)
+            !isPatchEnabled
+          }
 
-      if (isLintEnabled) {
-        filteredLints += lint
-      }
+        if (hasEscape) EmptyPatch
+        else underlying
+
+      case Concat(a, b) =>
+        Concat(loop(name, a), loop(name, b))
+
+      case lp @ LintPatch(orphanLint) =>
+        val lint = orphanLint.withOwner(name)
+        val (isLintEnabled, culprit) = isEnabled(lint.id, lint.position.start)
 
-      culprit.foreach(escape => usedEscapes += escape.offset)
+        culprit.foreach(escape => usedEscapes += escape.offset)
+
+        if (isLintEnabled) {
+          filteredLints += lint
+        }
+
+        EmptyPatch
+
+      case e => e
     }
 
+    val patches =
+      patchesByName.map {
+        case (name, patch) =>
+          loop(name, patch)
+      }.asPatch
+
     val unusedDisableWarning =
       LintCategory
         .warning(
@@ -94,7 +122,7 @@ class EscapeHatch(
       (disableRules -- usedEscapes).values
         .map(unused => unusedDisableWarning.at(unused.anchorPosition.value))
 
-    filteredLints.result() ++ unusedEnable ++ unusedEscapesWarning
+    (patches, filteredLints.result() ++ unusedEnable ++ unusedEscapesWarning)
   }
 }
 

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

@@ -28,6 +28,6 @@ case class RemoveUnusedImports(index: SemanticdbIndex)
   }
   override def fix(ctx: RuleCtx): Patch =
     ctx.tree.collect {
-      case i: Importee if isUnused(i) => ctx.removeImportee(i)
+      case i: Importee if isUnused(i) => ctx.removeImportee(i).atomic
     }.asPatch
 }

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

@@ -63,8 +63,10 @@ case class RuleCtxImpl(tree: Tree, config: ScalafixConfig) extends RuleCtx {
     )
   }
 
-  def filterLintMessage(lints: List[LintMessage]): List[LintMessage] =
-    escapeHatch.filter(lints)
+  def filter(
+      patchesByName: Map[RuleName, Patch],
+      index: SemanticdbIndex): (Patch, List[LintMessage]) =
+    escapeHatch.filter(patchesByName, this, index)
 
   def lint(msg: LintMessage): Patch =
     LintPatch(msg)

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

@@ -48,12 +48,14 @@ sealed abstract class Patch {
   def ++(other: Iterable[Patch]): Patch = other.foldLeft(this)(_ + _)
   def isEmpty: Boolean = this == EmptyPatch
   def nonEmpty: Boolean = !isEmpty
+  def atomic: Patch = AtomicPatch(this)
 }
 
 //////////////////////////////
 // Low-level patches
 //////////////////////////////
 trait LowLevelPatch
+
 abstract class TokenPatch(val tok: Token, val newTok: String)
     extends Patch
     with LowLevelPatch {
@@ -92,6 +94,7 @@ private[scalafix] object TreePatch {
 }
 
 // implementation detail
+private[scalafix] case class AtomicPatch(underlying: Patch) 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
@@ -118,36 +121,23 @@ object Patch {
     case _ => throw Failure.TokenPatchMergeError(a, b)
   }
 
-  private[scalafix] def lintMessages(
-      patches: Map[RuleName, Patch],
-      ctx: RuleCtx): List[LintMessage] = {
-
-    val builder = List.newBuilder[LintMessage]
-    patches.map {
-      case (owner, patch) =>
-        foreach(patch) {
-          case LintPatch(orphanLint) =>
-            builder += orphanLint.withOwner(owner)
-          case _ => ()
-        }
-    }
-
-    ctx.filterLintMessage(builder.result())
-  }
-
   // 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,
+      patchesByName: Map[scalafix.rule.RuleName, scalafix.Patch],
       ctx: RuleCtx,
       index: Option[SemanticdbIndex]
-  ): String = {
-    treePatchApply(p)(ctx, index.getOrElse(SemanticdbIndex.empty))
+  ): (String, List[LintMessage]) = {
+    val idx = index.getOrElse(SemanticdbIndex.empty)
+    val (patch, lints) = ctx.filter(patchesByName, idx)
+    val patches = treePatchApply(patch)(ctx, idx)
+    (tokenPatchApply(ctx, patches), lints)
   }
 
-  private def treePatchApply(
-      patch: Patch)(implicit ctx: RuleCtx, index: SemanticdbIndex): String = {
+  def treePatchApply(patch: Patch)(
+      implicit ctx: RuleCtx,
+      index: SemanticdbIndex): Iterable[TokenPatch] = {
     val base = underlying(patch)
     val moveSymbol = underlying(
       ReplaceSymbolOps.naiveMoveSymbolPatch(base.collect {
@@ -169,7 +159,7 @@ object Patch {
               s"Expected TokenPatch, got $els")
         }
     }
-    tokenPatchApply(ctx, importTokenPatches ++ tokenPatches)
+    importTokenPatches ++ tokenPatches
   }
 
   private def tokenPatchApply(

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

@@ -81,18 +81,18 @@ abstract class Rule(ruleName: RuleName) { self =>
   final def apply(ctx: RuleCtx, patch: Patch): String =
     apply(ctx, Map(name -> patch))
   final def apply(ctx: RuleCtx, patches: Map[RuleName, Patch]): String = {
-    val result = Patch(patches.values.asPatch, ctx, semanticOption)
+    val (fixed, lintMessages) = Patch(patches, ctx, semanticOption)
     // This overload of apply if purely for convenience
     // Use fixWithName to iterate over LintMessage
     // without printing to the console
-    Patch.lintMessages(patches, ctx).foreach(ctx.printLintMessage)
-    result
+    // Patch.lintMessages(patches, ctx).
+    lintMessages.foreach(ctx.printLintMessage)
+    fixed
   }
   final def applyAndLint(ctx: RuleCtx): (String, List[LintMessage]) = {
     val patches = fixWithName(ctx)
-    val fixed = Patch(patches.values.asPatch, ctx, semanticOption)
-    val messages = Patch.lintMessages(patches, ctx)
-    (fixed, messages)
+    val (fixed, lintMessages) = Patch(patches, ctx, semanticOption)
+    (fixed, lintMessages)
   }
 
   /** Returns unified diff from applying this patch */

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

@@ -52,9 +52,10 @@ trait RuleCtx extends PatchOps {
   private[scalafix] def toks(t: Tree): Tokens
   private[scalafix] def config: ScalafixConfig
   private[scalafix] def printLintMessage(msg: LintMessage): Unit
-  private[scalafix] def filterLintMessage(
-      lints: List[LintMessage]): List[LintMessage]
 
+  private[scalafix] def filter(
+      patchesByName: Map[RuleName, Patch],
+      index: SemanticdbIndex): (Patch, List[LintMessage])
 }
 
 object RuleCtx {

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

@@ -73,12 +73,8 @@ abstract class SemanticRuleSuite(
     }
 
   private def assertLintMessagesAreReported(
-      rule: Rule,
-      ctx: RuleCtx,
-      patches: Map[RuleName, Patch],
+      reportedLintMessages: List[LintMessage],
       tokens: Tokens): Unit = {
-
-    val reportedLintMessages = Patch.lintMessages(patches, ctx)
     val expectedLintMessages = CommentAssertion.extract(tokens)
     val diff = AssertDiff(reportedLintMessages, expectedLintMessages)
 
@@ -97,9 +93,11 @@ abstract class SemanticRuleSuite(
         config.copy(dialect = diffTest.document.dialect)
       )
       val patches = rule.fixWithName(ctx)
-      assertLintMessagesAreReported(rule, ctx, patches, ctx.tokens)
+      val (obtainedWithComment, lintMessages) =
+        Patch.apply(patches, ctx, rule.semanticOption)
+      assertLintMessagesAreReported(lintMessages, ctx.tokens)
+
       val patch = patches.values.asPatch
-      val obtainedWithComment = Patch.apply(patch, ctx, rule.semanticOption)
       val tokens = obtainedWithComment.tokenize.get
       val obtained = SemanticRuleSuite.stripTestkitComments(tokens)
       val candidateOutputFiles = expectedOutputSourceroot.flatMap { root =>

scalafix-tests/input/src/main/scala/test/removeUnused/RemoveUnusedImportsEscapeHatch.scala

@@ -0,0 +1,8 @@
+/*
+rule = RemoveUnusedImports
+ */
+package test.removeUnused
+
+// unused
+import scala.collection.mutable
+import scala.collection.immutable // scalafix:ok RemoveUnusedImports

scalafix-tests/output/src/main/scala/test/removeUnused/RemoveUnusedImportsEscapeHatch.scala

@@ -0,0 +1,4 @@
+package test.removeUnused
+
+// unused
+import scala.collection.immutable // scalafix:ok RemoveUnusedImports