Partial redesign of incremental compiler invalidation.

We now do the right thing when packages are either newly created or deleted. Previously there was a problem when a new package was created inside a system package (and, unofrtunately, root is a system package). That's fixed now. We also approximate more tightly now when packages are newly created (iei the newly created symbol gets rescanned, instead of its owner).

Incremental class invalidation: dealing with empty package.

The compiler can now also invalidate the empty package. Previously, no invalidation was done because empty was identified with root, which is considered a system package.

(1) Fixed NPE when creating a new toplevel package in invalidation. (2) generalized interface to deal with multiple entries at a time.

6 files changed, 158 insertions, 81 deletions

diff --git a/src/compiler/scala/tools/nsc/Global.scala b/src/compiler/scala/tools/nsc/Global.scala
index 9089be5cc0..ceb75b575d 100644
--- a/src/compiler/scala/tools/nsc/Global.scala
+++ b/src/compiler/scala/tools/nsc/Global.scala
@@ -12,7 +12,7 @@ import scala.tools.util.PathResolver
 import scala.collection.{ mutable, immutable }
 import io.{ SourceReader, AbstractFile, Path }
 import reporters.{ Reporter, ConsoleReporter }
-import util.{ NoPosition, Exceptional, ClassPath, SourceFile, NoSourceFile, Statistics, StatisticsInfo, BatchSourceFile, ScriptSourceFile, ScalaClassLoader, returning }
+import util.{ NoPosition, Exceptional, ClassPath, MergedClassPath, SourceFile, NoSourceFile, Statistics, StatisticsInfo, BatchSourceFile, ScriptSourceFile, ScalaClassLoader, returning }
 import scala.reflect.internal.pickling.{ PickleBuffer, PickleFormat }
 import settings.{ AestheticSettings }
 import symtab.{ Flags, SymbolTable, SymbolLoaders, SymbolTrackers }
@@ -73,6 +73,7 @@ class Global(var currentSettings: Settings, var reporter: Reporter) extends Symb
     else new { val global: Global.this.type = Global.this } with JavaPlatform
 
   type PlatformClassPath = ClassPath[platform.BinaryRepr]
+  type OptClassPath = Option[PlatformClassPath]
 
   def classPath: PlatformClassPath = platform.classPath
 
@@ -879,20 +880,38 @@ class Global(var currentSettings: Settings, var reporter: Reporter) extends Symb
    *    - a list of of packages that should have been invalidated but were not because
    *      they are system packages.
    */
