Fix #7926: Redo parameter aliasing optimization

odersky · odersky · commit b8b32be99cc3 · 2020-03-05T17:51:43.000+01:00
- Move main optimization after unpickling, so that changes
   to parameters in superclasses do not break Tasty compatibility.
 - Replace internal Alias annotation by SuperParamAlias flag
 - Don't generate illegal private overrides
 - Make the transformation more robust under separate compilation
   and wrt compilation order
diff --git a/compiler/src/dotty/tools/dotc/Compiler.scala b/compiler/src/dotty/tools/dotc/Compiler.scala
@@ -90,6 +90,7 @@ class Compiler {
          new AugmentScala2Traits,    // Augments Scala2 traits with additional members needed for mixin composition.
          new ResolveSuper,           // Implement super accessors
          new FunctionXXLForwarders,  // Add forwarders for FunctionXXL apply method
+         new ParamForwarding,        // Add forwarders for aliases of superclass parameters
          new TupleOptimizations,     // Optimize generic operations on tuples
          new ArrayConstructors) ::   // Intercept creation of (non-generic) arrays and intrinsify.
     List(new Erasure) ::             // Rewrite types to JVM model, erasing all type parameters, abstract types and refinements.
diff --git a/compiler/src/dotty/tools/dotc/core/Annotations.scala b/compiler/src/dotty/tools/dotc/core/Annotations.scala
@@ -145,10 +145,6 @@ object Annotations {
     def deferredResolve(atp: Type, args: List[Tree])(implicit ctx: Context): Annotation =
       deferred(atp.classSymbol)(resolveConstructor(atp, args))
 
-    def makeAlias(sym: TermSymbol)(implicit ctx: Context): Annotation =
-      apply(defn.AliasAnnot, List(
-        ref(TermRef(sym.owner.thisType, sym.name, sym))))
-
     /** Extractor for child annotations */
     object Child {
 
diff --git a/compiler/src/dotty/tools/dotc/core/Definitions.scala b/compiler/src/dotty/tools/dotc/core/Definitions.scala
@@ -780,7 +780,6 @@ class Definitions {
   @tu lazy val RefiningAnnotationClass: ClassSymbol = ctx.requiredClass("scala.annotation.RefiningAnnotation")
 
   // Annotation classes
-  @tu lazy val AliasAnnot: ClassSymbol = ctx.requiredClass("scala.annotation.internal.Alias")
   @tu lazy val AnnotationDefaultAnnot: ClassSymbol = ctx.requiredClass("scala.annotation.internal.AnnotationDefault")
   @tu lazy val BodyAnnot: ClassSymbol = ctx.requiredClass("scala.annotation.internal.Body")
   @tu lazy val ChildAnnot: ClassSymbol = ctx.requiredClass("scala.annotation.internal.Child")
diff --git a/compiler/src/dotty/tools/dotc/core/Flags.scala b/compiler/src/dotty/tools/dotc/core/Flags.scala
@@ -296,7 +296,7 @@ object Flags {
   val (_, CaseAccessor @ _, _) = newFlags(25, "<caseaccessor>")
 
   /** A Scala 2x super accessor / an unpickled Scala 2.x class */
-  val (SuperAccessorOrScala2x @ _, Scala2SuperAccessor @ _, Scala2x @ _) = newFlags(26, "<superaccessor>", "<scala-2.x>")
+  val (SuperParamAliasOrScala2x @ _, SuperParamAlias @ _, Scala2x @ _) = newFlags(26, "<super-param-alias>", "<scala-2.x>")
 
   /** A method that has default params */
   val (_, DefaultParameterized @ _, _) = newFlags(27, "<defaultparam>")
@@ -371,8 +371,9 @@ object Flags {
   /** Symbol is a self name */
   val (_, SelfName @ _, _) = newFlags(54, "<selfname>")
 
-  /** An existentially bound symbol (Scala 2.x only) */
-  val (Scala2ExistentialCommon @ _, _, Scala2Existential @ _) = newFlags(55, "<existential>")
+  /** A Scala 2 superaccessor (only needed during Scala2Unpickling) /
+   *  an existentially bound symbol (Scala 2.x only) */
+  val (Scala2SpecialFlags @ _, Scala2SuperAccessor @ _, Scala2Existential @ _) = newFlags(55, "<existential>")
 
   /** Children were queried on this class */
   val (_, _, ChildrenQueried @ _) = newFlags(56, "<children-queried>")
@@ -439,10 +440,10 @@ object Flags {
   val FromStartFlags: FlagSet = commonFlags(
     Module, Package, Deferred, Method, Case,
     HigherKinded, Param, ParamAccessor,
-    Scala2ExistentialCommon, MutableOrOpen, Opaque, Touched, JavaStatic,
+    Scala2SpecialFlags, MutableOrOpen, Opaque, Touched, JavaStatic,
     OuterOrCovariant, LabelOrContravariant, CaseAccessor,
     Extension, NonMember, Implicit, Given, Permanent, Synthetic,
-    SuperAccessorOrScala2x, Inline, Macro)
+    SuperParamAliasOrScala2x, Inline, Macro)
 
   /** Flags that are not (re)set when completing the denotation, or, if symbol is
    *  a top-level class or object, when completing the denotation once the class
@@ -561,7 +562,7 @@ object Flags {
   val PrivateMethod: FlagSet                 = Method | Private
   val NoInitsInterface: FlagSet              = NoInits | PureInterface
   val NoInitsTrait: FlagSet                  = NoInits | Trait                                // A trait that does not need to be initialized
-  val ValidForeverFlags: FlagSet             = Package | Permanent | Scala2ExistentialCommon
+  val ValidForeverFlags: FlagSet             = Package | Permanent | Scala2SpecialFlags
   val TermParamOrAccessor: FlagSet           = Param | ParamAccessor
   val PrivateParamAccessor: FlagSet          = ParamAccessor | Private
   val PrivateOrSynthetic: FlagSet            = Private | Synthetic
diff --git a/compiler/src/dotty/tools/dotc/core/NameKinds.scala b/compiler/src/dotty/tools/dotc/core/NameKinds.scala
@@ -354,12 +354,12 @@ object NameKinds {
   val InlineAccessorName: PrefixNameKind = new PrefixNameKind(INLINEACCESSOR, "inline$")
 
   val AvoidClashName: SuffixNameKind = new SuffixNameKind(AVOIDCLASH, "$_avoid_name_clash_$")
-  val CacheName = new SuffixNameKind(CACHE, "$_cache")
   val DirectMethodName: SuffixNameKind = new SuffixNameKind(DIRECT, "$direct") { override def definesNewName = true }
   val FieldName: SuffixNameKind = new SuffixNameKind(FIELD, "$$local") {
       override def mkString(underlying: TermName, info: ThisInfo) = underlying.toString
   }
   val ExtMethName: SuffixNameKind = new SuffixNameKind(EXTMETH, "$extension")
+  val ParamAccessorName: SuffixNameKind = new SuffixNameKind(PARAMACC, "$accessor")
   val ModuleClassName: SuffixNameKind = new SuffixNameKind(OBJECTCLASS, "$", optInfoString = "ModuleClass")
   val ImplMethName: SuffixNameKind = new SuffixNameKind(IMPLMETH, "$")
   val AdaptedClosureName: SuffixNameKind = new SuffixNameKind(ADAPTEDCLOSURE, "$adapted") { override def definesNewName = true }
diff --git a/compiler/src/dotty/tools/dotc/core/NameTags.scala b/compiler/src/dotty/tools/dotc/core/NameTags.scala
@@ -34,7 +34,7 @@ object NameTags extends TastyFormat.NameTags {
   final val IMPLMETH = 32       // Used to define methods in implementation classes
                                 // (can probably be removed).
 
-  final val CACHE = 33          // Used as a cache for the rhs of an alias implicit.
+  final val PARAMACC = 33       // Used for a private parameter alias
 
   def nameTagToString(tag: Int): String = tag match {
     case UTF8 => "UTF8"
@@ -55,6 +55,8 @@ object NameTags extends TastyFormat.NameTags {
     case DIRECT => "DIRECT"
     case FIELD => "FIELD"
     case EXTMETH => "EXTMETH"
+    case IMPLMETH => "IMPLMETH"
+    case PARAMACC => "PARAMACC"
     case ADAPTEDCLOSURE => "ADAPTEDCLOSURE"
     case OBJECTCLASS => "OBJECTCLASS"
 
diff --git a/compiler/src/dotty/tools/dotc/core/tasty/TreePickler.scala b/compiler/src/dotty/tools/dotc/core/tasty/TreePickler.scala
@@ -682,6 +682,7 @@ class TreePickler(pickler: TastyPickler) {
       if (flags.is(StableRealizable)) writeModTag(STABLE)
       if (flags.is(Extension)) writeModTag(EXTENSION)
       if (flags.is(ParamAccessor)) writeModTag(PARAMsetter)
+      if (flags.is(SuperParamAlias)) writeModTag(PARAMalias)
       if (flags.is(Exported)) writeModTag(EXPORTED)
       assert(!(flags.is(Label)))
     }
diff --git a/compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala b/compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala
@@ -644,6 +644,7 @@ class TreeUnpickler(reader: TastyReader,
           case EXTENSION => addFlag(Extension)
           case GIVEN => addFlag(Given)
           case PARAMsetter => addFlag(ParamAccessor)
+          case PARAMalias => addFlag(SuperParamAlias)
           case EXPORTED => addFlag(Exported)
           case OPEN => addFlag(Open)
           case PRIVATEqualified =>
diff --git a/compiler/src/dotty/tools/dotc/core/unpickleScala2/Scala2Unpickler.scala b/compiler/src/dotty/tools/dotc/core/unpickleScala2/Scala2Unpickler.scala
@@ -598,7 +598,7 @@ class Scala2Unpickler(bytes: Array[Byte], classRoot: ClassDenotation, moduleClas
                   denot.info matches denot.owner.thisType.memberInfo(alt)
                 }
               val alias = readDisambiguatedSymbolRef(disambiguate).asTerm
-              denot.addAnnotation(Annotation.makeAlias(alias))
+              if alias.name == denot.name then denot.setFlag(SuperParamAlias)
             }
         }
         // println(s"unpickled ${denot.debugString}, info = ${denot.info}") !!! DEBUG
diff --git a/compiler/src/dotty/tools/dotc/transform/CacheAliasImplicits.scala b/compiler/src/dotty/tools/dotc/transform/CacheAliasImplicits.scala
@@ -8,7 +8,6 @@ import core.Contexts._
 import core.Types._
 import core.Flags._
 import core.StdNames.nme
-import core.NameKinds.CacheName
 import core.Constants.Constant
 import core.Decorators._
 import core.TypeErasure.erasure
diff --git a/compiler/src/dotty/tools/dotc/transform/ParamForwarding.scala b/compiler/src/dotty/tools/dotc/transform/ParamForwarding.scala
@@ -1,88 +1,82 @@
-package dotty.tools.dotc
+package dotty.tools
+package dotc
 package transform
 
 import core._
 import ast.Trees._
 import Contexts._, Types._, Symbols._, Flags._, TypeUtils._, DenotTransformers._, StdNames._
 import Decorators._
+import MegaPhase._
+import NameKinds.ParamAccessorName
 import config.Printers.typr
 
-/** For all parameter accessors
+/** For all private parameter accessors
  *
- *      val x: T = ...
+ *      private val x: T = ...
  *
- *  if
- *  (1) x is forwarded in the supercall to a parameter that's also named `x`
- *  (2) the superclass parameter accessor for `x` is accessible from the current class
- *  change the accessor to
+ *  If there is a chain of parameter accessors starting with `x` such that
+ *  (1) The last parameter accessor in the chain is a field that's accessible
+ *      from the current class, and
+ *  (2) each preceding parameter is forwarded in the supercall of its class
+ *      to a parameter that's also named `x`
+ *  then change the accessor to
  *
- *      def x: T = super.x.asInstanceOf[T]
+ *      private def x$accessor: T = super.x'.asInstanceOf[T]
+ *
+ *  where x' is a reference to the final parameter in the chain.
+ *  Property (1) is established by the @see forwardParamAccessors method in PostTyper.
+ *
+ *  The reason for renaming `x` to `x$accessor` is that private methods in the JVM
+ *  cannot override public ones.
  *
- *  Do the same also if there are intermediate inaccessible parameter accessor forwarders.
  *  The aim of this transformation is to avoid redundant parameter accessor fields.
  */
-class ParamForwarding(thisPhase: DenotTransformer) {
+class ParamForwarding extends MiniPhase with IdentityDenotTransformer:
   import ast.tpd._
 
-  def forwardParamAccessors(impl: Template)(implicit ctx: Context): Template = {
-    def fwd(stats: List[Tree])(implicit ctx: Context): List[Tree] = {
-      val (superArgs, superParamNames) = impl.parents match {
-        case superCall @ Apply(fn, args) :: _ =>
-          fn.tpe.widen match {
-            case MethodType(paramNames) => (args, paramNames)
-            case _ => (Nil, Nil)
-          }
-        case _ => (Nil, Nil)
-      }
-      def inheritedAccessor(sym: Symbol): Symbol = {
-        /**
-         * Dmitry: having it have the same name is needed to maintain correctness in presence of subclassing
-         * if you would use parent param-name `a` to implement param-field `b`
-         * overriding field `b` will actually override field `a`, that is wrong!
-         *
-         * class A(val s: Int);
-         * class B(val b: Int) extends A(b)
-         * class C extends A(2) {
-         *   def s = 3
-         *   assert(this.b == 2)
-         * }
-         */
-        val candidate = sym.owner.asClass.superClass
-          .info.decl(sym.name).suchThat(_.is(ParamAccessor, butNot = Mutable)).symbol
-        if (candidate.isAccessibleFrom(currentClass.thisType, superAccess = true)) candidate
-        else if (candidate.exists) inheritedAccessor(candidate)
-        else NoSymbol
-      }
-      def forwardParamAccessor(stat: Tree): Tree = {
-        stat match {
-          case stat: ValDef =>
-            val sym = stat.symbol.asTerm
-            if (sym.is(ParamAccessor, butNot = Mutable) && !sym.info.isInstanceOf[ExprType]) {
-              // ElimByName gets confused with methods returning an ExprType,
-              // so avoid param forwarding if parameter is by name. See i1766.scala
-              val idx = superArgs.indexWhere(_.symbol == sym)
-              if (idx >= 0 && superParamNames(idx) == stat.name) { // supercall to like-named parameter
-                val alias = inheritedAccessor(sym)
-                if (alias.exists) {
-                  def forwarder(implicit ctx: Context) = {
-                    sym.copySymDenotation(initFlags = sym.flags | Method | StableRealizable, info = sym.info.ensureMethodic)
-                      .installAfter(thisPhase)
-                    val superAcc =
-                      Super(This(currentClass), tpnme.EMPTY, inConstrCall = false).select(alias)
-                    typr.println(i"adding param forwarder $superAcc")
-                    DefDef(sym, superAcc.ensureConforms(sym.info.widen)).withSpan(stat.span)
-                  }
-                  return forwarder(ctx.withPhase(thisPhase.next))
-                }
-              }
-            }
-          case _ =>
-        }
-        stat
-      }
-      stats map forwardParamAccessor
-    }
+  private def thisPhase: ParamForwarding = this
+
+  val phaseName: String = "paramForwarding"
+
+  def transformIfParamAlias(mdef: ValOrDefDef)(using ctx: Context): Tree =
+
+    def inheritedAccessor(sym: Symbol)(using Context): Symbol =
+      val candidate = sym.owner.asClass.superClass
+        .info.decl(sym.name).suchThat(_.is(ParamAccessor, butNot = Mutable))
+        .symbol
+      if !candidate.is(Private)  // candidate might be private and accessible if it is in an outer class
+         && candidate.isAccessibleFrom(currentClass.thisType, superAccess = true)
+      then
+        candidate
+      else if candidate.is(SuperParamAlias) then
+        inheritedAccessor(candidate)
+      else
+        NoSymbol
+
+    val sym = mdef.symbol.asTerm
+    if sym.is(SuperParamAlias) then
+      assert(sym.is(ParamAccessor, butNot = Mutable))
+      val alias = inheritedAccessor(sym)(using ctx.withPhase(thisPhase))
+      if alias.exists then
+        sym.copySymDenotation(
+            name = ParamAccessorName(sym.name),
+            initFlags = sym.flags | Method | StableRealizable,
+            info = sym.info.ensureMethodic
+          ).installAfter(thisPhase)
+        val superAcc =
+          Super(This(currentClass), tpnme.EMPTY, inConstrCall = false)
+            .withSpan(mdef.span)
+            .select(alias)
+            .ensureApplied
+            .ensureConforms(sym.info.finalResultType)
+        ctx.log(i"adding param forwarder $superAcc")
+        DefDef(sym, superAcc)
+      else mdef
+    else mdef
+  end transformIfParamAlias
+
+  override def transformValDef(mdef: ValDef)(using Context): Tree =
+    transformIfParamAlias(mdef)
 
-    cpy.Template(impl)(body = fwd(impl.body)(ctx.withPhase(thisPhase)))
-  }
-}
+  override def transformDefDef(mdef: DefDef)(using Context): Tree =
+    transformIfParamAlias(mdef)
diff --git a/compiler/src/dotty/tools/dotc/transform/PostTyper.scala b/compiler/src/dotty/tools/dotc/transform/PostTyper.scala
@@ -11,6 +11,7 @@ import Types._, Contexts._, Names._, Flags._, DenotTransformers._, Phases._
 import SymDenotations._, StdNames._, Annotations._, Trees._, Scopes._
 import Decorators._
 import Symbols._, SymUtils._
+import config.Printers.typr
 import reporting.diagnostic.messages._
 
 object PostTyper {
@@ -24,7 +25,7 @@ object PostTyper {
  *  (2) Convert parameter fields that have the same name as a corresponding
  *      public parameter field in a superclass to a forwarder to the superclass
  *      field (corresponding = super class field is initialized with subclass field)
- *      (@see ForwardParamAccessors)
+ *      @see forwardParamAccessors.
  *
  *  (3) Add synthetic members (@see SyntheticMembers)
  *
@@ -72,7 +73,6 @@ class PostTyper extends MacroTransform with IdentityDenotTransformer { thisPhase
     new PostTyperTransformer
 
   val superAcc: SuperAccessors = new SuperAccessors(thisPhase)
-  val paramFwd: ParamForwarding = new ParamForwarding(thisPhase)
   val synthMbr: SyntheticMembers = new SyntheticMembers(thisPhase)
 
   private def newPart(tree: Tree): Option[New] = methPart(tree) match {
@@ -98,6 +98,30 @@ class PostTyper extends MacroTransform with IdentityDenotTransformer { thisPhase
 
     def isCheckable(t: New): Boolean = !inJavaAnnot && !noCheckNews.contains(t)
 
+    /** Mark parameter accessors that are aliases of like-named parameters
+     *  in their superclass with SuperParamAlias.
+     *  This info is used in phase ParamForwarding
+     */
+    private def forwardParamAccessors(impl: Template)(using Context): Unit = impl.parents match
+      case superCall @ Apply(fn, superArgs) :: _ if superArgs.nonEmpty =>
+        fn.tpe.widen match
+          case MethodType(superParamNames) =>
+            for case stat: ValDef <- impl.body do
+              val sym = stat.symbol
+              if sym.isAllOf(PrivateParamAccessor, butNot = Mutable)
+                 && !sym.info.isInstanceOf[ExprType] // val-parameters cannot be call-by name, so no need to try to forward to them
+              then
+                val idx = superArgs.indexWhere(_.symbol == sym)
+                if idx >= 0 && superParamNames(idx) == stat.name then
+                  // Supercall to like-named parameter.
+                  // Having it have the same name is needed to maintain correctness in presence of subclassing
+                  // if you would use parent param-name `a` to implement param-field `b`
+                  // overriding field `b` will actually override field `a`, that is wrong!
+                  typr.println(i"super alias: ${sym.showLocated}")
+                  sym.setFlag(SuperParamAlias)
+          case _ =>
+      case _ =>
+
     private def transformAnnot(annot: Tree)(implicit ctx: Context): Tree = {
       val saved = inJavaAnnot
       inJavaAnnot = annot.symbol.is(JavaDefined)
@@ -227,11 +251,11 @@ class PostTyper extends MacroTransform with IdentityDenotTransformer { thisPhase
           val pos = call.sourcePos
           val callTrace = Inliner.inlineCallTrace(call.symbol, pos)(ctx.withSource(pos.source))
           cpy.Inlined(tree)(callTrace, transformSub(bindings), transform(expansion)(inlineContext(call)))
-        case tree: Template =>
-          withNoCheckNews(tree.parents.flatMap(newPart)) {
-            val templ1 = paramFwd.forwardParamAccessors(tree)
+        case templ: Template =>
+          withNoCheckNews(templ.parents.flatMap(newPart)) {
+            forwardParamAccessors(templ)
             synthMbr.addSyntheticMembers(
-                superAcc.wrapTemplate(templ1)(
+                superAcc.wrapTemplate(templ)(
                   super.transform(_).asInstanceOf[Template]))
           }
         case tree: ValDef =>
diff --git a/library/src/scala/annotation/internal/Alias.scala b/library/src/scala/annotation/internal/Alias.scala
@@ -4,6 +4,7 @@ import scala.annotation.Annotation
 
 /** An annotation to record a Scala2 pickled alias.
  *  @param aliased  A TermRef pointing to the aliased field.
+ *  TODO: Drop once the new param alias scheme is in the bootstrap compiler
  */
 class Alias(aliased: Any) extends Annotation {
 
diff --git a/tasty/src/dotty/tools/tasty/TastyFormat.scala b/tasty/src/dotty/tools/tasty/TastyFormat.scala
diff --git a/tests/pos/paramAliases.scala b/tests/pos/paramAliases.scala
diff --git a/tests/run/i7926.check b/tests/run/i7926.check
diff --git a/tests/run/i7926.scala b/tests/run/i7926.scala
diff --git a/tests/run/paramForwarding.check b/tests/run/paramForwarding.check
diff --git a/tests/run/paramForwarding.scala b/tests/run/paramForwarding.scala

Original file line number	Diff line number	Diff line change
`@@ -682,6 +682,7 @@ class TreePickler(pickler: TastyPickler) {`
`682`	`682`	`if (flags.is(StableRealizable)) writeModTag(STABLE)`
`683`	`683`	`if (flags.is(Extension)) writeModTag(EXTENSION)`
`684`	`684`	`if (flags.is(ParamAccessor)) writeModTag(PARAMsetter)`
	`685`	`+ if (flags.is(SuperParamAlias)) writeModTag(PARAMalias)`
`685`	`686`	`if (flags.is(Exported)) writeModTag(EXPORTED)`
`686`	`687`	`assert(!(flags.is(Label)))`
`687`	`688`	`}`
Original file line number	Diff line number	Diff line change
`@@ -598,7 +598,7 @@ class Scala2Unpickler(bytes: Array[Byte], classRoot: ClassDenotation, moduleClas`
`598`	`598`	`denot.info matches denot.owner.thisType.memberInfo(alt)`
`599`	`599`	`}`
`600`	`600`	`val alias = readDisambiguatedSymbolRef(disambiguate).asTerm`
`601`		`- denot.addAnnotation(Annotation.makeAlias(alias))`
	`601`	`+ if alias.name == denot.name then denot.setFlag(SuperParamAlias)`
`602`	`602`	`}`
`603`	`603`	`}`
`604`	`604`	`// println(s"unpickled ${denot.debugString}, info = ${denot.info}") !!! DEBUG`