Merge pull request #8253 from dotty-staging/fix-#8033

Fix #8033: Eliminate unnecessary outer accessors

Author: odersky

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

@@ -110,9 +110,11 @@ class Compiler {
          new GetClass) ::            // Rewrites getClass calls on primitive types.
     List(new LinkScala2Impls,        // Redirect calls to trait methods defined by Scala 2.x, so that they now go to
          new LambdaLift,             // Lifts out nested functions to class scope, storing free variables in environments
-                                        // Note: in this mini-phase block scopes are incorrect. No phases that rely on scopes should be here
-         new ElimStaticThis) ::      // Replace `this` references to static objects by global identifiers
-    List(new Flatten,                // Lift all inner classes to package scope
+                                     // Note: in this mini-phase block scopes are incorrect. No phases that rely on scopes should be here
+         new ElimStaticThis,         // Replace `this` references to static objects by global identifiers
+         new CountOuterAccesses) ::  // Identify outer accessors that can be dropped
+    List(new DropOuterAccessors,     // Drop unused outer accessors
+         new Flatten,                // Lift all inner classes to package scope
          new RenameLifted,           // Renames lifted classes to local numbering scheme
          new TransformWildcards,     // Replace wildcards with default values
          new MoveStatics,            // Move static methods from companion to the class itself

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

@@ -230,6 +230,7 @@ object Phases {
     private var myErasurePhase: Phase = _
     private var myElimErasedValueTypePhase: Phase = _
     private var myLambdaLiftPhase: Phase = _
+    private var myCountOuterAccessesPhase: Phase = _
     private var myFlattenPhase: Phase = _
     private var myGenBCodePhase: Phase = _
 
@@ -248,6 +249,7 @@ object Phases {
     final def erasurePhase: Phase = myErasurePhase
     final def elimErasedValueTypePhase: Phase = myElimErasedValueTypePhase
     final def lambdaLiftPhase: Phase = myLambdaLiftPhase
+    final def countOuterAccessesPhase = myCountOuterAccessesPhase
     final def flattenPhase: Phase = myFlattenPhase
     final def genBCodePhase: Phase = myGenBCodePhase
 
@@ -267,6 +269,7 @@ object Phases {
       myElimErasedValueTypePhase = phaseOfClass(classOf[ElimErasedValueType])
       myPatmatPhase = phaseOfClass(classOf[PatternMatcher])
       myLambdaLiftPhase = phaseOfClass(classOf[LambdaLift])
+      myCountOuterAccessesPhase = phaseOfClass(classOf[CountOuterAccesses])
       myFlattenPhase = phaseOfClass(classOf[Flatten])
       myExplicitOuterPhase = phaseOfClass(classOf[ExplicitOuter])
       myGettersPhase = phaseOfClass(classOf[Getters])

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

@@ -1079,9 +1079,15 @@ object SymDenotations {
     final def lexicallyEnclosingClass(implicit ctx: Context): Symbol =
       if (!exists || isClass) symbol else owner.lexicallyEnclosingClass
 
+    /** A class is extensible if it is not final, nor a module class,
+     *  nor an anonymous class.
+     */
+    final def isExtensibleClass(using Context): Boolean =
+      isClass && !isOneOf(FinalOrModuleClass) && !isAnonymousClass
+
     /** A symbol is effectively final if it cannot be overridden in a subclass */
     final def isEffectivelyFinal(implicit ctx: Context): Boolean =
-      isOneOf(EffectivelyFinalFlags) || !owner.isClass || owner.isOneOf(FinalOrModuleClass) || owner.isAnonymousClass
+      isOneOf(EffectivelyFinalFlags) || !owner.isExtensibleClass
 
     /** A class is effectively sealed if has the `final` or `sealed` modifier, or it
      *  is defined in Scala 3 and is neither abstract nor open.

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

@@ -177,6 +177,8 @@ class Constructors extends MiniPhase with IdentityDenotTransformer { thisPhase =
              ) &&
              fn.symbol.info.resultType.classSymbol == outerParam.info.classSymbol =>
           ref(outerParam)
+        case tree: RefTree if tree.symbol.is(ParamAccessor) && tree.symbol.name == nme.OUTER =>
+          ref(outerParam)
         case _ =>
           super.transform(tree)
       }

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

@@ -0,0 +1,58 @@
+package dotty.tools.dotc
+package transform
+
+import core._
+import MegaPhase.MiniPhase
+import dotty.tools.dotc.core.Contexts.Context
+import ast._
+import Trees._
+import Flags._
+import Symbols._
+import Decorators._
+import ExplicitOuter.isOuterParamAccessor
+
+import collection.mutable
+
+object CountOuterAccesses:
+  val name: String = "countOuterAccesses"
+
+  /** Characterizes outer accessors and outer fields that can be dropped
+   *  if there are no references to them from within the toplevel class
+   *  where they are defined.
+   */
+  def mightBeDropped(sym: Symbol)(using Context) =
+    def isLocal(cls: Symbol) =
+      cls.isAnonymousClass
+      || cls.owner.isTerm
+      || cls.accessBoundary(defn.RootClass).isContainedIn(cls.topLevelClass)
+    (sym.is(OuterAccessor) || sym.isOuterParamAccessor) && isLocal(sym.owner)
+
+/** Counts number of accesses to outer accessors and outer fields of
+ *  classes that are visible only within one source file. The info
+ *  is collected in `outerAccessCount` and used in the subsequent
+ *  DropOuterAccessors phase
+ */
+class CountOuterAccesses extends MiniPhase:
+  thisPhase =>
+  import tpd._
+
+  override def phaseName: String = CountOuterAccesses.name
+
+  override def runsAfter: Set[String] = Set(LambdaLift.name)
+    // LambdaLift can create outer paths. These need to be known in this phase.
+
+  /** The number of times an outer accessor that might be dropped is accessed */
+  val outerAccessCount = new mutable.HashMap[Symbol, Int] {
+    override def default(s: Symbol): Int = 0
+  }
+
+  private def markAccessed(tree: RefTree)(implicit ctx: Context): Tree =
+    val sym = tree.symbol
+    if CountOuterAccesses.mightBeDropped(sym) then outerAccessCount(sym) += 1
+    tree
+
+  override def transformIdent(tree: Ident)(using Context): Tree =
+    markAccessed(tree)
+
+  override def transformSelect(tree: Select)(using Context): Tree =
+    markAccessed(tree)

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

@@ -0,0 +1,86 @@
+package dotty.tools.dotc
+package transform
+
+import core._
+import MegaPhase.MiniPhase
+import dotty.tools.dotc.core.Contexts.Context
+import ast._
+import Trees._
+import Flags._
+import Symbols._
+import Contexts._
+import Decorators._
+import DenotTransformers._
+import ExplicitOuter.isOuterParamAccessor
+import CountOuterAccesses.mightBeDropped
+import collection.mutable
+import annotation.threadUnsafe
+
+object DropOuterAccessors:
+  val name: String = "dropOuterAccessors"
+
+/** Drops unused outer accessors of inner classes that are visible only in one
+ *  toplevel class. For other classes, we can't tell whether an outer accessor
+ *  is used or not. It could for instance be used in a type test in some other source.
+ */
+class DropOuterAccessors extends MiniPhase with IdentityDenotTransformer:
+  thisPhase =>
+  import tpd._
+
+  override def phaseName: String = DropOuterAccessors.name
+
+  override def runsAfterGroupsOf: Set[String] = Set(CountOuterAccesses.name)
+
+  override def changesMembers: Boolean = true // the phase drops outer accessors
+
+  override def transformTemplate(impl: Template)(using ctx: Context): Tree =
+    val outerAccessCount = ctx.base.countOuterAccessesPhase
+      .asInstanceOf[CountOuterAccesses]
+      .outerAccessCount
+
+    def dropOuterAccessor(stat: Tree): Boolean = stat match
+      case stat: DefDef
+      if stat.symbol.is(OuterAccessor)
+          && mightBeDropped(stat.symbol)
+          && outerAccessCount(stat.symbol) == 0 =>
+        assert(stat.rhs.isInstanceOf[RefTree], stat)
+        assert(outerAccessCount(stat.rhs.symbol) > 0)
+        outerAccessCount(stat.rhs.symbol) -= 1
+        stat.symbol.dropAfter(thisPhase)
+        true
+      case _ =>
+        false
+
+    val droppedParamAccessors = mutable.Set[Symbol]()
+
+    def dropOuterParamAccessor(stat: Tree): Boolean = stat match
+      case stat: ValDef
+      if stat.symbol.isOuterParamAccessor
+          && mightBeDropped(stat.symbol)
+          && outerAccessCount(stat.symbol) == 1 =>
+        droppedParamAccessors += stat.symbol
+        stat.symbol.dropAfter(thisPhase)
+        true
+      case _ =>
+        false
+
+    def dropOuterInit(stat: Tree): Boolean = stat match
+      case Assign(lhs, rhs) => droppedParamAccessors.remove(lhs.symbol)
+      case _ => false
+
+    val body1 = impl.body
+      .filterNot(dropOuterAccessor)
+      .filterNot(dropOuterParamAccessor)
+    val constr1 =
+      if droppedParamAccessors.isEmpty then impl.constr
+      else cpy.DefDef(impl.constr)(
+        rhs = impl.constr.rhs match {
+          case rhs @ Block(inits, expr) =>
+            cpy.Block(rhs)(inits.filterNot(dropOuterInit), expr)
+        })
+    assert(droppedParamAccessors.isEmpty,
+      i"""Failed to eliminate: $droppedParamAccessors
+          when dropping outer accessors for ${ctx.owner} with
+          $impl""")
+    cpy.Template(impl)(constr = constr1, body = body1)
+  end transformTemplate

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

@@ -1,4 +1,5 @@
-package dotty.tools.dotc
+package dotty.tools
+package dotc
 package transform
 
 import MegaPhase._
@@ -178,7 +179,7 @@ object ExplicitOuter {
 
   /** A new param accessor for the outer field in class `cls` */
   private def newOuterParamAccessor(cls: ClassSymbol)(implicit ctx: Context) =
-    newOuterSym(cls, cls, nme.OUTER, Private | ParamAccessor)
+    newOuterSym(cls, cls, nme.OUTER, Private | Local | ParamAccessor)
 
   /** A new outer accessor for class `cls` which is a member of `owner` */
   private def newOuterAccessor(owner: ClassSymbol, cls: ClassSymbol)(implicit ctx: Context) = {
@@ -322,6 +323,9 @@ object ExplicitOuter {
       tpe
   }
 
+  def (sym: Symbol).isOuterParamAccessor(using Context): Boolean =
+    sym.is(ParamAccessor) && sym.name == nme.OUTER
+
   def outer(implicit ctx: Context): OuterOps = new OuterOps(ctx)
 
   /** The operations in this class
@@ -386,26 +390,28 @@ object ExplicitOuter {
      */
     def path(start: Tree = This(ctx.owner.lexicallyEnclosingClass.asClass),
              toCls: Symbol = NoSymbol,
-             count: Int = -1): Tree = try {
-      @tailrec def loop(tree: Tree, count: Int): Tree = {
-        val treeCls = tree.tpe.widen.classSymbol
-        val outerAccessorCtx = ctx.withPhaseNoLater(ctx.lambdaLiftPhase) // lambdalift mangles local class names, which means we cannot reliably find outer acessors anymore
-        ctx.log(i"outer to $toCls of $tree: ${tree.tpe}, looking for ${outerAccName(treeCls.asClass)(outerAccessorCtx)} in $treeCls")
-        if (count == 0 || count < 0 && treeCls == toCls) tree
-        else {
-          val acc = outerAccessor(treeCls.asClass)(outerAccessorCtx)
-          assert(acc.exists,
-              i"failure to construct path from ${ctx.owner.ownersIterator.toList}%/% to `this` of ${toCls.showLocated};\n${treeCls.showLocated} does not have an outer accessor")
-          loop(tree.select(acc).ensureApplied, count - 1)
-        }
-      }
-      ctx.log(i"computing outerpath to $toCls from ${ctx.outersIterator.map(_.owner).toList}")
-      loop(start, count)
-    }
-    catch {
-      case ex: ClassCastException =>
+             count: Int = -1): Tree =
+      try
+        @tailrec def loop(tree: Tree, count: Int): Tree =
+          val treeCls = tree.tpe.widen.classSymbol
+          val outerAccessorCtx = ctx.withPhaseNoLater(ctx.lambdaLiftPhase) // lambdalift mangles local class names, which means we cannot reliably find outer acessors anymore
+          ctx.log(i"outer to $toCls of $tree: ${tree.tpe}, looking for ${outerAccName(treeCls.asClass)(outerAccessorCtx)} in $treeCls")
+          if (count == 0 || count < 0 && treeCls == toCls) tree
+          else
+            val enclClass = ctx.owner.lexicallyEnclosingClass.asClass
+            val outerAcc = tree match
+              case tree: This if tree.symbol == enclClass && !enclClass.is(Trait) =>
+                outerParamAccessor(enclClass)(using outerAccessorCtx)
+              case _ =>
+                outerAccessor(treeCls.asClass)(using outerAccessorCtx)
+            assert(outerAcc.exists,
+                i"failure to construct path from ${ctx.owner.ownersIterator.toList}%/% to `this` of ${toCls.showLocated};\n${treeCls.showLocated} does not have an outer accessor")
+            loop(tree.select(outerAcc).ensureApplied, count - 1)
+
+        ctx.log(i"computing outerpath to $toCls from ${ctx.outersIterator.map(_.owner).toList}")
+        loop(start, count)
+      catch case ex: ClassCastException =>
         throw new ClassCastException(i"no path exists from ${ctx.owner.enclosingClass} to $toCls")
-    }
 
     /** The outer parameter definition of a constructor if it needs one */
     def paramDefs(constr: Symbol): List[ValDef] =

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

@@ -23,6 +23,8 @@ object LambdaLift {
   import ast.tpd._
   private class NoPath extends Exception
 
+  val name: String = "lambdaLift"
+
   /** The core lambda lift functionality. */
   class Lifter(thisPhase: MiniPhase with DenotTransformer)(implicit ctx: Context) {
 
@@ -500,7 +502,7 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisPhase =>
   import ast.tpd._
 
   /** the following two members override abstract members in Transform */
-  val phaseName: String = "lambdaLift"
+  val phaseName: String = LambdaLift.name
 
   override def relaxedTypingInGroup: Boolean = true
     // Because it adds free vars as additional proxy parameters

tests/run/i8033.scala

@@ -0,0 +1,36 @@
+trait Okay extends Serializable {
+  def okay: Okay
+}
+
+class Foo {
+  def okay1: Okay = new Okay() {
+    val okay: Okay = this
+    override def toString = "okay1"
+  }
+  def okay2: Okay = new Okay {
+    val okay: Okay = okay1
+    override def toString = "okay2"
+  }
+}
+
+object Test {
+  def main(args: Array[String]): Unit = {
+    val foo = new Foo
+    assert(roundTrip(foo.okay1).toString == "okay1")
+    assert(roundTrip(foo.okay2).toString == "okay2")
+  }
+
+  def roundTrip[A](a: A): A = {
+    import java.io._
+
+    val aos = new ByteArrayOutputStream()
+    val oos = new ObjectOutputStream(aos)
+    oos.writeObject(a)
+    oos.close()
+    val ais = new ByteArrayInputStream(aos.toByteArray())
+    val ois: ObjectInputStream = new ObjectInputStream(ais)
+    val newA = ois.readObject()
+    ois.close()
+    newA.asInstanceOf[A]
+  }
+}

tests/run/outer-accessors.scala

@@ -0,0 +1,18 @@
+class A:
+  val a = 2
+
+  class B:
+    val b = 3
+
+    trait T:
+      def t = a + b
+
+  val bb = B()
+
+  class C extends bb.T:
+    def result = a + t
+
+@main def Test =
+  val a = A()
+  val c = a.C()
+  assert(c.result == 7)