-  def invalidateClassPathEntry(path: String): (List[Symbol], List[Symbol]) = {
-    val invalidated, failed = new mutable.ListBuffer[Symbol]
+  def invalidateClassPathEntries(paths: String*): (List[ClassSymbol], List[ClassSymbol]) = {
+    val invalidated, failed = new mutable.ListBuffer[ClassSymbol]
     classPath match {
-      case cp: util.MergedClassPath[_] =>
-        val dir = AbstractFile getDirectory path
-        val canonical = Some(dir.canonicalPath)
-        cp.entries find (_.origin == canonical) match {
-          case Some(oldEntry) =>
-            val newEntry = cp.context.newClassPath(dir)
-            platform.updateClassPath(oldEntry, newEntry)
-            informProgress(s"classpath updated to $classPath")
-            reSync(definitions.RootClass, classPath, oldEntry, newEntry, invalidated, failed)
-          case None =>
-            error(s"cannot invalidate: no entry named $path in classpath $classPath")
+      case cp: MergedClassPath[_] =>
+        def assoc(path: String): List[(PlatformClassPath, PlatformClassPath)] = {
+          val dir = AbstractFile getDirectory path
+          val canonical = dir.canonicalPath
+          def matchesCanonical(e: ClassPath[_]) = e.origin match {
+            case Some(opath) =>
+              (AbstractFile getDirectory opath).canonicalPath == canonical
+            case None =>
+              false
+          }
+          cp.entries find matchesCanonical match {
+            case Some(oldEntry) =>
+              List(oldEntry -> cp.context.newClassPath(dir))
+            case None =>
+              println(s"canonical = $canonical, origins = ${cp.entries map (_.origin)}")
+              error(s"cannot invalidate: no entry named $path in classpath $classPath")
+              List()
+          }
+        }
+        val subst = Map(paths flatMap assoc: _*)
+        if (subst.nonEmpty) {
+          platform updateClassPath subst
+          informProgress(s"classpath updated on entries [${subst.keys mkString ","}]")
+          def mkClassPath(elems: Iterable[PlatformClassPath]): PlatformClassPath =
+            if (elems.size == 1) elems.head
+            else new MergedClassPath(elems, classPath.context)
+          val oldEntries = mkClassPath(subst.keys)
+          val newEntries = mkClassPath(subst.values)
+          reSync(definitions.RootClass, Some(classPath), Some(oldEntries), Some(newEntries), invalidated, failed)
         }
     }
     def show(msg: String, syms: collection.Traversable[Symbol]) =
@@ -903,27 +922,76 @@ class Global(var currentSettings: Settings, var reporter: Reporter) extends Symb
     (invalidated.toList, failed.toList)
   }
 
-  private def reSync(root: Symbol, all: PlatformClassPath,
-             oldEntry: PlatformClassPath, newEntry: PlatformClassPath,
-             invalidated: mutable.ListBuffer[Symbol], failed: mutable.ListBuffer[Symbol]) {
-    ifDebug(informProgress(s"syncing $root, $oldEntry -> $newEntry"))
+  /** Re-syncs symbol table with classpath
+   *  @param root         The root symbol to be resynced (a package class)
+   *  @param allEntries   Optionally, the corresponding package in the complete current classPath
+   *  @param oldEntries   Optionally, the corresponding package in the old classPath entries
+   *  @param newEntries   Optionally, the corresponding package in the new classPath entries
+   *  @param invalidated  A listbuffer collecting the invalidated package classes
+   *  @param failed       A listbuffer collecting system package classes which could not be invalidated
+   * The resyncing strategy is determined by the absence or presence of classes and packages.
+   * If either oldEntries or newEntries contains classes, root is invalidated, provided a corresponding package
+   * exists in allEntries, or otherwise is removed.
+   * Otherwise, the action is determined by the following matrix, with columns:
+   *
+   *      old new all sym   action
+   *       +   +   +   +    recurse into all child packages of old ++ new
+   *       +   -   +   +    invalidate root
+   *       +   -   -   +    remove root from its scope
+   *       -   +   +   +    invalidate root
+   *       -   +   +   -    create and enter root
+   *       -   -   *   *    no action
+   *
+   *  Here, old, new, all mean classpaths and sym means symboltable. + is presence of an
+   *  entry in its column, - is absence, * is don't care.
+   *
+   *  Note that new <= all and old <= sym, so the matrix above covers all possibilities.
+   */
+  private def reSync(root: ClassSymbol,
+             allEntries: OptClassPath, oldEntries: OptClassPath, newEntries: OptClassPath,
+             invalidated: mutable.ListBuffer[ClassSymbol], failed: mutable.ListBuffer[ClassSymbol]) {
+    ifDebug(informProgress(s"syncing $root, $oldEntries -> $newEntries"))
+
     val getName: ClassPath[platform.BinaryRepr] => String = (_.name)
-    val oldPackages = oldEntry.packages sortBy getName
-    val newPackages = newEntry.packages sortBy getName
-    val hasChanged =
-      oldEntry.classes.nonEmpty ||
-      newEntry.classes.nonEmpty ||
-      (oldPackages map getName) != (newPackages map getName)
-    if (hasChanged && !isSystemPackageClass(root)) {
-      root setInfo new loaders.PackageLoader(all)
+    def hasClasses(cp: OptClassPath) = cp.isDefined && cp.get.classes.nonEmpty
+    def invalidateOrRemove(root: ClassSymbol) = {
+      allEntries match {
+        case Some(cp) => root setInfo new loaders.PackageLoader(cp)
+        case None => root.owner.info.decls unlink root.sourceModule
+      }
       invalidated += root
+    }
+    def packageNames(cp: PlatformClassPath): Set[String] = cp.packages.toSet map getName
+    def subPackage(cp: PlatformClassPath, name: String): OptClassPath =
+      cp.packages find (cp1 => getName(cp1) == name)
+
+    val classesFound = hasClasses(oldEntries) || hasClasses(newEntries)
+    if (classesFound && !isSystemPackageClass(root)) {
+      invalidateOrRemove(root)
     } else {
-      if (hasChanged) failed += root
-      for ((oldNested, newNested) <- oldPackages zip newPackages) {
-        val pkgname = newNested.name
-        val pkg = root.info decl newTermName(pkgname)
-        val allNested = (all.packages find (_.name == pkgname)).get
-        reSync(pkg.moduleClass, allNested, oldNested, newNested, invalidated, failed)
+      if (classesFound) {
+        if (root.isRoot) invalidateOrRemove(definitions.EmptyPackageClass)
+        else failed += root
+      }
+      (oldEntries, newEntries) match {
+        case (Some(oldcp) , Some(newcp)) =>
+          for (pstr <- packageNames(oldcp) ++ packageNames(newcp)) {
+            val pname = newTermName(pstr)
+            val pkg = (root.info decl pname) orElse {
+              // package was created by external agent, create symbol to track it
+              assert(!subPackage(oldcp, pstr).isDefined)
+              loaders.enterPackage(root, pstr, new loaders.PackageLoader(allEntries.get))
+            }
+            reSync(
+                pkg.moduleClass.asInstanceOf[ClassSymbol],
+                subPackage(allEntries.get, pstr), subPackage(oldcp, pstr), subPackage(newcp, pstr),
+                invalidated, failed)
+          }
+        case (Some(oldcp), None) =>
+          invalidateOrRemove(root)
+        case (None, Some(newcp)) =>
+          invalidateOrRemove(root)
+        case (None, None) =>
       }
     }
   }
@@ -931,7 +999,7 @@ class Global(var currentSettings: Settings, var reporter: Reporter) extends Symb
   /** Invalidate contents of setting -Yinvalidate */
   def doInvalidation() = settings.Yinvalidate.value match {
     case "" =>
-    case entry => invalidateClassPathEntry(entry)
+    case entry => invalidateClassPathEntries(entry)
   }
 
   // ----------- Runs ---------------------------------------
@@ -953,7 +1021,7 @@ class Global(var currentSettings: Settings, var reporter: Reporter) extends Symb
    *
    *  @param    sym A class symbol, object symbol, package, or package class.
    */
-  @deprecated("use invalidateClassPathEntry instead")
+  @deprecated("use invalidateClassPathEntries instead")
   def clearOnNextRun(sym: Symbol) = false
     /* To try out clearOnNext run on the scala.tools.nsc project itself
      * replace `false` above with the following code
@@ -1210,7 +1278,7 @@ class Global(var currentSettings: Settings, var reporter: Reporter) extends Symb
     /** Reset all classes contained in current project, as determined by
      *  the clearOnNextRun hook
      */
-    @deprecated("use invalidateClassPathEntry instead")
+    @deprecated("use invalidateClassPathEntries instead")
     def resetProjectClasses(root: Symbol): Unit = try {
       def unlink(sym: Symbol) =
         if (sym != NoSymbol) root.info.decls.unlink(sym)
@@ -1443,6 +1511,7 @@ class Global(var currentSettings: Settings, var reporter: Reporter) extends Symb
     def compileUnits(units: List[CompilationUnit], fromPhase: Phase) {
       try compileUnitsInternal(units, fromPhase)
       catch { case ex =>
+        // ex.printStackTrace(Console.out) // DEBUG for fsc, note that error stacktraces do not print in fsc
         globalError(supplementErrorMessage("uncaught exception during compilation: " + ex.getClass.getName))
         throw ex
       }
diff --git a/src/compiler/scala/tools/nsc/backend/JavaPlatform.scala b/src/compiler/scala/tools/nsc/backend/JavaPlatform.scala
index e5e67e8811..62885cc73d 100644
--- a/src/compiler/scala/tools/nsc/backend/JavaPlatform.scala
+++ b/src/compiler/scala/tools/nsc/backend/JavaPlatform.scala
@@ -23,10 +23,10 @@ trait JavaPlatform extends Platform {
     if (currentClassPath.isEmpty) currentClassPath = Some(new PathResolver(settings).result)
     currentClassPath.get
   }
-    
+
   /** Update classpath with a substituted subentry */
-  def updateClassPath(oldEntry: ClassPath[BinaryRepr], newEntry: ClassPath[BinaryRepr]) = 
-    currentClassPath = Some(new DeltaClassPath(currentClassPath.get, oldEntry, newEntry))
+  def updateClassPath(subst: Map[ClassPath[BinaryRepr], ClassPath[BinaryRepr]]) =
+    currentClassPath = Some(new DeltaClassPath(currentClassPath.get, subst))
 
   def rootLoader = new loaders.PackageLoader(classPath.asInstanceOf[ClassPath[platform.BinaryRepr]])
     // [Martin] Why do we need a cast here?
diff --git a/src/compiler/scala/tools/nsc/backend/MSILPlatform.scala b/src/compiler/scala/tools/nsc/backend/MSILPlatform.scala
index 30447e1274..f4176c46b8 100644
--- a/src/compiler/scala/tools/nsc/backend/MSILPlatform.scala
+++ b/src/compiler/scala/tools/nsc/backend/MSILPlatform.scala
@@ -30,11 +30,11 @@ trait MSILPlatform extends Platform {
   lazy val classPath = MsilClassPath.fromSettings(settings)
   def rootLoader = new loaders.PackageLoader(classPath.asInstanceOf[ClassPath[platform.BinaryRepr]])
     // See discussion in JavaPlatForm for why we need a cast here.
-  
+
   /** Update classpath with a substituted subentry */
-  def updateClassPath(oldEntry: ClassPath[BinaryRepr], newEntry: ClassPath[BinaryRepr]) = 
+  def updateClassPath(subst: Map[ClassPath[BinaryRepr], ClassPath[BinaryRepr]]) =
     throw new UnsupportedOperationException("classpath invalidations not supported on MSIL")
-  
+
   def platformPhases = List(
     genMSIL   // generate .msil files
   )
diff --git a/src/compiler/scala/tools/nsc/backend/Platform.scala b/src/compiler/scala/tools/nsc/backend/Platform.scala
index f97d3e3d56..f770713093 100644
--- a/src/compiler/scala/tools/nsc/backend/Platform.scala
+++ b/src/compiler/scala/tools/nsc/backend/Platform.scala
@@ -23,9 +23,9 @@ trait Platform {
 
   /** The root symbol loader. */
   def rootLoader: LazyType
-  
-  /** Update classpath with a substituted subentry */
-  def updateClassPath(oldEntry: ClassPath[BinaryRepr], newEntry: ClassPath[BinaryRepr])
+
+  /** Update classpath with a substitution that maps entries to entries */
+  def updateClassPath(subst: Map[ClassPath[BinaryRepr], ClassPath[BinaryRepr]])
 
   /** Any platform-specific phases. */
   def platformPhases: List[SubComponent]
diff --git a/src/compiler/scala/tools/nsc/symtab/SymbolLoaders.scala b/src/compiler/scala/tools/nsc/symtab/SymbolLoaders.scala
index 390b1a5ec6..15edac16d5 100644
--- a/src/compiler/scala/tools/nsc/symtab/SymbolLoaders.scala
+++ b/src/compiler/scala/tools/nsc/symtab/SymbolLoaders.scala
@@ -51,6 +51,46 @@ abstract class SymbolLoaders {
     enterIfNew(owner, module, completer)
   }
 
+  /** Enter package with given `name` into scope of `root`
+   *  and give them `completer` as type.
+   */
+  def enterPackage(root: Symbol, name: String, completer: SymbolLoader): Symbol = {
+    val pname = newTermName(name)
+    val preExisting = root.info.decls lookup pname
+    if (preExisting != NoSymbol) {
+      // Some jars (often, obfuscated ones) include a package and
+      // object with the same name. Rather than render them unusable,
+      // offer a setting to resolve the conflict one way or the other.
+      // This was motivated by the desire to use YourKit probes, which
+      // require yjp.jar at runtime. See SI-2089.
+      if (settings.termConflict.isDefault)
+        throw new TypeError(
+          root+" contains object and package with same name: "+
+          name+"\none of them needs to be removed from classpath"
+        )
+      else if (settings.termConflict.value == "package") {
+        global.warning(
+          "Resolving package/object name conflict in favor of package " +
+          preExisting.fullName + ".  The object will be inaccessible."
+        )
+        root.info.decls.unlink(preExisting)
+      }
+      else {
+        global.warning(
+          "Resolving package/object name conflict in favor of object " +
+          preExisting.fullName + ".  The package will be inaccessible."
+        )
+        return NoSymbol
+      }
+    }
+    // todo: find out initialization sequence for pkg/pkg.moduleClass is different from enterModule
+    val pkg = root.newPackage(pname)
+    pkg.moduleClass setInfo completer
+    pkg setInfo pkg.moduleClass.tpe
+    root.info.decls enter pkg
+    pkg
+  }
+
   /** Enter class and module with given `name` into scope of `root`
    *  and give them `completer` as type.
    */
@@ -171,40 +211,6 @@ abstract class SymbolLoaders {
   class PackageLoader(classpath: ClassPath[platform.BinaryRepr]) extends SymbolLoader {
     protected def description = "package loader "+ classpath.name
 
-    def enterPackage(root: Symbol, name: String, completer: SymbolLoader) {
-      val preExisting = root.info.decls.lookup(newTermName(name))
-      if (preExisting != NoSymbol) {
-        // Some jars (often, obfuscated ones) include a package and
-        // object with the same name. Rather than render them unusable,
-        // offer a setting to resolve the conflict one way or the other.
-        // This was motivated by the desire to use YourKit probes, which
-        // require yjp.jar at runtime. See SI-2089.
-        if (settings.termConflict.isDefault)
-          throw new TypeError(
-            root+" contains object and package with same name: "+
-            name+"\none of them needs to be removed from classpath"
-          )
-        else if (settings.termConflict.value == "package") {
-          global.warning(
-            "Resolving package/object name conflict in favor of package " +
-            preExisting.fullName + ".  The object will be inaccessible."
-          )
-          root.info.decls.unlink(preExisting)
-        }
-        else {
-          global.warning(
-            "Resolving package/object name conflict in favor of object " +
-            preExisting.fullName + ".  The package will be inaccessible."
-          )
-          return
-        }
-      }
-      val pkg = root.newPackage(newTermName(name))
-      pkg.moduleClass.setInfo(completer)
-      pkg.setInfo(pkg.moduleClass.tpe)
-      root.info.decls.enter(pkg)
-    }
-
     protected def doComplete(root: Symbol) {
       assert(root.isPackageClass, root)
       root.setInfo(new PackageClassInfoType(newScope, root))
diff --git a/src/compiler/scala/tools/nsc/util/ClassPath.scala b/src/compiler/scala/tools/nsc/util/ClassPath.scala
index 18b9c7dd01..472b5180b4 100644
--- a/src/compiler/scala/tools/nsc/util/ClassPath.scala
+++ b/src/compiler/scala/tools/nsc/util/ClassPath.scala
@@ -312,9 +312,11 @@ class DirectoryClassPath(val dir: AbstractFile, val context: ClassPathContext[Ab
   override def toString() = "directory classpath: "+ origin.getOrElse("?")
 }
 
-class DeltaClassPath[T](original: MergedClassPath[T], oldEntry: ClassPath[T], newEntry: ClassPath[T]) 
-extends MergedClassPath[T](original.entries map (e => if (e == oldEntry) newEntry else e), original.context) {
-  require(original.entries contains oldEntry)
+class DeltaClassPath[T](original: MergedClassPath[T], subst: Map[ClassPath[T], ClassPath[T]])
+extends MergedClassPath[T](original.entries map (e => subst getOrElse (e, e)), original.context) {
+  // not sure we should require that here. Commented out for now.
+  // require(subst.keySet subsetOf original.entries.toSet)
+  // We might add specialized operations for computing classes packages here. Not sure it's worth it.
 }
 
 /**