From 044e71b8a85b37dac6b23a43edb33f2e65563b9d Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Thu, 6 Apr 2017 14:30:03 +0200 Subject: [PATCH] Fix #97: Avoid spurious recompilations when unrelated constructor changes The name hashing algorithm is designed to take implicit conversions into account: when a name "foo" is changed somewhere in a dependency of X, you have to recompile X if it uses the name "foo", even if the usage of "foo" in X is completely unrelated, just because this might have an effect on available implicit conversions. However, there is one case where we can be sure that implicit conversions will not kick in: when we call a constructor. A constructor name is always "", this PR now replaces this name by "pkgA;pkgB;className;init;", this mean that we no longer recompile classes when an unrelated constructor in a used class changed (see the new test `constructors-unrelated` for an example). --- .../src/main/scala/xsbt/ClassName.scala | 31 +++++++++++++++++++ .../src/main/scala/xsbt/ExtractAPI.scala | 8 +++-- .../main/scala/xsbt/ExtractUsedNames.scala | 10 +++--- ...actUsedNamesPerformanceSpecification.scala | 14 ++++----- .../xsbt/ExtractUsedNamesSpecification.scala | 8 ++--- .../scala/sbt/internal/inc/ClassToAPI.scala | 9 ++++-- .../constructors-unrelated/A.scala | 2 ++ .../constructors-unrelated/B.scala | 1 + .../constructors-unrelated/changes/A2.scala | 2 ++ .../constructors-unrelated/test | 4 +++ .../value-class-underlying/C.scala | 1 + 11 files changed, 67 insertions(+), 23 deletions(-) create mode 100644 zinc/src/sbt-test/source-dependencies/constructors-unrelated/A.scala create mode 100644 zinc/src/sbt-test/source-dependencies/constructors-unrelated/B.scala create mode 100644 zinc/src/sbt-test/source-dependencies/constructors-unrelated/changes/A2.scala create mode 100644 zinc/src/sbt-test/source-dependencies/constructors-unrelated/test diff --git a/internal/compiler-bridge/src/main/scala/xsbt/ClassName.scala b/internal/compiler-bridge/src/main/scala/xsbt/ClassName.scala index 1bc590e122..32c13a89a2 100644 --- a/internal/compiler-bridge/src/main/scala/xsbt/ClassName.scala +++ b/internal/compiler-bridge/src/main/scala/xsbt/ClassName.scala @@ -32,6 +32,37 @@ trait ClassName extends Compat { */ protected def classNameAsString(s: Symbol): String = pickledNameAsString(s) + /** + * Given a class symbol `cls`, construct a name representing this constructor. + * For a class: + * + * a.b.Foo + * + * this is: + * + * a;b;Foo;init; + * + * The prefix is important to avoid name hashing all constructors together + * (see #97), the weird format is necessary to avoid scalac or zinc trying to + * interpret this name (in particular we should not use '.' and we should not + * use ''), we use ';' because it is one of the few characters that + * cannot appear in a valid JVM name. + */ + protected def constructorName(cls: Symbol): Name = + newTermName(constructorNameAsString(cls)) + + protected def constructorNameAsString(cls: Symbol): String = + cls.fullName(';') ++ ";init;" + + /** + * Mangle a JVM symbol name in a format better suited for internal uses by sbt. + */ + protected def mangledName(s: Symbol): Name = + if (s.name == nme.CONSTRUCTOR) + constructorName(s.enclClass) + else + s.name + /** * Create a (source) name for the class symbol `s` with a prefix determined by the class symbol `in`. * diff --git a/internal/compiler-bridge/src/main/scala/xsbt/ExtractAPI.scala b/internal/compiler-bridge/src/main/scala/xsbt/ExtractAPI.scala index f0f3657c95..8c70b8d8e5 100644 --- a/internal/compiler-bridge/src/main/scala/xsbt/ExtractAPI.scala +++ b/internal/compiler-bridge/src/main/scala/xsbt/ExtractAPI.scala @@ -613,8 +613,12 @@ class ExtractAPI[GlobalType <: Global]( private def simpleName(s: Symbol): String = { val n = s.unexpandedName - val n2 = if (n.toString == "") n else n.decode - n2.toString.trim + val n2 = + if (n == nme.CONSTRUCTOR) + constructorNameAsString(s.enclClass) + else + n.decode.toString + n2.trim } private def staticAnnotations(annotations: List[AnnotationInfo]): List[AnnotationInfo] = { diff --git a/internal/compiler-bridge/src/main/scala/xsbt/ExtractUsedNames.scala b/internal/compiler-bridge/src/main/scala/xsbt/ExtractUsedNames.scala index 497db23946..bcff44e1e7 100644 --- a/internal/compiler-bridge/src/main/scala/xsbt/ExtractUsedNames.scala +++ b/internal/compiler-bridge/src/main/scala/xsbt/ExtractUsedNames.scala @@ -170,11 +170,9 @@ class ExtractUsedNames[GlobalType <: CallbackGlobal](val global: GlobalType) ext val addSymbol: (JavaSet[Name], Symbol) => Unit = { (names: JavaSet[Name], symbol: Symbol) => - if (!ignoredSymbol(symbol)) { - val name = symbol.name - // Synthetic names are no longer included. See https://github.com/sbt/sbt/issues/2537 - if (!isEmptyName(name)) - names.add(name) + // Synthetic names are no longer included. See https://github.com/sbt/sbt/issues/2537 + if (!ignoredSymbol(symbol) && !isEmptyName(symbol.name)) { + names.add(mangledName(symbol)) () } } @@ -209,7 +207,7 @@ class ExtractUsedNames[GlobalType <: CallbackGlobal](val global: GlobalType) ext private object PatMatDependencyTraverser extends TypeDependencyTraverser { override def addDependency(symbol: global.Symbol): Unit = { if (!ignoredSymbol(symbol) && symbol.isSealed) { - val name = symbol.name + val name = mangledName(symbol) if (!isEmptyName(name)) { val existingScopes = _currentScopedNamesCache.get(name) if (existingScopes == null) diff --git a/internal/compiler-bridge/src/test/scala/xsbt/ExtractUsedNamesPerformanceSpecification.scala b/internal/compiler-bridge/src/test/scala/xsbt/ExtractUsedNamesPerformanceSpecification.scala index 9e2497215d..2739c3f611 100644 --- a/internal/compiler-bridge/src/test/scala/xsbt/ExtractUsedNamesPerformanceSpecification.scala +++ b/internal/compiler-bridge/src/test/scala/xsbt/ExtractUsedNamesPerformanceSpecification.scala @@ -41,12 +41,12 @@ class ExtractUsedNamesPerformanceSpecification extends UnitSpec { val compilerForTesting = new ScalaCompilerForUnitTesting compilerForTesting.extractUsedNamesFromSrc(src) } - val expectedNamesForTupler = Set("", "Object", "scala", "tupler", "TuplerInstances", "DepFn1", "HNil", "$anon", "Out", "Out0", "Tupler", "hnilTupler", "acme", "L", "Aux", "HList", "Serializable", "Unit") - val expectedNamesForTuplerInstances = Set("E", "Tuple4", "e", "case7", "Tuple15", "s", "case19", "T7", "x", "TuplerInstances", "matchEnd19", "T20", "Tuple11", "HNil", "matchEnd6", "p16", "$anon", "T19", "p20", "T2", "p10", "case22", "p19", "n", "Tuple12", "case11", "Tuple22", "p12", "matchEnd7", "N", "p4", "T13", "case26", "Tuple19", "p7", "p5", "j", "Out", "T", "p23", "case15", "matchEnd20", "t", "p21", "matchEnd15", "J", "head", "case13", "u", "matchEnd18", "U", "Tupler", "f", "T8", "T16", "F", "Tuple3", "case8", "case18", "case24", "Boolean", "matchEnd21", "A", "matchEnd26", "a", "Tuple14", "T1", "::", "Nothing", "p18", "case20", "m", "matchEnd10", "M", "matchEnd25", "tail", "Tuple2", "matchEnd5", "p15", "matchEnd23", "I", "i", "matchEnd14", "AnyRef", "Tuple8", "matchEnd8", "case25", "T12", "p3", "case14", "case23", "T5", "matchEnd22", "T17", "v", "p22", "Tuple18", "G", "Tuple13", "matchEnd12", "", "V", "q", "p11", "Q", "case12", "L", "b", "apply", "Object", "g", "B", "l", "==", "Out0", "Tuple1", "matchEnd9", "P", "p2", "T15", "Aux", "matchEnd24", "p", "scala", "matchEnd11", "Tuple20", "HList", "case17", "T9", "p14", "Tuple7", "matchEnd17", "T4", "case28", "T22", "p17", "C", "Tuple6", "MatchError", "T11", "x1", "H", "case16", "matchEnd13", "c", "Tuple9", "h", "T6", "T18", "r", "K", "Tuple17", "p9", "R", "ne", "T14", "case21", "k", "case10", "Tuple21", "O", "case9", "Tuple10", "Any", "T10", "case27", "Tuple5", "D", "p13", "o", "p6", "p8", "matchEnd16", "S", "T21", "Tuple16", "d", "T3") + val expectedNamesForTupler = Set("java;lang;Object;init;", "Object", "acme;Tupler;$anon;init;", "scala", "tupler", "TuplerInstances", "DepFn1", "HNil", "$anon", "Out", "Out0", "Tupler", "hnilTupler", "acme", "L", "Aux", "HList", "Serializable", "Unit") + val expectedNamesForTuplerInstances = Set("E", "Tuple4", "e", "case7", "Tuple15", "s", "case19", "T7", "x", "acme;TuplerInstances;$anon;init;", "TuplerInstances", "matchEnd19", "T20", "Tuple11", "HNil", "matchEnd6", "p16", "$anon", "T19", "p20", "T2", "p10", "case22", "p19", "n", "Tuple12", "case11", "Tuple22", "p12", "matchEnd7", "N", "p4", "T13", "case26", "Tuple19", "p7", "p5", "j", "Out", "T", "p23", "case15", "matchEnd20", "t", "p21", "matchEnd15", "J", "head", "case13", "u", "matchEnd18", "U", "Tupler", "f", "T8", "T16", "F", "Tuple3", "case8", "case18", "case24", "Boolean", "matchEnd21", "A", "matchEnd26", "a", "Tuple14", "T1", "::", "Nothing", "p18", "case20", "m", "matchEnd10", "M", "matchEnd25", "tail", "Tuple2", "matchEnd5", "p15", "matchEnd23", "I", "i", "matchEnd14", "AnyRef", "Tuple8", "matchEnd8", "case25", "T12", "p3", "case14", "case23", "T5", "matchEnd22", "T17", "v", "p22", "Tuple18", "G", "Tuple13", "matchEnd12", "java;lang;Object;init;", "V", "q", "p11", "Q", "case12", "L", "b", "apply", "Object", "g", "B", "l", "==", "Out0", "Tuple1", "matchEnd9", "P", "p2", "T15", "Aux", "matchEnd24", "p", "scala", "matchEnd11", "Tuple20", "HList", "case17", "T9", "p14", "Tuple7", "matchEnd17", "T4", "case28", "T22", "p17", "C", "Tuple6", "MatchError", "scala;MatchError;init;", "T11", "x1", "H", "case16", "matchEnd13", "c", "Tuple9", "h", "T6", "T18", "r", "K", "Tuple17", "p9", "R", "ne", "T14", "case21", "k", "case10", "Tuple21", "O", "case9", "Tuple10", "Any", "T10", "case27", "Tuple5", "D", "p13", "o", "p6", "p8", "matchEnd16", "S", "T21", "Tuple16", "d", "T3") val expectedNamesForRefinement = Set("Out0") - val `expectedNamesFor::` = Set("x", "T2", "ScalaRunTime", "Iterator", "T", "head", "asInstanceOf", "Boolean", "A", "$" + "isInstanceOf", "T1", "||", "::", "Nothing", "x$1", "any2stringadd", "acme", "typedProductIterator", "tail", "Tuple2", "AnyRef", "isInstanceOf", "Int", "", "_hashCode", "apply", "Object", "x$0", "==", "Some", "IndexOutOfBoundsException", "T0", "Predef", "scala", "matchEnd4", "HList", "None", "x1", "toString", "H", "+", "&&", "Serializable", "Product", "case6", "::$1", "eq", "Any", "runtime", "String") + val `expectedNamesFor::` = Set("x", "T2", "ScalaRunTime", "Iterator", "T", "head", "asInstanceOf", "Boolean", "A", "$" + "isInstanceOf", "T1", "||", "acme;::;init;", "::", "Nothing", "x$1", "any2stringadd", "acme", "typedProductIterator", "tail", "Tuple2", "AnyRef", "isInstanceOf", "Int", "java;lang;Object;init;", "_hashCode", "apply", "Object", "x$0", "==", "Some", "IndexOutOfBoundsException", "java;lang;IndexOutOfBoundsException;init;", "T0", "Predef", "scala", "matchEnd4", "HList", "None", "x1", "toString", "H", "+", "&&", "Serializable", "Product", "case6", "::$1", "eq", "Any", "runtime", "String") val expectedNamesForDepFn1 = Set("DepFn1", "Out", "T", "AnyRef", "Object", "scala") - val expectedNamesForHNil = Set("x", "HNil", "ScalaRunTime", "Iterator", "Boolean", "A", "T", "$" + "isInstanceOf", "::", "Nothing", "x$1", "acme", "typedProductIterator", "Int", "", "apply", "Object", "IndexOutOfBoundsException", "scala", "HList", "toString", "H", "Serializable", "h", "Product", "Any", "runtime", "matchEnd3", "String", "T0") + val expectedNamesForHNil = Set("x", "HNil", "ScalaRunTime", "Iterator", "Boolean", "A", "T", "$" + "isInstanceOf", "::", "Nothing", "x$1", "acme", "typedProductIterator", "Int", "java;lang;Object;init;", "apply", "Object", "IndexOutOfBoundsException", "java;lang;IndexOutOfBoundsException;init;", "scala", "HList", "toString", "H", "Serializable", "h", "Product", "Any", "runtime", "matchEnd3", "String", "T0") val expectedNamesForHList = Set("Tupler", "acme", "scala", "Serializable", "Product") assert(usedNames("acme.Tupler") -- scalaDiff === expectedNamesForTupler -- scalaDiff) assert(usedNames("acme.TuplerInstances") -- scalaDiff === expectedNamesForTuplerInstances -- scalaDiff) @@ -96,9 +96,9 @@ class ExtractUsedNamesPerformanceSpecification extends UnitSpec { val (_, analysis) = compilerForTesting.compileSrcs(List(List(ext), List(cod)), reuseCompilerInstance = true) val usedNames = analysis.usedNames.toMap - val expectedNamesForFoo = Set("TypeApplyExtractor", "mkIdent", "package", "", "tpe", "in", "$u", "internal", "reify", "WeakTypeTag", "Name", "empty", "collection", "ThisType", "staticModule", "staticPackage", "Singleton", "T", "asInstanceOf", "ReificationSupportApi", "U", "Expr", "Universe", "TypeApply", "A", "Tree", "Nothing", "acme", "ClassSymbol", "blackbox", "AnyRef", "Context", "mkTypeTree", "immutable", "SelectExtractor", "", "$treecreator1", "apply", "Object", "macros", "moduleClass", "Foo", "T0", "Symbol", "Predef", "scala", "asModule", "Internal", "$m", "TypeCreator", "TermNameExtractor", "ModuleSymbol", "staticClass", "universe", "c", "", "TypeTree", "List", "Select", "TermName", "Mirror", "atag", "reificationSupport", "rootMirror", "reflect", "TypeRef", "Ident", "Any", "TreeCreator", "$typecreator2", "$m$untyped", "String", "Type") - val expectedNamesForBar = Set("experimental", "package", "WeakTypeTag", "Out", "foo_impl", "Expr", "A", "Nothing", "acme", "AnyRef", "Context", "", "language", "Object", "macros", "Bar", "Foo", "scala", "List", "Any") + val expectedNamesForFoo = Set("TypeApplyExtractor", "mkIdent", "package", "", "tpe", "in", "$u", "internal", "reify", "WeakTypeTag", "Name", "empty", "collection", "ThisType", "staticModule", "staticPackage", "Singleton", "T", "asInstanceOf", "ReificationSupportApi", "U", "Expr", "Universe", "TypeApply", "A", "Tree", "Nothing", "acme", "ClassSymbol", "blackbox", "AnyRef", "Context", "mkTypeTree", "immutable", "SelectExtractor", "java.lang.Object.init;", "$treecreator1", "apply", "Object", "macros", "moduleClass", "Foo", "T0", "Symbol", "Predef", "scala", "asModule", "Internal", "$m", "TypeCreator", "TermNameExtractor", "ModuleSymbol", "staticClass", "universe", "c", "", "TypeTree", "List", "Select", "TermName", "Mirror", "atag", "reificationSupport", "rootMirror", "reflect", "TypeRef", "Ident", "Any", "TreeCreator", "$typecreator2", "$m$untyped", "String", "Type") + val expectedNamesForBar = Set("experimental", "package", "WeakTypeTag", "Out", "foo_impl", "Expr", "A", "Nothing", "acme", "AnyRef", "Context", "java;lang;Object;init;", "language", "Object", "macros", "Bar", "Foo", "scala", "List", "Any") assert(usedNames("acme.Foo") === expectedNamesForFoo) assert(usedNames("acme.Bar") === expectedNamesForBar) } -} \ No newline at end of file +} diff --git a/internal/compiler-bridge/src/test/scala/xsbt/ExtractUsedNamesSpecification.scala b/internal/compiler-bridge/src/test/scala/xsbt/ExtractUsedNamesSpecification.scala index 5eff3182f8..d791dd0566 100644 --- a/internal/compiler-bridge/src/test/scala/xsbt/ExtractUsedNamesSpecification.scala +++ b/internal/compiler-bridge/src/test/scala/xsbt/ExtractUsedNamesSpecification.scala @@ -78,8 +78,8 @@ class ExtractUsedNamesSpecification extends UnitSpec { if (scalaVersion.contains("2.10")) Set("Nothing", "Any") else Set() val namesA = standardNames ++ Set("A") ++ versionDependentNames val namesAX = standardNames ++ Set("X", "x", "T", "A") - val namesB = Set("B", "A", "Int", "", "scala") - val namesC = Set("", "C", "B") + val namesB = Set("B", "A", "Int", "A;init;", "scala") + val namesC = Set("B;init;", "C", "B") val namesD = standardNames ++ Set("D", "C", "X", "foo", "Int", "T") assert(usedNames("A") === namesA) assert(usedNames("A.X") === namesAX) @@ -270,9 +270,7 @@ class ExtractUsedNamesSpecification extends UnitSpec { private val standardNames = Set( "scala", // The default parent of a class is "AnyRef" which is an alias for "Object" - "AnyRef", "Object", - // class receives a default constructor which is internally called "" - "" + "AnyRef", "Object", "java;lang;Object;init;" ) } diff --git a/internal/zinc-apiinfo/src/main/scala/sbt/internal/inc/ClassToAPI.scala b/internal/zinc-apiinfo/src/main/scala/sbt/internal/inc/ClassToAPI.scala index 4154b48be2..385ec591fc 100644 --- a/internal/zinc-apiinfo/src/main/scala/sbt/internal/inc/ClassToAPI.scala +++ b/internal/zinc-apiinfo/src/main/scala/sbt/internal/inc/ClassToAPI.scala @@ -102,7 +102,7 @@ object ClassToAPI { lazy val cf = classFileForClass(c) val methods = mergeMap(c, c.getDeclaredMethods, c.getMethods, methodToDef(enclPkg)) val fields = mergeMap(c, c.getDeclaredFields, c.getFields, fieldToDef(c, cf, enclPkg)) - val constructors = mergeMap(c, c.getDeclaredConstructors, c.getConstructors, constructorToDef(enclPkg)) + val constructors = mergeMap(c, c.getDeclaredConstructors, c.getConstructors, constructorToDef(enclPkg, c)) val classes = merge[Class[_]](c, c.getDeclaredClasses, c.getClasses, toDefinitions(cmap), (_: Seq[Class[_]]).partition(isStatic), _.getEnclosingClass != c) val all = methods ++ fields ++ constructors ++ classes val parentJavaTypes = allSuperTypes(c) @@ -218,8 +218,11 @@ object ClassToAPI { def methodToDef(enclPkg: Option[String])(m: Method): api.Def = defLike(m.getName, m.getModifiers, m.getDeclaredAnnotations, typeParameterTypes(m), m.getParameterAnnotations, parameterTypes(m), Option(returnType(m)), exceptionTypes(m), m.isVarArgs, enclPkg) - def constructorToDef(enclPkg: Option[String])(c: Constructor[_]): api.Def = - defLike("", c.getModifiers, c.getDeclaredAnnotations, typeParameterTypes(c), c.getParameterAnnotations, parameterTypes(c), None, exceptionTypes(c), c.isVarArgs, enclPkg) + def constructorToDef(enclPkg: Option[String], enclClass: Class[_])(c: Constructor[_]): api.Def = + defLike( + s"${name(c).replace('.', ';')};init;", // Use the format defined in xsbt.ClassName.constructorName documentation + c.getModifiers, c.getDeclaredAnnotations, typeParameterTypes(c), c.getParameterAnnotations, parameterTypes(c), None, exceptionTypes(c), c.isVarArgs, enclPkg + ) def defLike[T <: GenericDeclaration](name: String, mods: Int, annots: Array[Annotation], tps: Array[TypeVariable[T]], paramAnnots: Array[Array[Annotation]], paramTypes: Array[Type], retType: Option[Type], exceptions: Array[Type], varArgs: Boolean, enclPkg: Option[String]): api.Def = { diff --git a/zinc/src/sbt-test/source-dependencies/constructors-unrelated/A.scala b/zinc/src/sbt-test/source-dependencies/constructors-unrelated/A.scala new file mode 100644 index 0000000000..b8c61e25e1 --- /dev/null +++ b/zinc/src/sbt-test/source-dependencies/constructors-unrelated/A.scala @@ -0,0 +1,2 @@ +class A(a: Int) +object A { val x = 3 } diff --git a/zinc/src/sbt-test/source-dependencies/constructors-unrelated/B.scala b/zinc/src/sbt-test/source-dependencies/constructors-unrelated/B.scala new file mode 100644 index 0000000000..5578bb7a6d --- /dev/null +++ b/zinc/src/sbt-test/source-dependencies/constructors-unrelated/B.scala @@ -0,0 +1 @@ +class B { val y = A.x } diff --git a/zinc/src/sbt-test/source-dependencies/constructors-unrelated/changes/A2.scala b/zinc/src/sbt-test/source-dependencies/constructors-unrelated/changes/A2.scala new file mode 100644 index 0000000000..da14d8945d --- /dev/null +++ b/zinc/src/sbt-test/source-dependencies/constructors-unrelated/changes/A2.scala @@ -0,0 +1,2 @@ +class A(a: String) +object A { val x = 3 } diff --git a/zinc/src/sbt-test/source-dependencies/constructors-unrelated/test b/zinc/src/sbt-test/source-dependencies/constructors-unrelated/test new file mode 100644 index 0000000000..5c0f0453c6 --- /dev/null +++ b/zinc/src/sbt-test/source-dependencies/constructors-unrelated/test @@ -0,0 +1,4 @@ +> compile # First compilation round +$ copy-file changes/A2.scala A.scala +> compile # Second compilation round, there should be no third round (we don't need to recompile B.scala) +> checkIterations 2 # Check that there were only two rounds of compilation diff --git a/zinc/src/sbt-test/source-dependencies/value-class-underlying/C.scala b/zinc/src/sbt-test/source-dependencies/value-class-underlying/C.scala index 1a9a42bde9..0c09ea764f 100644 --- a/zinc/src/sbt-test/source-dependencies/value-class-underlying/C.scala +++ b/zinc/src/sbt-test/source-dependencies/value-class-underlying/C.scala @@ -1,5 +1,6 @@ object C { def main(args: Array[String]): Unit = { val x = B.foo + println("x: " + x) // Need to use x in an expression to see if it crashes or not } }