Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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.
Also, a integration test has been added which tests scenario
mentioned in sbt#823.
This commit fixes sbt#823.
Proper fix for unstable existential names.- Loading branch information
1 parent
e3e95f9
commit ede0cc3
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,41 @@ 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. | ||
| * | ||
| * This way, all names of existential type variables depend only on the structure of | ||
This comment has been minimized.
Sorry, something went wrong.
This comment has been minimized.
Sorry, something went wrong. |
||
| * existential types and are kept stable. | ||
| */ | ||
| 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 +381,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 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 | ||
| try { | ||
| existentialRenamings.enterExistentialTypeVariables(typeVariables) | ||
This comment has been minimized.
Sorry, something went wrong. 
retronym
|
||
| 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 +414,18 @@ class ExtractAPI[GlobalType <: CallbackGlobal](val global: GlobalType, | |
| case x => error("Unknown type parameter info: " + x.getClass) | ||
| } | ||
| } | ||
| 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)) | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| 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 | ||
|
|
||
| /** | ||
| * 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 RecordingAnalysisCallback | ||
| 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 ConsoleLogger extends Logger { | ||
This comment has been minimized.
Sorry, something went wrong. |
||
| def debug(msg: F0[String]): Unit = () | ||
| def warn(msg: F0[String]): Unit = () | ||
| def info(msg: F0[String]): Unit = () | ||
| def error(msg: F0[String]): Unit = println(msg.apply()) | ||
| def trace(msg: F0[Throwable]) = () | ||
| } | ||
|
|
||
| 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 = () | ||
| } | ||
|
|
||
| private class RecordingAnalysisCallback extends AnalysisCallback { | ||
This comment has been minimized.
Sorry, something went wrong.
This comment has been minimized.
Sorry, something went wrong. |
||
| val apis: scala.collection.mutable.Map[File, SourceAPI] = scala.collection.mutable.Map.empty | ||
| def beginSource(source: File): Unit = () | ||
| def sourceDependency(dependsOn: File, source: File, publicInherited: Boolean): Unit = () | ||
| def binaryDependency(binary: File, name: String, source: File, publicInherited: Boolean): Unit = () | ||
| def generatedClass(source: File, module: File, name: String): Unit = () | ||
| def endSource(sourcePath: File): Unit = () | ||
| def api(sourceFile: File, source: xsbti.api.SourceAPI): Unit = { | ||
| apis(sourceFile) = source | ||
| } | ||
| def problem(what: String, pos: Position, msg: String, severity: Severity, reported: Boolean): Unit = () | ||
| } | ||
|
|
||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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)) | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| package test | ||
|
|
||
| class Bar { | ||
| def f: Foo = null //we introduce dependency on Foo | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| package test | ||
|
|
||
| class Box[T] | ||
|
|
||
| class Foo { | ||
| def foo: Box[_] = null | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
1 comment
on commit ede0cc3
There was a problem hiding this comment.
Looks good besides the two minor comments! Nice work.
I feel this would benefit from one or two examples.