Fix overcompilation when inheriting inline defs

When the body of an inline def changes, all its callers need to be
recompiled. So when producing an API signature for an inline def to be
sent to Zinc, we need to somehow include that body. So far, this was
done by simply including the `toString` of the tree in the API
signature, but the API of a class includes its inherited members, and
when calling `toString` on an inherited inline def, its body ends up
being printed as just `TreeUnpickler$LazyReader@123...` which is clearly
not what we want, in practice this lead to overcompilation rather than
undercompilation because the string includes the identityHashCode of the
LazyReader which changes at each compilation run.

This commit fixes this by implementing a proper way to hash trees, this
is still not perfect since types aren't included in the hash (see
comments in the code), but that can be addressed at a later point (and
the exact strategy we want to use also depends on how we evolve Zinc).

Author: smarter

compiler/src/dotty/tools/dotc/sbt/ExtractAPI.scala

@@ -1,7 +1,7 @@
 package dotty.tools.dotc
 package sbt
 
-import ast.{Trees, tpd}
+import ast.{Positioned, Trees, tpd, untpd}
 import core._
 import core.Decorators._
 import Annotations._
@@ -11,6 +11,7 @@ import Phases._
 import Trees._
 import Types._
 import Symbols._
+import Names._
 import NameOps._
 import NameKinds.DefaultGetterName
 import typer.Inliner
@@ -582,13 +583,18 @@ private class ExtractAPICollector(using Context) extends ThunkHolder {
     val annots = new mutable.ListBuffer[api.Annotation]
     val inlineBody = Inliner.bodyToInline(s)
     if (!inlineBody.isEmpty) {
-      // FIXME: If the body of an inlineable method changes, all the reverse
-      // dependencies of this method need to be recompiled. sbt has no way
-      // of tracking method bodies, so as a hack we include the printed
-      // tree of the method as part of the signature we send to sbt.
-      // To do this properly we would need a way to hash trees and types in
-      // dotty itself.
-      annots += marker(inlineBody.toString)
+      // If the body of an inline def changes, all the reverse dependencies of
+      // this method need to be recompiled. sbt has no way of tracking method
+      // bodies, so we include the hash of the body of the method as part of the
+      // signature we send to sbt.
+      //
+      // FIXME: The API of a class we send to Zinc includes the signatures of
+      // inherited methods, which means that we repeatedly compute the hash of
+      // an inline def in every class that extends its owner. To avoid this we
+      // could store the hash as an annotation when pickling an inline def
+      // and retrieve it here instead of computing it on the fly.
+      val inlineBodyHash = treeHash(inlineBody)
+      annots += marker(inlineBodyHash.toString)
     }
 
     // In the Scala2 ExtractAPI phase we only extract annotations that extend
@@ -602,16 +608,66 @@ private class ExtractAPICollector(using Context) extends ThunkHolder {
     annots.toList
   }
 
+  /** Produce a hash for a tree that is as stable as possible:
+   *  it should stay the same across compiler runs, compiler instances,
+   *  JVMs, etc.
+   */
+  def treeHash(tree: Tree): Int =
+    import scala.util.hashing.MurmurHash3
+
+    def positionedHash(p: ast.Positioned, initHash: Int): Int =
+      p match
+        case p: WithLazyField[?] =>
+          p.forceIfLazy
+        case _ =>
+      // FIXME: If `p` is a tree we should probably take its type into account
+      // when hashing it, but producing a stable hash for a type is not trivial
+      // since the same type might have multiple representations, for method
+      // signatures this is already handled by `computeType` and the machinery
+      // in Zinc that generates hashes from that, if we can reliably produce
+      // stable hashes for types ourselves then we could bypass all that and
+      // send Zinc hashes directly.
+      val h = MurmurHash3.mix(initHash, p.productPrefix.hashCode)
+      iteratorHash(p.productIterator, h)
+    end positionedHash
+
+    def iteratorHash(it: Iterator[Any], initHash: Int): Int =
+      var h = initHash
+      while it.hasNext do
+        it.next() match
+          case p: Positioned =>
+            h = positionedHash(p, h)
+          case xs: List[?] =>
+            h = iteratorHash(xs.iterator, h)
+          case c: core.Constants.Constant =>
+            h = MurmurHash3.mix(h, c.tag)
+            h = MurmurHash3.mix(h, c.value.##) // We can't use `value.hashCode` since value might be null
+          case n: Name =>
+            // The hashCode of the name itself is not stable across compiler instances
+            h = MurmurHash3.mix(h, n.toString.hashCode)
+          case elem =>
+            report.warning(
+              i"""Internal error: Don't know how to produce a stable hash for `$elem` of unknown class ${elem.getClass}
+                 |Incremental compilation might not work correctly.""", tree.sourcePos)
+            h = MurmurHash3.mix(h, elem.toString.hashCode)
+      h
+    end iteratorHash
+
+    val seed = 4 // https://xkcd.com/221
+    val h = positionedHash(tree, seed)
+    MurmurHash3.finalizeHash(h, 0)
+  end treeHash
+
   def apiAnnotation(annot: Annotation): api.Annotation = {
-    // FIXME: To faithfully extract an API we should extract the annotation tree,
-    // sbt instead wants us to extract the annotation type and its arguments,
-    // to do this properly we would need a way to hash trees and types in dotty itself,
-    // instead we use the raw string representation of the annotation tree.
-    // However, we still need to extract the annotation type in the way sbt expect
-    // because sbt uses this information to find tests to run (for example
-    // junit tests are annotated @org.junit.Test).
+    // Like with inline defs, the whole body of the annotation and not just its
+    // type is part of its API so we need to store its hash, but Zinc wants us
+    // to extract the annotation type and its arguments, so we use a dummy
+    // annotation argument to store the hash of the tree. We still need to
+    // extract the annotation type in the way Zinc expects because sbt uses this
+    // information to find tests to run (for example junit tests are
+    // annotated @org.junit.Test).
     api.Annotation.of(
       apiType(annot.tree.tpe), // Used by sbt to find tests to run
-      Array(api.AnnotationArgument.of("FULLTREE", annot.tree.toString)))
+      Array(api.AnnotationArgument.of("TREE_HASH", treeHash(annot.tree).toString)))
   }
 }

