Address review comments

Author: allanrenucci

compiler/src/dotty/tools/dotc/Compiler.scala

@@ -72,7 +72,6 @@ class Compiler {
          new LiftTry,                // Put try expressions that might execute on non-empty stacks into their own methods
          new HoistSuperArgs,         // Hoist complex arguments of supercalls to enclosing scope
          new ClassOf,                // Expand `Predef.classOf` calls.
-         new CollectNullableFields,  // Collect fields that can be nulled out after use in lazy initialization
          new RefChecks) ::           // Various checks mostly related to abstract members and overriding
     List(new TryCatchPatterns,       // Compile cases in try/catch
          new PatternMatcher,         // Compile pattern matches
@@ -88,6 +87,7 @@ class Compiler {
          new InterceptedMethods,     // Special handling of `==`, `|=`, `getClass` methods
          new Getters,                // Replace non-private vals and vars with getter defs (fields are added later)
          new ElimByName,             // Expand by-name parameter references
+         new CollectNullableFields,  // Collect fields that can be nulled out after use in lazy initialization
          new ElimOuterSelect,        // Expand outer selections
          new AugmentScala2Traits,    // Expand traits defined in Scala 2.x to simulate old-style rewritings
          new ResolveSuper,           // Implement super accessors and add forwarders to trait methods

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

@@ -45,6 +45,15 @@ class CollectNullableFields extends MiniPhase {
 
   override def phaseName = CollectNullableFields.name
 
+  /** CollectNullableFields records usages of by names */
+
+  /** Running after `ElimByName` to see by names as nullable types.
+   *
+   *  We don't necessary need to run after `Getters`, but the implementation
+   *  could be simplified if we were to run before.
+   */
+  override def runsAfter = Set(Getters.name, ElimByName.name)
+
   private[this] sealed trait FieldInfo
   private[this] case object NotNullable extends FieldInfo
   private[this] case class Nullable(by: Symbol) extends FieldInfo
@@ -53,21 +62,20 @@ class CollectNullableFields extends MiniPhase {
   private[this] var nullability: IdentityHashMap[Symbol, FieldInfo] = _
 
   override def prepareForUnit(tree: Tree)(implicit ctx: Context) = {
-    nullability = new IdentityHashMap
+    if (nullability == null) nullability = new IdentityHashMap
     ctx
   }
 
   private def recordUse(tree: Tree)(implicit ctx: Context): Tree = {
-    val sym = tree.symbol
+    def isField(sym: Symbol) =
+       sym.isField || sym.isGetter // running after phase Getters
 
-    def isNullableType(tpe: Type) =
-      tpe.isInstanceOf[ExprType] ||
-      tpe.widenDealias.typeSymbol.isNullableClass
+    val sym = tree.symbol
     val isNullablePrivateField =
-      sym.isField &&
+      isField(sym) &&
       sym.is(Private, butNot = Lazy) &&
       !sym.owner.is(Trait) &&
-      isNullableType(sym.info)
+      sym.info.widenDealias.typeSymbol.isNullableClass
 
     if (isNullablePrivateField)
       nullability.get(sym) match {
@@ -76,8 +84,8 @@ class CollectNullableFields extends MiniPhase {
         case null => // not in the map
           val from = ctx.owner
           val isNullable =
-            from.is(Lazy) && from.isField && // used in lazy field initializer
-            from.owner.eq(sym.owner)         // lazy val and field in the same class
+            from.is(Lazy) && isField(from) && // used in lazy field initializer
+            from.owner.eq(sym.owner)          // lazy val and field defined in the same class
           val info = if (isNullable) Nullable(from) else NotNullable
           nullability.put(sym, info)
         case _ =>
@@ -96,16 +104,16 @@ class CollectNullableFields extends MiniPhase {
     recordUse(tree)
 
   /** Map lazy values to the fields they should null after initialization. */
-  def lazyValNullables(implicit ctx: Context): Map[Symbol, List[Symbol]] = {
-    val result = new mutable.HashMap[Symbol, mutable.ListBuffer[Symbol]]
+  def lazyValNullables(implicit ctx: Context): IdentityHashMap[Symbol, mutable.ListBuffer[Symbol]] = {
+    val result = new IdentityHashMap[Symbol, mutable.ListBuffer[Symbol]]
 
     nullability.forEach {
       case (sym, Nullable(from)) =>
-        val bldr = result.getOrElseUpdate(from, new mutable.ListBuffer)
+        val bldr = result.computeIfAbsent(from, _ => new mutable.ListBuffer)
         bldr += sym
       case _ =>
     }
 
-    result.mapValues(_.toList).toMap
+    result
   }
 }

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

@@ -49,7 +49,7 @@ import ValueClasses._
 class Getters extends MiniPhase with SymTransformer {
   import ast.tpd._
 
-  override def phaseName = "getters"
+  override def phaseName = Getters.name
 
   override def transformSym(d: SymDenotation)(implicit ctx: Context): SymDenotation = {
     def noGetterNeeded =
@@ -74,3 +74,7 @@ class Getters extends MiniPhase with SymTransformer {
   override def transformAssign(tree: Assign)(implicit ctx: Context): Tree =
     if (tree.lhs.symbol is Method) tree.lhs.becomes(tree.rhs).withPos(tree.pos) else tree
 }
+
+object Getters {
+  val name = "getters"
+}

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

@@ -26,6 +26,8 @@ import dotty.tools.dotc.core.SymDenotations.SymDenotation
 import dotty.tools.dotc.core.DenotTransformers.{SymTransformer, IdentityDenotTransformer, DenotTransformer}
 import Erasure.Boxing.adaptToType
 
+import java.util.IdentityHashMap
+
 class LazyVals extends MiniPhase with IdentityDenotTransformer {
   import LazyVals._
   import tpd._
@@ -51,14 +53,17 @@ class LazyVals extends MiniPhase with IdentityDenotTransformer {
   val containerFlagsMask = Flags.Method | Flags.Lazy | Flags.Accessor | Flags.Module
 
   /** A map of lazy values to the fields they should null after initialization. */
-  private[this] var lazyValNullables: Map[Symbol, List[Symbol]] = _
-  private def nullableFor(sym: Symbol)(implicit ctx: Context) =
-    if (sym.is(Flags.Module)) Nil
-    else lazyValNullables.getOrElse(sym, Nil)
+  private[this] var lazyValNullables: IdentityHashMap[Symbol, mutable.ListBuffer[Symbol]] = _
+  private def nullableFor(sym: Symbol)(implicit ctx: Context) = {
+    val nullables = lazyValNullables.remove(sym)
+    if (nullables == null || sym.is(Flags.Module)) Nil
+    else nullables.toList
+  }
 
 
   override def prepareForUnit(tree: Tree)(implicit ctx: Context) = {
-    lazyValNullables = ctx.collectNullableFieldsPhase.asInstanceOf[CollectNullableFields].lazyValNullables
+    if (lazyValNullables == null)
+      lazyValNullables = ctx.collectNullableFieldsPhase.asInstanceOf[CollectNullableFields].lazyValNullables
     ctx
   }
 

tests/run/i1692.scala

@@ -119,7 +119,7 @@ object Test {
 
     val fromTrait = new LazyTrait {}
     assert(fromTrait.l0 == "A")
-    assert(readField("LazyTrait$$a", fromTrait) != null) // fragile: test will break if compiler generated name change
+    assert(readField("LazyTrait$$a", fromTrait) != null) // fragile: test will break if compiler generated names change
   }
 
   def readField(fieldName: String, target: Any): Any = {