Fix #2099: Make param forwarders override the param they forward to

Previously, in the following code:

class A(val member: Int)
class SubA(member: Int) extends A(member)

We created a forwarder in SubA like this:
  private[this] def member: Int = super.member
Since a private method cannot have the same name as an inherited
non-private one, we subsequently name-mangled the forwarder in
ExpandPrivate. However, this was only done in the TreeTransformer, not
in the DenotTransformer, this lead to separate compilation issues (see
added testcase).

Since we cannot detect that a ParamAccessor is a forwarder in a
DenotTransformer, we cannot fix the problem locally in ExpandPrivate.
Instead, we change ParamForwarding so that the forwarder now looks like:

  override def member: Int = super.member

To make this work we restrict ParamForwarding to only create forwarders
for non-final fields.

Author: smarter

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

@@ -3,12 +3,13 @@ package transform
 
 import core._
 import dotty.tools.dotc.ast.tpd
-import dotty.tools.dotc.core.DenotTransformers.{SymTransformer, IdentityDenotTransformer}
+import dotty.tools.dotc.core.DenotTransformers.{SymTransformer, DenotTransformer}
 import Contexts.Context
 import Symbols._
 import Scopes._
 import Flags._
 import StdNames._
+import Denotations._
 import SymDenotations._
 import Types._
 import collection.mutable
@@ -25,15 +26,10 @@ import ValueClasses._
  *  Make private accessor in value class not-private. Ihis is necessary to unbox
  *  the value class when accessing it from separate compilation units
  *
- *  Also, make non-private any private parameter forwarders that forward to an inherited
- *  public or protected parameter accessor with the same name as the forwarder.
- *  This is necessary since private methods are not allowed to have the same name
- *  as inherited public ones.
- *
  *  See discussion in https://github.com/lampepfl/dotty/pull/784
  *  and https://github.com/lampepfl/dotty/issues/783
  */
-class ExpandPrivate extends MiniPhaseTransform with IdentityDenotTransformer { thisTransform =>
+class ExpandPrivate extends MiniPhaseTransform with DenotTransformer { thisTransform =>
   import ast.tpd._
 
   override def phaseName: String = "expandPrivate"
@@ -56,6 +52,15 @@ class ExpandPrivate extends MiniPhaseTransform with IdentityDenotTransformer { t
     }
   }
 
+  override def transform(ref: SingleDenotation)(implicit ctx: Context): SingleDenotation =
+    ref match {
+      case ref: SymDenotation if isVCPrivateParamAccessor(ref) =>
+        ref.ensureNotPrivate
+      case _ =>
+        ref
+    }
+
+
   private def isVCPrivateParamAccessor(d: SymDenotation)(implicit ctx: Context) =
     d.isTerm && d.is(PrivateParamAccessor) && isDerivedValueClass(d.owner)
 
@@ -65,9 +70,7 @@ class ExpandPrivate extends MiniPhaseTransform with IdentityDenotTransformer { t
    *  static members of the companion class, we should tighten the condition below.
    */
   private def ensurePrivateAccessible(d: SymDenotation)(implicit ctx: Context) =
-    if (isVCPrivateParamAccessor(d))
-      d.ensureNotPrivate.installAfter(thisTransform)
-    else if (d.is(PrivateTerm) && d.owner != ctx.owner.enclosingClass) {
+    if (d.is(PrivateTerm) && d.owner != ctx.owner.enclosingClass) {
       // Paths `p1` and `p2` are similar if they have a common suffix that follows
       // possibly different directory paths. That is, their common suffix extends
       // in both cases either to the start of the path or to a file separator character.
@@ -95,17 +98,4 @@ class ExpandPrivate extends MiniPhaseTransform with IdentityDenotTransformer { t
     ensurePrivateAccessible(tree.symbol)
     tree
   }
-
-  override def transformDefDef(tree: DefDef)(implicit ctx: Context, info: TransformerInfo) = {
-    val sym = tree.symbol
-    tree.rhs match {
-      case Apply(sel @ Select(_: Super, _), _)
-      if sym.is(PrivateParamAccessor) && sel.symbol.is(ParamAccessor) && sym.name == sel.symbol.name =>
-        sym.ensureNotPrivate.installAfter(thisTransform)
-      case _ =>
-        if (isVCPrivateParamAccessor(sym))
-          sym.ensureNotPrivate.installAfter(thisTransform)
-    }
-    tree
-  }
 }

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

@@ -46,7 +46,7 @@ class ParamForwarding(thisTransformer: DenotTransformer) {
          * }
          */
         val candidate = sym.owner.asClass.superClass
-          .info.decl(sym.name).suchThat(_ is (ParamAccessor, butNot = Mutable)).symbol
+          .info.decl(sym.name).suchThat(_ is (ParamAccessor, butNot = (Mutable | Final))).symbol
         if (candidate.isAccessibleFrom(currentClass.thisType, superAccess = true)) candidate
         else if (candidate is Method) inheritedAccessor(candidate)
         else NoSymbol
@@ -63,7 +63,10 @@ class ParamForwarding(thisTransformer: DenotTransformer) {
                 val alias = inheritedAccessor(sym)
                 if (alias.exists) {
                   def forwarder(implicit ctx: Context) = {
-                    sym.copySymDenotation(initFlags = sym.flags | Method | Stable, info = sym.info.ensureMethodic)
+                    // A private method cannot have the same name as an inherited non-private one,
+                    // so the forwarder should have the same access flags as the alias.
+                    val flags = (sym.flags &~ Private) | (alias.flags & AccessFlags) | Override | Method | Stable
+                    sym.copySymDenotation(initFlags = flags, info = sym.info.ensureMethodic)
                       .installAfter(thisTransformer)
                     val superAcc =
                       Super(This(currentClass), tpnme.EMPTY, inConstrCall = false).select(alias)

tests/pos/i2099/A_1.scala

@@ -0,0 +1,3 @@
+class A(val member: Int)
+
+class SubA(member: Int) extends A(member)

tests/pos/i2099/B_2.scala

@@ -0,0 +1 @@
+class B(member: Int) extends SubA(member)