cleans up initialization of runtime reflection

At first I just tried to remove syntheticCoreClasses from missingHook
and put them into the initializer of freshly created mirrors in order to
reduce the non-determinism in mutations of the global symbol table.

And then it didn't work, crashing on me claiming that AnyRef is missing.
Apparently we still need AnyRefClass in missingHook, just because it's
impossible to initialize (i.e. unpickle) ScalaPackageClass without it.

And then it still didn't work, whining about multiple overloaded defs
of some synthetic symbols. That was really tricky, but I figured it out
as well by initializing ScalaPackageClass first before forcing any
synthetic symbols (see the details in comments).

7 files changed, 59 insertions, 29 deletions

diff --git a/src/reflect/scala/reflect/internal/Definitions.scala b/src/reflect/scala/reflect/internal/Definitions.scala
index 6e4ca76382..52873dea12 100644
--- a/src/reflect/scala/reflect/internal/Definitions.scala
+++ b/src/reflect/scala/reflect/internal/Definitions.scala
@@ -1248,8 +1248,24 @@ trait Definitions extends api.StandardDefinitions {
 
     def init() {
       if (isInitialized) return
+      // forcing ScalaPackageClass first thing during startup is important, because syntheticCoreClasses such as AnyRefClass
+      // need to be entered into ScalaPackageClass, which entails calling ScalaPackageClass.info.decls.enter
+      // if ScalaPackageClass isn't initialized by that moment, the following will happen for runtime reflection:
+      // 1) initialization of ScalaPackageClass will trigger unpickling
+      // 2) unpickling will need to load some auxiliary types such as, for example, String
+      // 3) to load String, runtime reflection will call mirrorDefining(classOf[String])
+      // 4) this, in turn, will call runtimeMirror(classOf[String].getClassLoader)
+      // 5) for some classloader configurations, the resulting mirror will be different from rootMirror
+      // 6) in that case, initialization of the resulting mirror will try to import definitions.syntheticCoreClasses into the mirror
+      // 7) this will force all the lazy vals corresponding to syntheticCoreClasses
+      // 8) by that time, the completer of ScalaPackageClass will have already called setInfo on ScalaPackageClass, so there won't be any stack overflow
+      // so far so good, but there's a subtle detail. if forcing of ScalaPackageClass was called by a syntheticCoreClasses lazy val,
+      // then this lazy val will be entered twice: once during step 7 and once when returning from the original call
+      // to avoid this we need to initialize ScalaPackageClass first and only then synthesize the core classes
+      ScalaPackageClass.initialize
       // force initialization of every symbol that is synthesized or hijacked by the compiler
-      val forced = symbolsNotPresentInBytecode
+      val forced1 = symbolsNotPresentInBytecode
+      val forced2 = NoSymbol
       isInitialized = true
     } //init
 
diff --git a/src/reflect/scala/reflect/internal/Mirrors.scala b/src/reflect/scala/reflect/internal/Mirrors.scala
index 4771404b29..80c05666af 100644
--- a/src/reflect/scala/reflect/internal/Mirrors.scala
+++ b/src/reflect/scala/reflect/internal/Mirrors.scala
@@ -239,6 +239,19 @@ trait Mirrors extends api.Mirrors {
 
       RootClass.info.decls enter EmptyPackage
       RootClass.info.decls enter RootPackage
+
+      if (rootOwner != NoSymbol) {
+        // synthetic core classes are only present in root mirrors
+        // because Definitions.scala, which initializes and enters them, only affects rootMirror
+        // therefore we need to enter them manually for non-root mirrors
+        definitions.syntheticCoreClasses foreach (theirSym => {
+          val theirOwner = theirSym.owner
+          assert(theirOwner.isPackageClass, s"theirSym = $theirSym, theirOwner = $theirOwner")
+          val ourOwner = staticPackage(theirOwner.fullName)
+          val ourSym = theirSym // just copy the symbol into our branch of the symbol table
+          ourOwner.info.decls enter ourSym
+        })
+      }
     }
   }
 