sbt-dotty/sbt-test/source-dependencies/inline-inherited/A.scala

@@ -0,0 +1,10 @@
+class A {
+  inline def getInline: String = {
+    class Local {
+      private val y: Int = 1
+    }
+    println(new Local)
+    val x = 1
+    x.toString
+  }
+}

sbt-dotty/sbt-test/source-dependencies/inline-inherited/B.scala

@@ -0,0 +1 @@
+class B extends A

sbt-dotty/sbt-test/source-dependencies/inline-inherited/C.scala

@@ -0,0 +1,4 @@
+class C {
+  val b = new B
+  b.getInline
+}

sbt-dotty/sbt-test/source-dependencies/inline-inherited/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/inline-inherited/changes/B1.scala

@@ -0,0 +1,3 @@
+class B extends A {
+  val unrelatedChange: Int = 1
+}

sbt-dotty/sbt-test/source-dependencies/inline-inherited/dbg.sbt

@@ -0,0 +1,3 @@
+logLevel := Level.Debug
+incOptions in ThisBuild ~= { _.withApiDebug(true) }
+incOptions in ThisBuild ~= { _.withRelationsDebug(true) }

sbt-dotty/sbt-test/source-dependencies/inline-inherited/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/inline-inherited/project/plugins.sbt

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

sbt-dotty/sbt-test/source-dependencies/inline-inherited/test

@@ -0,0 +1,6 @@
+> compile
+# Force recompilation of B, B.getInline hasn't changed so C shouldn't be recompiled.
+$ copy-file changes/B1.scala B.scala
+> compile
+# One iteration for each call to `compile`
+> checkIterations 2