From 8fe3218654c7834d08fbceb53fd2eed79f5f1f71 Mon Sep 17 00:00:00 2001 From: Dmitry Petrashko Date: Thu, 18 Feb 2016 11:15:06 +0100 Subject: [PATCH 1/5] Erasure: no need to bridge paramaccessors. Just like normal accessors. --- src/dotty/tools/dotc/transform/Erasure.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dotty/tools/dotc/transform/Erasure.scala b/src/dotty/tools/dotc/transform/Erasure.scala index 3445b4c444ec..8d890902e805 100644 --- a/src/dotty/tools/dotc/transform/Erasure.scala +++ b/src/dotty/tools/dotc/transform/Erasure.scala @@ -611,7 +611,7 @@ object Erasure extends TypeTestsCasts{ traverse(newStats, oldStats) } - private final val NoBridgeFlags = Flags.Accessor | Flags.Deferred | Flags.Lazy + private final val NoBridgeFlags = Flags.Accessor | Flags.Deferred | Flags.Lazy | Flags.ParamAccessor /** Create a bridge DefDef which overrides a parent method. * From c3a78cf21901b2943caa823213f672c9849949c3 Mon Sep 17 00:00:00 2001 From: Dmitry Petrashko Date: Thu, 18 Feb 2016 11:15:48 +0100 Subject: [PATCH 2/5] Memoize: perform required tree adaptation in setter. Otherwise can create trees that do not pas Ycheck. --- src/dotty/tools/dotc/transform/Memoize.scala | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/dotty/tools/dotc/transform/Memoize.scala b/src/dotty/tools/dotc/transform/Memoize.scala index b775496ae4b6..1e4f8831bd25 100644 --- a/src/dotty/tools/dotc/transform/Memoize.scala +++ b/src/dotty/tools/dotc/transform/Memoize.scala @@ -68,6 +68,10 @@ import Decorators._ } lazy val field = sym.field.orElse(newField).asTerm + + def adaptToField(tree: Tree) = + if (tree.isEmpty) tree else tree.ensureConforms(field.info.widen) + if (sym.is(Accessor, butNot = NoFieldNeeded)) if (sym.isGetter) { def skipBlocks(t: Tree): Tree = t match { @@ -85,14 +89,15 @@ import Decorators._ case _ => var rhs = tree.rhs.changeOwnerAfter(sym, field, thisTransform) if (isWildcardArg(rhs)) rhs = EmptyTree - val fieldDef = transformFollowing(ValDef(field, rhs)) + + val fieldDef = transformFollowing(ValDef(field, adaptToField(rhs))) val getterDef = cpy.DefDef(tree)(rhs = transformFollowingDeep(ref(field))(ctx.withOwner(sym), info)) Thicket(fieldDef, getterDef) } } else if (sym.isSetter) { if (!sym.is(ParamAccessor)) { val Literal(Constant(())) = tree.rhs } // this is intended as an assertion field.setFlag(Mutable) // necessary for vals mixed in from Scala2 traits - val initializer = Assign(ref(field), ref(tree.vparamss.head.head.symbol)) + val initializer = Assign(ref(field), adaptToField(ref(tree.vparamss.head.head.symbol))) cpy.DefDef(tree)(rhs = transformFollowingDeep(initializer)(ctx.withOwner(sym), info)) } else tree // curiously, some accessors from Scala2 have ' ' suffixes. They count as From 614a2361ee6b7bcebf5bf6aa7fe159acd5e8ef19 Mon Sep 17 00:00:00 2001 From: Dmitry Petrashko Date: Thu, 18 Feb 2016 11:26:00 +0100 Subject: [PATCH 3/5] Change name of DropEmptyCompanions from dropEmpty to dropEmptyCompanions dropEmpty is not as clear, as it does not indicate what it drops. Additionally makes phaseName by in sync with class name. --- .../tools/dotc/transform/DropEmptyCompanions.scala.disabled | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dotty/tools/dotc/transform/DropEmptyCompanions.scala.disabled b/src/dotty/tools/dotc/transform/DropEmptyCompanions.scala.disabled index 65362f19914a..7b37c5881dc9 100644 --- a/src/dotty/tools/dotc/transform/DropEmptyCompanions.scala.disabled +++ b/src/dotty/tools/dotc/transform/DropEmptyCompanions.scala.disabled @@ -30,7 +30,7 @@ import dotty.tools.dotc.transform.TreeTransforms.TransformerInfo */ class DropEmptyCompanions extends MiniPhaseTransform { thisTransform => import ast.tpd._ - override def phaseName = "dropEmpty" + override def phaseName = "dropEmptyCompanions" override def runsAfter: Set[Class[_ <: Phase]] = Set(classOf[Flatten]) override def transformPackageDef(pdef: PackageDef)(implicit ctx: Context, info: TransformerInfo) = { From c103926177e1c5b34fc36036af8d686725490180 Mon Sep 17 00:00:00 2001 From: Dmitry Petrashko Date: Thu, 18 Feb 2016 12:57:03 +0100 Subject: [PATCH 4/5] Mixin: create less forwarders. There were two sources of inefficiency in previous scheme: - if symbol was no overriding anything the forwarder was still being created - the class that is will have the forwarder was not considered. Many methods do not require forwarders as JVM will dispatch correctly. --- src/dotty/tools/dotc/transform/MixinOps.scala | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/dotty/tools/dotc/transform/MixinOps.scala b/src/dotty/tools/dotc/transform/MixinOps.scala index db89f939bf7f..6cebf71970f8 100644 --- a/src/dotty/tools/dotc/transform/MixinOps.scala +++ b/src/dotty/tools/dotc/transform/MixinOps.scala @@ -41,11 +41,21 @@ class MixinOps(cls: ClassSymbol, thisTransform: DenotTransformer)(implicit ctx: ctx.atPhase(thisTransform) { implicit ctx => cls.info.member(sym.name).hasAltWith(_.symbol == sym) } - + + /** Does `method` need a forwarder to in class `cls` + * Method needs a forwarder in those cases: + * - there's a class defining a method with same signature + * - there are multiple traits defining method with same signature + */ def needsForwarder(meth: Symbol): Boolean = { - lazy val overridenSymbols = meth.allOverriddenSymbols - def needsDisambiguation = !overridenSymbols.forall(_ is Deferred) - def hasNonInterfaceDefinition = overridenSymbols.forall(!_.owner.is(Trait)) + lazy val competingMethods = cls.baseClasses.iterator + .filter(_ ne meth.owner) + .map(meth.overriddenSymbol) + .filter(_.exists) + .toList + + def needsDisambiguation = competingMethods.exists(x=> !(x is Deferred)) // multiple implementations are available + def hasNonInterfaceDefinition = competingMethods.exists(!_.owner.is(Trait)) // there is a definition originating from class meth.is(Method, butNot = PrivateOrAccessorOrDeferred) && isCurrent(meth) && (needsDisambiguation || hasNonInterfaceDefinition || meth.owner.is(Scala2x)) From 28a2c76952c753ea2c3efacfad93c87a63227259 Mon Sep 17 00:00:00 2001 From: Dmitry Petrashko Date: Thu, 18 Feb 2016 13:03:05 +0100 Subject: [PATCH 5/5] Add a test that checks that no useless forwarders are being created. --- tests/run/no-useless-forwarders.scala | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 tests/run/no-useless-forwarders.scala diff --git a/tests/run/no-useless-forwarders.scala b/tests/run/no-useless-forwarders.scala new file mode 100644 index 000000000000..699295027300 --- /dev/null +++ b/tests/run/no-useless-forwarders.scala @@ -0,0 +1,17 @@ +trait A { + def foo(a: Int): Int = a + def bar(a: Int): Int +} + +trait B { + def bar(a: Int): Int = a +} + +object Test extends A with B{ + def main(args: Array[String]) = { + assert(!this.getClass.getDeclaredMethods.exists{x: java.lang.reflect.Method => x.getName == "foo"}, + "no forwarder is needed here") + assert(!this.getClass.getDeclaredMethods.exists{x: java.lang.reflect.Method => x.getName == "bar"}, + "no forwarder is needed here") + } +}