Apply optimisation on private[this] fields only

Author: allanrenucci

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

@@ -34,23 +34,19 @@ object CollectNullableFields {
  *
  *  A field is nullable if all the conditions below hold:
  *    - belongs to a non trait-class
- *    - is private
+ *    - is private[this]
  *    - is not lazy
- *    - its type is nullable, or is an expression type (e.g. => Int)
- *    - is on used in a lazy val initializer
+ *    - its type is nullable
+ *    - is only used in a lazy val initializer
  *    - defined in the same class as the lazy val
  */
 class CollectNullableFields extends MiniPhase {
   import tpd._
 
   override def phaseName = CollectNullableFields.name
 
-  /** 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)
+  /** Running after `ElimByName` to see by names as nullable types. */
+  override def runsAfter = Set(ElimByName.name)
 
   private[this] sealed trait FieldInfo
   private[this] case object NotNullable extends FieldInfo
@@ -65,14 +61,12 @@ class CollectNullableFields extends MiniPhase {
   }
 
   private def recordUse(tree: Tree)(implicit ctx: Context): Tree = {
-    def isField(sym: Symbol) =
-       sym.isField || sym.isGetter // running after phase Getters
-
     val sym = tree.symbol
     val isNullablePrivateField =
-      isField(sym) &&
-      sym.is(Private, butNot = Lazy) &&
+      sym.isField &&
+      !sym.is(Lazy) &&
       !sym.owner.is(Trait) &&
+      sym.initial.is(PrivateLocal) &&
       sym.info.widenDealias.typeSymbol.isNullableClass
 
     if (isNullablePrivateField)
@@ -82,8 +76,9 @@ class CollectNullableFields extends MiniPhase {
         case null => // not in the map
           val from = ctx.owner
           val isNullable =
-            from.is(Lazy) && isField(from) && // used in lazy field initializer
-            from.owner.eq(sym.owner)          // lazy val and field defined in the same class
+            from.is(Lazy, butNot = Module) && // is lazy val
+            from.owner.isClass &&             // is field
+            from.owner.eq(sym.owner)          // is lazy val and field defined in the same class
           val info = if (isNullable) Nullable(from) else NotNullable
           nullability.put(sym, info)
         case _ =>

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.name
+  override def phaseName = "getters"
 
   override def transformSym(d: SymDenotation)(implicit ctx: Context): SymDenotation = {
     def noGetterNeeded =
@@ -74,7 +74,3 @@ 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

@@ -55,8 +55,9 @@ class LazyVals extends MiniPhase with IdentityDenotTransformer {
   /** A map of lazy values to the fields they should null after initialization. */
   private[this] var lazyValNullables: IdentityHashMap[Symbol, mutable.ListBuffer[Symbol]] = _
   private def nullableFor(sym: Symbol)(implicit ctx: Context) = {
+    // optimisation: value only used once, we can remove the value from the map
     val nullables = lazyValNullables.remove(sym)
-    if (nullables == null || sym.is(Flags.Module)) Nil
+    if (nullables == null) Nil
     else nullables.toList
   }
 
@@ -195,8 +196,7 @@ class LazyVals extends MiniPhase with IdentityDenotTransformer {
 
   private def nullOut(nullables: List[Symbol])(implicit ctx: Context): List[Tree] = {
     val nullConst = Literal(Constants.Constant(null))
-    nullables.map { sym =>
-      val field = if (sym.isGetter) sym.field else sym
+    nullables.map { field =>
       assert(field.isField)
       field.setFlag(Flags.Mutable)
       ref(field).becomes(nullConst)

tests/run/i1692.scala

@@ -4,46 +4,46 @@ class VCString(val x: String) extends AnyVal
 class LazyNullable(a: => Int) {
   lazy val l0 = a // null out a
 
-  private val b = "B"
+  private[this] val b = "B"
   lazy val l1 = b // null out b
 
-  private val c = "C"
+  private[this] val c = "C"
   @volatile lazy val l2 = c // null out c
 
-  private val d = "D"
+  private[this] val d = "D"
   lazy val l3 = d + d // null out d (Scalac require single use?)
 }
 
 object LazyNullable2 {
-  private val a = "A"
+  private[this] val a = "A"
   lazy val l0 = a // null out a
 }
 
 class LazyNotNullable {
-  private val a = 'A'.toInt // not nullable type
+  private[this] val a = 'A'.toInt // not nullable type
   lazy val l0 = a
 
-  private val b = new VCInt('B'.toInt) // not nullable type
+  private[this] val b = new VCInt('B'.toInt) // not nullable type
   lazy val l1 = b
 
-  private val c = new VCString("C") // should be nullable but is not??
+  private[this] val c = new VCString("C") // should be nullable but is not??
   lazy val l2 = c
 
-  private lazy val d = "D" // not nullable because lazy
+  private[this] lazy val d = "D" // not nullable because lazy
   lazy val l3 = d
 
-  val e = "E" // not nullable because not private
+  private val e = "E" // not nullable because not private[this]
   lazy val l4 = e
 
-  private val f = "F" // not nullable because used in mutiple lazy vals
+  private[this] val f = "F" // not nullable because used in mutiple lazy vals
   lazy val l5 = f
   lazy val l6 = f
 
-  private val g = "G" // not nullable because used outside a lazy val initializer
+  private[this] val g = "G" // not nullable because used outside a lazy val initializer
   def foo = g
   lazy val l7 = g
 
-  private val h = "H" // not nullable because field and lazy val not defined in the same class
+  private[this] val h = "H" // not nullable because field and lazy val not defined in the same class
   class Inner {
     lazy val l8 = h
   }
@@ -54,6 +54,13 @@ trait LazyTrait {
   lazy val l0 = a
 }
 
+class Foo(val x: String)
+
+class LazyNotNullable2(x: String) extends Foo(x) {
+  lazy val y = x // not nullable. Here x is super.x
+}
+
+
 object Test {
   def main(args: Array[String]): Unit = {
     nullableTests()
@@ -115,11 +122,15 @@ object Test {
 
     val inner = new lz.Inner
     assert(inner.l8 == "H")
-    assertNotNull("h")
+    assertNotNull("LazyNotNullable$$h") // fragile: test will break if compiler generated names change
 
     val fromTrait = new LazyTrait {}
     assert(fromTrait.l0 == "A")
     assert(readField("LazyTrait$$a", fromTrait) != null) // fragile: test will break if compiler generated names change
+
+    val lz2 = new LazyNotNullable2("Hello")
+    assert(lz2.y == "Hello")
+    assert(lz2.x == "Hello")
   }
 
   def readField(fieldName: String, target: Any): Any = {