Merge pull request #2507 from gkossakowski/self-variable-unstable

Fix instability of self variable API representation

compile/api/src/main/scala/xsbt/api/SameAPI.scala

@@ -46,7 +46,7 @@ object SameAPI {
   def apply(a: Source, b: Source): Boolean =
     a.apiHash == b.apiHash && (a.hash.nonEmpty && b.hash.nonEmpty) && apply(a.api, b.api)
 
-  def apply(a: Def, b: Def): Boolean =
+  def apply(a: Definition, b: Definition): Boolean =
     (new SameAPI(false, true)).sameDefinitions(List(a), List(b), true)
 
   def apply(a: SourceAPI, b: SourceAPI): Boolean =

compile/interface/src/main/scala/xsbt/ExtractAPI.scala

@@ -578,7 +578,7 @@ class ExtractAPI[GlobalType <: CallbackGlobal](val global: GlobalType,
     // as that invariant is established on completing the class symbol (`mkClassLike` calls `s.initialize` before calling us).
     // Technically, we could even ignore a self type that's a supertype of the class's type,
     // as it does not contribute any information relevant outside of the class definition.
-    if ((s.thisSym eq s) || s.typeOfThis == s.info) Constants.emptyType else processType(in, s.typeOfThis)
+    if ((s.thisSym eq s) || (s.thisSym.tpeHK == s.tpeHK)) Constants.emptyType else processType(in, s.typeOfThis)
 
   def classLike(in: Symbol, c: Symbol): ClassLike = classLikeCache.getOrElseUpdate((in, c), mkClassLike(in, c))
   private def mkClassLike(in: Symbol, c: Symbol): ClassLike = {

compile/interface/src/test/scala/xsbt/ExtractAPISpecification.scala

@@ -1,8 +1,7 @@
 package xsbt
 
 import org.junit.runner.RunWith
-import xsbti.api.ClassLike
-import xsbti.api.Def
+import xsbti.api._
 import xsbt.api.SameAPI
 import org.specs2.mutable.Specification
 import org.specs2.runner.JUnitRunner
@@ -39,4 +38,69 @@ class ExtractAPISpecification extends Specification {
     val fooMethodApi2 = compileAndGetFooMethodApi(src2)
     SameAPI.apply(fooMethodApi1, fooMethodApi2)
   }
+
+  /**
+   * Checks if representation of the inherited Namer class (with a declared self variable) in Global.Foo
+   * is stable between compiling from source and unpickling. We compare extracted APIs of Global when Global
+   * is compiled together with Namers or Namers is compiled first and then Global refers
+   * to Namers by unpickling types from class files.
+   *
+   * See https://github.com/sbt/sbt/issues/2504
+   */
+  "Self variable and no self type" in {
+    def selectNamer(api: SourceAPI): ClassLike = {
+      def selectClass(defs: Iterable[Definition], name: String): ClassLike = defs.collectFirst {
+        case cls: ClassLike if cls.name == name => cls
+      }.get
+      val global = selectClass(api.definitions, "Global")
+      val foo = selectClass(global.structure.declared, "Global.Foo")
+      selectClass(foo.structure.inherited, "Namers.Namer")
+    }
+    val src1 =
+      """|class Namers {
+         |  class Namer { thisNamer => }
+         |}
+         |""".stripMargin
+    val src2 =
+      """|class Global {
+         |  class Foo extends Namers
+         |}
+         |""".stripMargin
+    val compilerForTesting = new ScalaCompilerForUnitTesting
+    val apis = compilerForTesting.extractApisFromSrcs(reuseCompilerInstance = false)(List(src1, src2), List(src2))
+    val _ :: src2Api1 :: src2Api2 :: Nil = apis.toList
+    val namerApi1 = selectNamer(src2Api1)
+    val namerApi2 = selectNamer(src2Api2)
+    SameAPI(namerApi1, namerApi2)
+  }
+
+  /**
+   * Checks if self type is properly extracted in various cases of declaring a self type
+   * with our without a self variable.
+   */
+  "Self type" in {
+    def collectFirstClass(defs: Array[Definition]): ClassLike = defs.collectFirst {
+      case c: ClassLike => c
+    }.get
+    val srcX = "trait X"
+    val srcY = "trait Y"
+    val srcC1 = "class C1 { this: C1 => }"
+    val srcC2 = "class C2 { thisC: C2 => }"
+    val srcC3 = "class C3 { this: X => }"
+    val srcC4 = "class C4 { thisC: X => }"
+    val srcC5 = "class C5 extends AnyRef with X with Y { self: X with Y => }"
+    val srcC6 = "class C6 extends AnyRef with X { self: X with Y => }"
+    val srcC7 = "class C7 { _ => }"
+    val srcC8 = "class C8 { self => }"
+    val compilerForTesting = new ScalaCompilerForUnitTesting
+    val apis = compilerForTesting.extractApisFromSrcs(reuseCompilerInstance = true)(
+      List(srcX, srcY, srcC1, srcC2, srcC3, srcC4, srcC5, srcC6, srcC7, srcC8)
+    ).map(x => collectFirstClass(x.definitions))
+    val emptyType = new EmptyType
+    def hasSelfType(c: ClassLike): Boolean =
+      c.selfType != emptyType
+    val (withSelfType, withoutSelfType) = apis.partition(hasSelfType)
+    withSelfType.map(_.name).toSet === Set("C3", "C4", "C5", "C6")
+    withoutSelfType.map(_.name).toSet === Set("X", "Y", "C1", "C2", "C7", "C8")
+  }
 }

compile/interface/src/test/scala/xsbt/ScalaCompilerForUnitTesting.scala

@@ -31,6 +31,15 @@ class ScalaCompilerForUnitTesting(nameHashing: Boolean = false) {
     analysisCallback.apis(tempSrcFile)
   }
 
+  /**
+   * Compiles given source code using Scala compiler and returns API representation
+   * extracted by ExtractAPI class.
+   */
+  def extractApisFromSrcs(reuseCompilerInstance: Boolean)(srcs: List[String]*): Seq[SourceAPI] = {
+    val (tempSrcFiles, analysisCallback) = compileSrcs(srcs.toList, reuseCompilerInstance)
+    tempSrcFiles.map(analysisCallback.apis)
+  }
+
   def extractUsedNamesFromSrc(src: String): Set[String] = {
     val (Seq(tempSrcFile), analysisCallback) = compileSrcs(src)
     analysisCallback.usedNames(tempSrcFile)
@@ -64,7 +73,7 @@ class ScalaCompilerForUnitTesting(nameHashing: Boolean = false) {
   def extractDependenciesFromSrcs(srcs: List[Map[Symbol, String]]): ExtractedSourceDependencies = {
     val rawGroupedSrcs = srcs.map(_.values.toList)
     val symbols = srcs.flatMap(_.keys)
-    val (tempSrcFiles, testCallback) = compileSrcs(rawGroupedSrcs)
+    val (tempSrcFiles, testCallback) = compileSrcs(rawGroupedSrcs, reuseCompilerInstance = true)
     val fileToSymbol = (tempSrcFiles zip symbols).toMap
 
     val memberRefFileDeps = testCallback.sourceDependencies collect {
@@ -107,18 +116,29 @@ class ScalaCompilerForUnitTesting(nameHashing: Boolean = false) {
    * useful to compile macros, which cannot be used in the same compilation run that
    * defines them.
    *
+   * The `reuseCompilerInstance` parameter controls whether the same Scala compiler instance
+   * is reused between compiling source groups. Separate compiler instances can be used to
+   * test stability of API representation (with respect to pickling) or to test handling of
+   * binary dependencies.
+   *
    * The sequence of temporary files corresponding to passed snippets and analysis
    * callback is returned as a result.
    */
-  private def compileSrcs(groupedSrcs: List[List[String]]): (Seq[File], TestCallback) = {
+  private def compileSrcs(groupedSrcs: List[List[String]],
+    reuseCompilerInstance: Boolean): (Seq[File], TestCallback) = {
     withTemporaryDirectory { temp =>
       val analysisCallback = new TestCallback(nameHashing)
       val classesDir = new File(temp, "classes")
       classesDir.mkdir()
 
-      val compiler = prepareCompiler(classesDir, analysisCallback, classesDir.toString)
+      lazy val commonCompilerInstance = prepareCompiler(classesDir, analysisCallback, classesDir.toString)
 
       val files = for ((compilationUnit, unitId) <- groupedSrcs.zipWithIndex) yield {
+        // use a separate instance of the compiler for each group of sources to
+        // have an ability to test for bugs in instability between source and pickled
+        // representation of types
+        val compiler = if (reuseCompilerInstance) commonCompilerInstance else
+          prepareCompiler(classesDir, analysisCallback, classesDir.toString)
         val run = new compiler.Run
         val srcFiles = compilationUnit.toSeq.zipWithIndex map {
           case (src, i) =>
@@ -137,7 +157,7 @@ class ScalaCompilerForUnitTesting(nameHashing: Boolean = false) {
   }
 
   private def compileSrcs(srcs: String*): (Seq[File], TestCallback) = {
-    compileSrcs(List(srcs.toList))
+    compileSrcs(List(srcs.toList), reuseCompilerInstance = true)
   }
 
   private def prepareSrcFile(baseDir: File, fileName: String, src: String): File = {