Merge pull request #7006 from dotty-staging/fix-#6009

Fix #6909: Cache alias givens in lazy vals

Author: liufengyun

compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala

@@ -404,7 +404,10 @@ class JSCodeGen()(implicit ctx: Context) {
         case EmptyTree  => ()
         case dd: DefDef => generatedMethods ++= genMethod(dd)
         case _ =>
-          throw new FatalError("Illegal tree in gen of genInterface(): " + tree)
+          throw new FatalError(
+            i"""Illegal tree in gen of genInterface(): $tree
+               |class = $td
+               |in ${ctx.compilationUnit}""")
       }
     }
 

compiler/src/dotty/tools/dotc/ast/TreeInfo.scala

@@ -243,20 +243,6 @@ trait TreeInfo[T >: Untyped <: Type] { self: Trees.Instance[T] =>
     case y          => y
   }
 
-  /**  The largest subset of {NoInits, PureInterface} that a
-   *   trait or class enclosing this statement can have as flags.
-   */
-  def defKind(tree: Tree)(implicit ctx: Context): FlagSet = unsplice(tree) match {
-    case EmptyTree | _: Import => NoInitsInterface
-    case tree: TypeDef => if (tree.isClassDef) NoInits else NoInitsInterface
-    case tree: DefDef =>
-      if (tree.unforcedRhs == EmptyTree &&
-          tree.vparamss.forall(_.forall(_.rhs.isEmpty))) NoInitsInterface
-      else NoInits
-    case tree: ValDef => if (tree.unforcedRhs == EmptyTree) NoInitsInterface else EmptyFlags
-    case _ => EmptyFlags
-  }
-
   /**  The largest subset of {NoInits, PureInterface} that a
    *   trait or class with these parents can have as flags.
    */
@@ -266,12 +252,6 @@ trait TreeInfo[T >: Untyped <: Type] { self: Trees.Instance[T] =>
     case _ :: parents1 => parentsKind(parents1)
   }
 
-  /**  The largest subset of {NoInits, PureInterface} that a
-   *   trait or class with this body can have as flags.
-   */
-  def bodyKind(body: List[Tree])(implicit ctx: Context): FlagSet =
-    (NoInitsInterface /: body)((fs, stat) => fs & defKind(stat))
-
   /** Checks whether predicate `p` is true for all result parts of this expression,
    *  where we zoom into Ifs, Matches, and Blocks.
    */
@@ -342,6 +322,28 @@ trait UntypedTreeInfo extends TreeInfo[Untyped] { self: Trees.Instance[Untyped]
     case _ => false
   }
 
+  /**  The largest subset of {NoInits, PureInterface} that a
+   *   trait or class enclosing this statement can have as flags.
+   */
+  def defKind(tree: Tree)(implicit ctx: Context): FlagSet = unsplice(tree) match {
+    case EmptyTree | _: Import => NoInitsInterface
+    case tree: TypeDef => if (tree.isClassDef) NoInits else NoInitsInterface
+    case tree: DefDef =>
+      if (tree.unforcedRhs == EmptyTree &&
+          tree.vparamss.forall(_.forall(_.rhs.isEmpty))) NoInitsInterface
+      else if (tree.mods.is(Given) && tree.tparams.isEmpty && tree.vparamss.isEmpty)
+        EmptyFlags // might become a lazy val: TODO: check whether we need to suppress NoInits once we have new lazy val impl
+      else NoInits
+    case tree: ValDef => if (tree.unforcedRhs == EmptyTree) NoInitsInterface else EmptyFlags
+    case _ => EmptyFlags
+  }
+
+  /**  The largest subset of {NoInits, PureInterface} that a
+   *   trait or class with this body can have as flags.
+   */
+  def bodyKind(body: List[Tree])(implicit ctx: Context): FlagSet =
+    (NoInitsInterface /: body)((fs, stat) => fs & defKind(stat))
+
   // todo: fill with other methods from TreeInfo that only apply to untpd.Tree's
 }
 