diff --git a/src/reflect/scala/reflect/internal/StdNames.scala b/src/reflect/scala/reflect/internal/StdNames.scala
index bcda2bc1ae..09d1e649d9 100644
--- a/src/reflect/scala/reflect/internal/StdNames.scala
+++ b/src/reflect/scala/reflect/internal/StdNames.scala
@@ -265,6 +265,8 @@ trait StdNames {
     final val SourceFileATTR: NameType             = "SourceFile"
     final val SyntheticATTR: NameType              = "Synthetic"
 
+    final val scala_ : NameType = "scala"
+
     def dropSingletonName(name: Name): TypeName = (name dropRight SINGLETON_SUFFIX.length).toTypeName
     def singletonName(name: Name): TypeName     = (name append SINGLETON_SUFFIX).toTypeName
     def implClassName(name: Name): TypeName     = (name append IMPL_CLASS_SUFFIX).toTypeName
diff --git a/src/reflect/scala/reflect/internal/Symbols.scala b/src/reflect/scala/reflect/internal/Symbols.scala
index b980f3d8c2..43832eba7f 100644
--- a/src/reflect/scala/reflect/internal/Symbols.scala
+++ b/src/reflect/scala/reflect/internal/Symbols.scala
@@ -3208,10 +3208,11 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
     def name = nme.NO_NAME
     override def name_=(n: Name) = abort("Cannot set NoSymbol's name to " + n)
 
-    synchronized {
-      setInfo(NoType)
-      privateWithin = this
-    }
+    // Syncnote: no need to synchronize this, because NoSymbol's initialization is triggered by JavaUniverse.init
+    // which is called in universe's constructor - something that's inherently single-threaded
+    setInfo(NoType)
+    privateWithin = this
+
     override def info_=(info: Type) = {
       infos = TypeHistory(1, NoType, null)
       unlock()
diff --git a/src/reflect/scala/reflect/runtime/JavaMirrors.scala b/src/reflect/scala/reflect/runtime/JavaMirrors.scala
index b38b7d0f94..a61e3f7b83 100644
--- a/src/reflect/scala/reflect/runtime/JavaMirrors.scala
+++ b/src/reflect/scala/reflect/runtime/JavaMirrors.scala
@@ -46,16 +46,6 @@ private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUni
 
   trait JavaClassCompleter extends FlagAssigningCompleter
 
-  def init() = {
-    definitions.AnyValClass // force it.
-
-    // establish root association to avoid cyclic dependency errors later
-    rootMirror.classToScala(classOf[java.lang.Object]).initialize
-
-    // println("initializing definitions")
-    definitions.init()
-  }
-
   def runtimeMirror(cl: ClassLoader): Mirror = mirrors get cl match {
     case Some(WeakReference(m)) => m
     case _ => createMirror(rootMirror.RootClass, cl)
@@ -1251,11 +1241,6 @@ private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUni
     case _ => abort(s"${sym}.enclosingRootClass = ${sym.enclosingRootClass}, which is not a RootSymbol")
   }
 
-  private lazy val syntheticCoreClasses: Map[(String, Name), Symbol] = {
-    def mapEntry(sym: Symbol): ((String, Name), Symbol) = (sym.owner.fullName, sym.name) -> sym
-    Map() ++ (definitions.syntheticCoreClasses map mapEntry)
-  }
-
   /** 1. If `owner` is a package class (but not the empty package) and `name` is a term name, make a new package
    *  <owner>.<name>, otherwise return NoSymbol.
    *  Exception: If owner is root and a java class with given name exists, create symbol in empty package instead
@@ -1272,15 +1257,15 @@ private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUni
       if (name.isTermName && !owner.isEmptyPackageClass)
         return mirror.makeScalaPackage(
           if (owner.isRootSymbol) name.toString else owner.fullName+"."+name)
-      syntheticCoreClasses get (owner.fullName, name) match {
-        case Some(tsym) =>
-          // synthetic core classes are only present in root mirrors
-          // because Definitions.scala, which initializes and enters them, only affects rootMirror
-          // therefore we need to enter them manually for non-root mirrors
-          if (mirror ne thisUniverse.rootMirror) owner.info.decls enter tsym
-          return tsym
-        case None =>
-      }
+      if (name == tpnme.AnyRef && owner.owner.isRoot && owner.name == tpnme.scala_)
+        // when we synthesize the scala.AnyRef symbol, we need to add it to the scope of the scala package
+        // the problem is that adding to the scope implies doing something like `owner.info.decls enter anyRef`
+        // which entails running a completer for the scala package
+        // which will try to unpickle the stuff in scala/package.class
+        // which will transitively load scala.AnyRef
+        // which doesn't exist yet, because it hasn't been added to the scope yet
+        // this missing hook ties the knot without introducing synchronization problems like before
+        return definitions.AnyRefClass
     }
     info("*** missing: "+name+"/"+name.isTermName+"/"+owner+"/"+owner.hasPackageFlag+"/"+owner.info.decls.getClass)
     super.missingHook(owner, name)
diff --git a/src/reflect/scala/reflect/runtime/JavaUniverse.scala b/src/reflect/scala/reflect/runtime/JavaUniverse.scala
index 1b69ca4e89..7321214a65 100644
--- a/src/reflect/scala/reflect/runtime/JavaUniverse.scala
+++ b/src/reflect/scala/reflect/runtime/JavaUniverse.scala
@@ -24,5 +24,9 @@ class JavaUniverse extends internal.SymbolTable with ReflectSetup with runtime.S
   def newLazyTreeCopier: TreeCopier = new LazyTreeCopier
 
   init()
+
+  def init() {
+    definitions.init()
+  }
 }
 
diff --git a/src/reflect/scala/reflect/runtime/SymbolLoaders.scala b/src/reflect/scala/reflect/runtime/SymbolLoaders.scala
index 23c0e1889f..484053640f 100644
--- a/src/reflect/scala/reflect/runtime/SymbolLoaders.scala
+++ b/src/reflect/scala/reflect/runtime/SymbolLoaders.scala
@@ -76,6 +76,15 @@ private[reflect] trait SymbolLoaders { self: SymbolTable =>
   class PackageScope(pkgClass: Symbol) extends Scope(initFingerPrints = -1L) // disable fingerprinting as we do not know entries beforehand
       with SynchronizedScope {
     assert(pkgClass.isType)
+
+    // materializing multiple copies of the same symbol in PackageScope is a very popular bug
+    // this override does its best to guard against it
+    override def enter[T <: Symbol](sym: T): T = {
+      val existing = super.lookupEntry(sym.name)
+      assert(existing == null || existing.sym.isMethod, s"pkgClass = $pkgClass, sym = $sym, existing = $existing")
+      super.enter(sym)
+    }
+
     // disable fingerprinting as we do not know entries beforehand
     private val negatives = mutable.Set[Name]() // Syncnote: Performance only, so need not be protected.
     override def lookupEntry(name: Name): ScopeEntry = {