SI-8131 fixes residual race condition in runtime reflection

Apparently some completers can call setInfo while they’re not yet done,
which resets the LOCKED flag, and makes anything that uses LOCKED to
track completion unreliable. Unfortunately, that’s exactly the mechanism
that was used by runtime reflection to elide locking for symbols that are
known to be initialized.

This commit fixes the problematic lock elision strategy by introducing
an explicit communication channel between SynchronizedSymbol’s and their
completers. Now instead of trying hard to infer whether it’s already
initialized or not, every symbol gets a volatile field that can be queried
to provide necessary information.

Author: xeno-by

src/reflect/scala/reflect/internal/BuildUtils.scala

@@ -33,19 +33,19 @@ trait BuildUtils { self: SymbolTable =>
     }
 
     def newFreeTerm(name: String, value: => Any, flags: Long = 0L, origin: String = null): FreeTermSymbol =
-      newFreeTermSymbol(newTermName(name), value, flags, origin)
+      newFreeTermSymbol(newTermName(name), value, flags, origin).markFlagsCompleted(mask = AllFlags)
 
     def newFreeType(name: String, flags: Long = 0L, origin: String = null): FreeTypeSymbol =
-      newFreeTypeSymbol(newTypeName(name), flags, origin)
+      newFreeTypeSymbol(newTypeName(name), flags, origin).markFlagsCompleted(mask = AllFlags)
 
     def newNestedSymbol(owner: Symbol, name: Name, pos: Position, flags: Long, isClass: Boolean): Symbol =
-      owner.newNestedSymbol(name, pos, flags, isClass)
+      owner.newNestedSymbol(name, pos, flags, isClass).markFlagsCompleted(mask = AllFlags)
 
     def setAnnotations[S <: Symbol](sym: S, annots: List[AnnotationInfo]): S =
       sym.setAnnotations(annots)
 
     def setTypeSignature[S <: Symbol](sym: S, tpe: Type): S =
-      sym.setTypeSignature(tpe)
+      sym.setTypeSignature(tpe).markAllCompleted()
 
     def This(sym: Symbol): Tree = self.This(sym)
 

src/reflect/scala/reflect/internal/Definitions.scala

@@ -30,12 +30,12 @@ trait Definitions extends api.StandardDefinitions {
 
   private def enterNewClass(owner: Symbol, name: TypeName, parents: List[Type], flags: Long = 0L): ClassSymbol = {
     val clazz = owner.newClassSymbol(name, NoPosition, flags)
-    clazz setInfoAndEnter ClassInfoType(parents, newScope, clazz)
+    clazz setInfoAndEnter ClassInfoType(parents, newScope, clazz) markAllCompleted
   }
   private def newMethod(owner: Symbol, name: TermName, formals: List[Type], restpe: Type, flags: Long): MethodSymbol = {
     val msym   = owner.newMethod(name.encode, NoPosition, flags)
     val params = msym.newSyntheticValueParams(formals)
-    msym setInfo MethodType(params, restpe)
+    msym setInfo MethodType(params, restpe) markAllCompleted
   }
   private def enterNewMethod(owner: Symbol, name: TermName, formals: List[Type], restpe: Type, flags: Long = 0L): MethodSymbol =
     owner.info.decls enter newMethod(owner, name, formals, restpe, flags)
@@ -251,8 +251,8 @@ trait Definitions extends api.StandardDefinitions {
     }
 
     // top types
-    lazy val AnyClass    = enterNewClass(ScalaPackageClass, tpnme.Any, Nil, ABSTRACT)
-    lazy val AnyRefClass = newAlias(ScalaPackageClass, tpnme.AnyRef, ObjectTpe)
+    lazy val AnyClass    = enterNewClass(ScalaPackageClass, tpnme.Any, Nil, ABSTRACT) markAllCompleted
+    lazy val AnyRefClass = newAlias(ScalaPackageClass, tpnme.AnyRef, ObjectTpe) markAllCompleted
     lazy val ObjectClass = getRequiredClass(sn.Object.toString)
 
     // Cached types for core monomorphic classes
@@ -275,7 +275,7 @@ trait Definitions extends api.StandardDefinitions {
       val anyval    = enterNewClass(ScalaPackageClass, tpnme.AnyVal, AnyTpe :: Nil, ABSTRACT)
       val av_constr = anyval.newClassConstructor(NoPosition)
       anyval.info.decls enter av_constr
-      anyval
+      anyval markAllCompleted
     }).asInstanceOf[ClassSymbol]
       def AnyVal_getClass = getMemberMethod(AnyValClass, nme.getClass_)
 