compiler/src/dotty/tools/dotc/core/SymDenotations.scala

@@ -102,8 +102,12 @@ trait SymDenotations { this: Context =>
     }
   }
 
-  /** Configurable: Accept stale symbol with warning if in IDE */
-  def staleOK: Boolean = Config.ignoreStaleInIDE && mode.is(Mode.Interactive)
+  /** Configurable: Accept stale symbol with warning if in IDE
+   *  Always accept stale symbols when testing pickling.
+   */
+  def staleOK: Boolean =
+    Config.ignoreStaleInIDE && mode.is(Mode.Interactive) ||
+    settings.YtestPickler.value
 
   /** Possibly accept stale symbol with warning if in IDE */
   def acceptStale(denot: SingleDenotation): Boolean =

compiler/src/dotty/tools/dotc/core/Symbols.scala

@@ -553,9 +553,10 @@ object Symbols {
 
     /** This symbol entered into owner's scope (owner must be a class). */
     final def entered(implicit ctx: Context): this.type = {
-      assert(this.owner.isClass, s"symbol ($this) entered the scope of non-class owner ${this.owner}") // !!! DEBUG
-      this.owner.asClass.enter(this)
-      if (this.is(Module)) this.owner.asClass.enter(this.moduleClass)
+      if (this.owner.isClass) {
+        this.owner.asClass.enter(this)
+        if (this.is(Module)) this.owner.asClass.enter(this.moduleClass)
+      }
       this
     }
 
@@ -566,14 +567,16 @@ object Symbols {
      */
     def enteredAfter(phase: DenotTransformer)(implicit ctx: Context): this.type =
       if (ctx.phaseId != phase.next.id) enteredAfter(phase)(ctx.withPhase(phase.next))
-      else {
-        if (this.owner.is(Package)) {
-          denot.validFor |= InitialPeriod
-          if (this.is(Module)) this.moduleClass.validFor |= InitialPeriod
-        }
-        else this.owner.asClass.ensureFreshScopeAfter(phase)
-        assert(isPrivate || phase.changesMembers, i"$this entered in ${this.owner} at undeclared phase $phase")
-        entered
+      else this.owner match {
+        case owner: ClassSymbol =>
+          if (owner.is(Package)) {
+            denot.validFor |= InitialPeriod
+            if (this.is(Module)) this.moduleClass.validFor |= InitialPeriod
+          }
+          else owner.ensureFreshScopeAfter(phase)
+          assert(isPrivate || phase.changesMembers, i"$this entered in $owner at undeclared phase $phase")
+          entered
+        case _ => this
       }
 
     /** Remove symbol from scope of owning class */

compiler/src/dotty/tools/dotc/transform/CacheAliasImplicits.scala

@@ -27,25 +27,18 @@ object CacheAliasImplicits {
  *  is cached. It applies to all alias implicits that have neither type parameters
  *  nor a given clause. Example: The alias
  *
- *      implicit a for TC = rhs
+ *      given a as TC = rhs
  *
- *  is expanded before this phase
+ *  is expanded before this phase to:
  *
  *      implicit def a: TC = rhs
  *
  *  It is then expanded further as follows:
  *
- *  1. If `rhs` is a simple name `x` (possibly with a `this.` prefix), leave the definition as is.
- *  2. Otherwise, if `rhs` is a pure path, replace the definition with
+ *  1. If `rhs` is a simple name `x` (possibly with a `this.` prefix) that
+ *     refers to a value, leave it as is.
  *
- *      implicit val a: TC = rhs
- *
- *  3. Otherwise, if `TC` is a reference type, replace the definition with
- *
- *      private[this] var a$_cache: TC = null
- *      implicit def a: TC = { if (a$_cache == null) a$_cache = rhs; a$_cache }
- *
- *  4. Otherwise `TC` is a value type. Replace the definition with
+ *  2. Otherwise, replace the definition with
  *
  *      lazy implicit val a: TC = rhs
  */
@@ -56,47 +49,27 @@ class CacheAliasImplicits extends MiniPhase with IdentityDenotTransformer { this
 
   override def transformDefDef(tree: DefDef)(implicit ctx: Context): Tree = {
     val sym = tree.symbol
-    sym.info match {
-      case ExprType(rhsType) if sym.is(Given, butNot = CacheAliasImplicits.NoCacheFlags) =>
-        // If rhs is a simple TermRef, leave a def.
-        tree.rhs.tpe match {
-          case TermRef(pre, _) =>
-            pre match {
-              case NoPrefix => return tree
-              case pre: ThisType if pre.cls == ctx.owner.enclosingClass => return tree
-              case _ =>
-            }
-          case _ =>
-        }
-        def makeVal(additionalFlags: FlagSet) = {
-          sym.copySymDenotation(
-              initFlags = sym.flags &~ Method | additionalFlags,
-              info = rhsType)
-            .installAfter(thisPhase)
-          cpy.ValDef(tree)(tree.name, tree.tpt, tree.rhs)
-        }
-        if (isPurePath(tree.rhs)) makeVal(EmptyFlags)
-        else if (rhsType.classSymbol.isValueClass ||
-                 !erasure(rhsType).typeSymbol.derivesFrom(defn.ObjectClass)) makeVal(Lazy)
-        else {
-          val cacheFlags = if (ctx.owner.isClass) Private | Local | Mutable else Mutable
-          val cacheSym =
-            ctx.newSymbol(ctx.owner, CacheName(tree.name), cacheFlags, rhsType, coord = sym.coord)
-          if (ctx.owner.isClass) cacheSym.enteredAfter(thisPhase)
-          val cacheDef = ValDef(cacheSym, tpd.defaultValue(rhsType))
-          val cachingDef = cpy.DefDef(tree)(rhs =
-            Block(
-              If(
-                ref(cacheSym).select(defn.Any_==).appliedTo(nullLiteral),
-                Assign(ref(cacheSym), tree.rhs),
-                unitLiteral) :: Nil,
-              ref(cacheSym)
-            )
-          )
-          Thicket(cacheDef, cachingDef)
-        }
-      case _ => tree
+    val isCached = !sym.is(Inline) && {
+      sym.info match {
+        case ExprType(resTpe) if sym.is(Given, butNot = CacheAliasImplicits.NoCacheFlags) =>
+          tree.rhs.tpe match {
+            case rhsTpe @ TermRef(NoPrefix, _)
+            if rhsTpe.isStable => false
+            case rhsTpe @ TermRef(pre: ThisType, _)
+            if rhsTpe.isStable && pre.cls == sym.owner.enclosingClass => false
+            case _ => true
+          }
+        case _ => false
+      }
+    }
+    if (isCached) {
+      sym.copySymDenotation(
+        initFlags = sym.flags &~ Method | Lazy,
+        info = sym.info.widenExpr)
+      .installAfter(thisPhase)
+      cpy.ValDef(tree)(tree.name, tree.tpt, tree.rhs)
     }
+    else tree
   }
 }
 

compiler/src/dotty/tools/dotc/transform/HoistSuperArgs.scala

@@ -91,13 +91,13 @@ class HoistSuperArgs extends MiniPhase with IdentityDenotTransformer { thisPhase
         val argTypeWrtConstr = argType.subst(origParams, allParamRefs(constr.info))
         // argType with references to paramRefs of the primary constructor instead of
         // local parameter accessors
-        val meth = ctx.newSymbol(
+        ctx.newSymbol(
           owner = methOwner,
           name = SuperArgName.fresh(cls.name.toTermName),
           flags = Synthetic | Private | Method | staticFlag,
           info = replaceResult(constr.info, argTypeWrtConstr),
-          coord = constr.coord)
-        if (methOwner.isClass) meth.enteredAfter(thisPhase) else meth
+          coord = constr.coord
+        ).enteredAfter(thisPhase)
       }
 
       /** Type of a reference implies that it needs to be hoisted */

compiler/src/dotty/tools/dotc/transform/LambdaLift.scala

@@ -300,8 +300,9 @@ object LambdaLift {
         proxyMap(owner) = {
           for (fv <- freeValues.toList) yield {
             val proxyName = newName(fv)
-            val proxy = ctx.newSymbol(owner, proxyName.asTermName, newFlags, fv.info, coord = fv.coord)
-            if (owner.isClass) proxy.enteredAfter(thisPhase)
+            val proxy =
+              ctx.newSymbol(owner, proxyName.asTermName, newFlags, fv.info, coord = fv.coord)
+                .enteredAfter(thisPhase)
             (fv, proxy)
           }
         }.toMap

compiler/src/dotty/tools/dotc/typer/Namer.scala

@@ -1175,7 +1175,7 @@ class Namer { typer: Typer =>
       if (isDerivedValueClass(cls)) cls.setFlag(Final)
       cls.info = avoidPrivateLeaks(cls)
       cls.baseClasses.foreach(_.invalidateBaseTypeCache()) // we might have looked before and found nothing
-      cls.setNoInitsFlags(parentsKind(parents), bodyKind(rest))
+      cls.setNoInitsFlags(parentsKind(parents), untpd.bodyKind(rest))
       if (cls.isNoInitsClass) cls.primaryConstructor.setFlag(StableRealizable)
       processExports(localCtx)
     }

compiler/src/dotty/tools/dotc/typer/Typer.scala

@@ -2132,15 +2132,13 @@ class Typer extends Namer
                 buf += inlineExpansion(mdef1)
                   // replace body with expansion, because it will be used as inlined body
                   // from separately compiled files - the original BodyAnnotation is not kept.
+              case mdef1: TypeDef if mdef1.symbol.is(Enum, butNot = Case) =>
+                enumContexts(mdef1.symbol) = ctx
+                buf += mdef1
+              case EmptyTree =>
+                // clashing synthetic case methods are converted to empty trees, drop them here
               case mdef1 =>
-                import untpd.modsDeco
-                mdef match {
-                  case mdef: untpd.TypeDef if mdef.mods.isEnumClass =>
-                    enumContexts(mdef1.symbol) = ctx
-                  case _ =>
-                }
-                if (!mdef1.isEmpty) // clashing synthetic case methods are converted to empty trees
-                  buf += mdef1
+                buf += mdef1
             }
             traverse(rest)
         }

docs/docs/reference/contextual/relationship-implicits.md

@@ -28,32 +28,21 @@ Given instances can be mapped to combinations of implicit objects, classes and i
       class ListOrd[T](implicit ord: Ord[T]) extends Ord[List[T]] { ... }
       final implicit def ListOrd[T](implicit ord: Ord[T]): ListOrd[T] = new ListOrd[T]
     ```
- 3. Alias givens map to implicit methods. If an alias has neither type parameters nor a given clause, its right-hand side is cached in a variable. There are two cases that can be optimized:
-
-  - If the right hand side is a simple reference, we can
-    use a forwarder to that reference without caching it.
-  - If the right hand side is more complex, but still known to be pure, we can
-    create a `val` that computes it ahead of time.
+ 3. Alias givens map to implicit methods or implicit lazy vals. If an alias has neither type parameters nor a given clause,
+    it is treated as a lazy val, unless the right hand side is a simple reference, in which case we can use a forwarder to that
+    reference without caching it.
 
  Examples:
 
     ```scala
       given global as ExecutionContext = new ForkJoinContext()
-      given config as Config = default.config
 
       val ctx: Context
       given as Context = ctx
     ```
     would map to
     ```scala
-      private[this] var global$cache: ExecutionContext | Null = null
-      final implicit def global: ExecutionContext = {
-        if (global$cache == null) global$cache = new ForkJoinContext()
-        global$cache
-      }
-
-      final implicit val config: Config = default.config
-
+      final implicit lazy val global: ExecutionContext = new ForkJoinContext()
       final implicit def Context_given = ctx
     ```