From 0e350d566c897072b97803f386aa7d695dc4bda2 Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Tue, 1 Sep 2020 00:13:05 +0200 Subject: [PATCH] Fix undercompilation when depending on inner class To communicate to sbt a dependency between a class currently being compiled and some other class on the classpath, we use the `binaryDependency` callback which requires passing the "binary class name": for a class A in a package pkg, this is just "pkg.A", but for an inner class B in A, that would be "pkg.A$B", in other words the last component of the binary class name is the name of the corresponding classfile. Before this commit, we computed this name by extracting it from the path of the `associatedFile` but it turns out that `associatedFile` for an inner Scala class returns the classfile of the corresponding top-level class! This happens because the compiler never actually reads inner Scala classfiles since all the information is present in the top-level .class and .tasty files. This means that under separate compilation a dependency to an inner class was not recorded which could lead to undercompilation (see added tests). This commit fixes this by computing binary class names manually: they're just made of the fullName of the enclosing package, followed by ".", followed by the flatName of the current class. This is similar to what is done in the Scala 2 compiler bridge in Zinc. --- .../src/dotty/tools/dotc/core/Symbols.scala | 2 + .../tools/dotc/sbt/ExtractDependencies.scala | 44 +++++++++++-------- .../source-dependencies/inner-class/A.scala | 5 +++ .../source-dependencies/inner-class/B.scala | 7 +++ .../source-dependencies/inner-class/build.sbt | 12 +++++ .../inner-class/changes/A2.scala | 5 +++ .../inner-class/changes/B2.scala | 8 ++++ .../project/DottyInjectedPlugin.scala | 11 +++++ .../inner-class/project/plugins.sbt | 1 + .../source-dependencies/inner-class/test | 12 +++++ .../source-dependencies/inner-object/A.scala | 5 +++ .../source-dependencies/inner-object/B.scala | 6 +++ .../inner-object/build.sbt | 12 +++++ .../inner-object/changes/A2.scala | 5 +++ .../inner-object/changes/B2.scala | 7 +++ .../project/DottyInjectedPlugin.scala | 11 +++++ .../inner-object/project/plugins.sbt | 1 + .../source-dependencies/inner-object/test | 12 +++++ 18 files changed, 148 insertions(+), 18 deletions(-) create mode 100644 sbt-dotty/sbt-test/source-dependencies/inner-class/A.scala create mode 100644 sbt-dotty/sbt-test/source-dependencies/inner-class/B.scala create mode 100644 sbt-dotty/sbt-test/source-dependencies/inner-class/build.sbt create mode 100644 sbt-dotty/sbt-test/source-dependencies/inner-class/changes/A2.scala create mode 100644 sbt-dotty/sbt-test/source-dependencies/inner-class/changes/B2.scala create mode 100644 sbt-dotty/sbt-test/source-dependencies/inner-class/project/DottyInjectedPlugin.scala create mode 100644 sbt-dotty/sbt-test/source-dependencies/inner-class/project/plugins.sbt create mode 100644 sbt-dotty/sbt-test/source-dependencies/inner-class/test create mode 100644 sbt-dotty/sbt-test/source-dependencies/inner-object/A.scala create mode 100644 sbt-dotty/sbt-test/source-dependencies/inner-object/B.scala create mode 100644 sbt-dotty/sbt-test/source-dependencies/inner-object/build.sbt create mode 100644 sbt-dotty/sbt-test/source-dependencies/inner-object/changes/A2.scala create mode 100644 sbt-dotty/sbt-test/source-dependencies/inner-object/changes/B2.scala create mode 100644 sbt-dotty/sbt-test/source-dependencies/inner-object/project/DottyInjectedPlugin.scala create mode 100644 sbt-dotty/sbt-test/source-dependencies/inner-object/project/plugins.sbt create mode 100644 sbt-dotty/sbt-test/source-dependencies/inner-object/test diff --git a/compiler/src/dotty/tools/dotc/core/Symbols.scala b/compiler/src/dotty/tools/dotc/core/Symbols.scala index 7ba320159531..d6d73b9dea61 100644 --- a/compiler/src/dotty/tools/dotc/core/Symbols.scala +++ b/compiler/src/dotty/tools/dotc/core/Symbols.scala @@ -250,6 +250,8 @@ object Symbols { /** The source or class file from which this class or * the class containing this symbol was generated, null if not applicable. + * Note that this the returned classfile might be the top-level class + * containing this symbol instead of the directly enclosing class. * Overridden in ClassSymbol */ def associatedFile(using Context): AbstractFile = diff --git a/compiler/src/dotty/tools/dotc/sbt/ExtractDependencies.scala b/compiler/src/dotty/tools/dotc/sbt/ExtractDependencies.scala index 885aa98d6237..41478ead4053 100644 --- a/compiler/src/dotty/tools/dotc/sbt/ExtractDependencies.scala +++ b/compiler/src/dotty/tools/dotc/sbt/ExtractDependencies.scala @@ -112,32 +112,22 @@ class ExtractDependencies extends Phase { def binaryDependency(file: File, binaryClassName: String) = ctx.sbtCallback.binaryDependency(file, binaryClassName, fromClassName, sourceFile, dep.context) - def processExternalDependency(depFile: AbstractFile) = { - def binaryClassName(classSegments: List[String]) = - classSegments.mkString(".").stripSuffix(".class") - + def processExternalDependency(depFile: AbstractFile, binaryClassName: String) = { depFile match { case ze: ZipArchive#Entry => // The dependency comes from a JAR - for (zip <- ze.underlyingSource; zipFile <- Option(zip.file)) { - val classSegments = io.File(ze.path).segments - binaryDependency(zipFile, binaryClassName(classSegments)) - } - + ze.underlyingSource match + case Some(zip) if zip.file != null => + binaryDependency(zip.file, binaryClassName) + case _ => case pf: PlainFile => // The dependency comes from a class file - val packages = dep.to.ownersIterator - .count(x => x.is(PackageClass) && !x.isEffectiveRoot) - // We can recover the fully qualified name of a classfile from - // its path - val classSegments = pf.givenPath.segments.takeRight(packages + 1) // FIXME: pf.file is null for classfiles coming from the modulepath // (handled by JrtClassPath) because they cannot be represented as // java.io.File, since the `binaryDependency` callback must take a // java.io.File, this means that we cannot record dependencies coming // from the modulepath. For now this isn't a big deal since we only // support having the standard Java library on the modulepath. - if (pf.file != null) - binaryDependency(pf.file, binaryClassName(classSegments)) - + if pf.file != null then + binaryDependency(pf.file, binaryClassName) case _ => report.warning(s"sbt-deps: Ignoring dependency $depFile of class ${depFile.getClass}}") } @@ -149,7 +139,25 @@ class ExtractDependencies extends Phase { def allowLocal = dep.context == DependencyByInheritance || dep.context == LocalDependencyByInheritance if (depFile.extension == "class") { // Dependency is external -- source is undefined - processExternalDependency(depFile) + + // The fully qualified name on the JVM of the class corresponding to `dep.to` + val binaryClassName = { + val builder = new StringBuilder + val pkg = dep.to.enclosingPackageClass + if (!pkg.isEffectiveRoot) { + builder.append(pkg.fullName.mangledString) + builder.append(".") + } + val flatName = dep.to.flatName + // We create fake companion object symbols to hold the static members + // of Java classes, make sure to use the name of the actual Java class + // here. + val clsFlatName = if (dep.to.is(JavaDefined)) flatName.stripModuleClassSuffix else flatName + builder.append(clsFlatName.mangledString) + builder.toString + } + + processExternalDependency(depFile, binaryClassName) } else if (allowLocal || depFile.file != sourceFile) { // We cannot ignore dependencies coming from the same source file because // the dependency info needs to propagate. See source-dependencies/trait-trait-211. diff --git a/sbt-dotty/sbt-test/source-dependencies/inner-class/A.scala b/sbt-dotty/sbt-test/source-dependencies/inner-class/A.scala new file mode 100644 index 000000000000..724e1f86f82f --- /dev/null +++ b/sbt-dotty/sbt-test/source-dependencies/inner-class/A.scala @@ -0,0 +1,5 @@ +object A { + class InnerClass { + def foo: Int = 1 + } +} diff --git a/sbt-dotty/sbt-test/source-dependencies/inner-class/B.scala b/sbt-dotty/sbt-test/source-dependencies/inner-class/B.scala new file mode 100644 index 000000000000..065220c1a440 --- /dev/null +++ b/sbt-dotty/sbt-test/source-dependencies/inner-class/B.scala @@ -0,0 +1,7 @@ +object B { + def main(args: Array[String]): Unit = { + val innerClass = new A.InnerClass + val x = innerClass.foo + println(x) + } +} diff --git a/sbt-dotty/sbt-test/source-dependencies/inner-class/build.sbt b/sbt-dotty/sbt-test/source-dependencies/inner-class/build.sbt new file mode 100644 index 000000000000..d7524d433978 --- /dev/null +++ b/sbt-dotty/sbt-test/source-dependencies/inner-class/build.sbt @@ -0,0 +1,12 @@ +import complete.DefaultParsers._ + +val checkIterations = inputKey[Unit]("Verifies the accumlated number of iterations of incremental compilation.") + +checkIterations := { + val analysis = (compile in Compile).value.asInstanceOf[sbt.internal.inc.Analysis] + + val expected: Int = (Space ~> NatBasic).parsed + val actual: Int = analysis.compilations.allCompilations.size + assert(expected == actual, s"Expected $expected compilations, got $actual") +} + diff --git a/sbt-dotty/sbt-test/source-dependencies/inner-class/changes/A2.scala b/sbt-dotty/sbt-test/source-dependencies/inner-class/changes/A2.scala new file mode 100644 index 000000000000..028828896e7c --- /dev/null +++ b/sbt-dotty/sbt-test/source-dependencies/inner-class/changes/A2.scala @@ -0,0 +1,5 @@ +object A { + class InnerClass { + def foo: String = "foo" + } +} diff --git a/sbt-dotty/sbt-test/source-dependencies/inner-class/changes/B2.scala b/sbt-dotty/sbt-test/source-dependencies/inner-class/changes/B2.scala new file mode 100644 index 000000000000..2400fc93c983 --- /dev/null +++ b/sbt-dotty/sbt-test/source-dependencies/inner-class/changes/B2.scala @@ -0,0 +1,8 @@ +object B { + def main(args: Array[String]): Unit = { + val innerClass = new A.InnerClass + val x = innerClass.foo + println(x) + } + val forceRecompileDummy = "" +} diff --git a/sbt-dotty/sbt-test/source-dependencies/inner-class/project/DottyInjectedPlugin.scala b/sbt-dotty/sbt-test/source-dependencies/inner-class/project/DottyInjectedPlugin.scala new file mode 100644 index 000000000000..fb946c4b8c61 --- /dev/null +++ b/sbt-dotty/sbt-test/source-dependencies/inner-class/project/DottyInjectedPlugin.scala @@ -0,0 +1,11 @@ +import sbt._ +import Keys._ + +object DottyInjectedPlugin extends AutoPlugin { + override def requires = plugins.JvmPlugin + override def trigger = allRequirements + + override val projectSettings = Seq( + scalaVersion := sys.props("plugin.scalaVersion") + ) +} diff --git a/sbt-dotty/sbt-test/source-dependencies/inner-class/project/plugins.sbt b/sbt-dotty/sbt-test/source-dependencies/inner-class/project/plugins.sbt new file mode 100644 index 000000000000..c17caab2d98c --- /dev/null +++ b/sbt-dotty/sbt-test/source-dependencies/inner-class/project/plugins.sbt @@ -0,0 +1 @@ +addSbtPlugin("ch.epfl.lamp" % "sbt-dotty" % sys.props("plugin.version")) diff --git a/sbt-dotty/sbt-test/source-dependencies/inner-class/test b/sbt-dotty/sbt-test/source-dependencies/inner-class/test new file mode 100644 index 000000000000..e13b5ba1160f --- /dev/null +++ b/sbt-dotty/sbt-test/source-dependencies/inner-class/test @@ -0,0 +1,12 @@ +> run + +# Recompile B (no meaningful change, this is just so that the dependencies on A.InnerClass are +# recorded using the `binaryDependency` sbt callback, which will only work correctly +# if we call it with the inner class binary name and not the top-level class name). +$ copy-file changes/B2.scala B.scala +> compile + +# Change the signature of A.Inner#foo, this requires B to be recompiled, +# otherwise run will fail: +$ copy-file changes/A2.scala A.scala +> run diff --git a/sbt-dotty/sbt-test/source-dependencies/inner-object/A.scala b/sbt-dotty/sbt-test/source-dependencies/inner-object/A.scala new file mode 100644 index 000000000000..ab3489979498 --- /dev/null +++ b/sbt-dotty/sbt-test/source-dependencies/inner-object/A.scala @@ -0,0 +1,5 @@ +object A { + object InnerObject { + def bla: Int = 2 + } +} diff --git a/sbt-dotty/sbt-test/source-dependencies/inner-object/B.scala b/sbt-dotty/sbt-test/source-dependencies/inner-object/B.scala new file mode 100644 index 000000000000..be316631288d --- /dev/null +++ b/sbt-dotty/sbt-test/source-dependencies/inner-object/B.scala @@ -0,0 +1,6 @@ +object B { + def main(args: Array[String]): Unit = { + val o = A.InnerObject.bla + println(o) + } +} diff --git a/sbt-dotty/sbt-test/source-dependencies/inner-object/build.sbt b/sbt-dotty/sbt-test/source-dependencies/inner-object/build.sbt new file mode 100644 index 000000000000..d7524d433978 --- /dev/null +++ b/sbt-dotty/sbt-test/source-dependencies/inner-object/build.sbt @@ -0,0 +1,12 @@ +import complete.DefaultParsers._ + +val checkIterations = inputKey[Unit]("Verifies the accumlated number of iterations of incremental compilation.") + +checkIterations := { + val analysis = (compile in Compile).value.asInstanceOf[sbt.internal.inc.Analysis] + + val expected: Int = (Space ~> NatBasic).parsed + val actual: Int = analysis.compilations.allCompilations.size + assert(expected == actual, s"Expected $expected compilations, got $actual") +} + diff --git a/sbt-dotty/sbt-test/source-dependencies/inner-object/changes/A2.scala b/sbt-dotty/sbt-test/source-dependencies/inner-object/changes/A2.scala new file mode 100644 index 000000000000..9ce1d3057d9b --- /dev/null +++ b/sbt-dotty/sbt-test/source-dependencies/inner-object/changes/A2.scala @@ -0,0 +1,5 @@ +object A { + object InnerObject { + def bla: String = "bla" + } +} diff --git a/sbt-dotty/sbt-test/source-dependencies/inner-object/changes/B2.scala b/sbt-dotty/sbt-test/source-dependencies/inner-object/changes/B2.scala new file mode 100644 index 000000000000..dd6f366405fa --- /dev/null +++ b/sbt-dotty/sbt-test/source-dependencies/inner-object/changes/B2.scala @@ -0,0 +1,7 @@ +object B { + def main(args: Array[String]): Unit = { + val o = A.InnerObject.bla + println(o) + } + val forceRecompileDummy = "" +} diff --git a/sbt-dotty/sbt-test/source-dependencies/inner-object/project/DottyInjectedPlugin.scala b/sbt-dotty/sbt-test/source-dependencies/inner-object/project/DottyInjectedPlugin.scala new file mode 100644 index 000000000000..fb946c4b8c61 --- /dev/null +++ b/sbt-dotty/sbt-test/source-dependencies/inner-object/project/DottyInjectedPlugin.scala @@ -0,0 +1,11 @@ +import sbt._ +import Keys._ + +object DottyInjectedPlugin extends AutoPlugin { + override def requires = plugins.JvmPlugin + override def trigger = allRequirements + + override val projectSettings = Seq( + scalaVersion := sys.props("plugin.scalaVersion") + ) +} diff --git a/sbt-dotty/sbt-test/source-dependencies/inner-object/project/plugins.sbt b/sbt-dotty/sbt-test/source-dependencies/inner-object/project/plugins.sbt new file mode 100644 index 000000000000..c17caab2d98c --- /dev/null +++ b/sbt-dotty/sbt-test/source-dependencies/inner-object/project/plugins.sbt @@ -0,0 +1 @@ +addSbtPlugin("ch.epfl.lamp" % "sbt-dotty" % sys.props("plugin.version")) diff --git a/sbt-dotty/sbt-test/source-dependencies/inner-object/test b/sbt-dotty/sbt-test/source-dependencies/inner-object/test new file mode 100644 index 000000000000..dee41f3bfccb --- /dev/null +++ b/sbt-dotty/sbt-test/source-dependencies/inner-object/test @@ -0,0 +1,12 @@ +> run + +# Recompile B (no meaningful change, this is just so that the dependency on A.InnerObject are +# recorded using the `binaryDependency` sbt callback, which will only work correctly +# if we call it with the inner object binary name and not the top-level class name). +$ copy-file changes/B2.scala B.scala +> compile + +# Change the signature of A.Inner#foo, this requires B to be recompiled, +# otherwise run will fail: +$ copy-file changes/A2.scala A.scala +> run