From a6ae3395157b9d1b4d3d65a11babf8186e28f1bf Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Tue, 8 Mar 2016 22:24:26 +0100 Subject: [PATCH] Fix instability of self variable API representation The reason for instability is a bit tricky so let's unpack what the previous code checking if there's self type declared was doing. It would check if `thisSym` of a class is equal to a symbol representing the class. If that's true, we know that there's no self type. If it's false, then `thisSym` represents either a self type or a self variable. The second (type test) was supposed to check whether the type of `thisSym` is different from a type of the class. However, it would always yield false because TypeRef of `thisSym` was compared to ClassInfoType of a class. So if you had a self variable the logic would see a self type (and that's what API representation would give you). Now the tricky bit: `thisSym` is not pickled when it's representing just a self variable because self variable doesn't affect other classes referring to a class. If you looked at a type after unpickling, the symbol equality test would yield true and we would not see self type when just a self variable was declared. The fix is to check equality of type refs on both side of the type equality check. This makes the pending test passing. Also, I added another test that checks if self types are represented in various combinations of declaring a self variable or/and self type. Fixes #2504. --- .../src/main/scala/xsbt/ExtractAPI.scala | 2 +- .../scala/xsbt/ExtractAPISpecification.scala | 44 ++++++++++++++++--- 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/compile/interface/src/main/scala/xsbt/ExtractAPI.scala b/compile/interface/src/main/scala/xsbt/ExtractAPI.scala index 07336d31b..75733b1fa 100644 --- a/compile/interface/src/main/scala/xsbt/ExtractAPI.scala +++ b/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 = { diff --git a/compile/interface/src/test/scala/xsbt/ExtractAPISpecification.scala b/compile/interface/src/test/scala/xsbt/ExtractAPISpecification.scala index c4009eabc..d0a9f6908 100644 --- a/compile/interface/src/test/scala/xsbt/ExtractAPISpecification.scala +++ b/compile/interface/src/test/scala/xsbt/ExtractAPISpecification.scala @@ -1,7 +1,7 @@ package xsbt import org.junit.runner.RunWith -import xsbti.api.{ Definition, SourceAPI, ClassLike, Def } +import xsbti.api._ import xsbt.api.SameAPI import org.specs2.mutable.Specification import org.specs2.runner.JUnitRunner @@ -40,11 +40,13 @@ class ExtractAPISpecification extends Specification { } /** - * 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. - */ + * 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 { @@ -70,5 +72,33 @@ class ExtractAPISpecification extends Specification { val namerApi1 = selectNamer(src2Api1) val namerApi2 = selectNamer(src2Api2) SameAPI(namerApi1, namerApi2) - }.pendingUntilFixed("have unstable representation (#2504)") + } + + /** + * 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 compilerForTesting = new ScalaCompilerForUnitTesting + val apis = compilerForTesting.extractApisFromSrcs(reuseCompilerInstance = true)( + List(srcX, srcY, srcC1, srcC2, srcC3, srcC4, srcC5, srcC6) + ).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") + } }