@@ -287,8 +287,10 @@ trait Definitions extends api.StandardDefinitions {
       locally {
         this initFlags ABSTRACT | FINAL
         this setInfoAndEnter ClassInfoType(List(parent.tpe), newScope, this)
+        this markAllCompleted
       }
       final override def isBottomClass = true
+      final override def isThreadsafe(purpose: SymbolOps): Boolean = true
     }
     final object NothingClass extends BottomClassSymbol(tpnme.Nothing, AnyClass) {
       override def isSubClass(that: Symbol) = true
@@ -357,7 +359,7 @@ trait Definitions extends api.StandardDefinitions {
       def delayedInitMethod = getMemberMethod(DelayedInitClass, nme.delayedInit)
 
     lazy val TypeConstraintClass   = requiredClass[scala.annotation.TypeConstraint]
-    lazy val SingletonClass        = enterNewClass(ScalaPackageClass, tpnme.Singleton, AnyTpe :: Nil, ABSTRACT | TRAIT | FINAL)
+    lazy val SingletonClass        = enterNewClass(ScalaPackageClass, tpnme.Singleton, AnyTpe :: Nil, ABSTRACT | TRAIT | FINAL) markAllCompleted
     lazy val SerializableClass     = requiredClass[scala.Serializable]
     lazy val JavaSerializableClass = requiredClass[java.io.Serializable] modifyInfo fixupAsAnyTrait
     lazy val ComparableClass       = requiredClass[java.lang.Comparable[_]] modifyInfo fixupAsAnyTrait
@@ -1126,6 +1128,7 @@ trait Definitions extends api.StandardDefinitions {
     lazy val AnnotationDefaultAttr: ClassSymbol = {
       val sym = RuntimePackageClass.newClassSymbol(tpnme.AnnotationDefaultATTR, NoPosition, 0L)
       sym setInfo ClassInfoType(List(AnnotationClass.tpe), newScope, sym)
+      markAllCompleted(sym)
       RuntimePackageClass.info.decls.toList.filter(_.name == sym.name) match {
         case existing :: _ =>
           existing.asInstanceOf[ClassSymbol]
@@ -1225,7 +1228,7 @@ trait Definitions extends api.StandardDefinitions {
       val tparam  = clazz.newSyntheticTypeParam("T0", flags)
       val parents = List(AnyRefTpe, parentFn(tparam))
 
-      clazz setInfo GenPolyType(List(tparam), ClassInfoType(parents, newScope, clazz))
+      clazz setInfo GenPolyType(List(tparam), ClassInfoType(parents, newScope, clazz)) markAllCompleted
     }
 
     def newPolyMethod(typeParamCount: Int, owner: Symbol, name: TermName, flags: Long)(createFn: PolyMethodCreator): MethodSymbol = {
@@ -1236,7 +1239,7 @@ trait Definitions extends api.StandardDefinitions {
         case (_, restpe)             => NullaryMethodType(restpe)
       }
 
-      msym setInfoAndEnter genPolyType(tparams, mtpe)
+      msym setInfoAndEnter genPolyType(tparams, mtpe) markAllCompleted
     }
 
     /** T1 means one type parameter.

src/reflect/scala/reflect/internal/Importers.scala

@@ -4,6 +4,7 @@ package internal
 
 import scala.collection.mutable.WeakHashMap
 import scala.ref.WeakReference
+import scala.reflect.internal.Flags._
 
 // SI-6241: move importers to a mirror
 trait Importers extends api.Importers { to: SymbolTable =>
@@ -87,6 +88,7 @@ trait Importers extends api.Importers { to: SymbolTable =>
             }
             my setInfo GenPolyType(mytypeParams, importType(theirCore))
             my setAnnotations (their.annotations map importAnnotationInfo)
+            markAllCompleted(my)
           }
         }
       } finally {
@@ -142,6 +144,7 @@ trait Importers extends api.Importers { to: SymbolTable =>
           myowner.newTypeSymbol(myname.toTypeName, mypos, myflags)
       }
       symMap.weakUpdate(their, my)
+      markFlagsCompleted(my)(mask = AllFlags)
       my setInfo recreatedSymbolCompleter(my, their)
     }
 

src/reflect/scala/reflect/internal/Symbols.scala

@@ -55,16 +55,6 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
   def newFreeTypeSymbol(name: TypeName, flags: Long = 0L, origin: String): FreeTypeSymbol =
     new FreeTypeSymbol(name, origin) initFlags flags
 
-  /** Determines whether the given information request should trigger the given symbol's completer.
-   *  See comments to `Symbol.needsInitialize` for details.
-   */
-  protected def shouldTriggerCompleter(symbol: Symbol, completer: Type, isFlagRelated: Boolean, mask: Long) =
-    completer match {
-      case null => false
-      case _: FlagAgnosticCompleter => !isFlagRelated
-      case _ => abort(s"unsupported completer: $completer of class ${if (completer != null) completer.getClass else null} for symbol ${symbol.fullName}")
-    }
-
   /** The original owner of a class. Used by the backend to generate
    *  EnclosingMethod attributes.
    */
@@ -106,12 +96,14 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
     }
 
     def knownDirectSubclasses = {
-      if (!isCompilerUniverse && needsInitialize(isFlagRelated = false, mask = 0)) initialize
+      // See `getFlag` to learn more about the `isThreadsafe` call in the body of this method.
+      if (!isCompilerUniverse && !isThreadsafe(purpose = AllOps)) initialize
       children
     }
 
     def selfType = {
-      if (!isCompilerUniverse && needsInitialize(isFlagRelated = false, mask = 0)) initialize
+      // See `getFlag` to learn more about the `isThreadsafe` call in the body of this method.
+      if (!isCompilerUniverse && !isThreadsafe(purpose = AllOps)) initialize
       typeOfThis
     }
 
@@ -147,6 +139,11 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
              with Annotatable[Symbol]
              with Attachable {
 
+    // makes sure that all symbols that runtime reflection deals with are synchronized
+    private def isSynchronized = this.isInstanceOf[scala.reflect.runtime.SynchronizedSymbols#SynchronizedSymbol]
+    private def isAprioriThreadsafe = isThreadsafe(AllOps)
+    assert(isCompilerUniverse || isSynchronized || isAprioriThreadsafe, s"unsafe symbol $initName (child of $initOwner) in runtime reflection universe")
+
     type AccessBoundaryType = Symbol
     type AnnotationType     = AnnotationInfo
 
@@ -616,20 +613,55 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
       && isTopLevel
       && nme.isReplWrapperName(name)
     )
+
+    /** In our current architecture, symbols for top-level classes and modules
+     *  are created as dummies. Package symbols just call newClass(name) or newModule(name) and
+     *  consider their job done.
+     *
+     *  In order for such a dummy to provide meaningful info (e.g. a list of its members),
+     *  it needs to go through unpickling. Unpickling is a process of reading Scala metadata
+     *  from ScalaSignature annotations and assigning it to symbols and types.
+     *
+     *  A single unpickling session takes a top-level class or module, parses the ScalaSignature annotation
+     *  and then reads metadata for the unpicklee, its companion (if any) and all their members recursively
+     *  (i.e. the pickle not only contains info about directly nested classes/modules, but also about
+     *  classes/modules nested into those and so on).
+     *
+     *  Unpickling is triggered automatically whenever typeSignature (info in compiler parlance) is called.
+     *  This happens because package symbols assign completer thunks to the dummies they create.
+     *  Therefore metadata loading happens lazily and transparently.
+     *
+     *  Almost transparently. Unfortunately metadata isn't limited to just signatures (i.e. lists of members).
+     *  It also includes flags (which determine e.g. whether a class is sealed or not), annotations and privateWithin.
+     *  This gives rise to unpleasant effects like in SI-6277, when a flag test called on an uninitialize symbol
+     *  produces incorrect results.
+     *
+     *  One might think that the solution is simple: automatically call the completer
+     *  whenever one needs flags, annotations and privateWithin - just like it's done for typeSignature.
+     *  Unfortunately, this leads to weird crashes in scalac, and currently we can't attempt
+     *  to fix the core of the compiler risk stability a few weeks before the final release.
+     *  upd. Haha, "a few weeks before the final release". This surely sounds familiar :)
+     *
+     *  However we do need to fix this for runtime reflection, since this idionsynchrazy is not something
+     *  we'd like to expose to reflection users. Therefore a proposed solution is to check whether we're in a
+     *  runtime reflection universe, and if yes and if we've not yet loaded the requested info, then to commence initialization.
+     */
     final def getFlag(mask: Long): Long = {
-      if (!isCompilerUniverse && needsInitialize(isFlagRelated = true, mask = mask)) initialize
+      if (!isCompilerUniverse && !isThreadsafe(purpose = FlagOps(mask))) initialize
       flags & mask
     }
     /** Does symbol have ANY flag in `mask` set? */
     final def hasFlag(mask: Long): Boolean = {
-      if (!isCompilerUniverse && needsInitialize(isFlagRelated = true, mask = mask)) initialize
+      // See `getFlag` to learn more about the `isThreadsafe` call in the body of this method.
+      if (!isCompilerUniverse && !isThreadsafe(purpose = FlagOps(mask))) initialize
       (flags & mask) != 0
     }
     def hasFlag(mask: Int): Boolean = hasFlag(mask.toLong)
 
     /** Does symbol have ALL the flags in `mask` set? */
     final def hasAllFlags(mask: Long): Boolean = {
-      if (!isCompilerUniverse && needsInitialize(isFlagRelated = true, mask = mask)) initialize
+      // See `getFlag` to learn more about the `isThreadsafe` call in the body of this method.
+      if (!isCompilerUniverse && !isThreadsafe(purpose = FlagOps(mask))) initialize
       (flags & mask) == mask
     }
 
@@ -950,12 +982,21 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
     final def isInitialized: Boolean =
       validTo != NoPeriod
 
-    /** Some completers call sym.setInfo when still in-flight and then proceed with initialization (e.g. see LazyPackageType)
-     *  setInfo sets _validTo to current period, which means that after a call to setInfo isInitialized will start returning true.
-     *  Unfortunately, this doesn't mean that info becomes ready to be used, because subsequent initialization might change the info.
-     *  Therefore we need this method to distinguish between initialized and really initialized symbol states.
+    /** We consider a symbol to be thread-safe, when multiple concurrent threads can call its methods
+     *  (either directly or indirectly via public reflection or internal compiler infrastructure),
+     *  without any locking and everything works as it should work.
+     *
+     *  In its basic form, `isThreadsafe` always returns false. Runtime reflection augments reflection infrastructure
+     *  with threadsafety-tracking mechanism implemented in `SynchronizedSymbol` that communicates with underlying completers
+     *  and can sometimes return true if the symbol has been completed to the point of thread safety.
+     *
+     *  The `purpose` parameter signifies whether we want to just check immutability of certain flags for the given mask.
+     *  This is necessary to enable robust auto-initialization of `Symbol.flags` for runtime reflection, and is also quite handy
+     *  in avoiding unnecessary initializations when requesting for flags that have already been set.
      */
-    final def isFullyInitialized: Boolean = _validTo != NoPeriod && (flags & LOCKED) == 0
+    def isThreadsafe(purpose: SymbolOps): Boolean = false
+    def markFlagsCompleted(mask: Long): this.type = this
+    def markAllCompleted(): this.type = this
 
     /** Can this symbol be loaded by a reflective mirror?
      *
@@ -1232,7 +1273,8 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
      */
     private[this] var _privateWithin: Symbol = _
     def privateWithin = {
-      if (!isCompilerUniverse && needsInitialize(isFlagRelated = false, mask = 0)) initialize
+      // See `getFlag` to learn more about the `isThreadsafe` call in the body of this method.
+      if (!isCompilerUniverse && !isThreadsafe(purpose = AllOps)) initialize
       _privateWithin
     }
     def privateWithin_=(sym: Symbol) { _privateWithin = sym }
@@ -1490,46 +1532,6 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
       catch { case _: CyclicReference => debuglog("Hit cycle in maybeInitialize of $this") ; false }
     }
 
-    /** Called when the programmer requests information that might require initialization of the underlying symbol.
-     *
-     *  `isFlagRelated` and `mask` describe the nature of this information.
-     *  isFlagRelated = true means that the programmer needs particular bits in flags.
-     *  isFlagRelated = false means that the request is unrelated to flags (annotations or privateWithin).
-     *
-     *  In our current architecture, symbols for top-level classes and modules
-     *  are created as dummies. Package symbols just call newClass(name) or newModule(name) and
-     *  consider their job done.
-     *
-     *  In order for such a dummy to provide meaningful info (e.g. a list of its members),
-     *  it needs to go through unpickling. Unpickling is a process of reading Scala metadata
-     *  from ScalaSignature annotations and assigning it to symbols and types.
-     *
-     *  A single unpickling session takes a top-level class or module, parses the ScalaSignature annotation
-     *  and then reads metadata for the unpicklee, its companion (if any) and all their members recursively
-     *  (i.e. the pickle not only contains info about directly nested classes/modules, but also about
-     *  classes/modules nested into those and so on).
-     *
-     *  Unpickling is triggered automatically whenever typeSignature (info in compiler parlance) is called.
-     *  This happens because package symbols assign completer thunks to the dummies they create.
-     *  Therefore metadata loading happens lazily and transparently.
-     *
-     *  Almost transparently. Unfortunately metadata isn't limited to just signatures (i.e. lists of members).
-     *  It also includes flags (which determine e.g. whether a class is sealed or not), annotations and privateWithin.
-     *  This gives rise to unpleasant effects like in SI-6277, when a flag test called on an uninitialize symbol
-     *  produces incorrect results.
-     *
-     *  One might think that the solution is simple: automatically call the completer whenever one needs
-     *  flags, annotations and privateWithin - just like it's done for typeSignature. Unfortunately, this
-     *  leads to weird crashes in scalac, and currently we can't attempt to fix the core of the compiler
-     *  risk stability a few weeks before the final release.
-     *
-     *  However we do need to fix this for runtime reflection, since it's not something we'd like to
-     *  expose to reflection users. Therefore a proposed solution is to check whether we're in a
-     *  runtime reflection universe and if yes then to commence initialization.
-     */
-    protected def needsInitialize(isFlagRelated: Boolean, mask: Long) =
-      !isInitialized && (flags & LOCKED) == 0 && shouldTriggerCompleter(this, if (infos ne null) infos.info else null, isFlagRelated, mask)
-
     /** Was symbol's type updated during given phase? */
     final def hasTypeAt(pid: Phase#Id): Boolean = {
       assert(isCompilerUniverse)
@@ -1688,7 +1690,8 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
      *  the annotations attached to member a definition (class, method, type, field).
      */
     def annotations: List[AnnotationInfo] = {
-      if (!isCompilerUniverse && needsInitialize(isFlagRelated = false, mask = 0)) initialize
+      // See `getFlag` to learn more about the `isThreadsafe` call in the body of this method.
+      if (!isCompilerUniverse && !isThreadsafe(purpose = AllOps)) initialize
       _annotations
     }
 
@@ -3512,6 +3515,17 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
 
   Statistics.newView("#symbols")(ids)
 
+
+// -------------- Completion --------------------------------------------------------
+
+  // is used to differentiate levels of thread-safety in `Symbol.isThreadsafe`
+  case class SymbolOps(isFlagRelated: Boolean, mask: Long)
+  val AllOps = SymbolOps(isFlagRelated = false, mask = 0L)
+  def FlagOps(mask: Long) = SymbolOps(isFlagRelated = true, mask = mask)
+
+  private def relevantSymbols(syms: Seq[Symbol]) = syms.flatMap(sym => List(sym, sym.moduleClass, sym.sourceModule))
+  def markFlagsCompleted(syms: Symbol*)(mask: Long): Unit = relevantSymbols(syms).foreach(_.markFlagsCompleted(mask))
+  def markAllCompleted(syms: Symbol*): Unit = relevantSymbols(syms).foreach(_.markAllCompleted)
 }
 
 object SymbolsStats {