From c8bf46e893588b4578ee453602ad0a7a1da56892 Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Mon, 1 Feb 2016 19:14:44 +0100 Subject: [PATCH 1/3] Expand `pkg-self` scripted test to cover #2326 The previous version of `pkg-self` covered scenario where a package object inherited directly from a class that is being modified and recompiled in subsequent steps. The #2326 shows that name hashing fails to handle a package object that inherits indirectly from a modified class. In that case, name hashing fails to invalidate the source file that defines the package object. This commit expands `pkg-self` with a class B that inherits from modified class `A` and makes package object inherit from B. We mark that test as pending to show that this indeed trigger the bug in name hashing. --- .../sbt-test/source-dependencies/pkg-self/changes/B.scala | 6 ++---- .../sbt-test/source-dependencies/pkg-self/changes/C.scala | 5 +++++ .../source-dependencies/pkg-self/changes/package.scala | 2 +- .../sbt-test/source-dependencies/pkg-self/{test => pending} | 3 ++- 4 files changed, 10 insertions(+), 6 deletions(-) create mode 100644 sbt/src/sbt-test/source-dependencies/pkg-self/changes/C.scala rename sbt/src/sbt-test/source-dependencies/pkg-self/{test => pending} (85%) diff --git a/sbt/src/sbt-test/source-dependencies/pkg-self/changes/B.scala b/sbt/src/sbt-test/source-dependencies/pkg-self/changes/B.scala index 54ffd4574..f67c77c3e 100644 --- a/sbt/src/sbt-test/source-dependencies/pkg-self/changes/B.scala +++ b/sbt/src/sbt-test/source-dependencies/pkg-self/changes/B.scala @@ -1,5 +1,3 @@ -package demo +package demo.sub -object B { - 3.y -} +class B extends A diff --git a/sbt/src/sbt-test/source-dependencies/pkg-self/changes/C.scala b/sbt/src/sbt-test/source-dependencies/pkg-self/changes/C.scala new file mode 100644 index 000000000..25be86f59 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/pkg-self/changes/C.scala @@ -0,0 +1,5 @@ +package demo + +object D { + 3.y +} diff --git a/sbt/src/sbt-test/source-dependencies/pkg-self/changes/package.scala b/sbt/src/sbt-test/source-dependencies/pkg-self/changes/package.scala index 3c01ff4e4..e49f15ac2 100644 --- a/sbt/src/sbt-test/source-dependencies/pkg-self/changes/package.scala +++ b/sbt/src/sbt-test/source-dependencies/pkg-self/changes/package.scala @@ -1,3 +1,3 @@ -package object demo extends sub.A { +package object demo extends sub.B { val y = 9 } diff --git a/sbt/src/sbt-test/source-dependencies/pkg-self/test b/sbt/src/sbt-test/source-dependencies/pkg-self/pending similarity index 85% rename from sbt/src/sbt-test/source-dependencies/pkg-self/test rename to sbt/src/sbt-test/source-dependencies/pkg-self/pending index 595138bba..0486d693b 100644 --- a/sbt/src/sbt-test/source-dependencies/pkg-self/test +++ b/sbt/src/sbt-test/source-dependencies/pkg-self/pending @@ -1,8 +1,9 @@ # Here we have a package object (demo) that extends a class in a subpackage (demo.sub.A) -# demo.sub.A provides an implicit used by demo.B +# demo.sub.A provides an implicit used by demo.D $ copy-file changes/package.scala src/main/scala/demo/package.scala $ copy-file changes/A1.scala src/main/scala/demo/sub/A.scala $ copy-file changes/B.scala src/main/scala/demo/B.scala +$ copy-file changes/C.scala src/main/scala/demo/C.scala > compile # When recompiling A, we delete the class files for A From f78b7112a2cb25314d484c164b3ebbc7decbd527 Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Mon, 1 Feb 2016 19:23:12 +0100 Subject: [PATCH 2/3] Print both memberRef and inheritance relations. To aid debugging, modify Relations.toString to include `inheritance` in addition to already printed `memberRef` relation. Modify it for both name hashing and old implementation. --- .../src/main/scala/sbt/inc/Relations.scala | 32 ++++++++++--------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/compile/inc/src/main/scala/sbt/inc/Relations.scala b/compile/inc/src/main/scala/sbt/inc/Relations.scala index 0ad2e3aa7..0cf989784 100644 --- a/compile/inc/src/main/scala/sbt/inc/Relations.scala +++ b/compile/inc/src/main/scala/sbt/inc/Relations.scala @@ -618,13 +618,14 @@ private class MRelationsDefaultImpl(srcProd: Relation[File, File], binaryDep: Re override def toString = ( """ - |Relations: - | products: %s - | bin deps: %s - | src deps: %s - | ext deps: %s - | class names: %s - """.trim.stripMargin.format(List(srcProd, binaryDep, internalSrcDep, externalDep, classes) map relation_s: _*) + |Relations: + | products: %s + | bin deps: %s + | src deps direct: %s + | src deps inherited: %s + | ext deps: %s + | class names: %s + """.trim.stripMargin.format(List(srcProd, binaryDep, internalSrcDep, publicInherited.internal, externalDep, classes) map relation_s: _*) ) } @@ -739,14 +740,15 @@ private class MRelationsNameHashing(srcProd: Relation[File, File], binaryDep: Re override def toString = ( """ - |Relations (with name hashing enabled): - | products: %s - | bin deps: %s - | src deps: %s - | ext deps: %s - | class names: %s - | used names: %s - """.trim.stripMargin.format(List(srcProd, binaryDep, internalSrcDep, externalDep, classes, names) map relation_s: _*) + |Relations (with name hashing enabled): + | products: %s + | bin deps: %s + | src deps memberRef: %s + | src deps inheritance: %s + | ext deps: %s + | class names: %s + | used names: %s + """.trim.stripMargin.format(List(srcProd, binaryDep, memberRef.internal, inheritance.internal, externalDep, classes, names) map relation_s: _*) ) } From 65f79588980f231c630af68d5705e2aca0d26707 Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Tue, 2 Feb 2016 01:33:43 +0100 Subject: [PATCH 3/3] Invalidate package objects transitively in name hashing Modify `invalidatedPackageObjects` to look for package objects that transitively inherit from an invalidated class instead of just inheriting directly. This is the correct behavior that should have been implemented in the first place. The bug comes from a subtle copy&paste mistake. The old implementation used `publicInherited` relation and new one looks identical but uses `inheritance` relation. The difference is that `publicInherited` was a relation that was expanded transitively along inheritance chain but `inheritance` is not. We have to perform the transitive walk in `IncrementalNameHashing.invalidatedPackageObjects` implementation. Mark `pkg-self` test as passing. Fixes #2326 --- .../inc/src/main/scala/sbt/inc/IncrementalNameHashing.scala | 4 +++- .../sbt-test/source-dependencies/pkg-self/{pending => test} | 0 2 files changed, 3 insertions(+), 1 deletion(-) rename sbt/src/sbt-test/source-dependencies/pkg-self/{pending => test} (100%) diff --git a/compile/inc/src/main/scala/sbt/inc/IncrementalNameHashing.scala b/compile/inc/src/main/scala/sbt/inc/IncrementalNameHashing.scala index 1aaff67b4..49c0c1d4c 100644 --- a/compile/inc/src/main/scala/sbt/inc/IncrementalNameHashing.scala +++ b/compile/inc/src/main/scala/sbt/inc/IncrementalNameHashing.scala @@ -18,7 +18,9 @@ private final class IncrementalNameHashing(log: Logger, options: IncOptions) ext // Package objects are fragile: if they inherit from an invalidated source, get "class file needed by package is missing" error // This might be too conservative: we probably only need package objects for packages of invalidated sources. override protected def invalidatedPackageObjects(invalidated: Set[File], relations: Relations): Set[File] = - invalidated flatMap relations.inheritance.internal.reverse filter { _.getName == "package.scala" } + transitiveDeps(invalidated)(relations.inheritance.internal.reverse).filter { + _.getName == "package.scala" + } override protected def sameAPI[T](src: T, a: Source, b: Source): Option[APIChange[T]] = { if (SameAPI(a, b)) diff --git a/sbt/src/sbt-test/source-dependencies/pkg-self/pending b/sbt/src/sbt-test/source-dependencies/pkg-self/test similarity index 100% rename from sbt/src/sbt-test/source-dependencies/pkg-self/pending rename to sbt/src/sbt-test/source-dependencies/pkg-self/test