Properly support xsbti.compile.IncOptions#useOptimizedSealed

This option is meant to avoid overcompilation involving sealed classes. By
default, Zinc invalidates all usages of a sealed class when a child is added or
removed, even though `val x: Sealed = new Child1` will work the same if
`Sealed` gets an extra child. When this option is on, Zinc only takes children
into account if the name usage is reported with `UseScope.PatMatTarget`. Note
that currently this is only set when traversing the scrutinee of a pattern
match (just like in the Scala 2 implementation), but to safely turn this option
on by default we would need to kept track of all call to `children` and
`sealedDescendants` in the compiler.

There were two issues related to this option before this commit:
- ExtractAPI was unnecessarily extracting the `@Child`
  annotation, so the API hash of a sealed class changed even with
  `useOptimizedSealed` enabled.
- When registering a used name with `UseScope.PatMatTarget` we should also
  register it with `UseScope.Default` because (part of) the scrutinee type
  might not be a sealed class, in which case it won't have
  a name hash for PatMatTarget but should still lead to invalidations.

Since we only ever use two values for the `UseScope` parameter of the
`usedName` callback, we create two vals `DefaultScopes` and `PatMatScopes` that
we reuse for performance.

Author: smarter

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

@@ -676,11 +676,16 @@ private class ExtractAPICollector(using Context) extends ThunkHolder {
 
     // In the Scala2 ExtractAPI phase we only extract annotations that extend
     // StaticAnnotation, but in Dotty we currently pickle all annotations so we
-    // extract everything (except annotations missing from the classpath which
-    // we simply skip over, and inline body annotations which are handled above).
+    // extract everything, except:
+    // - annotations missing from the classpath which we simply skip over
+    // - inline body annotations which are handled above
+    // - the Child annotation since we already extract children via
+    //   `api.ClassLike#childrenOfSealedClass` and adding this annotation would
+    //   lead to overcompilation when using zinc's
+    //   `IncOptions#useOptimizedSealed`.
     s.annotations.foreach { annot =>
       val sym = annot.symbol
-      if sym.exists && sym != defn.BodyAnnot then
+      if sym.exists && sym != defn.BodyAnnot && sym != defn.ChildAnnot then
         annots += apiAnnotation(annot)
     }
 

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

@@ -108,29 +108,6 @@ object ExtractDependencies {
     report.error(em"Internal error in the incremental compiler while compiling ${ctx.compilationUnit.source}: $msg", pos)
 }
 
-/** An object that maintain the set of used names from within a class */
-private final class UsedNamesInClass {
-  private val _names = new mutable.HashMap[Name, EnumSet[UseScope]]
-  def names: collection.Map[Name, EnumSet[UseScope]] = _names
-
-  def update(name: Name, scope: UseScope): Unit = {
-    val scopes = _names.getOrElseUpdate(name, EnumSet.noneOf(classOf[UseScope]))
-    scopes.add(scope)
-  }
-
-  override def toString(): String = {
-    val builder = new StringBuilder
-    names.foreach { case (name, scopes) =>
-      builder.append(name.mangledString)
-      builder.append(" in [")
-      scopes.forEach(scope => builder.append(scope.toString))
-      builder.append("]")
-      builder.append(", ")
-    }
-    builder.toString()
-  }
-}
-
 /** Extract the dependency information of a compilation unit.
  *
  *  To understand why we track the used names see the section "Name hashing
@@ -144,7 +121,7 @@ private class ExtractDependenciesCollector(rec: DependencyRecorder) extends tpd.
 
   private def addMemberRefDependency(sym: Symbol)(using Context): Unit =
     if (!ignoreDependency(sym)) {
-      rec.addUsedName(sym, UseScope.Default)
+      rec.addUsedName(sym)
       // packages have class symbol. Only record them as used names but not dependency
       if (!sym.is(Package)) {
         val enclOrModuleClass = if (sym.is(ModuleVal)) sym.moduleClass else sym.enclosingClass
@@ -202,7 +179,7 @@ private class ExtractDependenciesCollector(rec: DependencyRecorder) extends tpd.
         for sel <- selectors if !sel.isWildcard do
           addImported(sel.name)
           if sel.rename != sel.name then
-            rec.addUsedRawName(sel.rename, UseScope.Default)
+            rec.addUsedRawName(sel.rename)
       case exp @ Export(expr, selectors) =>
         val dep = expr.tpe.classSymbol
         if dep.exists && selectors.exists(_.isWildcard) then
@@ -322,7 +299,7 @@ private class ExtractDependenciesCollector(rec: DependencyRecorder) extends tpd.
     val traverser = new TypeDependencyTraverser {
       def addDependency(symbol: Symbol) =
         if (!ignoreDependency(symbol) && symbol.is(Sealed)) {
-          rec.addUsedName(symbol, UseScope.PatMatTarget)
+          rec.addUsedName(symbol, includeSealedChildren = true)
         }
     }
     traverser.traverse(tpe)
@@ -347,26 +324,80 @@ class DependencyRecorder {
    */
   def usedNames: collection.Map[Symbol, UsedNamesInClass] = _usedNames
 
-  /** Record a reference to the name of `sym` used according to `scope`
-   *  from the current non-local enclosing class.
+  /** Record a reference to the name of `sym` from the current non-local
+   *  enclosing class.
+   *
+   *  @param includeSealedChildren  See documentation of `addUsedRawName`.
    */
-  def addUsedName(sym: Symbol, scope: UseScope)(using Context): Unit =
-    addUsedRawName(sym.zincMangledName, scope)
+  def addUsedName(sym: Symbol, includeSealedChildren: Boolean = false)(using Context): Unit =
+    addUsedRawName(sym.zincMangledName, includeSealedChildren)
 
-  /** Record a reference to `name` used according to `scope`
-   *  from the current non-local enclosing class.
+  /** Record a reference to `name` from the current non-local enclosing class (aka, "from class").
    *
-   *  Most of the time, prefer to sue `addUsedName` which takes
+   *  Most of the time, prefer to use `addUsedName` which takes
    *  care of name mangling.
+   *
+   *  Zinc will use this information to invalidate the current non-local
+   *  enclosing class if something changes in the set of definitions named
+   *  `name` among the possible dependencies of the from class.
+   *
+   *  @param includeSealedChildren  If true, the addition or removal of children
+   *                                to a sealed class called `name` will also
+   *                                invalidate the from class.
+   *                                Note that this only has an effect if zinc's
+   *                                `IncOptions.useOptimizedSealed` is enabled,
+   *                                otherwise the addition or removal of children
+   *                                always lead to invalidation.
+   *
+   *  TODO: If the compiler reported to zinc all usages of
+   *  `SymDenotation#{children,sealedDescendants}` (including from macro code),
+   *  we should be able to turn `IncOptions.useOptimizedSealed` on by default
+   *  safely.
    */
-  def addUsedRawName(name: Name, scope: UseScope)(using Context): Unit = {
+  def addUsedRawName(name: Name, includeSealedChildren: Boolean = false)(using Context): Unit = {
     val fromClass = resolveDependencySource
     if (fromClass.exists) {
       val usedName = _usedNames.getOrElseUpdate(fromClass, new UsedNamesInClass)
-      usedName.update(name, scope)
+      usedName.update(name, includeSealedChildren)
     }
   }
 
+  // The two possible value of `UseScope`. To avoid unnecessary allocations,
+  // we use vals here, but that means we must be careful to never mutate these sets.
+  private val DefaultScopes = EnumSet.of(UseScope.Default)
+  private val PatMatScopes = EnumSet.of(UseScope.Default, UseScope.PatMatTarget)
+
+  /** An object that maintain the set of used names from within a class */
+  final class UsedNamesInClass {
+    /** Each key corresponds to a name used in the class. To understand the meaning
+     *  of the associated value, see the documentation of parameter `includeSealedChildren`
+     *  of `addUsedRawName`.
+     */
+    private val _names = new mutable.HashMap[Name, DefaultScopes.type | PatMatScopes.type]
+
+    def names: collection.Map[Name, EnumSet[UseScope]] = _names
+
+    private[DependencyRecorder] def update(name: Name, includeSealedChildren: Boolean): Unit = {
+      if (includeSealedChildren)
+        _names(name) = PatMatScopes
+      else
+        _names.getOrElseUpdate(name, DefaultScopes)
+    }
+
+    override def toString(): String = {
+      val builder = new StringBuilder
+      names.foreach { case (name, scopes) =>
+        builder.append(name.mangledString)
+        builder.append(" in [")
+        scopes.forEach(scope => builder.append(scope.toString))
+        builder.append("]")
+        builder.append(", ")
+      }
+      builder.toString()
+    }
+  }
+
+
   private val _classDependencies = new mutable.HashSet[ClassDependency]
 
   def classDependencies: Set[ClassDependency] = _classDependencies

sbt-test/source-dependencies/useOptimizedSealed/Sealed.scala

@@ -0,0 +1,2 @@
+sealed trait Sealed
+class Child1 extends Sealed

sbt-test/source-dependencies/useOptimizedSealed/Test.scala

@@ -0,0 +1,3 @@
+class Test {
+  val s: Sealed = new Child1
+}

sbt-test/source-dependencies/useOptimizedSealed/build.sbt

@@ -0,0 +1,29 @@
+scalaVersion := sys.props("plugin.scalaVersion")
+
+ThisBuild / incOptions ~= { _.withUseOptimizedSealed(true) }
+
+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")
+}

sbt-test/source-dependencies/useOptimizedSealed/changes/Sealed1.scala

@@ -0,0 +1,3 @@
+sealed trait Sealed
+class Child1 extends Sealed
+class Child2 extends Sealed

sbt-test/source-dependencies/useOptimizedSealed/changes/Sealed2.scala

@@ -0,0 +1,4 @@
+sealed trait Sealed
+class Child1 extends Sealed
+class Child2 extends Sealed
+class Child3 extends Sealed

sbt-test/source-dependencies/useOptimizedSealed/changes/Test1.scala

@@ -0,0 +1,7 @@
+class Test {
+  def foo(x: Sealed): Int = x match
+    case _: Child1 => 1
+    case _: Child2 => 1
+
+  val s: Sealed = new Child1
+}

sbt-test/source-dependencies/useOptimizedSealed/project/CompileState.scala

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

sbt-test/source-dependencies/useOptimizedSealed/test

@@ -0,0 +1,24 @@
+# Compile Sealed.scala and Test.scala
+> compile
+> recordPreviousIterations
+
+# Add an extra children to Sealed
+$ copy-file changes/Sealed1.scala Sealed.scala
+
+# Only Sealed.scala needs to be recompiled because Test.scala does not
+# match on a value of type `Sealed`.
+> compile
+> checkIterations 1
+
+> clean
+$ copy-file changes/Test1.scala Test.scala
+> compile
+> recordPreviousIterations
+
+# Add an extra children to Sealed again
+$ copy-file changes/Sealed2.scala Sealed.scala
+
+# Test.scala will be recompiled in a second round because it matches
+# on a value of type `Sealed`.
+> compile
+> checkIterations 2