From 40ebc825317ed29a2ab87be07246fcdcfc6d1e3d Mon Sep 17 00:00:00 2001 From: Martin Duhem Date: Mon, 17 Aug 2015 11:59:48 +0200 Subject: [PATCH 1/2] Include private members in API hash of traits. During compilation, scalac generates getters and setters for the fields of traits, regardless of their access modifiers. Therefore, when a field of a trait is modified, all its implementors must be recompiled to take these changes into account. Private fields of traits were not included in the API hash of traits, and their implementors were thus not recompiled when modified. This commit changes the way the API hash is computed for traits only, so that the generated hash includes the private members of traits. Fixes sbt/sbt#2155 --- .../api/src/main/scala/xsbt/api/HashAPI.scala | 16 +-- .../xsbt/api/NameHashingSpecification.scala | 114 ++++++++++++++++++ .../trait-private-var/{pending => test} | 0 3 files changed, 123 insertions(+), 7 deletions(-) rename sbt/src/sbt-test/source-dependencies/trait-private-var/{pending => test} (100%) diff --git a/compile/api/src/main/scala/xsbt/api/HashAPI.scala b/compile/api/src/main/scala/xsbt/api/HashAPI.scala index 89452f237..56485373b 100644 --- a/compile/api/src/main/scala/xsbt/api/HashAPI.scala +++ b/compile/api/src/main/scala/xsbt/api/HashAPI.scala @@ -60,6 +60,8 @@ final class HashAPI(includePrivate: Boolean, includeParamNames: Boolean, include } } + private[this] def isTrait(cl: ClassLike) = cl.definitionType == DefinitionType.Trait + private[this] final val ValHash = 1 private[this] final val VarHash = 2 private[this] final val DefHash = 3 @@ -184,7 +186,7 @@ final class HashAPI(includePrivate: Boolean, includeParamNames: Boolean, include extend(ClassHash) hashParameterizedDefinition(c) hashType(c.selfType) - hashStructure(c.structure, includeDefinitions) + hashStructure(c.structure, includeDefinitions, isTrait(c)) } def hashField(f: FieldLike): Unit = { f match { @@ -343,14 +345,14 @@ final class HashAPI(includePrivate: Boolean, includeParamNames: Boolean, include hashType(a.baseType) hashAnnotations(a.annotations) } - final def hashStructure(structure: Structure, includeDefinitions: Boolean) = - visit(visitedStructures, structure)(structure => hashStructure0(structure, includeDefinitions)) - def hashStructure0(structure: Structure, includeDefinitions: Boolean): Unit = { + final def hashStructure(structure: Structure, includeDefinitions: Boolean, isTrait: Boolean = false) = + visit(visitedStructures, structure)(structure => hashStructure0(structure, includeDefinitions, isTrait)) + def hashStructure0(structure: Structure, includeDefinitions: Boolean, isTrait: Boolean = false): Unit = { extend(StructureHash) hashTypes(structure.parents, includeDefinitions) - if (includeDefinitions) { - hashDefinitions(structure.declared, false) - hashDefinitions(structure.inherited, false) + if (includeDefinitions || isTrait) { + hashDefinitions(structure.declared, isTrait) + hashDefinitions(structure.inherited, isTrait) } } def hashParameters(parameters: Seq[TypeParameter], base: Type): Unit = diff --git a/compile/api/src/test/scala/xsbt/api/NameHashingSpecification.scala b/compile/api/src/test/scala/xsbt/api/NameHashingSpecification.scala index cb6a54ef7..6014a1e52 100644 --- a/compile/api/src/test/scala/xsbt/api/NameHashingSpecification.scala +++ b/compile/api/src/test/scala/xsbt/api/NameHashingSpecification.scala @@ -164,6 +164,115 @@ class NameHashingSpecification extends Specification { assertNameHashNotEqualForRegularName("bar", nameHashes1, nameHashes2) } + /** + * Checks that private members are included in the hash of the public API of traits. + * Including the private members of traits is required because classes that implement a trait + * have to define the private members of the trait. Therefore, if a private member of a trait is added, + * modified or removed we need to recompile the classes that implement this trait. + * For instance, if trait Foo is initially defined as: + * trait Foo { private val x = new A } + * changing it to + * trait Foo { private val x = new B } + * requires us to recompile all implementors of trait Foo, because scalac generates setters and getters + * for the private fields of trait Foo in its implementor. If the clients of trait Foo are not recompiled, + * we get abstract method errors at runtime, because the types expected by the setter (for instance) does not + * match. + */ + "private members in traits" in { + /* trait Foo { private val x } */ + val fooTrait1 = + simpleTrait("Foo", + simpleStructure(new Val(emptyType, "x", privateAccess, defaultModifiers, Array.empty)), + publicAccess) + + /* trait Foo */ + val fooTrait2 = + simpleTrait("Foo", + simpleStructure(), + publicAccess) + + val api1 = new SourceAPI(Array.empty, Array(fooTrait1)) + val api2 = new SourceAPI(Array.empty, Array(fooTrait2)) + + HashAPI(api1) !== HashAPI(api2) + + } + + /** + * Checks that private members in non-top-level traits are included as well. + */ + "private members in nested traits" in { + /* class A { trait Foo { private val x } } */ + val classA1 = + simpleClass("A", + simpleTrait("Foo", + simpleStructure(new Val(emptyType, "x", privateAccess, defaultModifiers, Array.empty)), + publicAccess)) + + /* class A { trait Foo } */ + val classA2 = + simpleClass("A", + simpleTrait("Foo", + simpleStructure(), + publicAccess)) + + val api1 = new SourceAPI(Array.empty, Array(classA1)) + val api2 = new SourceAPI(Array.empty, Array(classA2)) + + HashAPI(api1) !== HashAPI(api2) + + } + + /** + * Checks that private traits are NOT included in the hash. + */ + "private traits" in { + /* class Foo { private trait T { private val x } } */ + val classFoo1 = + simpleClass("Foo", + simpleTrait("T", + simpleStructure(new Val(emptyType, "x", privateAccess, defaultModifiers, Array.empty)), + privateAccess)) + + /** class Foo { private trait T } */ + val classFoo2 = + simpleClass("Foo", + simpleTrait("T", + simpleStructure(), + privateAccess)) + + /** class Foo */ + val classFoo3 = + simpleClass("Foo") + + val api1 = new SourceAPI(Array.empty, Array(classFoo1)) + val api2 = new SourceAPI(Array.empty, Array(classFoo2)) + val api3 = new SourceAPI(Array.empty, Array(classFoo3)) + + HashAPI(api1) === HashAPI(api2) && HashAPI(api2) === HashAPI(api3) + } + + /** + * Checks that private members are NOT included in the hash of the public API of classes. + */ + "private members in classes" in { + /* class Foo { private val x } */ + val classFoo1 = + simpleClass("Foo", + simpleStructure(new Val(emptyType, "x", privateAccess, defaultModifiers, Array.empty))) + + /* class Foo */ + val classFoo2 = + simpleClass("Foo", + simpleStructure()) + + val api1 = new SourceAPI(Array.empty, Array(classFoo1)) + val api2 = new SourceAPI(Array.empty, Array(classFoo2)) + + HashAPI(api1) === HashAPI(api2) + + } + private def assertNameHashEqualForRegularName(name: String, nameHashes1: _internalOnly_NameHashes, nameHashes2: _internalOnly_NameHashes) = { val nameHash1 = nameHashForRegularName(nameHashes1, name) @@ -204,10 +313,15 @@ class NameHashingSpecification extends Specification { new ClassLike(DefinitionType.ClassDef, lzy(emptyType), lzy(structure), Array.empty, Array.empty, name, publicAccess, defaultModifiers, Array.empty) } + private def simpleTrait(name: String, structure: Structure, access: Access): ClassLike = { + new ClassLike(DefinitionType.Trait, lzy(emptyType), lzy(structure), Array.empty, Array.empty, name, access, defaultModifiers, Array.empty) + } + private val emptyType = new EmptyType private val intTpe = new Projection(emptyType, "Int") private val strTpe = new Projection(emptyType, "String") private val publicAccess = new Public + private val privateAccess = new Private(new Unqualified) private val defaultModifiers = new Modifiers(false, false, false, false, false, false, false) } diff --git a/sbt/src/sbt-test/source-dependencies/trait-private-var/pending b/sbt/src/sbt-test/source-dependencies/trait-private-var/test similarity index 100% rename from sbt/src/sbt-test/source-dependencies/trait-private-var/pending rename to sbt/src/sbt-test/source-dependencies/trait-private-var/test From 8a3170225357479071fb38d3e116ed66f8b6fa89 Mon Sep 17 00:00:00 2001 From: Martin Duhem Date: Mon, 17 Aug 2015 16:31:09 +0200 Subject: [PATCH 2/2] Add overloads to restore binary compatibility --- compile/api/src/main/scala/xsbt/api/HashAPI.scala | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/compile/api/src/main/scala/xsbt/api/HashAPI.scala b/compile/api/src/main/scala/xsbt/api/HashAPI.scala index 56485373b..6ed4054e0 100644 --- a/compile/api/src/main/scala/xsbt/api/HashAPI.scala +++ b/compile/api/src/main/scala/xsbt/api/HashAPI.scala @@ -278,7 +278,7 @@ final class HashAPI(includePrivate: Boolean, includeParamNames: Boolean, include hashSeq(ts, (t: Type) => hashType(t, includeDefinitions)) def hashType(t: Type, includeDefinitions: Boolean = true): Unit = t match { - case s: Structure => hashStructure(s, includeDefinitions) + case s: Structure => hashStructure(s, includeDefinitions, isTrait = false) case e: Existential => hashExistential(e) case c: Constant => hashConstant(c) case p: Polymorphic => hashPolymorphic(p) @@ -345,8 +345,14 @@ final class HashAPI(includePrivate: Boolean, includeParamNames: Boolean, include hashType(a.baseType) hashAnnotations(a.annotations) } - final def hashStructure(structure: Structure, includeDefinitions: Boolean, isTrait: Boolean = false) = + @deprecated("Use the overload that indicates if the definition is a trait.", "0.14") + final def hashStructure(structure: Structure, includeDefinitions: Boolean): Unit = + hashStructure(structure, includeDefinitions, isTrait = false) + final def hashStructure(structure: Structure, includeDefinitions: Boolean, isTrait: Boolean = false): Unit = visit(visitedStructures, structure)(structure => hashStructure0(structure, includeDefinitions, isTrait)) + @deprecated("Use the overload that indicates if the definition is a trait.", "0.14") + def hashStructure0(structure: Structure, includeDefinitions: Boolean): Unit = + hashStructure0(structure, includeDefinitions, isTrait = false) def hashStructure0(structure: Structure, includeDefinitions: Boolean, isTrait: Boolean = false): Unit = { extend(StructureHash) hashTypes(structure.parents, includeDefinitions)