Do not depend on tree transformations for the add import refactoring

This is a similar change to the rename refactoring. The overall idea is
to not depend on any tree transformations when an import is added to a
file. Theoretically, doing any transformations on the tree is the
optimal solution but only if the tree is a concrete syntax tree. The
scalac trees however are abstract syntax trees and therefore don't do
what we want. For the add import refactoring, they remove all the
formatting of the imports, especially removed new lines are a concern.

This commit therefore replaces the tree transformations by generating
textual code changes. This way, the refactoring is only doing a minimal
change, which in our case is to add one or multiple imports. A side
effect is that some functionality is lost but I consider that as a good
change. Formerly, the add import refactoring also did some minor
cleanups to the regions around imports. It added newlines, which means
that an import that got added to a code snippet like the following

  class A {
    val f = new File
  }

did result in this:

  import java.io.File

  class A {
    val f = new File
  }

Whereas after the refactorings of this commit the extra newline is no
longer printed:

  import java.io.File
  class A {
    val f = new File
  }

This may not look better but it makes the implementation easier because
we don't have to check if a newline already exists. Also, conceptually
it is the job of the organize imports refactoring to take care of such a
cleanup. The add import refactoring should only do a minimal change and
that is exactly what it is doing right now.

Fix #1002514

src/main/scala/scala/tools/refactoring/implementations/AddImportStatement.scala

