From 74ffe5d064960c04cc1c83a206110e051d0fd81b Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Tue, 2 Feb 2016 16:37:19 +0100 Subject: [PATCH 1/7] Add a pending test for a modified member of a trait The test shows that name hashing optimization is broken for members defined in traits. The problem is that modification of a member of a trait triggers change of a hash of trait's name. The behavior covered by this test regressed in 40ebc825317ed29a2ab87be07246fcdcfc6d1e3d The bug is tracked in #2436. --- .../trait-member-modified/build.sbt | 27 +++++++++++++++++++ .../trait-member-modified/changes/A1.scala | 3 +++ .../trait-member-modified/pending | 9 +++++++ .../src/main/scala/A.scala | 1 + .../src/main/scala/B.scala | 1 + 5 files changed, 41 insertions(+) create mode 100644 sbt/src/sbt-test/compiler-project/trait-member-modified/build.sbt create mode 100644 sbt/src/sbt-test/compiler-project/trait-member-modified/changes/A1.scala create mode 100644 sbt/src/sbt-test/compiler-project/trait-member-modified/pending create mode 100644 sbt/src/sbt-test/compiler-project/trait-member-modified/src/main/scala/A.scala create mode 100644 sbt/src/sbt-test/compiler-project/trait-member-modified/src/main/scala/B.scala diff --git a/sbt/src/sbt-test/compiler-project/trait-member-modified/build.sbt b/sbt/src/sbt-test/compiler-project/trait-member-modified/build.sbt new file mode 100644 index 000000000..949d78231 --- /dev/null +++ b/sbt/src/sbt-test/compiler-project/trait-member-modified/build.sbt @@ -0,0 +1,27 @@ +/* Performs checks related to compilations: + * a) checks in which compilation given set of files was recompiled + * b) checks overall number of compilations performed + */ +TaskKey[Unit]("check-compilations") := { + val analysis = (compile in Compile).value + val srcDir = (scalaSource in Compile).value + def relative(f: java.io.File): java.io.File = f.relativeTo(srcDir) getOrElse f + val allCompilations = analysis.compilations.allCompilations + val recompiledFiles: Seq[Set[java.io.File]] = allCompilations map { c => + val recompiledFiles = analysis.apis.internal.collect { + case (file, api) if api.compilation.startTime == c.startTime => relative(file) + } + recompiledFiles.toSet + } + def recompiledFilesInIteration(iteration: Int, fileNames: Set[String]) = { + val files = fileNames.map(new java.io.File(_)) + assert(recompiledFiles(iteration) == files, "%s != %s".format(recompiledFiles(iteration), files)) + } + assert(allCompilations.size == 2) + // B.scala is just compiled at the beginning + recompiledFilesInIteration(0, Set("B.scala")) + // A.scala is changed and recompiled + recompiledFilesInIteration(1, Set("A.scala")) +} + +logLevel := Level.Debug diff --git a/sbt/src/sbt-test/compiler-project/trait-member-modified/changes/A1.scala b/sbt/src/sbt-test/compiler-project/trait-member-modified/changes/A1.scala new file mode 100644 index 000000000..57a1f34c6 --- /dev/null +++ b/sbt/src/sbt-test/compiler-project/trait-member-modified/changes/A1.scala @@ -0,0 +1,3 @@ +trait A { + def foo: Int = 12 +} diff --git a/sbt/src/sbt-test/compiler-project/trait-member-modified/pending b/sbt/src/sbt-test/compiler-project/trait-member-modified/pending new file mode 100644 index 000000000..f8f7cb076 --- /dev/null +++ b/sbt/src/sbt-test/compiler-project/trait-member-modified/pending @@ -0,0 +1,9 @@ +# Test if adding a member to a trait affects classes that refer to that trait +# by a member reference +> compile +# add `foo` method to `A` +$ copy-file changes/A1.scala src/main/scala/A.scala +# only A.scala should be recompiled +> compile +# check if there are only two compile iterations performed +> check-compilations diff --git a/sbt/src/sbt-test/compiler-project/trait-member-modified/src/main/scala/A.scala b/sbt/src/sbt-test/compiler-project/trait-member-modified/src/main/scala/A.scala new file mode 100644 index 000000000..0eab80adc --- /dev/null +++ b/sbt/src/sbt-test/compiler-project/trait-member-modified/src/main/scala/A.scala @@ -0,0 +1 @@ +trait A diff --git a/sbt/src/sbt-test/compiler-project/trait-member-modified/src/main/scala/B.scala b/sbt/src/sbt-test/compiler-project/trait-member-modified/src/main/scala/B.scala new file mode 100644 index 000000000..c4d3f7e97 --- /dev/null +++ b/sbt/src/sbt-test/compiler-project/trait-member-modified/src/main/scala/B.scala @@ -0,0 +1 @@ +class B(a: A) From 207dd762f4044c05d7d7577f030dddc735d49f4c Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Wed, 3 Feb 2016 14:34:31 +0100 Subject: [PATCH 2/7] Move `trait-member-modified` to source-dependencies It's not clear what the distinction between compiler-project and source-dependencies scripted tests is so let's stick to the one that has the biggest number of tests for incremental compiler. --- .../trait-member-modified/build.sbt | 0 .../trait-member-modified/changes/A1.scala | 0 .../trait-member-modified/pending | 0 .../trait-member-modified/src/main/scala/A.scala | 0 .../trait-member-modified/src/main/scala/B.scala | 0 5 files changed, 0 insertions(+), 0 deletions(-) rename sbt/src/sbt-test/{compiler-project => source-dependencies}/trait-member-modified/build.sbt (100%) rename sbt/src/sbt-test/{compiler-project => source-dependencies}/trait-member-modified/changes/A1.scala (100%) rename sbt/src/sbt-test/{compiler-project => source-dependencies}/trait-member-modified/pending (100%) rename sbt/src/sbt-test/{compiler-project => source-dependencies}/trait-member-modified/src/main/scala/A.scala (100%) rename sbt/src/sbt-test/{compiler-project => source-dependencies}/trait-member-modified/src/main/scala/B.scala (100%) diff --git a/sbt/src/sbt-test/compiler-project/trait-member-modified/build.sbt b/sbt/src/sbt-test/source-dependencies/trait-member-modified/build.sbt similarity index 100% rename from sbt/src/sbt-test/compiler-project/trait-member-modified/build.sbt rename to sbt/src/sbt-test/source-dependencies/trait-member-modified/build.sbt diff --git a/sbt/src/sbt-test/compiler-project/trait-member-modified/changes/A1.scala b/sbt/src/sbt-test/source-dependencies/trait-member-modified/changes/A1.scala similarity index 100% rename from sbt/src/sbt-test/compiler-project/trait-member-modified/changes/A1.scala rename to sbt/src/sbt-test/source-dependencies/trait-member-modified/changes/A1.scala diff --git a/sbt/src/sbt-test/compiler-project/trait-member-modified/pending b/sbt/src/sbt-test/source-dependencies/trait-member-modified/pending similarity index 100% rename from sbt/src/sbt-test/compiler-project/trait-member-modified/pending rename to sbt/src/sbt-test/source-dependencies/trait-member-modified/pending diff --git a/sbt/src/sbt-test/compiler-project/trait-member-modified/src/main/scala/A.scala b/sbt/src/sbt-test/source-dependencies/trait-member-modified/src/main/scala/A.scala similarity index 100% rename from sbt/src/sbt-test/compiler-project/trait-member-modified/src/main/scala/A.scala rename to sbt/src/sbt-test/source-dependencies/trait-member-modified/src/main/scala/A.scala diff --git a/sbt/src/sbt-test/compiler-project/trait-member-modified/src/main/scala/B.scala b/sbt/src/sbt-test/source-dependencies/trait-member-modified/src/main/scala/B.scala similarity index 100% rename from sbt/src/sbt-test/compiler-project/trait-member-modified/src/main/scala/B.scala rename to sbt/src/sbt-test/source-dependencies/trait-member-modified/src/main/scala/B.scala From be8ba763fa2257224646caf70c2b8295901053f0 Mon Sep 17 00:00:00 2001 From: Martin Duhem Date: Wed, 3 Feb 2016 13:49:15 +0100 Subject: [PATCH 3/7] Include ONLY non-public members in API hash of traits The previous logic was wrong and included all members of traits in their API hash, which degraded the performance of the incremental compiler. This commit changes this logic so that only the non-public members of traits are included into their API hash. Fixes #2436 --- compile/api/src/main/scala/xsbt/api/HashAPI.scala | 7 ++++++- .../trait-member-modified/{pending => test} | 0 2 files changed, 6 insertions(+), 1 deletion(-) rename sbt/src/sbt-test/source-dependencies/trait-member-modified/{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 6ed4054e0..59b59797c 100644 --- a/compile/api/src/main/scala/xsbt/api/HashAPI.scala +++ b/compile/api/src/main/scala/xsbt/api/HashAPI.scala @@ -356,7 +356,12 @@ final class HashAPI(includePrivate: Boolean, includeParamNames: Boolean, include def hashStructure0(structure: Structure, includeDefinitions: Boolean, isTrait: Boolean = false): Unit = { extend(StructureHash) hashTypes(structure.parents, includeDefinitions) - if (includeDefinitions || isTrait) { + if (isTrait && !includeDefinitions) { + def public(d: Definition): Boolean = d.access match { case _: xsbti.api.Public => true; case _ => false } + hashDefinitions(structure.declared.filterNot(public), isTrait) + hashDefinitions(structure.inherited.filterNot(public), isTrait) + } + if (includeDefinitions) { hashDefinitions(structure.declared, isTrait) hashDefinitions(structure.inherited, isTrait) } diff --git a/sbt/src/sbt-test/source-dependencies/trait-member-modified/pending b/sbt/src/sbt-test/source-dependencies/trait-member-modified/test similarity index 100% rename from sbt/src/sbt-test/source-dependencies/trait-member-modified/pending rename to sbt/src/sbt-test/source-dependencies/trait-member-modified/test From 8e17269c97c255403be6577ab135801c7fbe7ab7 Mon Sep 17 00:00:00 2001 From: Martin Duhem Date: Tue, 9 Feb 2016 14:35:12 +0100 Subject: [PATCH 4/7] Include non-public vars in API hash of traits --- .../api/src/main/scala/xsbt/api/HashAPI.scala | 26 +++-- .../xsbt/api/NameHashingSpecification.scala | 101 ++++++++++++++---- 2 files changed, 98 insertions(+), 29 deletions(-) diff --git a/compile/api/src/main/scala/xsbt/api/HashAPI.scala b/compile/api/src/main/scala/xsbt/api/HashAPI.scala index 59b59797c..02cc6aded 100644 --- a/compile/api/src/main/scala/xsbt/api/HashAPI.scala +++ b/compile/api/src/main/scala/xsbt/api/HashAPI.scala @@ -139,16 +139,29 @@ final class HashAPI(includePrivate: Boolean, includeParamNames: Boolean, include { hash = startHash(0) hashSymmetric(s.packages, hashPackage) - hashDefinitions(s.definitions, true) + hashDefinitions(s.definitions, topLevel = true, isTrait = false) finalizeHash } def hashPackage(p: Package) = hashString(p.name) + @deprecated("Use the overload that indicates if the enclosing definition is a trait.", "0.14") def hashDefinitions(ds: Seq[Definition], topLevel: Boolean): Unit = + hashDefinitions(ds, topLevel, isTrait = false) + + def hashDefinitions(ds: Seq[Definition], topLevel: Boolean, isTrait: Boolean): Unit = { + // If the enclosing definition is a trait, then we must include private vars in the API hash + // of the trait, because scalac will generate setters and getters for these vars in the traits + // implementors. + val traitPrivateVars = + if (!includePrivate && !topLevel && isTrait) { + def isPublic(d: Definition): Boolean = d.access match { case _: xsbti.api.Public => true; case _ => false } + ds.collect { case v: Var if !isPublic(v) => v } + } else Seq.empty + val defs = SameAPI.filterDefinitions(ds, topLevel, includePrivate) - hashSymmetric(defs, hashDefinition) + hashSymmetric(traitPrivateVars ++ defs, hashDefinition) } /** @@ -356,14 +369,9 @@ final class HashAPI(includePrivate: Boolean, includeParamNames: Boolean, include def hashStructure0(structure: Structure, includeDefinitions: Boolean, isTrait: Boolean = false): Unit = { extend(StructureHash) hashTypes(structure.parents, includeDefinitions) - if (isTrait && !includeDefinitions) { - def public(d: Definition): Boolean = d.access match { case _: xsbti.api.Public => true; case _ => false } - hashDefinitions(structure.declared.filterNot(public), isTrait) - hashDefinitions(structure.inherited.filterNot(public), isTrait) - } if (includeDefinitions) { - hashDefinitions(structure.declared, isTrait) - hashDefinitions(structure.inherited, isTrait) + hashDefinitions(structure.declared, topLevel = false, isTrait) + hashDefinitions(structure.inherited, topLevel = false, 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 4849838b2..470254dda 100644 --- a/compile/api/src/test/scala/xsbt/api/NameHashingSpecification.scala +++ b/compile/api/src/test/scala/xsbt/api/NameHashingSpecification.scala @@ -165,24 +165,25 @@ class NameHashingSpecification extends Specification { } /** - * 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. + * Checks that private vars are included in the hash of the public API of traits. + * Including the private vars of traits is required because classes that implement a trait + * have to define getters and setters for these vars. * For instance, if trait Foo is initially defined as: - * trait Foo { private val x = new A } + * trait Foo { private var x = new A } * changing it to - * trait Foo { private val x = new B } + * trait Foo { private var 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, + * for the private vars 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. + * + * NOTE: This logic is important for vars only. No other private member needs to be included. */ - "private members in traits" in { - /* trait Foo { private val x } */ + "private var in traits are included in API hash" in { + /* trait Foo { private var x } */ val fooTrait1 = simpleTrait("Foo", - simpleStructure(new Val(emptyType, "x", privateAccess, defaultModifiers, Array.empty)), + simpleStructure(new Var(emptyType, "x", privateAccess, defaultModifiers, Array.empty)), publicAccess) /* trait Foo */ @@ -198,15 +199,75 @@ class NameHashingSpecification extends Specification { } + "private vals in traits are NOT included in API hash" 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) + + } + + "private def in traits are not included in API hash" in { + /* trait Foo { private def x } */ + val fooTrait1 = + simpleTrait("Foo", + simpleStructure(new Def(Array.empty, emptyType, Array.empty, "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) + + } + + "private types in traits are included not in API hash" in { + /* trait Foo { private type x } */ + val fooTrait1 = + simpleTrait("Foo", + simpleStructure(new TypeAlias(emptyType, Array.empty, "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. + * Checks that private vars in non-top-level traits are included as well. */ "private members in nested traits" in { - /* class A { trait Foo { private val x } } */ + /* class A { trait Foo { private var x } } */ val classA1 = simpleClass("A", simpleTrait("Foo", - simpleStructure(new Val(emptyType, "x", privateAccess, defaultModifiers, Array.empty)), + simpleStructure(new Var(emptyType, "x", privateAccess, defaultModifiers, Array.empty)), publicAccess)) /* class A { trait Foo } */ @@ -227,11 +288,11 @@ class NameHashingSpecification extends Specification { * Checks that private traits are NOT included in the hash. */ "private traits" in { - /* class Foo { private trait T { private val x } } */ + /* class Foo { private trait T { private var x } } */ val classFoo1 = simpleClass("Foo", simpleTrait("T", - simpleStructure(new Val(emptyType, "x", privateAccess, defaultModifiers, Array.empty)), + simpleStructure(new Var(emptyType, "x", privateAccess, defaultModifiers, Array.empty)), privateAccess)) /** class Foo { private trait T } */ @@ -256,10 +317,10 @@ class NameHashingSpecification extends Specification { * Checks that private members are NOT included in the hash of the public API of classes. */ "private members in classes are not included in the api hash" in { - /* class Foo { private val x } */ + /* class Foo { private var x } */ val classFoo1 = simpleClass("Foo", - simpleStructure(new Val(emptyType, "x", privateAccess, defaultModifiers, Array.empty))) + simpleStructure(new Var(emptyType, "x", privateAccess, defaultModifiers, Array.empty))) /* class Foo */ val classFoo2 = @@ -274,9 +335,9 @@ class NameHashingSpecification extends Specification { } /** - * Checks that private members do NOT contribute to name hashes. - * Test for https://github.com/sbt/sbt/issues/2324 - */ + * Checks that private members do NOT contribute to name hashes. + * Test for https://github.com/sbt/sbt/issues/2324 + */ "private members in classes do not contribute to name hashes" in { /* class Foo { private val x } */ val classFoo = From 7d9425110852205eb2d24126aac00867fbec127f Mon Sep 17 00:00:00 2001 From: Martin Duhem Date: Tue, 9 Feb 2016 14:55:30 +0100 Subject: [PATCH 5/7] Also include private vals --- compile/api/src/main/scala/xsbt/api/HashAPI.scala | 10 +++++----- .../test/scala/xsbt/api/NameHashingSpecification.scala | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/compile/api/src/main/scala/xsbt/api/HashAPI.scala b/compile/api/src/main/scala/xsbt/api/HashAPI.scala index 02cc6aded..0f0baac56 100644 --- a/compile/api/src/main/scala/xsbt/api/HashAPI.scala +++ b/compile/api/src/main/scala/xsbt/api/HashAPI.scala @@ -151,17 +151,17 @@ final class HashAPI(includePrivate: Boolean, includeParamNames: Boolean, include def hashDefinitions(ds: Seq[Definition], topLevel: Boolean, isTrait: Boolean): Unit = { - // If the enclosing definition is a trait, then we must include private vars in the API hash - // of the trait, because scalac will generate setters and getters for these vars in the traits + // If the enclosing definition is a trait, then we must include private vars and vals in the API hash + // of the trait, because scalac will generate setters, getters and fields for them in the traits // implementors. - val traitPrivateVars = + val traitPrivateFields = if (!includePrivate && !topLevel && isTrait) { def isPublic(d: Definition): Boolean = d.access match { case _: xsbti.api.Public => true; case _ => false } - ds.collect { case v: Var if !isPublic(v) => v } + ds.collect { case fl: FieldLike if !isPublic(fl) => fl } } else Seq.empty val defs = SameAPI.filterDefinitions(ds, topLevel, includePrivate) - hashSymmetric(traitPrivateVars ++ defs, hashDefinition) + hashSymmetric(traitPrivateFields ++ defs, hashDefinition) } /** diff --git a/compile/api/src/test/scala/xsbt/api/NameHashingSpecification.scala b/compile/api/src/test/scala/xsbt/api/NameHashingSpecification.scala index 470254dda..5d75708dd 100644 --- a/compile/api/src/test/scala/xsbt/api/NameHashingSpecification.scala +++ b/compile/api/src/test/scala/xsbt/api/NameHashingSpecification.scala @@ -177,7 +177,7 @@ class NameHashingSpecification extends Specification { * we get abstract method errors at runtime, because the types expected by the setter (for instance) does not * match. * - * NOTE: This logic is important for vars only. No other private member needs to be included. + * NOTE: This logic is important for vars and vals only. No other private member needs to be included. */ "private var in traits are included in API hash" in { /* trait Foo { private var x } */ @@ -199,7 +199,7 @@ class NameHashingSpecification extends Specification { } - "private vals in traits are NOT included in API hash" in { + "private vals in traits are included in API hash" in { /* trait Foo { private val x } */ val fooTrait1 = simpleTrait("Foo", @@ -215,7 +215,7 @@ class NameHashingSpecification extends Specification { val api1 = new SourceAPI(Array.empty, Array(fooTrait1)) val api2 = new SourceAPI(Array.empty, Array(fooTrait2)) - HashAPI(api1) === HashAPI(api2) + HashAPI(api1) !== HashAPI(api2) } From edc11744685fe7c439c97c6d0588561580f79fc7 Mon Sep 17 00:00:00 2001 From: Martin Duhem Date: Tue, 9 Feb 2016 15:28:35 +0100 Subject: [PATCH 6/7] Extend test `trait-private-var` with vals Private vars and private vals defined in a trait impact the bytecode generated for the implementors of these traits. The scripted test `source-dependencies/trait-private-var` only accounted for private vars. It now tries the same scenario both with private vars and private vals. --- .../{A.scala => changes/A0.scala} | 0 .../changes/{A.scala => A1.scala} | 0 .../trait-private-var/changes/A2.scala | 5 ++++ .../trait-private-var/test | 26 ++++++++++++++----- 4 files changed, 25 insertions(+), 6 deletions(-) rename sbt/src/sbt-test/source-dependencies/trait-private-var/{A.scala => changes/A0.scala} (100%) rename sbt/src/sbt-test/source-dependencies/trait-private-var/changes/{A.scala => A1.scala} (100%) create mode 100644 sbt/src/sbt-test/source-dependencies/trait-private-var/changes/A2.scala diff --git a/sbt/src/sbt-test/source-dependencies/trait-private-var/A.scala b/sbt/src/sbt-test/source-dependencies/trait-private-var/changes/A0.scala similarity index 100% rename from sbt/src/sbt-test/source-dependencies/trait-private-var/A.scala rename to sbt/src/sbt-test/source-dependencies/trait-private-var/changes/A0.scala diff --git a/sbt/src/sbt-test/source-dependencies/trait-private-var/changes/A.scala b/sbt/src/sbt-test/source-dependencies/trait-private-var/changes/A1.scala similarity index 100% rename from sbt/src/sbt-test/source-dependencies/trait-private-var/changes/A.scala rename to sbt/src/sbt-test/source-dependencies/trait-private-var/changes/A1.scala diff --git a/sbt/src/sbt-test/source-dependencies/trait-private-var/changes/A2.scala b/sbt/src/sbt-test/source-dependencies/trait-private-var/changes/A2.scala new file mode 100644 index 000000000..60641457d --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/trait-private-var/changes/A2.scala @@ -0,0 +1,5 @@ +trait A { + private val foo = 12 + // we need to access foo to trigger AbstractMethodError + def bar: Int = foo +} diff --git a/sbt/src/sbt-test/source-dependencies/trait-private-var/test b/sbt/src/sbt-test/source-dependencies/trait-private-var/test index d928246a9..c120697d8 100644 --- a/sbt/src/sbt-test/source-dependencies/trait-private-var/test +++ b/sbt/src/sbt-test/source-dependencies/trait-private-var/test @@ -1,14 +1,28 @@ +$ copy-file changes/A0.scala A.scala + # compile and run for the first time to verify that everything works > run # introduce private var and refer to it in a trait that we inherit from # there'll be pair of getters and setters generated for private var that # has to be implemented by a class (where you can declare corresponding field) -$ copy-file changes/A.scala A.scala +$ copy-file changes/A1.scala A.scala -# this fails with AbstractMethodError because getters and setters for -# a private var are not generated because introduction of a private var -# does not trigger recompilation of B -# B is not recompiled because incremental compiler tracks only public -# interace (members visible from outside of given trait/class) +# If the introduction of a private var did not trigger the recompilation of B, +# then this will fail with AbstractMethodError because the getters and setters +# for the private var have not been generated. +> run + +# Try again with a private val +> clean + +$ copy-file changes/A0.scala A.scala + +# compile and run a clean project to verify that everything works +> run + +# introduce a private val in the trait +$ copy-file changes/A2.scala A.scala + +# Verify that B has been recompiled and that everything runs fine. > run From 65768446986d4e521cdb3f82f807d4886474c216 Mon Sep 17 00:00:00 2001 From: Martin Duhem Date: Wed, 10 Feb 2016 18:15:44 +0100 Subject: [PATCH 7/7] Hash of traits: include private fields, objects and super accessors --- .../api/src/main/scala/sbt/ClassToAPI.scala | 2 +- .../api/src/main/scala/xsbt/api/APIUtil.scala | 2 +- .../api/src/main/scala/xsbt/api/HashAPI.scala | 22 +++++--- .../xsbt/api/NameHashingSpecification.scala | 52 ++++++++++++++++--- .../scala/sbt/inc/TestCaseGenerators.scala | 2 +- .../src/main/scala/xsbt/ExtractAPI.scala | 2 +- .../src/main/java/xsbti/api/Modifiers.java | 12 +++-- .../trait-private-object/A.scala | 3 ++ .../trait-private-object/B.scala | 5 ++ .../trait-private-object/changes/A.scala | 4 ++ .../trait-private-object/test | 5 ++ 11 files changed, 91 insertions(+), 20 deletions(-) create mode 100644 sbt/src/sbt-test/source-dependencies/trait-private-object/A.scala create mode 100644 sbt/src/sbt-test/source-dependencies/trait-private-object/B.scala create mode 100644 sbt/src/sbt-test/source-dependencies/trait-private-object/changes/A.scala create mode 100644 sbt/src/sbt-test/source-dependencies/trait-private-object/test diff --git a/compile/api/src/main/scala/sbt/ClassToAPI.scala b/compile/api/src/main/scala/sbt/ClassToAPI.scala index 9589865ec..1eb91f44b 100644 --- a/compile/api/src/main/scala/sbt/ClassToAPI.scala +++ b/compile/api/src/main/scala/sbt/ClassToAPI.scala @@ -261,7 +261,7 @@ object ClassToAPI { def modifiers(i: Int): api.Modifiers = { import Modifier.{ isAbstract, isFinal } - new api.Modifiers(isAbstract(i), false, isFinal(i), false, false, false, false) + new api.Modifiers(isAbstract(i), false, isFinal(i), false, false, false, false, false) } def access(i: Int, pkg: Option[String]): api.Access = { diff --git a/compile/api/src/main/scala/xsbt/api/APIUtil.scala b/compile/api/src/main/scala/xsbt/api/APIUtil.scala index d8c4e88cc..577d2cb8f 100644 --- a/compile/api/src/main/scala/xsbt/api/APIUtil.scala +++ b/compile/api/src/main/scala/xsbt/api/APIUtil.scala @@ -12,7 +12,7 @@ object APIUtil { } val byteToModifiers = (b: Byte) => { def x(bit: Int) = (b & (1 << bit)) != 0 - new Modifiers(x(0), x(1), x(2), x(3), x(4), x(5), x(6)) + new Modifiers(x(0), x(1), x(2), x(3), x(4), x(5), x(6), x(7)) } def isScalaSourceName(name: String): Boolean = name.endsWith(".scala") diff --git a/compile/api/src/main/scala/xsbt/api/HashAPI.scala b/compile/api/src/main/scala/xsbt/api/HashAPI.scala index 0f0baac56..f600d5416 100644 --- a/compile/api/src/main/scala/xsbt/api/HashAPI.scala +++ b/compile/api/src/main/scala/xsbt/api/HashAPI.scala @@ -151,17 +151,25 @@ final class HashAPI(includePrivate: Boolean, includeParamNames: Boolean, include def hashDefinitions(ds: Seq[Definition], topLevel: Boolean, isTrait: Boolean): Unit = { - // If the enclosing definition is a trait, then we must include private vars and vals in the API hash - // of the trait, because scalac will generate setters, getters and fields for them in the traits - // implementors. - val traitPrivateFields = + def isPublic(d: Definition): Boolean = d.access match { case _: xsbti.api.Public => true; case _ => false } + def isTraitBreaker(d: Definition): Boolean = d match { + // Vars and vals in traits introduce getters, setters and fields in the implementing classes. + // See test `source-dependencies/trait-private-var + case _: FieldLike => true + // Objects in traits introduce fields in the implementing classes. + // See test `source-dependencies/trait-private-object` + case cl: ClassLike => cl.definitionType == DefinitionType.Module + // super calls introduce accessors that are not part of the public API + case d: Def => d.modifiers.isSuperAccessor + case _ => false + } + val includedPrivateDefinitions = if (!includePrivate && !topLevel && isTrait) { - def isPublic(d: Definition): Boolean = d.access match { case _: xsbti.api.Public => true; case _ => false } - ds.collect { case fl: FieldLike if !isPublic(fl) => fl } + ds filter (x => isTraitBreaker(x) && !isPublic(x)) } else Seq.empty val defs = SameAPI.filterDefinitions(ds, topLevel, includePrivate) - hashSymmetric(traitPrivateFields ++ defs, hashDefinition) + hashSymmetric(includedPrivateDefinitions ++ defs, hashDefinition) } /** diff --git a/compile/api/src/test/scala/xsbt/api/NameHashingSpecification.scala b/compile/api/src/test/scala/xsbt/api/NameHashingSpecification.scala index 5d75708dd..1ec910058 100644 --- a/compile/api/src/test/scala/xsbt/api/NameHashingSpecification.scala +++ b/compile/api/src/test/scala/xsbt/api/NameHashingSpecification.scala @@ -176,8 +176,6 @@ class NameHashingSpecification extends Specification { * for the private vars 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. - * - * NOTE: This logic is important for vars and vals only. No other private member needs to be included. */ "private var in traits are included in API hash" in { /* trait Foo { private var x } */ @@ -219,7 +217,28 @@ class NameHashingSpecification extends Specification { } - "private def in traits are not included in API hash" in { + "private objects in traits are included in API hash" in { + /* trait Foo { private object x } */ + val fooTrait1 = + simpleTrait("Foo", + simpleStructure( + new ClassLike(DefinitionType.Module, lzy(emptyType), lzy(simpleStructure()), Array.empty, Array.empty, "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) + + } + + "private non-synthetic def in traits are not included in API hash" in { /* trait Foo { private def x } */ val fooTrait1 = simpleTrait("Foo", @@ -239,6 +258,27 @@ class NameHashingSpecification extends Specification { } + "private synthetic def in traits are included in API hash" in { + /* trait Foo { private def x } */ + val modifiers = new xsbti.api.Modifiers(false, false, false, false, false, false, false, true) + val fooTrait1 = + simpleTrait("Foo", + simpleStructure(new Def(Array.empty, emptyType, Array.empty, "x", privateAccess, modifiers, 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) + + } + "private types in traits are included not in API hash" in { /* trait Foo { private type x } */ val fooTrait1 = @@ -262,7 +302,7 @@ class NameHashingSpecification extends Specification { /** * Checks that private vars in non-top-level traits are included as well. */ - "private members in nested traits" in { + "private variables in nested traits are include in the API hash" in { /* class A { trait Foo { private var x } } */ val classA1 = simpleClass("A", @@ -287,7 +327,7 @@ class NameHashingSpecification extends Specification { /** * Checks that private traits are NOT included in the hash. */ - "private traits" in { + "private inner traits are not included in the API hash" in { /* class Foo { private trait T { private var x } } */ val classFoo1 = simpleClass("Foo", @@ -397,6 +437,6 @@ class NameHashingSpecification extends Specification { 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) + private val defaultModifiers = new Modifiers(false, false, false, false, false, false, false, false) } diff --git a/compile/inc/src/test/scala/sbt/inc/TestCaseGenerators.scala b/compile/inc/src/test/scala/sbt/inc/TestCaseGenerators.scala index 7591183b1..3379ced55 100644 --- a/compile/inc/src/test/scala/sbt/inc/TestCaseGenerators.scala +++ b/compile/inc/src/test/scala/sbt/inc/TestCaseGenerators.scala @@ -76,7 +76,7 @@ object TestCaseGenerators { private[this] def makeDefinition(name: String): Definition = new ClassLike(DefinitionType.ClassDef, lzy(new EmptyType()), lzy(new Structure(lzy(Array()), lzy(Array()), lzy(Array()))), Array(), Array(), - name, new Public(), new Modifiers(false, false, false, false, false, false, false), Array()) + name, new Public(), new Modifiers(false, false, false, false, false, false, false, false), Array()) private[this] def lzy[T <: AnyRef](x: T) = SafeLazy.strict(x) diff --git a/compile/interface/src/main/scala/xsbt/ExtractAPI.scala b/compile/interface/src/main/scala/xsbt/ExtractAPI.scala index 011479be6..07336d31b 100644 --- a/compile/interface/src/main/scala/xsbt/ExtractAPI.scala +++ b/compile/interface/src/main/scala/xsbt/ExtractAPI.scala @@ -438,7 +438,7 @@ class ExtractAPI[GlobalType <: CallbackGlobal](val global: GlobalType, val absOver = s.hasFlag(ABSOVERRIDE) val abs = s.hasFlag(ABSTRACT) || s.hasFlag(DEFERRED) || absOver val over = s.hasFlag(OVERRIDE) || absOver - new xsbti.api.Modifiers(abs, over, s.isFinal, s.hasFlag(SEALED), isImplicit(s), s.hasFlag(LAZY), hasMacro(s)) + new xsbti.api.Modifiers(abs, over, s.isFinal, s.hasFlag(SEALED), isImplicit(s), s.hasFlag(LAZY), hasMacro(s), s.hasFlag(SUPERACCESSOR)) } private def isImplicit(s: Symbol) = s.hasFlag(Flags.IMPLICIT) diff --git a/interface/src/main/java/xsbti/api/Modifiers.java b/interface/src/main/java/xsbti/api/Modifiers.java index 5e103c7ec..c91c3ec28 100644 --- a/interface/src/main/java/xsbti/api/Modifiers.java +++ b/interface/src/main/java/xsbti/api/Modifiers.java @@ -9,13 +9,14 @@ public final class Modifiers implements java.io.Serializable private static final int ImplicitBit = 4; private static final int LazyBit = 5; private static final int MacroBit = 6; + private static final int SuperAccessorBit = 7; private static int flag(boolean set, int bit) { return set ? (1 << bit) : 0; } - public Modifiers(boolean isAbstract, boolean isOverride, boolean isFinal, boolean isSealed, boolean isImplicit, boolean isLazy, boolean isMacro) + public Modifiers(boolean isAbstract, boolean isOverride, boolean isFinal, boolean isSealed, boolean isImplicit, boolean isLazy, boolean isMacro, boolean isSuperAccessor) { this.flags = (byte)( flag(isAbstract, AbstractBit) | @@ -24,7 +25,8 @@ public final class Modifiers implements java.io.Serializable flag(isSealed, SealedBit) | flag(isImplicit, ImplicitBit) | flag(isLazy, LazyBit) | - flag(isMacro, MacroBit) + flag(isMacro, MacroBit) | + flag(isSuperAccessor, SuperAccessorBit) ); } @@ -68,6 +70,10 @@ public final class Modifiers implements java.io.Serializable { return flag(MacroBit); } + public final boolean isSuperAccessor() + { + return flag(SuperAccessorBit); + } public boolean equals(Object o) { return (o instanceof Modifiers) && flags == ((Modifiers)o).flags; @@ -78,6 +84,6 @@ public final class Modifiers implements java.io.Serializable } public String toString() { - return "Modifiers(" + "isAbstract: " + isAbstract() + ", " + "isOverride: " + isOverride() + ", " + "isFinal: " + isFinal() + ", " + "isSealed: " + isSealed() + ", " + "isImplicit: " + isImplicit() + ", " + "isLazy: " + isLazy() + ", " + "isMacro: " + isMacro()+ ")"; + return "Modifiers(" + "isAbstract: " + isAbstract() + ", " + "isOverride: " + isOverride() + ", " + "isFinal: " + isFinal() + ", " + "isSealed: " + isSealed() + ", " + "isImplicit: " + isImplicit() + ", " + "isLazy: " + isLazy() + ", " + "isMacro: " + isMacro()+ ", isSuperAccessor:" + isSuperAccessor() + ")"; } } diff --git a/sbt/src/sbt-test/source-dependencies/trait-private-object/A.scala b/sbt/src/sbt-test/source-dependencies/trait-private-object/A.scala new file mode 100644 index 000000000..cbcda3176 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/trait-private-object/A.scala @@ -0,0 +1,3 @@ +trait A { + val foo = 0 +} \ No newline at end of file diff --git a/sbt/src/sbt-test/source-dependencies/trait-private-object/B.scala b/sbt/src/sbt-test/source-dependencies/trait-private-object/B.scala new file mode 100644 index 000000000..5da0f8a71 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/trait-private-object/B.scala @@ -0,0 +1,5 @@ +object B extends A { + def main(args: Array[String]): Unit = { + println(foo) + } +} diff --git a/sbt/src/sbt-test/source-dependencies/trait-private-object/changes/A.scala b/sbt/src/sbt-test/source-dependencies/trait-private-object/changes/A.scala new file mode 100644 index 000000000..b5600153b --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/trait-private-object/changes/A.scala @@ -0,0 +1,4 @@ +trait A { + val foo = 0 + X.a + private object X { val a = 1 } +} \ No newline at end of file diff --git a/sbt/src/sbt-test/source-dependencies/trait-private-object/test b/sbt/src/sbt-test/source-dependencies/trait-private-object/test new file mode 100644 index 000000000..5aab7a143 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/trait-private-object/test @@ -0,0 +1,5 @@ +> run + +$ copy-file changes/A.scala A.scala + +> run \ No newline at end of file