From a37d8d4770ba2776d4d2fa2380030592527cee4f Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Fri, 2 Aug 2013 16:27:47 -0700 Subject: [PATCH] Fix unstable existential type names bug. Fix the problem with unstable names synthesized for existential types (declared with underscore syntax) by renaming type variables to a scheme that is guaranteed to be stable no matter where given the existential type appears. The sheme we use are De Bruijn-like indices that capture both position of type variable declarion within single existential type and nesting level of nested existential type. This way we properly support nested existential types by avoiding name clashes. In general, we can perform renamings like that because type variables declared in existential types are scoped to those types so the renaming operation is local. There's a specs2 unit test covering instability of existential types. The test is included in compiler-interface project and the build definition has been modified to enable building and executing tests in compiler-interface project. Some dependencies has been modified: * compiler-interface project depends on api project for testing (test makes us of SameAPI) * dependency on junit has been introduced because it's needed for `@RunWith` annotation which declares that specs2 unit test should be ran with JUnitRunner SameAPI has been modified to expose a method that allows us to compare two definitions. This commit also adds `ScalaCompilerForUnitTesting` class that allows to compile a piece of Scala code and inspect information recorded callbacks defined in `AnalysisCallback` interface. That class uses existing ConsoleLogger for logging. I considered doing the same for ConsoleReporter. There's LoggingReporter defined which would fit our usecase but it's defined in compile subproject that compiler-interface doesn't depend on so we roll our own. ScalaCompilerForUnit testing uses TestCallback from compiler-interface subproject for recording information passed to callbacks. In order to be able to access TestCallback from compiler-interface subproject I had to tweak dependencies between interface and compiler-interface so test classes from the former are visible in the latter. I also modified the TestCallback itself to accumulate apis in a HashMap instead of a buffer of tuples for easier lookup. An integration test has been added which tests scenario mentioned in #823. This commit fixes #823. --- .../api/src/main/scala/xsbt/api/SameAPI.scala | 9 +- .../src/main/scala/xsbt/ExtractAPI.scala | 98 ++++++++++++++++++- .../scala/xsbt/ExtractAPISpecification.scala | 42 ++++++++ .../xsbt/ScalaCompilerForUnitTesting.scala | 71 ++++++++++++++ .../src/test/scala/xsbti/TestCallback.scala | 12 ++- project/Sbt.scala | 12 ++- project/Util.scala | 3 +- .../api/unstable-existential-names/build.sbt | 10 ++ .../changes/Foo1.scala | 13 +++ .../src/main/scala/Bar.scala | 5 + .../src/main/scala/Foo.scala | 7 ++ .../api/unstable-existential-names/test | 12 +++ 12 files changed, 281 insertions(+), 13 deletions(-) create mode 100644 compile/interface/src/test/scala/xsbt/ExtractAPISpecification.scala create mode 100644 compile/interface/src/test/scala/xsbt/ScalaCompilerForUnitTesting.scala create mode 100644 sbt/src/sbt-test/api/unstable-existential-names/build.sbt create mode 100644 sbt/src/sbt-test/api/unstable-existential-names/changes/Foo1.scala create mode 100644 sbt/src/sbt-test/api/unstable-existential-names/src/main/scala/Bar.scala create mode 100644 sbt/src/sbt-test/api/unstable-existential-names/src/main/scala/Foo.scala create mode 100644 sbt/src/sbt-test/api/unstable-existential-names/test diff --git a/compile/api/src/main/scala/xsbt/api/SameAPI.scala b/compile/api/src/main/scala/xsbt/api/SameAPI.scala index 5d0d87c2e..2621189b3 100644 --- a/compile/api/src/main/scala/xsbt/api/SameAPI.scala +++ b/compile/api/src/main/scala/xsbt/api/SameAPI.scala @@ -30,7 +30,7 @@ object TopLevel val (avalues, atypes) = definitions(a) val (bvalues, btypes) = definitions(b) - + val (newTypes, removedTypes) = changes(names(atypes), names(btypes)) val (newTerms, removedTerms) = changes(names(avalues), names(bvalues)) @@ -46,10 +46,13 @@ object SameAPI def apply(a: Source, b: Source): Boolean = a.apiHash == b.apiHash && (a.hash.length > 0 && b.hash.length > 0) && apply(a.api, b.api) + def apply(a: Def, b: Def): Boolean = + (new SameAPI(false, true)).sameDefinitions(List(a), List(b), true) + def apply(a: SourceAPI, b: SourceAPI): Boolean = { val start = System.currentTimeMillis - + /*println("\n=========== API #1 ================") import DefaultShowAPI._ println(ShowAPI.show(a)) @@ -219,7 +222,7 @@ class SameAPI(includePrivate: Boolean, includeParamNames: Boolean) argumentMap(a) == argumentMap(b) def argumentMap(a: Seq[AnnotationArgument]): Map[String,String] = Map() ++ a.map(arg => (arg.name, arg.value)) - + def sameDefinitionSpecificAPI(a: Definition, b: Definition): Boolean = (a, b) match { diff --git a/compile/interface/src/main/scala/xsbt/ExtractAPI.scala b/compile/interface/src/main/scala/xsbt/ExtractAPI.scala index 258afe940..acdc89e03 100644 --- a/compile/interface/src/main/scala/xsbt/ExtractAPI.scala +++ b/compile/interface/src/main/scala/xsbt/ExtractAPI.scala @@ -39,6 +39,78 @@ class ExtractAPI[GlobalType <: CallbackGlobal](val global: GlobalType, private[this] val emptyStringArray = new Array[String](0) + /** + * Implements a work-around for https://github.com/sbt/sbt/issues/823 + * + * The strategy is to rename all type variables bound by existential type to stable + * names by assigning to each type variable a De Bruijn-like index. As a result, each + * type variable gets name of this shape: + * + * "existential_${nestingLevel}_${i}" + * + * where `nestingLevel` indicates nesting level of existential types and `i` variable + * indicates position of type variable in given existential type. + * + * For example, let's assume we have the following classes declared: + * + * class A[T]; class B[T,U] + * + * and we have type A[_] that is expanded by Scala compiler into + * + * A[_$1] forSome { type _$1 } + * + * After applying our renaming strategy we get + * + * A[existential_0_0] forSome { type existential_0_0 } + * + * Let's consider a bit more complicated example which shows how our strategy deals with + * nested existential types: + * + * A[_ <: B[_, _]] + * + * which gets expanded into: + * + * A[_$1] forSome { + * type _$1 <: B[_$2, _$3] forSome { type _$2; type _$3 } + * } + * + * After applying our renaming strategy we get + * + * A[existential_0_0] forSome { + * type existential_0_0 <: B[existential_1_0, existential_1_1] forSome { + * type existential_1_0; type existential_1_1 + * } + * } + * + * Note how the first index (nesting level) is bumped for both existential types. + * + * This way, all names of existential type variables depend only on the structure of + * existential types and are kept stable. + * + * Both examples presented above used placeholder syntax for existential types but our + * strategy is applied uniformly to all existential types no matter if they are written + * using placeholder syntax or explicitly. + */ + private[this] object existentialRenamings { + private var nestingLevel: Int = 0 + import scala.collection.mutable.Map + private var renameTo: Map[Symbol, String] = Map.empty + + def leaveExistentialTypeVariables(typeVariables: Seq[Symbol]): Unit = { + nestingLevel -= 1 + assert(nestingLevel >= 0) + typeVariables.foreach(renameTo.remove) + } + def enterExistentialTypeVariables(typeVariables: Seq[Symbol]): Unit = { + nestingLevel += 1 + typeVariables.zipWithIndex foreach { case (tv, i) => + val newName = "existential_" + nestingLevel + "_" + i + renameTo(tv) = newName + } + } + def renaming(symbol: Symbol): Option[String] = renameTo.get(symbol) + } + // call back to the xsbti.SafeLazy class in main sbt code to construct a SafeLazy instance // we pass a thunk, whose class is loaded by the interface class loader (this class's loader) // SafeLazy ensures that once the value is forced, the thunk is nulled out and so @@ -346,13 +418,24 @@ class ExtractAPI[GlobalType <: CallbackGlobal](val global: GlobalType, case SuperType(thistpe: Type, supertpe: Type) => warning("sbt-api: Super type (not implemented): this=" + thistpe + ", super=" + supertpe); Constants.emptyType case at: AnnotatedType => annotatedType(in, at) case rt: CompoundType => structure(rt) - case ExistentialType(tparams, result) => new xsbti.api.Existential(processType(in, result), typeParameters(in, tparams)) + case t: ExistentialType => makeExistentialType(in, t) case NoType => Constants.emptyType // this can happen when there is an error that will be reported by a later phase case PolyType(typeParams, resultType) => new xsbti.api.Polymorphic(processType(in, resultType), typeParameters(in, typeParams)) case Nullary(resultType) => warning("sbt-api: Unexpected nullary method type " + in + " in " + in.owner); Constants.emptyType case _ => warning("sbt-api: Unhandled type " + t.getClass + " : " + t); Constants.emptyType } } + private def makeExistentialType(in: Symbol, t: ExistentialType): xsbti.api.Existential = { + val ExistentialType(typeVariables, qualified) = t + existentialRenamings.enterExistentialTypeVariables(typeVariables) + try { + val typeVariablesConverted = typeParameters(in, typeVariables) + val qualifiedConverted = processType(in, qualified) + new xsbti.api.Existential(qualifiedConverted, typeVariablesConverted) + } finally { + existentialRenamings.leaveExistentialTypeVariables(typeVariables) + } + } private def typeParameters(in: Symbol, s: Symbol): Array[xsbti.api.TypeParameter] = typeParameters(in, s.typeParams) private def typeParameters(in: Symbol, s: List[Symbol]): Array[xsbti.api.TypeParameter] = s.map(typeParameter(in,_)).toArray[xsbti.api.TypeParameter] private def typeParameter(in: Symbol, s: Symbol): xsbti.api.TypeParameter = @@ -368,7 +451,18 @@ class ExtractAPI[GlobalType <: CallbackGlobal](val global: GlobalType, case x => error("Unknown type parameter info: " + x.getClass) } } - private def tparamID(s: Symbol) = s.fullName + private def tparamID(s: Symbol): String = { + val renameTo = existentialRenamings.renaming(s) + renameTo match { + case Some(rename) => + // can't use debuglog because it doesn't exist in Scala 2.9.x + if (settings.debug.value) + log("Renaming existential type variable " + s.fullName + " to " + rename) + rename + case None => + s.fullName + } + } private def selfType(in: Symbol, s: Symbol): xsbti.api.Type = processType(in, s.thisSym.typeOfThis) def classLike(in: Symbol, c: Symbol): ClassLike = classLikeCache.getOrElseUpdate( (in,c), mkClassLike(in, c)) diff --git a/compile/interface/src/test/scala/xsbt/ExtractAPISpecification.scala b/compile/interface/src/test/scala/xsbt/ExtractAPISpecification.scala new file mode 100644 index 000000000..f9af98966 --- /dev/null +++ b/compile/interface/src/test/scala/xsbt/ExtractAPISpecification.scala @@ -0,0 +1,42 @@ +package xsbt + +import org.junit.runner.RunWith +import xsbti.api.ClassLike +import xsbti.api.Def +import xsbt.api.SameAPI +import org.specs2.mutable.Specification +import org.specs2.runner.JUnitRunner + +@RunWith(classOf[JUnitRunner]) +class ExtractAPISpecification extends Specification { + + "Existential types in method signatures" should { + "have stable names" in { stableExistentialNames } + } + + def stableExistentialNames: Boolean = { + def compileAndGetFooMethodApi(src: String): Def = { + val compilerForTesting = new ScalaCompilerForUnitTesting + val sourceApi = compilerForTesting.compileSrc(src) + val FooApi = sourceApi.definitions().find(_.name() == "Foo").get.asInstanceOf[ClassLike] + val fooMethodApi = FooApi.structure().declared().find(_.name == "foo").get + fooMethodApi.asInstanceOf[Def] + } + val src1 = """ + |class Box[T] + |class Foo { + | def foo: Box[_] = null + | + }""".stripMargin + val fooMethodApi1 = compileAndGetFooMethodApi(src1) + val src2 = """ + |class Box[T] + |class Foo { + | def bar: Box[_] = null + | def foo: Box[_] = null + | + }""".stripMargin + val fooMethodApi2 = compileAndGetFooMethodApi(src2) + SameAPI.apply(fooMethodApi1, fooMethodApi2) + } +} diff --git a/compile/interface/src/test/scala/xsbt/ScalaCompilerForUnitTesting.scala b/compile/interface/src/test/scala/xsbt/ScalaCompilerForUnitTesting.scala new file mode 100644 index 000000000..e077647e1 --- /dev/null +++ b/compile/interface/src/test/scala/xsbt/ScalaCompilerForUnitTesting.scala @@ -0,0 +1,71 @@ +package xsbt + +import xsbti.compile.SingleOutput +import java.io.File +import _root_.scala.tools.nsc.reporters.ConsoleReporter +import _root_.scala.tools.nsc.Settings +import xsbti._ +import xsbti.api.SourceAPI +import sbt.IO.withTemporaryDirectory +import xsbti.api.ClassLike +import xsbti.api.Definition +import xsbti.api.Def +import xsbt.api.SameAPI +import sbt.ConsoleLogger + +/** + * Provides common functionality needed for unit tests that require compiling + * source code using Scala compiler. + */ +class ScalaCompilerForUnitTesting { + + /** + * Compiles given source code using Scala compiler and returns API representation + * extracted by ExtractAPI class. + */ + def compileSrc(src: String): SourceAPI = { + import java.io.FileWriter + withTemporaryDirectory { temp => + val analysisCallback = new TestCallback + val classesDir = new File(temp, "classes") + classesDir.mkdir() + val compiler = prepareCompiler(classesDir, analysisCallback) + val run = new compiler.Run + val srcFile = new File(temp, "Test.scala") + srcFile.createNewFile() + val fw = new FileWriter(srcFile) + fw.write(src) + fw.close() + run.compile(List(srcFile.getAbsolutePath())) + analysisCallback.apis(srcFile) + } + } + + private def prepareCompiler(outputDir: File, analysisCallback: AnalysisCallback): CachedCompiler0#Compiler = { + val args = Array.empty[String] + object output extends SingleOutput { + def outputDirectory: File = outputDir + } + val weakLog = new WeakLog(ConsoleLogger(), ConsoleReporter) + val cachedCompiler = new CachedCompiler0(args, output, weakLog, false) + val settings = cachedCompiler.settings + settings.usejavacp.value = true + val scalaReporter = new ConsoleReporter(settings) + val delegatingReporter = DelegatingReporter(settings, ConsoleReporter) + val compiler = cachedCompiler.compiler + compiler.set(analysisCallback, delegatingReporter) + compiler + } + + private object ConsoleReporter extends Reporter { + def reset(): Unit = () + def hasErrors: Boolean = false + def hasWarnings: Boolean = false + def printWarnings(): Unit = () + def problems: Array[Problem] = Array.empty + def log(pos: Position, msg: String, sev: Severity): Unit = println(msg) + def comment(pos: Position, msg: String): Unit = () + def printSummary(): Unit = () + } + +} diff --git a/interface/src/test/scala/xsbti/TestCallback.scala b/interface/src/test/scala/xsbti/TestCallback.scala index 5c0de068e..f8454336a 100644 --- a/interface/src/test/scala/xsbti/TestCallback.scala +++ b/interface/src/test/scala/xsbti/TestCallback.scala @@ -1,19 +1,23 @@ package xsbti - import java.io.File - import scala.collection.mutable.ArrayBuffer +import java.io.File +import scala.collection.mutable.ArrayBuffer +import xsbti.api.SourceAPI class TestCallback extends AnalysisCallback { val sourceDependencies = new ArrayBuffer[(File, File, Boolean)] val binaryDependencies = new ArrayBuffer[(File, String, File, Boolean)] val products = new ArrayBuffer[(File, File, String)] - val apis = new ArrayBuffer[(File, xsbti.api.SourceAPI)] + val apis: scala.collection.mutable.Map[File, SourceAPI] = scala.collection.mutable.Map.empty def sourceDependency(dependsOn: File, source: File, inherited: Boolean) { sourceDependencies += ((dependsOn, source, inherited)) } def binaryDependency(binary: File, name: String, source: File, inherited: Boolean) { binaryDependencies += ((binary, name, source, inherited)) } def generatedClass(source: File, module: File, name: String) { products += ((source, module, name)) } - def api(source: File, sourceAPI: xsbti.api.SourceAPI) { apis += ((source, sourceAPI)) } + def api(source: File, sourceAPI: SourceAPI): Unit = { + assert(!apis.contains(source), s"The `api` method should be called once per source file: $source") + apis(source) = sourceAPI + } def problem(category: String, pos: xsbti.Position, message: String, severity: xsbti.Severity, reported: Boolean) {} } diff --git a/project/Sbt.scala b/project/Sbt.scala index 291d34771..1209df2ce 100644 --- a/project/Sbt.scala +++ b/project/Sbt.scala @@ -96,7 +96,7 @@ object Sbt extends Build // Compiler-side interface to compiler that is compiled against the compiler being used either in advance or on the fly. // Includes API and Analyzer phases that extract source API and relationships. - lazy val compileInterfaceSub = baseProject(compilePath / "interface", "Compiler Interface") dependsOn(interfaceSub, ioSub % "test->test", logSub % "test->test", launchSub % "test->test") settings( compileInterfaceSettings : _*) + lazy val compileInterfaceSub = baseProject(compilePath / "interface", "Compiler Interface") dependsOn(interfaceSub % "compile;test->test", ioSub % "test->test", logSub % "test->test", launchSub % "test->test", apiSub % "test->test") settings( compileInterfaceSettings : _*) lazy val precompiled282 = precompiled("2.8.2") lazy val precompiled292 = precompiled("2.9.2") lazy val precompiled293 = precompiled("2.9.3") @@ -251,12 +251,15 @@ object Sbt extends Build scalacOptions := Nil, ivyScala ~= { _.map(_.copy(checkExplicit = false, overrideScalaVersion = false)) }, exportedProducts in Compile := Nil, - exportedProducts in Test := Nil, libraryDependencies <+= scalaVersion( "org.scala-lang" % "scala-compiler" % _ % "provided") ) // def compileInterfaceSettings: Seq[Setting[_]] = precompiledSettings ++ Seq[Setting[_]]( exportJars := true, + // we need to fork because in unit tests we set usejavacp = true which means + // we are expecting all of our dependencies to be on classpath so Scala compiler + // can use them while constructing its own classpath for compilation + fork in Test := true, artifact in (Compile, packageSrc) := Artifact(srcID).copy(configurations = Compile :: Nil).extra("e:component" -> srcID) ) def compilerSettings = Seq( @@ -268,7 +271,10 @@ object Sbt extends Build scalaVersion <<= (scalaVersion in ThisBuild) { sbtScalaV => assert(sbtScalaV != scalav, "Precompiled compiler interface cannot have the same Scala version (" + scalav + ") as sbt.") scalav - } + }, + // we disable compiling and running tests in precompiled subprojects of compiler interface + // so we do not need to worry about cross-versioning testing dependencies + sources in Test := Nil ) def ioSettings: Seq[Setting[_]] = Seq( libraryDependencies <+= scalaVersion("org.scala-lang" % "scala-compiler" % _ % "test") diff --git a/project/Util.scala b/project/Util.scala index bc75575a7..4ffe70876 100644 --- a/project/Util.scala +++ b/project/Util.scala @@ -51,7 +51,8 @@ object Util def testDependencies = libraryDependencies <++= includeTestDependencies { incl => if(incl) Seq( "org.scalacheck" %% "scalacheck" % "1.10.0" % "test", - "org.specs2" %% "specs2" % "1.12.3" % "test" + "org.specs2" %% "specs2" % "1.12.3" % "test", + "junit" % "junit" % "4.11" % "test" ) else Seq() } diff --git a/sbt/src/sbt-test/api/unstable-existential-names/build.sbt b/sbt/src/sbt-test/api/unstable-existential-names/build.sbt new file mode 100644 index 000000000..8bc82565f --- /dev/null +++ b/sbt/src/sbt-test/api/unstable-existential-names/build.sbt @@ -0,0 +1,10 @@ +// checks number of compilation iterations performed since last `clean` run +InputKey[Unit]("check-number-of-compiler-iterations") <<= inputTask { (argTask: TaskKey[Seq[String]]) => + (argTask, compile in Compile) map { (args: Seq[String], a: sbt.inc.Analysis) => + assert(args.size == 1) + val expectedIterationsNumber = args(0).toInt + val allCompilationsSize = a.compilations.allCompilations.size + assert(allCompilationsSize == expectedIterationsNumber, + "allCompilationsSize == %d (expected %d)".format(allCompilationsSize, expectedIterationsNumber)) + } +} diff --git a/sbt/src/sbt-test/api/unstable-existential-names/changes/Foo1.scala b/sbt/src/sbt-test/api/unstable-existential-names/changes/Foo1.scala new file mode 100644 index 000000000..dd6a3c6da --- /dev/null +++ b/sbt/src/sbt-test/api/unstable-existential-names/changes/Foo1.scala @@ -0,0 +1,13 @@ +package test + +class Box[T] + +class Foo { + /** + * This method shouldn't affect public API of Foo + * but due to instability of synthesized names for + * existentials causes change of `foo` method API. + */ + private def abc: Box[_] = null + def foo: Box[_] = null +} diff --git a/sbt/src/sbt-test/api/unstable-existential-names/src/main/scala/Bar.scala b/sbt/src/sbt-test/api/unstable-existential-names/src/main/scala/Bar.scala new file mode 100644 index 000000000..104483e49 --- /dev/null +++ b/sbt/src/sbt-test/api/unstable-existential-names/src/main/scala/Bar.scala @@ -0,0 +1,5 @@ +package test + +class Bar { + def f: Foo = null //we introduce dependency on Foo +} diff --git a/sbt/src/sbt-test/api/unstable-existential-names/src/main/scala/Foo.scala b/sbt/src/sbt-test/api/unstable-existential-names/src/main/scala/Foo.scala new file mode 100644 index 000000000..cbf10143d --- /dev/null +++ b/sbt/src/sbt-test/api/unstable-existential-names/src/main/scala/Foo.scala @@ -0,0 +1,7 @@ +package test + +class Box[T] + +class Foo { + def foo: Box[_] = null +} diff --git a/sbt/src/sbt-test/api/unstable-existential-names/test b/sbt/src/sbt-test/api/unstable-existential-names/test new file mode 100644 index 000000000..01486aa30 --- /dev/null +++ b/sbt/src/sbt-test/api/unstable-existential-names/test @@ -0,0 +1,12 @@ +# Tests if existential types are pickled correctly so they +# do not introduce unnecessary compile iterations + +# introduces first compile iteration +> compile +# this change is local to a method and does not change the api so introduces +# only one additional compile iteration +$ copy-file changes/Foo1.scala src/main/scala/Foo.scala +# second iteration +> compile +# check if there are only two compile iterations being performed +> check-number-of-compiler-iterations 2