@@ -12,17 +12,15 @@ import scala.tools.refactoring.common.TextChange
 
 abstract class AddImportStatement extends Refactoring with InteractiveScalaCompiler {
 
-  val global: tools.nsc.interactive.Global
+  override val global: tools.nsc.interactive.Global
 
   def addImport(file: AbstractFile, fqName: String): List[TextChange] = addImports(file, List(fqName))
 
   def addImports(file: AbstractFile, importsToAdd: Iterable[String]): List[TextChange] = {
 
     val astRoot = abstractFileToTree(file)
 
-    refactor((addImportTransformation(importsToAdd) apply astRoot).toList) collect {
-      case tc: TextChange => tc
-    }
+    addImportTransformation(importsToAdd.toSeq)(astRoot).toList
   }
 
   @deprecated("Use addImport(file, ..) instead", "0.4.0")

src/main/scala/scala/tools/refactoring/implementations/MoveClass.scala

@@ -1,15 +1,12 @@
 package scala.tools.refactoring
 package implementations
 
+import scala.reflect.internal.util.SourceFile
 import scala.tools.refactoring.analysis.CompilationUnitDependencies
-import scala.tools.refactoring.common.TreeExtractors
-import scala.tools.refactoring.common.NewFileChange
-import scala.tools.refactoring.common.InteractiveScalaCompiler
 import scala.tools.refactoring.common.Change
+import scala.tools.refactoring.common.InteractiveScalaCompiler
+import scala.tools.refactoring.common.TreeExtractors
 import scala.tools.refactoring.transformation.TreeFactory
-import scala.collection.mutable.ListBuffer
-import scala.tools.refactoring.common.TextChange
-import scala.reflect.internal.util.SourceFile
 
 abstract class MoveClass extends MultiStageRefactoring with TreeFactory with analysis.Indexes with TreeExtractors with InteractiveScalaCompiler with CompilationUnitDependencies with ImportsHelper {
 
@@ -19,13 +16,13 @@ abstract class MoveClass extends MultiStageRefactoring with TreeFactory with ana
    * Returns Some if the user selected a single ImplDef in the source file.
    * None means that all ImplDefs should be moved.
    */
-  type PreparationResult = Option[ImplDef]
+  override type PreparationResult = Option[ImplDef]
 
   /**
    * We don't really need the preparation result in the refactoring, instead
    * the user has the option of ignoring the selected ImplDef, therefore we
    * take a copy of the PreparationResult as a parameter.
-   * */
+   */
   case class RefactoringParameters(packageName: String, moveSingleImpl: PreparationResult)
 
   /**
@@ -43,7 +40,7 @@ abstract class MoveClass extends MultiStageRefactoring with TreeFactory with ana
    *
    * is not.
    */
-  def prepare(s: Selection) = {
+  override def prepare(s: Selection) = {
     def hasSinglePackageDeclaration = topPackageDef(s.root.asInstanceOf[PackageDef]) match {
       case PackageDef(_, stats) => stats forall (stmt => stmt.isInstanceOf[ImplDef] || stmt.isInstanceOf[Import])
       case _ => false
@@ -90,7 +87,7 @@ abstract class MoveClass extends MultiStageRefactoring with TreeFactory with ana
     }
   }
 
-  def perform(selection: Selection, preparationResult: PreparationResult, parameters: RefactoringParameters): Either[RefactoringError, List[Change]] = {
+  override def perform(selection: Selection, preparationResult: PreparationResult, parameters: RefactoringParameters): Either[RefactoringError, List[Change]] = {
 
     val toMove = parameters.moveSingleImpl
 
@@ -106,7 +103,7 @@ abstract class MoveClass extends MultiStageRefactoring with TreeFactory with ana
      *
      *  2) The complete file is moved, this is easier because we simply move
      *     the file and don't need to create a new source file.
-     * */
+     */
     val movedClassChanges = if(moveOnlyPartOfSourceFile(selection, parameters)) {
       trace("Moving only a part of %s", selection.file.name)
 
@@ -131,7 +128,7 @@ abstract class MoveClass extends MultiStageRefactoring with TreeFactory with ana
     /*
      * We need to adapt the imports of all the files that reference one of the moved classes.
      * This include imports to the moved classes and fully qualified names.
-     * */
+     */
     val otherFiles = adaptDependentFiles(selection, toMove, parameters.packageName)
 
     Right(movedClassChanges ++ otherFiles)
@@ -143,7 +140,7 @@ abstract class MoveClass extends MultiStageRefactoring with TreeFactory with ana
 
   /**
    * Returns a transformation that creates the contents of the target file.
-   * */
+   */
   private def createRenamedTargetPackageTransformation(parameters: RefactoringParameters, implsToMove: List[Tree], importsFor: Tree) = {
 
     val targetPackages = parameters.packageName.split("\\.").toList
@@ -189,7 +186,7 @@ abstract class MoveClass extends MultiStageRefactoring with TreeFactory with ana
 
   /**
    * Returns the list of changes that adapt the old file (only when we don't move the complete file)
-   * */
+   */
   private def removeClassFromOldFileAndAddImportToNewIfNecessary(selection: Selection, parameters: RefactoringParameters) = {
 
     val toMove = parameters.moveSingleImpl.get
@@ -207,14 +204,15 @@ abstract class MoveClass extends MultiStageRefactoring with TreeFactory with ana
     }
 
     val removeClassFromOldFile = replaceTree(toMove, EmptyTree)
+    val moveClassChange = transformFile(selection.file, removeClassFromOldFile).toList
 
-    val trans = if(hasRelativeReferenceToMovedClass) {
-      removeClassFromOldFile &> addImportTransformation(List(parameters.packageName + "." + toMove.nameString))
-    } else {
-      removeClassFromOldFile
+    if(hasRelativeReferenceToMovedClass) {
+      val trans = addImportTransformation(List(parameters.packageName + "." + toMove.nameString))
+      val addImportChange = trans(abstractFileToTree(selection.file)).toList
+      moveClassChange ++ addImportChange
     }
-
-    transformFile(selection.file, trans)
+    else
+      moveClassChange
   }
 
   private def topLevelStats(selection: Selection): List[Tree] = {
@@ -280,7 +278,7 @@ abstract class MoveClass extends MultiStageRefactoring with TreeFactory with ana
         }
 
         if(!alreadyHasImportSelector && hasReferenceWithoutFullName) {
-          val addImport = new AddImportStatement { val global = MoveClass.this.global }
+          val addImport = new AddImportStatement { override val global = MoveClass.this.global }
           addImport.addImport(sourceFile.file, newFullPackageName + "." + referencedName)
         } else {
 

src/main/scala/scala/tools/refactoring/transformation/TreeTransformations.scala

@@ -5,15 +5,15 @@
 package scala.tools.refactoring
 package transformation
 
-import language.implicitConversions
+import scala.tools.refactoring.common.TextChange
 
 trait TreeTransformations extends Transformations with TreeFactory {
 
   this: common.EnrichedTrees with common.CompilerAccess =>
 
   import global._
 
-  def traverse(tree: Tree, f: Tree => Tree): Tree = {
+  override def traverse(tree: Tree, f: Tree => Tree): Tree = {
 
     /**
      * Hooks into the Scala compiler's Transformer but applies only
@@ -177,40 +177,79 @@ trait TreeTransformations extends Transformations with TreeFactory {
     case t: global.Tree => t.pos = global.NoPosition; t
   }
 
-  def addImportTransformation(importsToAdd: Iterable[String]): Transformation[Tree, Tree] = {
-
-    import global._
-
-    def importTrees = {
-      val SplitAtDot = "(.*)\\.(.*?)".r
-      importsToAdd.map {
-        case SplitAtDot(pkg, name) => mkImportFromStrings(pkg, name)
-      }.toList
+  def addImportTransformation(importsToAdd: Seq[String]): Transformation[Tree, TextChange] = {
+    def splitImports(p: PackageDef, stats: List[Tree]) = {
+      val (imports, others) = stats partition (_.isInstanceOf[Import])
+      (p, imports map (_.asInstanceOf[Import]), others)
     }
 
-    val addImportStatement = once(findBestPackageForImports &> transformation[(PackageDef, List[Import], List[Tree]), Tree] {
+    def importsAsSrc(indent: String) = {
+      def escaped(imp: String) = imp.split('.').map(escapeScalaKeywordsForImport).mkString(".")
+      importsToAdd.map(imp ⇒ s"${indent}import ${escaped(imp)}").mkString("\n")
+    }
 
-      // For an empty PackageDef, with no exiting imports but with an Impl that has an annotation, we need to
-      // modify positions so that the annotation, which is not in the AST, doesn't get assigned to the PackageDef
-      // but to the Impl
-      case (p @ PackageDef(Ident(nme.EMPTY_PACKAGE_NAME), _), Nil, others @ (impl :: _)) if !impl.symbol.annotations.isEmpty =>
+    def insertAfter(pos: Position) = {
+      val line = pos.source.lineToString(pos.source.offsetToLine(pos.start))
+      val indent = line.takeWhile(Character.isWhitespace)
+      val insertPos = pos.end
+      TextChange(pos.source, insertPos, insertPos, "\n" + importsAsSrc(indent))
+    }
 
-        // The `pid` is invisible anyway, but by erasing its position we make sure
-        // it doesn't get any layout associated
-        val ignoredPid = p.pid setPos NoPosition
+    def insertBefore(pos: Position) = {
+      val lineNumber = pos.source.offsetToLine(pos.start)
+      val line = pos.source.lineToString(lineNumber)
+      val indent = line.takeWhile(Character.isWhitespace)
+      val insertPos = pos.source.lineToOffset(lineNumber)
+      TextChange(pos.source, insertPos, insertPos, importsAsSrc(indent) + "\n")
+    }
 
-        // The empty PackageDef starts at the position of its first child, so the annotation of the Impl
-        // is outside its parent's range. The Source Generator can't handle this, so we let the PackageDef
-        // start at position 0 so that the annotation gets associated to the child.
-        val pos = p.pos withStart 0
+    /*
+     * `foundPackage` is a big fat hack to remember the `PackageDef` that is
+     * found in `findBestPackageForImports`. For some reason the transformation
+     * is run multiple times, even though, the `once` call later should prevent
+     * that. Therefore the `PackageDef` that is returned by
+     * `findBestPackageForImports` is not the same as the one that is found
+     * inside of it (and I don't really understand why).
+     */
+    var foundPackage: PackageDef = null
+    val findBestPackageForImports = {
+      def isPackageDef(tree: Tree) = tree.isInstanceOf[PackageDef]
 
-        p copy (pid = ignoredPid, stats = (importTrees ::: others)) setPos pos
+      def isBestPlaceForImports(trees: List[Tree]) = {
+        val (pkgDefs, otherTrees) = trees.partition(isPackageDef)
+        otherTrees.nonEmpty || pkgDefs.size >= 2
+      }
 
-      case (p, imports, others) =>
-        p copy (stats = (imports ::: importTrees ::: others)) replaces p
-    })
+      transformation[Tree, Tree] {
+        case p @ PackageDef(_, stats) if isBestPlaceForImports(stats) =>
+          if (foundPackage == null)
+            foundPackage = p
+          p
+      }
+    }
 
     // first try it at the top level to avoid traversing the complete AST
-    addImportStatement |> topdown(matchingChildren(addImportStatement))
+    val insertLocation = findBestPackageForImports |> topdown(matchingChildren(findBestPackageForImports))
+
+    val addImportStatement = once(insertLocation) &> transformation { case _ ⇒
+      splitImports(foundPackage, foundPackage.stats) match {
+        // For an empty PackageDef, with no exiting imports but with an Impl that has an annotation, we need to
+        // modify positions so that the annotation, which is not in the AST, doesn't get assigned to the PackageDef
+        // but to the Impl
+        case (p @ PackageDef(Ident(nme.EMPTY_PACKAGE_NAME), _), Nil, others @ (impl :: _)) if !impl.symbol.annotations.isEmpty ⇒
+          // The empty PackageDef starts at the position of its first child, so the annotation of the Impl
+          // is outside its parent's range. The Source Generator can't handle this, so we let the PackageDef
+          // start at position 0 so that the annotation gets associated to the child.
+          val pos = p.pos withStart 0
+          insertBefore(pos)
+        case (p, imports, others) ⇒
+          if (imports.nonEmpty)
+            insertAfter(imports.last.pos)
+          else
+            insertBefore(others.head.pos)
+      }
+    }
+
+    addImportStatement
   }
 }

src/test/scala/scala/tools/refactoring/tests/implementations/MoveClassTest.scala

@@ -13,7 +13,7 @@ class MoveClassTest extends TestHelper with TestRefactoring {
 
   private def createRefactoring(pro: FileSet) = {
     new TestRefactoringImpl(pro) {
-      val refactoring = new MoveClass with TestProjectIndex
+      override val refactoring = new MoveClass with TestProjectIndex
     }
   }
 
@@ -666,7 +666,6 @@ class MoveClassTest extends TestHelper with TestRefactoring {
       package a.b.c
 
       import x.y.ToMove
-
       class User extends ToMove
     """
   } applyRefactoring(moveTo("x.y"))
@@ -875,7 +874,6 @@ class MoveClassTest extends TestHelper with TestRefactoring {
     }
     """ becomes """
     import bar.Bar64
-
     class Foo64 {
       import Bar64.instance
 
@@ -934,7 +932,6 @@ class MoveClassTest extends TestHelper with TestRefactoring {
       package c
 
       import x.y.ToMove
-
       trait Xy {
         val other: ToMove
       }