From 11254b33db954ed44c722ca90476365345b1e4a7 Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Tue, 14 Mar 2017 15:58:26 +0100 Subject: [PATCH 1/2] 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. --- .../tools/dotc/transform/ExpandPrivate.scala | 36 +++++++------------ .../dotc/transform/ParamForwarding.scala | 7 ++-- tests/pos/i2099/A_1.scala | 3 ++ tests/pos/i2099/B_2.scala | 1 + 4 files changed, 22 insertions(+), 25 deletions(-) create mode 100644 tests/pos/i2099/A_1.scala create mode 100644 tests/pos/i2099/B_2.scala diff --git a/compiler/src/dotty/tools/dotc/transform/ExpandPrivate.scala b/compiler/src/dotty/tools/dotc/transform/ExpandPrivate.scala index 83cd395ff03e..f43abbccc7fa 100644 --- a/compiler/src/dotty/tools/dotc/transform/ExpandPrivate.scala +++ b/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 - } } diff --git a/compiler/src/dotty/tools/dotc/transform/ParamForwarding.scala b/compiler/src/dotty/tools/dotc/transform/ParamForwarding.scala index a72e10681f45..5b27d361a7f7 100644 --- a/compiler/src/dotty/tools/dotc/transform/ParamForwarding.scala +++ b/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) diff --git a/tests/pos/i2099/A_1.scala b/tests/pos/i2099/A_1.scala new file mode 100644 index 000000000000..7e01f3ef1df9 --- /dev/null +++ b/tests/pos/i2099/A_1.scala @@ -0,0 +1,3 @@ +class A(val member: Int) + +class SubA(member: Int) extends A(member) diff --git a/tests/pos/i2099/B_2.scala b/tests/pos/i2099/B_2.scala new file mode 100644 index 000000000000..6f022beacbac --- /dev/null +++ b/tests/pos/i2099/B_2.scala @@ -0,0 +1 @@ +class B(member: Int) extends SubA(member) From 5f9d66b3d5b544e80d8051d271fd548038996471 Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Tue, 14 Mar 2017 19:39:58 +0100 Subject: [PATCH 2/2] Alternative fix for #2099: Expand param forwarders early Making param forwarders override the param they forward like we did in the previous commit changes semantics. Instead, we do the same thing that ExpandPrivate previously did, but early in ParamForwarding, so separate compilation still works. --- .../dotc/transform/ParamForwarding.scala | 27 +++++++++++++------ 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/transform/ParamForwarding.scala b/compiler/src/dotty/tools/dotc/transform/ParamForwarding.scala index 5b27d361a7f7..24d761203b1d 100644 --- a/compiler/src/dotty/tools/dotc/transform/ParamForwarding.scala +++ b/compiler/src/dotty/tools/dotc/transform/ParamForwarding.scala @@ -4,6 +4,7 @@ package transform import core._ import ast.Trees._ import Contexts._, Types._, Symbols._, Flags._, TypeUtils._, DenotTransformers._, StdNames._ +import Names._, NameOps._ /** For all parameter accessors * @@ -32,6 +33,8 @@ class ParamForwarding(thisTransformer: DenotTransformer) { } case _ => (Nil, Nil) } + def findAccessor(cls: Symbol, name: Name): Symbol = + cls.info.decl(name).suchThat(_ is (ParamAccessor, butNot = Mutable)).symbol def inheritedAccessor(sym: Symbol): Symbol = { /** * Dmitry: having it have the same name is needed to maintain correctness in presence of subclassing @@ -44,9 +47,17 @@ class ParamForwarding(thisTransformer: DenotTransformer) { * def s = 3 * assert(this.b == 2) * } - */ - val candidate = sym.owner.asClass.superClass - .info.decl(sym.name).suchThat(_ is (ParamAccessor, butNot = (Mutable | Final))).symbol + */ + val superCls = sym.owner.asClass.superClass + val candidate = { + val candidate0 = findAccessor(superCls, sym.name) + if (candidate0.exists) + candidate0 + else + // If a forwarder was private, it will have been name-mangled by `ensureNotPrivate` + findAccessor(superCls, sym.name.expandedName(superCls)) + } + if (candidate.isAccessibleFrom(currentClass.thisType, superAccess = true)) candidate else if (candidate is Method) inheritedAccessor(candidate) else NoSymbol @@ -63,10 +74,9 @@ class ParamForwarding(thisTransformer: DenotTransformer) { val alias = inheritedAccessor(sym) if (alias.exists) { def forwarder(implicit ctx: Context) = { - // 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 + val flags = sym.flags | Method | Stable sym.copySymDenotation(initFlags = flags, info = sym.info.ensureMethodic) + .ensureNotPrivate .installAfter(thisTransformer) val superAcc = Super(This(currentClass), tpnme.EMPTY, inConstrCall = false).select(alias) @@ -89,9 +99,10 @@ class ParamForwarding(thisTransformer: DenotTransformer) { def adaptRef[T <: RefTree](tree: T)(implicit ctx: Context): T = tree.tpe match { case tpe: TermRefWithSignature if tpe.sig == Signature.NotAMethod && tpe.symbol.is(Method) => - // It's a param forwarder; adapt the signature + // It's a param forwarder; adapt the signature and the name + val name = tpe.symbol.asTerm.name tree.withType( - TermRef.withSig(tpe.prefix, tpe.name, tpe.prefix.memberInfo(tpe.symbol).signature)) + TermRef.withSig(tpe.prefix, name, tpe.prefix.memberInfo(tpe.symbol).signature)) .asInstanceOf[T] case _ => tree