From adc4f8295eecc2c802067ff7375006564700fff6 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 1 Mar 2016 15:30:31 +0100 Subject: [PATCH 1/9] Fix a loophole where outer paths are missing at phase lambda lift. Test will come in llift.scala. --- src/dotty/tools/dotc/transform/ExplicitOuter.scala | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/dotty/tools/dotc/transform/ExplicitOuter.scala b/src/dotty/tools/dotc/transform/ExplicitOuter.scala index 024247734e38..3f93e64070a7 100644 --- a/src/dotty/tools/dotc/transform/ExplicitOuter.scala +++ b/src/dotty/tools/dotc/transform/ExplicitOuter.scala @@ -209,7 +209,7 @@ object ExplicitOuter { private def hasOuterParam(cls: ClassSymbol)(implicit ctx: Context): Boolean = !cls.is(Trait) && needsOuterIfReferenced(cls) && outerAccessor(cls).exists - /** Tree references a an outer class of `cls` which is not a static owner. + /** Tree references an outer class of `cls` which is not a static owner. */ def referencesOuter(cls: Symbol, tree: Tree)(implicit ctx: Context): Boolean = { def isOuter(sym: Symbol) = @@ -231,7 +231,12 @@ object ExplicitOuter { case _ => false } case nw: New => - isOuter(nw.tpe.classSymbol.owner.enclosingClass) + val newCls = nw.tpe.classSymbol + isOuter(newCls.owner.enclosingClass) || + newCls.owner.isTerm && cls.isProperlyContainedIn(newCls) + // newCls might get proxies for free variables. If current class is + // properly contained in newCls, it needs an outer path to newCls access the + // proxies and forward them to the new instance. case _ => false } From 67de3d35019816a1297215770f1a8012c390db0e Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 1 Mar 2016 15:55:09 +0100 Subject: [PATCH 2/9] Fix outer paths generated during lambda lift from constructors. If lambda lift needs to create an outer path from a constructor, the path needs to start from the $outer parameter of the constructor, not the this of the enclosing class, which is not yet available. --- src/dotty/tools/dotc/transform/ExplicitOuter.scala | 4 ++-- src/dotty/tools/dotc/transform/LambdaLift.scala | 8 +++++++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/dotty/tools/dotc/transform/ExplicitOuter.scala b/src/dotty/tools/dotc/transform/ExplicitOuter.scala index 3f93e64070a7..4cf076c45d3f 100644 --- a/src/dotty/tools/dotc/transform/ExplicitOuter.scala +++ b/src/dotty/tools/dotc/transform/ExplicitOuter.scala @@ -313,7 +313,7 @@ object ExplicitOuter { /** The path of outer accessors that references `toCls.this` starting from * the context owner's this node. */ - def path(toCls: Symbol): Tree = try { + def path(toCls: Symbol, start: Tree = This(ctx.owner.enclosingClass.asClass)): Tree = try { def loop(tree: Tree): 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 @@ -322,7 +322,7 @@ object ExplicitOuter { else loop(tree.select(outerAccessor(treeCls.asClass)(outerAccessorCtx)).ensureApplied) } ctx.log(i"computing outerpath to $toCls from ${ctx.outersIterator.map(_.owner).toList}") - loop(This(ctx.owner.enclosingClass.asClass)) + loop(start) } catch { case ex: ClassCastException => throw new ClassCastException(i"no path exists from ${ctx.owner.enclosingClass} to $toCls") diff --git a/src/dotty/tools/dotc/transform/LambdaLift.scala b/src/dotty/tools/dotc/transform/LambdaLift.scala index 19ad06085ffc..3b86ba13deb6 100644 --- a/src/dotty/tools/dotc/transform/LambdaLift.scala +++ b/src/dotty/tools/dotc/transform/LambdaLift.scala @@ -358,7 +358,13 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform private def memberRef(sym: Symbol)(implicit ctx: Context, info: TransformerInfo): Tree = { val clazz = sym.enclosingClass val qual = - if (clazz.isStaticOwner) singleton(clazz.thisType) + if (clazz.isStaticOwner || ctx.owner.enclosingClass == clazz) + singleton(clazz.thisType) + else if (ctx.owner.isConstructor) + outerParam.get(ctx.owner) match { + case Some(param) => outer.path(clazz, Ident(param.termRef)) + case _ => outer.path(clazz) + } else outer.path(clazz) transformFollowingDeep(qual.select(sym)) } From e94e8ef5200471a99facd818a6c1a25f994b163d Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 1 Mar 2016 16:43:53 +0100 Subject: [PATCH 3/9] Handle local traits in lambda lift --- .../tools/dotc/transform/LambdaLift.scala | 123 ++++++++++++++---- tests/pos/llift.scala | 9 -- tests/run/llift.scala | 100 ++++++++++++++ 3 files changed, 199 insertions(+), 33 deletions(-) delete mode 100644 tests/pos/llift.scala create mode 100644 tests/run/llift.scala diff --git a/src/dotty/tools/dotc/transform/LambdaLift.scala b/src/dotty/tools/dotc/transform/LambdaLift.scala index 3b86ba13deb6..480e07339ca8 100644 --- a/src/dotty/tools/dotc/transform/LambdaLift.scala +++ b/src/dotty/tools/dotc/transform/LambdaLift.scala @@ -26,6 +26,37 @@ object LambdaLift { private class NoPath extends Exception } +/** This phase performs the necessary rewritings to eliminate classes and methods + * nested in other methods. In detail: + * 1. It adds all free variables of local functions as additional parameters (proxies). + * 2. It rebinds references to free variables to the corresponding proxies, + * 3. It lifts all local functions and classes out as far as possible, but at least + * to the enclosing class. + * 4. It stores free variables of non-trait classes as additional fields of the class. + * The fields serve as proxies for methods in the class, which avoids the need + * of passing additional parameters to these methods. + * + * A particularly tricky case are local traits. These cannot store free variables + * as field proxies, because LambdaLift runs after Mixin, so the fields cannot be + * expanded anymore. Instead, methods of local traits get free variables of + * the trait as additional proxy parameters. The difference between local classes + * and local traits is illustrated by the two rewritings below. + * + * def f(x: Int) = { def f(x: Int) = new C(x).f2 + * class C { ==> class C(x$1: Int) { + * def f2 = x def f2 = x$1 + * } } + * new C().f2 + * } + * + * def f(x: Int) = { def f(x: Int) = new C().f2(x) + * trait T { ==> trait T + * def f2 = x def f2(x$1: Int) = x$1 + * } } + * class C extends T class C extends T + * new C().f2 + * } + */ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform => import LambdaLift._ import ast.tpd._ @@ -67,9 +98,20 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform * For methods and classes that do not have any dependencies this will be the enclosing package. * symbols with packages as lifted owners will subsequently represented as static * members of their toplevel class, unless their enclosing class was already static. + * Note: During tree transform (which runs at phase LambdaLift + 1), liftedOwner + * is also used to decide whether a method had a term owner before. */ private val liftedOwner = new HashMap[Symbol, Symbol] + /** All methods of local traits; these also get augmented with the free variables of + * methods enclosing the owning trait, because traits cannot cache free references + * like normal classes do. + */ + private val localTraitMethods = new HashSet[Symbol] + + /** The outer parameter of a constructor */ + private val outerParam = new HashMap[Symbol, Symbol] + /** Buffers for lifted out classes and methods, indexed by owner */ private val liftedDefs = new HashMap[Symbol, mutable.ListBuffer[Tree]] @@ -90,6 +132,12 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform free.getOrElse(sym, Nil).toList.map(pm) } + /** A symbol is local if it is owned by a term or a local trait */ + def isLocal(sym: Symbol)(implicit ctx: Context): Boolean = { + val owner = sym.maybeOwner + owner.isTerm || owner.is(Trait) && isLocal(owner) + } + /** Set `liftedOwner(sym)` to `owner` if `owner` is more deeply nested * than the previous value of `liftedowner(sym)`. */ @@ -149,26 +197,27 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform if (enclosure == sym.enclosure) NoSymbol else { ctx.log(i"mark free: ${sym.showLocated} with owner ${sym.maybeOwner} marked free in $enclosure") - ctx.debuglog(i"$enclosure != ${sym.enclosure}") val intermediate = if (enclosure.is(PackageClass)) enclosure else markFree(sym, enclosure.skipConstructor.enclosure) // `enclosure` might be a constructor, in which case we want the enclosure // of the enclosing class, so skipConstructor is needed here. - if (intermediate.exists) { - narrowLiftedOwner(enclosure, intermediate) - intermediate - } - else { - narrowLiftedOwner(enclosure, sym.enclosingClass) - val ss = symSet(free, enclosure) - if (!ss(sym)) { - ss += sym - changedFreeVars = true - ctx.debuglog(i"$sym is free in $enclosure") + narrowLiftedOwner(enclosure, intermediate orElse sym.enclosingClass) + if (!intermediate.isClass || intermediate.is(Trait)) { + // Methods nested inside traits get the free variables of the enclosing trait. + // Conversely, local traits do not get free variables. + if (!enclosure.is(Trait)) { + val ss = symSet(free, enclosure) + if (!ss(sym)) { + ss += sym + changedFreeVars = true + ctx.debuglog(i"$sym is free in $enclosure") + } } - if (enclosure.isClass) enclosure else NoSymbol } + if (intermediate.exists) intermediate + else if (enclosure.isClass) enclosure + else NoSymbol } } catch { case ex: NoPath => @@ -178,7 +227,7 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform private def markCalled(callee: Symbol, caller: Symbol)(implicit ctx: Context): Unit = { ctx.debuglog(i"mark called: $callee of ${callee.owner} is called by $caller") - assert(callee.skipConstructor.owner.isTerm) + assert(isLocal(callee.skipConstructor)) symSet(called, caller) += callee if (callee.enclosingClass != caller.enclosingClass) calledFromInner += callee } @@ -194,18 +243,18 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform else enclClass) // unknown this reference, play it safe and assume the narrowest possible owner } tree match { - case tree: Ident => - if (sym.maybeOwner.isTerm) { + case tree: RefTree => + if (isLocal(sym)) { if (sym is Label) assert(enclosure == sym.enclosure, i"attempt to refer to label $sym from nested $enclosure") else if (sym is Method) markCalled(sym, enclosure) else if (sym.isTerm) markFree(sym, enclosure) - } else if (sym.maybeOwner.isClass) + } else if (sym.maybeOwner.isClass) { narrowTo(sym.owner.asClass) - case tree: Select => - if (sym.isConstructor && sym.owner.owner.isTerm) - markCalled(sym, enclosure) + if (sym.isConstructor && sym.owner.owner.isTerm) + markCalled(sym, enclosure) + } case tree: This => narrowTo(tree.symbol.asClass) case tree: DefDef => @@ -216,8 +265,20 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform // On the other hand, all other methods will be indirectly owned by their // top-level class. This avoids possible deadlocks when a static method // has to access its enclosing object from the outside. - else if (sym.isPrimaryConstructor && sym.owner.owner.isTerm) - symSet(called, sym) += sym.owner + else if (sym.owner.is(Trait) && isLocal(sym.owner)) + localTraitMethods += sym + if (sym.isConstructor) { + if (sym.isPrimaryConstructor && sym.owner.owner.isTerm && !sym.owner.is(Trait)) + // add a call edge from the constructor of a local non-trait class to + // the class itself. This is done so that the constructor inherits + // the free variables of the class. + symSet(called, sym) += sym.owner + + tree.vparamss.head.find(_.name == nme.OUTER) match { + case Some(vdef) => outerParam(sym) = vdef.symbol + case _ => + } + } case tree: TypeDef => if (sym.owner.isTerm) liftedOwner(sym) = sym.topLevelClass.owner case tree: Template => @@ -251,7 +312,19 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform for { caller <- called.keys callee <- called(caller) - } narrowLiftedOwner(caller, liftedOwner(callee.skipConstructor)) + } { + val normalizedCallee = callee.skipConstructor + val calleeOwner = normalizedCallee.owner + if (calleeOwner.isTerm) narrowLiftedOwner(caller, liftedOwner(normalizedCallee)) + else { + assert(calleeOwner.is(Trait)) + // methods nested inside local trait methods cannot be lifted out + // beyond the trait. Note that we can also call a trait method through + // a qualifier; in that case no restriction to lifted owner arises. + if (caller.isContainedIn(calleeOwner)) + narrowLiftedOwner(caller, calleeOwner) + } + } } while (changedLiftedOwner) private def newName(sym: Symbol)(implicit ctx: Context): Name = @@ -312,6 +385,8 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform member.copySymDenotation(info = linfo).installAfter(thisTransform) } } + for (local <- localTraitMethods) + local.copySymDenotation(info = liftedInfo(local)).installAfter(thisTransform) } private def init(implicit ctx: Context) = { @@ -453,7 +528,7 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform override def transformDefDef(tree: DefDef)(implicit ctx: Context, info: TransformerInfo) = { val sym = tree.symbol val proxyHolder = sym.skipConstructor - if (needsLifting(proxyHolder)) { + if (needsLifting(proxyHolder) || localTraitMethods.contains(sym)) { val paramsAdded = addFreeParams(tree, proxies(proxyHolder)).asInstanceOf[DefDef] if (sym.isConstructor) paramsAdded else liftDef(paramsAdded) } diff --git a/tests/pos/llift.scala b/tests/pos/llift.scala deleted file mode 100644 index 51631ac970fd..000000000000 --- a/tests/pos/llift.scala +++ /dev/null @@ -1,9 +0,0 @@ -class A { - class B { - def outer(): Unit = { - def inner(): Int = 2 - - val fi: Function0[Int] = () => inner() - } - } -} diff --git a/tests/run/llift.scala b/tests/run/llift.scala new file mode 100644 index 000000000000..0c8c9b4be202 --- /dev/null +++ b/tests/run/llift.scala @@ -0,0 +1,100 @@ +class A { + class B { + def outer(): Unit = { + def inner(): Int = 2 + + val fi: Function0[Int] = () => inner() + } + } +} +object Test { + def foo(x: Int) = { + trait B { + def bar = x + } + class C extends B { + override def bar = super[B].bar + } + new C().bar + } + + + def f1(x: Int) = { + class C1 { + def f2 = { + class C2 { + def f3 = { + def f4 = x + f4 + } + } + new C2().f3 + } + } + new C1().f2 + } + + def f1a(x: Int) = { +// class C1 { + def f2 = { + trait T2 { + def f3 = { + def f4 = x + def f5 = f4 + f5 + } + } + class C2 extends T2 + new C2().f3 + } +// } + /*new C1().*/f2 + } + + def f1b(x: Int) = { + class T1 { + def f2 = { + trait T2 { + def f3 = { + def f4 = x + def f5 = f4 + f5 + } + } + class C2 extends T2 + new C2().f3 + } + } + class C1 extends T1 + new C1().f2 + } + + def f1c(x: Int) = { + class T1 { + def f2 = { + trait T2 { + def f3: Int = { + def f4 = x + def f5 = f4 + def f7 = this.f3 + f5 + } + } + class C2 extends T2 + class C3 extends T1 + new C2().f3 + new C3().f6 + } + def f6 = x + } + class C1 extends T1 + new C1().f2 + } + + def main(args: Array[String]) = { + assert(foo(3) == 3) + assert(f1(4) == 4) + assert(f1a(5) == 5) + assert(f1b(6) == 6) + assert(f1c(6) == 12) + } +} From 9d3424da9e34905a20783d4f597d1bb02d0df5d0 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Wed, 2 Mar 2016 14:55:16 +0100 Subject: [PATCH 4/9] Simplification: get rid of local trait methods --- .../tools/dotc/transform/LambdaLift.scala | 34 ++++++------------- 1 file changed, 10 insertions(+), 24 deletions(-) diff --git a/src/dotty/tools/dotc/transform/LambdaLift.scala b/src/dotty/tools/dotc/transform/LambdaLift.scala index 480e07339ca8..9b0f70160a22 100644 --- a/src/dotty/tools/dotc/transform/LambdaLift.scala +++ b/src/dotty/tools/dotc/transform/LambdaLift.scala @@ -103,12 +103,6 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform */ private val liftedOwner = new HashMap[Symbol, Symbol] - /** All methods of local traits; these also get augmented with the free variables of - * methods enclosing the owning trait, because traits cannot cache free references - * like normal classes do. - */ - private val localTraitMethods = new HashSet[Symbol] - /** The outer parameter of a constructor */ private val outerParam = new HashMap[Symbol, Symbol] @@ -265,15 +259,13 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform // On the other hand, all other methods will be indirectly owned by their // top-level class. This avoids possible deadlocks when a static method // has to access its enclosing object from the outside. - else if (sym.owner.is(Trait) && isLocal(sym.owner)) - localTraitMethods += sym - if (sym.isConstructor) { + else if (sym.isConstructor) { if (sym.isPrimaryConstructor && sym.owner.owner.isTerm && !sym.owner.is(Trait)) // add a call edge from the constructor of a local non-trait class to // the class itself. This is done so that the constructor inherits // the free variables of the class. symSet(called, sym) += sym.owner - + tree.vparamss.head.find(_.name == nme.OUTER) match { case Some(vdef) => outerParam(sym) = vdef.symbol case _ => @@ -377,16 +369,10 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform initFlags = local.flags &~ (InSuperCall | Module) | Private | maybeStatic, // drop Module because class is no longer a singleton in the lifted context. info = liftedInfo(local)).installAfter(thisTransform) - if (local.isClass) - for (member <- local.asClass.info.decls) - if (member.isConstructor) { - val linfo = liftedInfo(member) - if (linfo ne member.info) - member.copySymDenotation(info = linfo).installAfter(thisTransform) - } } - for (local <- localTraitMethods) - local.copySymDenotation(info = liftedInfo(local)).installAfter(thisTransform) + for (local <- free.keys) + if (!liftedOwner.contains(local)) + local.copySymDenotation(info = liftedInfo(local)).installAfter(thisTransform) } private def init(implicit ctx: Context) = { @@ -528,11 +514,11 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform override def transformDefDef(tree: DefDef)(implicit ctx: Context, info: TransformerInfo) = { val sym = tree.symbol val proxyHolder = sym.skipConstructor - if (needsLifting(proxyHolder) || localTraitMethods.contains(sym)) { - val paramsAdded = addFreeParams(tree, proxies(proxyHolder)).asInstanceOf[DefDef] - if (sym.isConstructor) paramsAdded else liftDef(paramsAdded) - } - else tree + val paramsAdded = + if (free.contains(sym)) addFreeParams(tree, proxies(proxyHolder)).asInstanceOf[DefDef] + else tree + if (needsLifting(sym)) liftDef(paramsAdded) + else paramsAdded } override def transformReturn(tree: Return)(implicit ctx: Context, info: TransformerInfo) = tree.expr match { From 69b6b892793e45a7158d006cdfb5554edc5db633 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Wed, 2 Mar 2016 17:13:16 +0100 Subject: [PATCH 5/9] LambdaLift redesign Simplifications in order to avoid the freqent special casing of constructors and prepare the way for proper handling of trait constructors (which cause problems; see pending/pos/llift.scala. --- .../tools/dotc/transform/LambdaLift.scala | 75 ++++++++----------- src/dotty/tools/dotc/transform/SymUtils.scala | 14 +++- tests/pending/pos/llift.scala | 16 ++++ tests/run/llift.scala | 46 +++++++++++- 4 files changed, 102 insertions(+), 49 deletions(-) create mode 100644 tests/pending/pos/llift.scala diff --git a/src/dotty/tools/dotc/transform/LambdaLift.scala b/src/dotty/tools/dotc/transform/LambdaLift.scala index 9b0f70160a22..dfc8c77dc1e6 100644 --- a/src/dotty/tools/dotc/transform/LambdaLift.scala +++ b/src/dotty/tools/dotc/transform/LambdaLift.scala @@ -126,10 +126,14 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform free.getOrElse(sym, Nil).toList.map(pm) } - /** A symbol is local if it is owned by a term or a local trait */ + /** A symbol is local if it is owned by a term or a local trait, + * or if it is a constructor of a local symbol. + */ def isLocal(sym: Symbol)(implicit ctx: Context): Boolean = { val owner = sym.maybeOwner - owner.isTerm || owner.is(Trait) && isLocal(owner) + owner.isTerm || + owner.is(Trait) && isLocal(owner) || + sym.isConstructor && isLocal(sym.owner) } /** Set `liftedOwner(sym)` to `owner` if `owner` is more deeply nested @@ -186,18 +190,17 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform * } * } */ - private def markFree(sym: Symbol, enclosure: Symbol)(implicit ctx: Context): Symbol = try { + private def markFree(sym: Symbol, enclosure: Symbol, propagating: Boolean = false)(implicit ctx: Context): Symbol = try { if (!enclosure.exists) throw new NoPath if (enclosure == sym.enclosure) NoSymbol else { - ctx.log(i"mark free: ${sym.showLocated} with owner ${sym.maybeOwner} marked free in $enclosure") + ctx.debuglog(i"mark free: ${sym.showLocated} with owner ${sym.maybeOwner} marked free in $enclosure") val intermediate = if (enclosure.is(PackageClass)) enclosure - else markFree(sym, enclosure.skipConstructor.enclosure) - // `enclosure` might be a constructor, in which case we want the enclosure - // of the enclosing class, so skipConstructor is needed here. + else markFree(sym, enclosure.enclosure) narrowLiftedOwner(enclosure, intermediate orElse sym.enclosingClass) - if (!intermediate.isClass || intermediate.is(Trait)) { + if (!intermediate.isClass || intermediate.is(Trait) || + enclosure.isConstructor && propagating) { // Methods nested inside traits get the free variables of the enclosing trait. // Conversely, local traits do not get free variables. if (!enclosure.is(Trait)) { @@ -205,7 +208,7 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform if (!ss(sym)) { ss += sym changedFreeVars = true - ctx.debuglog(i"$sym is free in $enclosure") + ctx.log(i"$sym is free in $enclosure") } } } @@ -221,14 +224,13 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform private def markCalled(callee: Symbol, caller: Symbol)(implicit ctx: Context): Unit = { ctx.debuglog(i"mark called: $callee of ${callee.owner} is called by $caller") - assert(isLocal(callee.skipConstructor)) + assert(isLocal(callee)) symSet(called, caller) += callee if (callee.enclosingClass != caller.enclosingClass) calledFromInner += callee } private class CollectDependencies extends EnclosingMethodTraverser { - def traverse(enclMeth: Symbol, tree: Tree)(implicit ctx: Context) = try { //debug - val enclosure = enclMeth.skipConstructor + def traverse(enclosure: Symbol, tree: Tree)(implicit ctx: Context) = try { //debug val sym = tree.symbol def narrowTo(thisClass: ClassSymbol) = { val enclClass = enclosure.enclosingClass @@ -237,18 +239,17 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform else enclClass) // unknown this reference, play it safe and assume the narrowest possible owner } tree match { - case tree: RefTree => + case tree: Ident => if (isLocal(sym)) { if (sym is Label) assert(enclosure == sym.enclosure, i"attempt to refer to label $sym from nested $enclosure") else if (sym is Method) markCalled(sym, enclosure) else if (sym.isTerm) markFree(sym, enclosure) - } else if (sym.maybeOwner.isClass) { - narrowTo(sym.owner.asClass) - if (sym.isConstructor && sym.owner.owner.isTerm) - markCalled(sym, enclosure) } + if (sym.maybeOwner.isClass) narrowTo(sym.owner.asClass) + case tree: Select => + if (sym.is(Method) && isLocal(sym)) markCalled(sym, enclosure) case tree: This => narrowTo(tree.symbol.asClass) case tree: DefDef => @@ -294,7 +295,7 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform callee <- called(caller) fvs <- free get callee fv <- fvs - } markFree(fv, caller) + } markFree(fv, caller, propagating = true) } while (changedFreeVars) /** Compute final liftedOwner map by closing over caller dependencies */ @@ -340,7 +341,7 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform private def liftedInfo(local: Symbol)(implicit ctx: Context): Type = local.info match { case mt @ MethodType(pnames, ptypes) => - val ps = proxies(local.skipConstructor) + val ps = proxies(local) MethodType( ps.map(_.name.asTermName) ++ pnames, ps.map(_.info) ++ ptypes, @@ -390,16 +391,17 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform } private def currentEnclosure(implicit ctx: Context) = - ctx.owner.enclosingMethod.skipConstructor + ctx.owner.enclosingMethodOrClass private def inCurrentOwner(sym: Symbol)(implicit ctx: Context) = sym.enclosure == currentEnclosure private def proxy(sym: Symbol)(implicit ctx: Context): Symbol = { + def liftedEnclosure(sym: Symbol) = liftedOwner.getOrElse(sym, sym.enclosure) def searchIn(enclosure: Symbol): Symbol = { if (!enclosure.exists) { def enclosures(encl: Symbol): List[Symbol] = - if (encl.exists) encl :: enclosures(encl.enclosure) else Nil + if (encl.exists) encl :: enclosures(liftedEnclosure(encl)) else Nil throw new IllegalArgumentException(i"Could not find proxy for ${sym.showDcl} in ${sym.ownersIterator.toList}, encl = $currentEnclosure, owners = ${currentEnclosure.ownersIterator.toList}%, %; enclosures = ${enclosures(currentEnclosure)}%, %") } ctx.debuglog(i"searching for $sym(${sym.owner}) in $enclosure") @@ -411,7 +413,7 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform } case none => } - searchIn(enclosure.enclosure) + searchIn(liftedEnclosure(enclosure)) } if (inCurrentOwner(sym)) sym else searchIn(currentEnclosure) } @@ -445,35 +447,25 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform case Nil => tree case proxies => val sym = tree.symbol - val ownProxies = - if (!sym.isConstructor) proxies - else proxies.map(_.copy(owner = sym, flags = Synthetic | Param)) - val freeParamDefs = ownProxies.map(proxy => + val freeParamDefs = proxies.map(proxy => transformFollowingDeep(ValDef(proxy.asTerm).withPos(tree.pos)).asInstanceOf[ValDef]) def proxyInit(field: Symbol, param: Symbol) = transformFollowingDeep(memberRef(field).becomes(ref(param))) - /** Map references to proxy fields `this.proxy` to proxy parameters */ - def mapProxies = new TreeMap { - override def transform(tree: Tree)(implicit ctx: Context) = tree match { - case Select(This(_), _) if proxies contains tree.symbol => - ref(tree.symbol.subst(proxies, ownProxies)) - case _ => - super.transform(tree) - } - } - /** Initialize proxy fields from proxy parameters and map `rhs` from fields to parameters */ def copyParams(rhs: Tree) = { - ctx.log(i"copy params ${proxies.map(_.showLocated)}%, %, own = ${ownProxies.map(_.showLocated)}%, %") - seq((proxies, ownProxies).zipped.map(proxyInit), mapProxies.transform(rhs)) + val classProxies = this.proxies(sym.owner) + ctx.debuglog(i"copy params ${proxies.map(_.showLocated)}%, % to ${classProxies.map(_.showLocated)}%, %}") + seq((classProxies, proxies).zipped.map(proxyInit), rhs) } tree match { case tree: DefDef => cpy.DefDef(tree)( vparamss = tree.vparamss.map(freeParamDefs ++ _), - rhs = if (sym.isPrimaryConstructor) copyParams(tree.rhs) else tree.rhs) + rhs = + if (sym.isPrimaryConstructor && !sym.owner.is(Trait)) copyParams(tree.rhs) + else tree.rhs) case tree: Template => cpy.Template(tree)(body = freeParamDefs ++ tree.body) } @@ -506,16 +498,15 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform } override def transformApply(tree: Apply)(implicit ctx: Context, info: TransformerInfo) = - cpy.Apply(tree)(tree.fun, addFreeArgs(tree.symbol.skipConstructor, tree.args)).withPos(tree.pos) + cpy.Apply(tree)(tree.fun, addFreeArgs(tree.symbol, tree.args)).withPos(tree.pos) override def transformClosure(tree: Closure)(implicit ctx: Context, info: TransformerInfo) = cpy.Closure(tree)(env = addFreeArgs(tree.meth.symbol, tree.env)) override def transformDefDef(tree: DefDef)(implicit ctx: Context, info: TransformerInfo) = { val sym = tree.symbol - val proxyHolder = sym.skipConstructor val paramsAdded = - if (free.contains(sym)) addFreeParams(tree, proxies(proxyHolder)).asInstanceOf[DefDef] + if (free.contains(sym)) addFreeParams(tree, proxies(sym)).asInstanceOf[DefDef] else tree if (needsLifting(sym)) liftDef(paramsAdded) else paramsAdded diff --git a/src/dotty/tools/dotc/transform/SymUtils.scala b/src/dotty/tools/dotc/transform/SymUtils.scala index 9d4fa9788df2..05305575e91c 100644 --- a/src/dotty/tools/dotc/transform/SymUtils.scala +++ b/src/dotty/tools/dotc/transform/SymUtils.scala @@ -53,10 +53,16 @@ class SymUtils(val self: Symbol) extends AnyVal { final def skipConstructor(implicit ctx: Context): Symbol = if (self.isConstructor) self.owner else self - /** The logically enclosing method or class for this symbol. - * Instead of constructors one always picks the enclosing class. - */ - final def enclosure(implicit ctx: Context) = self.owner.enclosingMethod.skipConstructor + /** The closest properly enclosing method or class of this symbol. */ + final def enclosure(implicit ctx: Context) = { + self.owner.enclosingMethodOrClass + } + + /** The closest enclosing method or class of this symbol */ + final def enclosingMethodOrClass(implicit ctx: Context): Symbol = + if (self.is(Method, butNot = Label) || self.isClass) self + else if (self.exists) self.owner.enclosingMethodOrClass + else NoSymbol /** Apply symbol/symbol substitution to this symbol */ def subst(from: List[Symbol], to: List[Symbol]): Symbol = { diff --git a/tests/pending/pos/llift.scala b/tests/pending/pos/llift.scala new file mode 100644 index 000000000000..b2a1e163b668 --- /dev/null +++ b/tests/pending/pos/llift.scala @@ -0,0 +1,16 @@ +object Test { + def f1d(x: Int) = { + trait T1 { +// def f2 = { + trait T2 { + def f3: Int = x + } + class C2 extends T2 + new C2().f3 +// } + def f6 = x + } + class C1 extends T1 + new C1().f6 + } +} diff --git a/tests/run/llift.scala b/tests/run/llift.scala index 0c8c9b4be202..28456b8ccdfc 100644 --- a/tests/run/llift.scala +++ b/tests/run/llift.scala @@ -7,6 +7,7 @@ class A { } } } + object Test { def foo(x: Int) = { trait B { @@ -18,7 +19,6 @@ object Test { new C().bar } - def f1(x: Int) = { class C1 { def f2 = { @@ -79,10 +79,11 @@ object Test { def f7 = this.f3 f5 } + def f3a = f3 } class C2 extends T2 class C3 extends T1 - new C2().f3 + new C3().f6 + new C2().f3a + new C3().f6 } def f6 = x } @@ -90,11 +91,50 @@ object Test { new C1().f2 } + def f1d(x: Int) = { + trait T1 { + def f2 = { + trait T2 { + def f3: Int = { + def f4 = x + def f5 = f4 + def f7 = this.f3 + f5 + } + def f3a = f3 + } + class C2 extends T2 + class C3 extends T1 + new C2().f3a + new C3().f6 + } + def f6 = x + } + class C1 extends T1 + new C1().f2 + } + + def f1e(x: Int) = { + trait T1 { + def f2 = { + trait T2 { + def f3: Int = x + } + class C2 extends T2 + new C2().f3 + } + def f6 = x + } + class C1 extends T1 + new C1().f6 + } + def main(args: Array[String]) = { assert(foo(3) == 3) assert(f1(4) == 4) assert(f1a(5) == 5) assert(f1b(6) == 6) - assert(f1c(6) == 12) + assert(f1c(7) == 14) + assert(f1d(8) == 16) + assert(f1e(9) == 9) } } From 25da2152f89c9c8a25188222fa395951b064e639 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Wed, 2 Mar 2016 17:52:50 +0100 Subject: [PATCH 6/9] Fix call propagation from constructor to class --- src/dotty/tools/dotc/Compiler.scala | 2 +- src/dotty/tools/dotc/transform/LambdaLift.scala | 6 +++--- tests/pending/pos/llift.scala | 16 ---------------- tests/run/llift.scala | 16 ++++++++++++++++ 4 files changed, 20 insertions(+), 20 deletions(-) delete mode 100644 tests/pending/pos/llift.scala diff --git a/src/dotty/tools/dotc/Compiler.scala b/src/dotty/tools/dotc/Compiler.scala index d526903b8015..967fb395aaac 100644 --- a/src/dotty/tools/dotc/Compiler.scala +++ b/src/dotty/tools/dotc/Compiler.scala @@ -75,7 +75,7 @@ class Compiler { new Constructors, // constructors changes decls in transformTemplate, no InfoTransformers should be added after it new FunctionalInterfaces, new GetClass), // getClass transformation should be applied to specialized methods - List(new LambdaLift, // in this mini-phase block scopes are incorrect. No phases that rely on scopes should be here + List(new LambdaLift, // in this mini-phase block scopes are incorrect. No phases that rely on scopes should be here new ElimStaticThis, new Flatten, // new DropEmptyCompanions, diff --git a/src/dotty/tools/dotc/transform/LambdaLift.scala b/src/dotty/tools/dotc/transform/LambdaLift.scala index dfc8c77dc1e6..8b79bec58ec4 100644 --- a/src/dotty/tools/dotc/transform/LambdaLift.scala +++ b/src/dotty/tools/dotc/transform/LambdaLift.scala @@ -133,7 +133,7 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform val owner = sym.maybeOwner owner.isTerm || owner.is(Trait) && isLocal(owner) || - sym.isConstructor && isLocal(sym.owner) + sym.isConstructor && isLocal(owner) } /** Set `liftedOwner(sym)` to `owner` if `owner` is more deeply nested @@ -223,7 +223,7 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform } private def markCalled(callee: Symbol, caller: Symbol)(implicit ctx: Context): Unit = { - ctx.debuglog(i"mark called: $callee of ${callee.owner} is called by $caller") + ctx.debuglog(i"mark called: $callee of ${callee.owner} is called by $caller in ${caller.owner}") assert(isLocal(callee)) symSet(called, caller) += callee if (callee.enclosingClass != caller.enclosingClass) calledFromInner += callee @@ -261,7 +261,7 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform // top-level class. This avoids possible deadlocks when a static method // has to access its enclosing object from the outside. else if (sym.isConstructor) { - if (sym.isPrimaryConstructor && sym.owner.owner.isTerm && !sym.owner.is(Trait)) + if (sym.isPrimaryConstructor && isLocal(sym.owner) && !sym.owner.is(Trait)) // add a call edge from the constructor of a local non-trait class to // the class itself. This is done so that the constructor inherits // the free variables of the class. diff --git a/tests/pending/pos/llift.scala b/tests/pending/pos/llift.scala deleted file mode 100644 index b2a1e163b668..000000000000 --- a/tests/pending/pos/llift.scala +++ /dev/null @@ -1,16 +0,0 @@ -object Test { - def f1d(x: Int) = { - trait T1 { -// def f2 = { - trait T2 { - def f3: Int = x - } - class C2 extends T2 - new C2().f3 -// } - def f6 = x - } - class C1 extends T1 - new C1().f6 - } -} diff --git a/tests/run/llift.scala b/tests/run/llift.scala index 28456b8ccdfc..60a1f4dcefc5 100644 --- a/tests/run/llift.scala +++ b/tests/run/llift.scala @@ -128,6 +128,21 @@ object Test { new C1().f6 } + def f1f(x: Int) = { + trait T1 { + trait T2 { + def f3: Int = x + } + class C2 extends T2 { + override def f3 = super.f3 + } + new C2().f3 + def f6 = x + } + class C1 extends T1 + new C1().f6 + } + def main(args: Array[String]) = { assert(foo(3) == 3) assert(f1(4) == 4) @@ -136,5 +151,6 @@ object Test { assert(f1c(7) == 14) assert(f1d(8) == 16) assert(f1e(9) == 9) + assert(f1f(10) == 10) } } From f02cb0cc910e3320eeb19e2b1cad7e03a16c9c42 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Thu, 3 Mar 2016 11:48:25 +0100 Subject: [PATCH 7/9] Cleanup lambda lift 1. Make clearer what markFree is supposed to do and get rid of `propagated` mode bit. 2. Harden copyParams so that we make sure corresponding parameters and fields are copied. --- .../tools/dotc/transform/LambdaLift.scala | 45 +++++++++++-------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/src/dotty/tools/dotc/transform/LambdaLift.scala b/src/dotty/tools/dotc/transform/LambdaLift.scala index 8b79bec58ec4..b140746450dc 100644 --- a/src/dotty/tools/dotc/transform/LambdaLift.scala +++ b/src/dotty/tools/dotc/transform/LambdaLift.scala @@ -121,10 +121,11 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform private def symSet(f: LinkedHashMap[Symbol, SymSet], sym: Symbol): SymSet = f.getOrElseUpdate(sym, newSymSet) - def proxies(sym: Symbol): List[Symbol] = { - val pm: Map[Symbol, Symbol] = proxyMap.getOrElse(sym, Map.empty) // Dotty deviation: Type annotation needed. TODO: figure out why - free.getOrElse(sym, Nil).toList.map(pm) - } + def freeVars(sym: Symbol): List[Symbol] = free.getOrElse(sym, Nil).toList + + def proxyOf(sym: Symbol, fv: Symbol) = proxyMap.getOrElse(sym, Map.empty)(fv) + + def proxies(sym: Symbol): List[Symbol] = freeVars(sym).map(proxyOf(sym, _)) /** A symbol is local if it is owned by a term or a local trait, * or if it is a constructor of a local symbol. @@ -139,7 +140,7 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform /** Set `liftedOwner(sym)` to `owner` if `owner` is more deeply nested * than the previous value of `liftedowner(sym)`. */ - def narrowLiftedOwner(sym: Symbol, owner: Symbol)(implicit ctx: Context) = { + def narrowLiftedOwner(sym: Symbol, owner: Symbol)(implicit ctx: Context) = if (sym.maybeOwner.isTerm && owner.isProperlyContainedIn(liftedOwner(sym)) && owner != sym) { @@ -147,7 +148,6 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform changedLiftedOwner = true liftedOwner(sym) = owner } - } /** Mark symbol `sym` as being free in `enclosure`, unless `sym` is defined * in `enclosure` or there is an intermediate class properly containing `enclosure` @@ -162,10 +162,13 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform * 2. If there is no intermediate class, `enclosure` must be contained * in the class enclosing `sym`. * - * Return the closest enclosing intermediate class between `enclosure` and - * the owner of sym, or NoSymbol if none exists. + * @return If there is a non-trait class between `enclosure` and + * the owner of `sym`, the largest such class. + * Otherwise, if there is a trait between `enclosure` and + * the owner of `sym`, the largest such trait. + * Otherwise, NoSymbol. * - * pre: sym.owner.isTerm, (enclosure.isMethod || enclosure.isClass) + * @pre sym.owner.isTerm, (enclosure.isMethod || enclosure.isClass) * * The idea of `markFree` is illustrated with an example: * @@ -190,7 +193,7 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform * } * } */ - private def markFree(sym: Symbol, enclosure: Symbol, propagating: Boolean = false)(implicit ctx: Context): Symbol = try { + private def markFree(sym: Symbol, enclosure: Symbol)(implicit ctx: Context): Symbol = try { if (!enclosure.exists) throw new NoPath if (enclosure == sym.enclosure) NoSymbol else { @@ -199,9 +202,9 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform if (enclosure.is(PackageClass)) enclosure else markFree(sym, enclosure.enclosure) narrowLiftedOwner(enclosure, intermediate orElse sym.enclosingClass) - if (!intermediate.isClass || intermediate.is(Trait) || - enclosure.isConstructor && propagating) { - // Methods nested inside traits get the free variables of the enclosing trait. + if (!intermediate.isRealClass || enclosure.isConstructor) { + // Constructors and methods nested inside traits get the free variables + // of the enclosing trait or class. // Conversely, local traits do not get free variables. if (!enclosure.is(Trait)) { val ss = symSet(free, enclosure) @@ -212,7 +215,9 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform } } } - if (intermediate.exists) intermediate + if (intermediate.isRealClass) intermediate + else if (enclosure.isRealClass) enclosure + else if (intermediate.isClass) intermediate else if (enclosure.isClass) enclosure else NoSymbol } @@ -261,7 +266,7 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform // top-level class. This avoids possible deadlocks when a static method // has to access its enclosing object from the outside. else if (sym.isConstructor) { - if (sym.isPrimaryConstructor && isLocal(sym.owner) && !sym.owner.is(Trait)) + if (false && sym.isPrimaryConstructor && isLocal(sym.owner) && !sym.owner.is(Trait)) // add a call edge from the constructor of a local non-trait class to // the class itself. This is done so that the constructor inherits // the free variables of the class. @@ -295,7 +300,7 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform callee <- called(caller) fvs <- free get callee fv <- fvs - } markFree(fv, caller, propagating = true) + } markFree(fv, caller) } while (changedFreeVars) /** Compute final liftedOwner map by closing over caller dependencies */ @@ -454,9 +459,11 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform /** Initialize proxy fields from proxy parameters and map `rhs` from fields to parameters */ def copyParams(rhs: Tree) = { - val classProxies = this.proxies(sym.owner) - ctx.debuglog(i"copy params ${proxies.map(_.showLocated)}%, % to ${classProxies.map(_.showLocated)}%, %}") - seq((classProxies, proxies).zipped.map(proxyInit), rhs) + val fvs = freeVars(sym.owner) + val classProxies = fvs.map(proxyOf(sym.owner, _)) + val constrProxies = fvs.map(proxyOf(sym, _)) + ctx.debuglog(i"copy params ${constrProxies.map(_.showLocated)}%, % to ${classProxies.map(_.showLocated)}%, %}") + seq((classProxies, constrProxies).zipped.map(proxyInit), rhs) } tree match { From ff27c8c12f1f584718acb8799b2609df952751b0 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Thu, 3 Mar 2016 13:01:42 +0100 Subject: [PATCH 8/9] Drop accidentally left-in inhibitor switch --- src/dotty/tools/dotc/transform/LambdaLift.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dotty/tools/dotc/transform/LambdaLift.scala b/src/dotty/tools/dotc/transform/LambdaLift.scala index b140746450dc..ef7a110b1c70 100644 --- a/src/dotty/tools/dotc/transform/LambdaLift.scala +++ b/src/dotty/tools/dotc/transform/LambdaLift.scala @@ -266,7 +266,7 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform // top-level class. This avoids possible deadlocks when a static method // has to access its enclosing object from the outside. else if (sym.isConstructor) { - if (false && sym.isPrimaryConstructor && isLocal(sym.owner) && !sym.owner.is(Trait)) + if (sym.isPrimaryConstructor && isLocal(sym.owner) && !sym.owner.is(Trait)) // add a call edge from the constructor of a local non-trait class to // the class itself. This is done so that the constructor inherits // the free variables of the class. From cf2fed8138cb399beb7d1249227107b943fe3905 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 7 Mar 2016 16:13:06 +0100 Subject: [PATCH 9/9] Address reviewer comments. --- src/dotty/tools/dotc/transform/LambdaLift.scala | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/dotty/tools/dotc/transform/LambdaLift.scala b/src/dotty/tools/dotc/transform/LambdaLift.scala index ef7a110b1c70..3ef684e5548f 100644 --- a/src/dotty/tools/dotc/transform/LambdaLift.scala +++ b/src/dotty/tools/dotc/transform/LambdaLift.scala @@ -206,14 +206,11 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform // Constructors and methods nested inside traits get the free variables // of the enclosing trait or class. // Conversely, local traits do not get free variables. - if (!enclosure.is(Trait)) { - val ss = symSet(free, enclosure) - if (!ss(sym)) { - ss += sym + if (!enclosure.is(Trait)) + if (symSet(free, enclosure).add(sym)) { changedFreeVars = true ctx.log(i"$sym is free in $enclosure") } - } } if (intermediate.isRealClass) intermediate else if (enclosure.isRealClass) enclosure