Skip to content

Commit

Permalink
fix scala#11861, mix in nested inline def or val
Browse files Browse the repository at this point in the history
  • Loading branch information
bishabosha committed Dec 15, 2021
1 parent 9d844b3 commit 641a8bb
Show file tree
Hide file tree
Showing 70 changed files with 656 additions and 39 deletions.
130 changes: 103 additions & 27 deletions compiler/src/dotty/tools/dotc/sbt/ExtractAPI.scala
Original file line number Diff line number Diff line change
Expand Up @@ -599,7 +599,7 @@ private class ExtractAPICollector(using Context) extends ThunkHolder {
// 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)
val inlineBodyHash = treeHash(inlineBody, inlineSym = s)
annots += marker(inlineBodyHash.toString)
}

Expand All @@ -620,27 +620,122 @@ private class ExtractAPICollector(using Context) extends ThunkHolder {
* it should stay the same across compiler runs, compiler instances,
* JVMs, etc.
*/
def treeHash(tree: Tree): Int =
def treeHash(tree: Tree, inlineSym: Symbol): Int =
import scala.util.hashing.MurmurHash3
import core.Constants.*

val seenInlines = mutable.HashSet.empty[Symbol]

if inlineSym ne NoSymbol then
seenInlines += inlineSym // do not hash twice a recursive def

def nameHash(n: Name, initHash: Int): Int =
val h =
if n.isTermName then
MurmurHash3.mix(initHash, TermNameHash)
else
MurmurHash3.mix(initHash, TypeNameHash)

// The hashCode of the name itself is not stable across compiler instances
MurmurHash3.mix(h, n.toString.hashCode)
end nameHash

def typeHash(tp: Type, initHash: Int): Int =
// Go through `apiType` to get a value with a stable hash, it'd
// be better to use Murmur here too instead of relying on
// `hashCode`, but that would essentially mean duplicating
// https://github.com/sbt/zinc/blob/develop/internal/zinc-apiinfo/src/main/scala/xsbt/api/HashAPI.scala
// and at that point we might as well do type hashing on our own
// representation.
var h = initHash
tp match
case ConstantType(c) =>
h = constantHash(c, h)
case TypeBounds(lo, hi) =>
h = MurmurHash3.mix(h, apiType(lo).hashCode)
h = MurmurHash3.mix(h, apiType(hi).hashCode)
case tp =>
h = MurmurHash3.mix(h, apiType(tp).hashCode)
h
end typeHash

def constantHash(c: Constant, initHash: Int): Int =
var h = MurmurHash3.mix(initHash, c.tag)
c.tag match
case NullTag =>
// No value to hash, the tag is enough.
case ClazzTag =>
h = typeHash(c.typeValue, h)
case _ =>
h = MurmurHash3.mix(h, c.value.hashCode)
h
end constantHash

/**An inline method that calls another inline method will eventually inline the call
* at a non-inline callsite, in this case if the implementation of the nested call
* changes, then the callsite will have a different API, we should hash the definition
*/
def inlineReferenceHash(ref: Symbol, rhs: Tree, initHash: Int): Int =
var h = initHash

def paramssHash(paramss: List[List[Symbol]], initHash: Int): Int = paramss match
case Nil :: paramss1 =>
paramssHash(paramss1, MurmurHash3.mix(initHash, EmptyParamHash))
case params :: paramss1 =>
var h = initHash
val paramsIt = params.iterator
while paramsIt.hasNext do
val param = paramsIt.next
h = nameHash(param.name, h)
h = typeHash(param.info, h)
if param.is(Inline) then
h = MurmurHash3.mix(h, InlineParamHash) // inline would change the generated code
paramssHash(paramss1, h)
case Nil =>
initHash
end paramssHash

h = paramssHash(ref.paramSymss, h)
h = typeHash(ref.info.finalResultType, h)
positionedHash(rhs, h)
end inlineReferenceHash

def err(what: String, elem: Any, pos: Positioned, initHash: Int): Int =
internalError(i"Don't know how to produce a stable hash for $what", pos.sourcePos)
MurmurHash3.mix(initHash, elem.toString.hashCode)

def positionedHash(p: ast.Positioned, initHash: Int): Int =
var h = initHash

p match
case p: WithLazyField[?] =>
p.forceIfLazy
case _ =>

p match
case ref: RefTree @unchecked =>
val sym = ref.symbol
if sym.is(Inline, butNot = Param) && !seenInlines.contains(sym) then
seenInlines += sym // dont re-enter hashing this ref
sym.defTree match
case defTree: ValOrDefDef =>
h = inlineReferenceHash(sym, defTree.rhs, h)
case _ =>
h = err(i"inline method reference `${ref.name}`", ref.name, ref, h)
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)
h = MurmurHash3.mix(h, p.productPrefix.hashCode)
iteratorHash(p.productIterator, h)
end positionedHash

