Merge pull request #168 from olafurpg/unused-imports

Fix two buggy cases discovered while running on akka + scalding.

Author: olafurpg

.scalafmt.conf

@@ -8,5 +8,6 @@ align.tokens = [
 onTestFailure = "To fix this, run `./scalafmt` from the project base directory"
 optIn.annotationNewlines = true
 project.excludeFilters = [
+  scalafix-tests/input
   scalafix-tests/output
 ]

scalafix-core/src/main/scala/scalafix/patch/ImportPatchOps.scala

@@ -53,8 +53,10 @@ object ImportPatchOps {
         .toSet
     val curlyBraceRemoves = allImporters.map { importer =>
       val keptImportees = importer.importees.filterNot(isRemovedImportee)
+      val hasRemovedImportee = importer.importees.exists(isRemovedImportee)
       keptImportees match {
-        case (Importee.Wildcard() | Importee.Name(_)) +: Nil =>
+        case (Importee.Wildcard() | Importee.Name(_)) +: Nil
+            if hasRemovedImportee =>
           ctx
             .toks(importer)
             .collectFirst {
@@ -70,34 +72,43 @@ object ImportPatchOps {
         case _ => Patch.empty
       }
     }
+    // NOTE: keeps track of which comma is removed by which tree to prevent the
+    // same comma being removed twice.
+    val isRemovedComma = mutable.Map.empty[Token.Comma, Tree]
     val isRemovedImport =
       allImports.filter(_.importers.forall(isRemovedImporter))
     def remove(toRemove: Tree) = {
       val tokens = ctx.toks(toRemove)
-      def removeFirstComma(lst: Iterable[Token]) =
+      def removeFirstComma(lst: Iterable[Token]) = {
         lst
           .takeWhile {
             case Token.Space() => true
-            case Token.Comma() => true
+            case comma @ Token.Comma() =>
+              if (!isRemovedComma.contains(comma)) {
+                isRemovedComma(comma) = toRemove
+              }
+              true
             case _ => false
           }
           .map(ctx.removeToken(_))
+      }
       val leadingComma =
-        removeFirstComma(ctx.tokenList.to(tokens.head))
+        removeFirstComma(ctx.tokenList.leading(tokens.head))
       val hadLeadingComma = leadingComma.exists {
-        case TokenPatch.Add(_: Token.Comma, _, _, keepTok @ false) => true
+        case TokenPatch.Add(comma: Token.Comma, _, _, keepTok @ false) =>
+          isRemovedComma.get(comma).contains(toRemove)
         case _ => false
       }
       val trailingComma =
         if (hadLeadingComma) List(Patch.empty)
-        else removeFirstComma(ctx.tokenList.from(tokens.last))
+        else removeFirstComma(ctx.tokenList.trailing(tokens.last))
       PatchOps.removeTokens(tokens) ++ trailingComma ++ leadingComma
     }
 
     val leadingNewlines = isRemovedImport.map { i =>
       var newline = false
       ctx.tokenList
-        .to(ctx.toks(i).head)
+        .leading(ctx.toks(i).head)
         .takeWhile(x =>
           !newline && {
             x.is[Token.Space] || {

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

@@ -56,7 +56,9 @@ abstract class TokenPatch(val tok: Token, val newTok: String)
     extends Patch
     with LowLevelPatch {
   override def toString: String =
-    s"TokenPatch.${this.getClass.getSimpleName}(${tok.syntax.revealWhiteSpace}, ${tok.structure}, $newTok)"
+    if (newTok.isEmpty) s"TokenPatch.Remove(${tok.structure.revealWhiteSpace})"
+    else
+      s"TokenPatch.${this.getClass.getSimpleName}(${tok.syntax.revealWhiteSpace}, ${tok.structure}, $newTok)"
 }
 private[scalafix] object TokenPatch {
   case class Remove(override val tok: Token) extends TokenPatch(tok, "")

scalafix-core/src/main/scala/scalafix/util/TokenList.scala

@@ -7,9 +7,9 @@ import scala.meta.tokens.Tokens
 
 /** Helper to traverse tokens as a doubly linked list.  */
 class TokenList(tokens: Tokens) {
-  def from(token: Token): SeqView[Token, IndexedSeq[Token]] =
+  def trailing(token: Token): SeqView[Token, IndexedSeq[Token]] =
     tokens.view(tok2idx(token), tokens.length).drop(1)
-  def to(token: Token): SeqView[Token, IndexedSeq[Token]] =
+  def leading(token: Token): SeqView[Token, IndexedSeq[Token]] =
     tokens.view(0, tok2idx(token)).drop(1).reverse
   private[this] val tok2idx = {
     val map = Map.newBuilder[Token, Int]

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

@@ -9,6 +9,18 @@ import scala.concurrent.{Await, Future}
 import scala.util.{Properties, DynamicVariable, Try}
 import scala.util.{Success => Successful, Random}
 import scala.sys.process._
+import scala.concurrent.{CancellationException, ExecutionException, ExecutionContext}
+import scala.runtime.{RichBoolean}
+import scala.concurrent.{ // formatting caveat
+  CancellationException,
+  ExecutionException,
+  TimeoutException
+}
+//import scala.concurrent.{
+//    CancellationException
+//  , ExecutionException
+//  , TimeoutException // ERROR
+//}
 
 object RemoveUnusedImports {
   import Future._
@@ -17,4 +29,7 @@ object RemoveUnusedImports {
   Properties.ScalaCompilerVersion
   Try(1)
   Successful(1)
+  ExecutionContext.defaultReporter
+  new RichBoolean(true)
+  new TimeoutException
 }

scalafix-tests/output/src/main/scala/test/RemoveUnusedImports.scala

@@ -4,11 +4,26 @@ import scala.util.control.NonFatal
 import scala.concurrent.Future
 import scala.util.{Properties, Try}
 import scala.util.{Success => Successful}
+import scala.concurrent.ExecutionContext
+import scala.runtime.{RichBoolean}
+import scala.concurrent. // formatting caveat
+
+
+  TimeoutException
+
+//import scala.concurrent.{
+//    CancellationException
+//  , ExecutionException
+//  , TimeoutException // ERROR
+//}
 
 object RemoveUnusedImports {
   val NonFatal(a) = new Exception
   Future.successful(1)
   Properties.ScalaCompilerVersion
   Try(1)
   Successful(1)
+  ExecutionContext.defaultReporter
+  new RichBoolean(true)
+  new TimeoutException
 }