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.

Author: smarter

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 =

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.

sbt-dotty/sbt-test/source-dependencies/inner-class/A.scala

@@ -0,0 +1,5 @@
+object A {
+  class InnerClass {
+    def foo: Int = 1
+  }
+}

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)
+  }
+}

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")
+}
+

sbt-dotty/sbt-test/source-dependencies/inner-class/changes/A2.scala

@@ -0,0 +1,5 @@
+object A {
+  class InnerClass {
+    def foo: String = "foo"
+  }
+}

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 = ""
+}

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")
+  )
+}

sbt-dotty/sbt-test/source-dependencies/inner-class/project/plugins.sbt

@@ -0,0 +1 @@
+addSbtPlugin("ch.epfl.lamp" % "sbt-dotty" % sys.props("plugin.version"))

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

sbt-dotty/sbt-test/source-dependencies/inner-object/A.scala

@@ -0,0 +1,5 @@
+object A {
+  object InnerObject {
+    def bla: Int = 2
+  }
+}

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)
+  }
+}

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")
+}
+

sbt-dotty/sbt-test/source-dependencies/inner-object/changes/A2.scala

@@ -0,0 +1,5 @@
+object A {
+  object InnerObject {
+    def bla: String = "bla"
+  }
+}

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 = ""
+}

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")
+  )
+}

sbt-dotty/sbt-test/source-dependencies/inner-object/project/plugins.sbt

@@ -0,0 +1 @@
+addSbtPlugin("ch.epfl.lamp" % "sbt-dotty" % sys.props("plugin.version"))

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