def iteratorHash(it: Iterator[Any], initHash: Int): Int =
import core.Constants._
var h = initHash
while it.hasNext do
it.next() match
Expand All @@ -649,30 +744,11 @@ private class ExtractAPICollector(using Context) extends ThunkHolder {
case xs: List[?] =>
h = iteratorHash(xs.iterator, h)
case c: Constant =>
h = MurmurHash3.mix(h, c.tag)
c.tag match
case NullTag =>
// No value to hash, the tag is enough.
case ClazzTag =>
// Go through `apiType` to get a value with a stable hash, it'd
// be better to use Murmur here too instead of relying on
// `hashCode`, but that would essentially mean duplicating
// https://github.com/sbt/zinc/blob/develop/internal/zinc-apiinfo/src/main/scala/xsbt/api/HashAPI.scala
// and at that point we might as well do type hashing on our own
// representation.
val apiValue = apiType(c.typeValue)
h = MurmurHash3.mix(h, apiValue.hashCode)
case _ =>
h = MurmurHash3.mix(h, c.value.hashCode)
h = constantHash(c, h)
case n: Name =>
// The hashCode of the name itself is not stable across compiler instances
h = MurmurHash3.mix(h, n.toString.hashCode)
h = nameHash(n, h)
case elem =>
internalError(
i"Don't know how to produce a stable hash for `$elem` of unknown class ${elem.getClass}",
tree.sourcePos)

h = MurmurHash3.mix(h, elem.toString.hashCode)
h = err(i"`$elem` of unknown class ${elem.getClass}", elem, tree, h)
h
end iteratorHash

Expand All @@ -691,6 +767,6 @@ private class ExtractAPICollector(using Context) extends ThunkHolder {
// annotated @org.junit.Test).
api.Annotation.of(
apiType(annot.tree.tpe), // Used by sbt to find tests to run
Array(api.AnnotationArgument.of("TREE_HASH", treeHash(annot.tree).toString)))
Array(api.AnnotationArgument.of("TREE_HASH", treeHash(annot.tree, inlineSym = NoSymbol).toString)))
}
}
5 changes: 5 additions & 0 deletions compiler/src/dotty/tools/dotc/sbt/package.scala
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ import dotty.tools.dotc.core.Symbols.Symbol
import dotty.tools.dotc.core.NameOps.stripModuleClassSuffix
import dotty.tools.dotc.core.Names.Name

inline val TermNameHash = 1987 // 300th prime
inline val TypeNameHash = 1993 // 301st prime
inline val EmptyParamHash = 1997 // 302nd prime
inline val InlineParamHash = 1999 // 303rd prime

extension (sym: Symbol)

def constructorName(using Context) =
Expand Down
5 changes: 5 additions & 0 deletions sbt-test/source-dependencies/inline-rec-change-inline/A.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
object A {

inline def callInline: Any = B.inlinedAny("yyy")

}
5 changes: 5 additions & 0 deletions sbt-test/source-dependencies/inline-rec-change-inline/B.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
object B {

inline def inlinedAny(x: String): x.type = x

}
3 changes: 3 additions & 0 deletions sbt-test/source-dependencies/inline-rec-change-inline/C.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
class C {
val n = A.callInline
}
25 changes: 25 additions & 0 deletions sbt-test/source-dependencies/inline-rec-change-inline/build.sbt
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import sbt.internal.inc.Analysis
import complete.DefaultParsers._

// Reset compiler iterations, necessary because tests run in batch mode
val recordPreviousIterations = taskKey[Unit]("Record previous iterations.")
recordPreviousIterations := {
val log = streams.value.log
CompileState.previousIterations = {
val previousAnalysis = (previousCompile in Compile).value.analysis.asScala
previousAnalysis match {
case None =>
log.info("No previous analysis detected")
0
case Some(a: Analysis) => a.compilations.allCompilations.size
}
}
}

val checkIterations = inputKey[Unit]("Verifies the accumulated number of iterations of incremental compilation.")

checkIterations := {
val expected: Int = (Space ~> NatBasic).parsed
val actual: Int = ((compile in Compile).value match { case a: Analysis => a.compilations.allCompilations.size }) - CompileState.previousIterations
assert(expected == actual, s"Expected $expected compilations, got $actual")
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
object B {

inline def inlinedAny(inline x: String): x.type = x

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// This is necessary because tests are run in batch mode
object CompileState {
@volatile var previousIterations: Int = -1
}
Original file line number Diff line number Diff line change
@@ -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")
)
}
9 changes: 9 additions & 0 deletions sbt-test/source-dependencies/inline-rec-change-inline/test
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
> compile
> recordPreviousIterations
# Force recompilation of A because B.inlinedAny, called by A.callInline, has added
# the inline flag to one of its parameters.
$ copy-file changes/B1.scala B.scala
> compile
# 1 to recompile B, then 1 more to recompile A due to B.inlinedAny change,
# then 1 final compilation to recompile C due to A.callInline change
> checkIterations 3
5 changes: 5 additions & 0 deletions sbt-test/source-dependencies/inline-rec-change-param/A.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
object A {

inline def callInline: Any = B.inlinedAny("yyy")

}
5 changes: 5 additions & 0 deletions sbt-test/source-dependencies/inline-rec-change-param/B.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
object B {

inline def inlinedAny(x: String): x.type = x

}
3 changes: 3 additions & 0 deletions sbt-test/source-dependencies/inline-rec-change-param/C.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
class C {
val n = A.callInline
}
25 changes: 25 additions & 0 deletions sbt-test/source-dependencies/inline-rec-change-param/build.sbt
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import sbt.internal.inc.Analysis
import complete.DefaultParsers._

// Reset compiler iterations, necessary because tests run in batch mode
val recordPreviousIterations = taskKey[Unit]("Record previous iterations.")
recordPreviousIterations := {
val log = streams.value.log
CompileState.previousIterations = {
val previousAnalysis = (previousCompile in Compile).value.analysis.asScala
previousAnalysis match {
case None =>
log.info("No previous analysis detected")
0
case Some(a: Analysis) => a.compilations.allCompilations.size
}
}
}

val checkIterations = inputKey[Unit]("Verifies the accumulated number of iterations of incremental compilation.")

checkIterations := {
val expected: Int = (Space ~> NatBasic).parsed
val actual: Int = ((compile in Compile).value match { case a: Analysis => a.compilations.allCompilations.size }) - CompileState.previousIterations
assert(expected == actual, s"Expected $expected compilations, got $actual")
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
object B {

inline def inlinedAny(x: Any): x.type = x

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// This is necessary because tests are run in batch mode
object CompileState {
@volatile var previousIterations: Int = -1
}
Original file line number Diff line number Diff line change
@@ -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")
)
}
9 changes: 9 additions & 0 deletions sbt-test/source-dependencies/inline-rec-change-param/test
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
> compile
> recordPreviousIterations
# Force recompilation of A because B.inlinedAny, called by A.callInline, has changed
# the type of its parameters.
$ copy-file changes/B1.scala B.scala
> compile
# 1 to recompile B, then 1 more to recompile A due to B.inlinedAny change,
# then 1 final compilation to recompile C due to A.callInline change
> checkIterations 3
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
object A {

inline def callInline: Any = B.inlinedAny("yyy")

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
object B {

transparent inline def inlinedAny(x: String): x.type = x

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
class C {
val n = A.callInline
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import sbt.internal.inc.Analysis
import complete.DefaultParsers._

// Reset compiler iterations, necessary because tests run in batch mode
val recordPreviousIterations = taskKey[Unit]("Record previous iterations.")
recordPreviousIterations := {
val log = streams.value.log
CompileState.previousIterations = {
val previousAnalysis = (previousCompile in Compile).value.analysis.asScala
previousAnalysis match {
case None =>
log.info("No previous analysis detected")
0
case Some(a: Analysis) => a.compilations.allCompilations.size
}
}
}

val checkIterations = inputKey[Unit]("Verifies the accumulated number of iterations of incremental compilation.")

checkIterations := {
val expected: Int = (Space ~> NatBasic).parsed
val actual: Int = ((compile in Compile).value match { case a: Analysis => a.compilations.allCompilations.size }) - CompileState.previousIterations
assert(expected == actual, s"Expected $expected compilations, got $actual")
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
object B {

transparent inline def inlinedAny(x: String): String = x

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// This is necessary because tests are run in batch mode
object CompileState {
@volatile var previousIterations: Int = -1
}
Loading

0 comments on commit 641a8bb

Please sign in to comment.