From e0a0c39a2578eca9927fa4c435ce14654422a825 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Fri, 17 Apr 2015 10:48:51 +0200 Subject: [PATCH 1/4] Fix #480 in LambdaLift Fix two errors, where the first masked the second: 1) Variables defined in a method are not free variables of that method. This was mispredicated before and caused liftedOwner to be updated to the class enclosing the method, thereby preventing any method that refers to a local parameter or symbol to be hoisted. Once this was fixed, methods were hoisted too far out. Test case in #480a, which takes code from Definitions. This was fixed by 2) markFree is updated to do an additional narrowLiftedOwner so that, if a free variable gets a proxy in an enclosing class, any inner classes and methods are kept within that class. --- .../tools/dotc/transform/LambdaLift.scala | 27 ++++++++++++------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/src/dotty/tools/dotc/transform/LambdaLift.scala b/src/dotty/tools/dotc/transform/LambdaLift.scala index a42e0cc352eb..64e7d5852f55 100644 --- a/src/dotty/tools/dotc/transform/LambdaLift.scala +++ b/src/dotty/tools/dotc/transform/LambdaLift.scala @@ -102,9 +102,10 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform /** Mark symbol `sym` as being free in `enclosure`, unless `sym` * is defined in `enclosure` or there is a class between `enclosure`s owner - * and the owner of `sym`. - * Return `true` if there is no class between `enclosure` and - * the owner of sym. + * and the owner of `sym`. Also, update lifted owner of `enclosure` so + * that `enclosure` can access `sym`, or its proxy in an intermediate class. + * Return the closest enclosing intermediate class between `enclosure` and + * the owner of sym, or NoSymbol is none exists. * pre: sym.owner.isTerm, (enclosure.isMethod || enclosure.isClass) * * The idea of `markFree` is illustrated with an example: @@ -130,22 +131,28 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform * } * } */ - private def markFree(sym: Symbol, enclosure: Symbol)(implicit ctx: Context): Boolean = try { + private def markFree(sym: Symbol, enclosure: Symbol)(implicit ctx: Context): Symbol = try { if (!enclosure.exists) throw new NoPath - ctx.log(i"mark free: ${sym.showLocated} with owner ${sym.maybeOwner} marked free in $enclosure") - narrowLiftedOwner(enclosure, sym.enclosingClass) - (enclosure == sym.enclosure) || { + 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}") - if (enclosure.is(PackageClass) || - !markFree(sym, enclosure.skipConstructor.enclosure)) false + val intermediate = + if (enclosure.is(PackageClass)) enclosure + else markFree(sym, enclosure.skipConstructor.enclosure) + 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") } - !enclosure.isClass + if (enclosure.isClass) enclosure else NoSymbol } } } catch { From 6bcc3187a81ec1f2078709a1a9249ec57a393aad Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Fri, 17 Apr 2015 10:52:15 +0200 Subject: [PATCH 2/4] Improve documentation and minimze test Documentation around markFree and narrowLiftedOwner was added. i480 was minimzed and dependencies on dotc were removed. (+1 squashed commit) Squashed commits: [1a84054] Test cases for #480 --- .../tools/dotc/transform/LambdaLift.scala | 25 +++++++++++++++---- tests/pos/i480.scala | 4 +++ tests/pos/i480a.scala | 15 +++++++++++ 3 files changed, 39 insertions(+), 5 deletions(-) create mode 100644 tests/pos/i480.scala create mode 100644 tests/pos/i480a.scala diff --git a/src/dotty/tools/dotc/transform/LambdaLift.scala b/src/dotty/tools/dotc/transform/LambdaLift.scala index 64e7d5852f55..fa76cd5d0a08 100644 --- a/src/dotty/tools/dotc/transform/LambdaLift.scala +++ b/src/dotty/tools/dotc/transform/LambdaLift.scala @@ -90,6 +90,9 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform free.getOrElse(sym, Nil).toList.map(pm) } + /** 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) = { if (sym.owner.isTerm && owner.isProperlyContainedIn(liftedOwner(sym)) && @@ -100,12 +103,22 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform } } - /** Mark symbol `sym` as being free in `enclosure`, unless `sym` - * is defined in `enclosure` or there is a class between `enclosure`s owner - * and the owner of `sym`. Also, update lifted owner of `enclosure` so + /** Mark symbol `sym` as being free in `enclosure`, unless `sym` is defined + * in `enclosure` or there is an intermediate class properly containing `enclosure` + * in which `sym` is also free. Also, update `liftedOwner` of `enclosure` so * that `enclosure` can access `sym`, or its proxy in an intermediate class. + * This means: + * + * 1. If there is an intermediate class in which `sym` is free, `enclosure` + * must be contained in that class (in order to access the `sym proxy stored + * in the class). + * + * 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 is none exists. + * the owner of sym, or NoSymbol if none exists. + * * pre: sym.owner.isTerm, (enclosure.isMethod || enclosure.isClass) * * The idea of `markFree` is illustrated with an example: @@ -139,7 +152,9 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform ctx.debuglog(i"$enclosure != ${sym.enclosure}") val intermediate = if (enclosure.is(PackageClass)) enclosure - else markFree(sym, enclosure.skipConstructor.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 diff --git a/tests/pos/i480.scala b/tests/pos/i480.scala new file mode 100644 index 000000000000..8c2bb057c472 --- /dev/null +++ b/tests/pos/i480.scala @@ -0,0 +1,4 @@ +object O { + val x: Function1[String, String] = a => a + val x2: Function1[String, String] = a => "1" +} diff --git a/tests/pos/i480a.scala b/tests/pos/i480a.scala new file mode 100644 index 000000000000..b388f6189273 --- /dev/null +++ b/tests/pos/i480a.scala @@ -0,0 +1,15 @@ +package test + +/** A class defining symbols and types of standard definitions */ +class Definitions { + + trait LazyType { def complete(): Unit } + + def f(vcs: List[Int]): Unit = { + val completer = new LazyType { + def complete(): Unit = + for (i <- 0 until vcs.length if vcs(i) != 0) + f(vcs.updated(i, 0)) + } + } +} From b0b690285bf46ba7e4df91e131bff6921430be48 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Fri, 17 Apr 2015 11:03:05 +0200 Subject: [PATCH 3/4] Reset inSuperCall when lifting methods After lambdaLift, methods are no longer local to cosntructors, so their inSuperCall flag is reset. --- 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 fa76cd5d0a08..5d47d2143b5e 100644 --- a/src/dotty/tools/dotc/transform/LambdaLift.scala +++ b/src/dotty/tools/dotc/transform/LambdaLift.scala @@ -294,7 +294,7 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform local.copySymDenotation( owner = newOwner, name = newName(local), - initFlags = local.flags | Private | maybeStatic | maybeNotJavaPrivate, + initFlags = local.flags &~ InSuperCall | Private | maybeStatic | maybeNotJavaPrivate, info = liftedInfo(local)).installAfter(thisTransform) if (local.isClass) for (member <- local.asClass.info.decls) From 0b4e4cb7e71367752558fac29129f7e182d9206a Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Fri, 17 Apr 2015 20:13:38 +0200 Subject: [PATCH 4/4] Fix #342 Lambda lift idents need to get new prefix Idents of lifted symbols become class members, need to carry the right reference with the right prefix as type. --- src/dotty/tools/dotc/transform/LambdaLift.scala | 9 +++++++-- tests/pos/i342.scala | 10 ++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) create mode 100644 tests/pos/i342.scala diff --git a/src/dotty/tools/dotc/transform/LambdaLift.scala b/src/dotty/tools/dotc/transform/LambdaLift.scala index 5d47d2143b5e..9b35d1d99def 100644 --- a/src/dotty/tools/dotc/transform/LambdaLift.scala +++ b/src/dotty/tools/dotc/transform/LambdaLift.scala @@ -394,8 +394,13 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform val sym = tree.symbol tree.tpe match { case tpe @ TermRef(prefix, _) => - if ((prefix eq NoPrefix) && sym.enclosure != currentEnclosure && !sym.isStatic) - (if (sym is Method) memberRef(sym) else proxyRef(sym)).withPos(tree.pos) + if (prefix eq NoPrefix) + if (sym.enclosure != currentEnclosure && !sym.isStatic) + (if (sym is Method) memberRef(sym) else proxyRef(sym)).withPos(tree.pos) + else if (sym.owner.isClass) // sym was lifted out + ref(sym).withPos(tree.pos) + else + tree else if (!prefixIsElidable(tpe)) ref(tpe) else tree case _ => diff --git a/tests/pos/i342.scala b/tests/pos/i342.scala new file mode 100644 index 000000000000..bb57bae8e231 --- /dev/null +++ b/tests/pos/i342.scala @@ -0,0 +1,10 @@ +object Test { + def test2: Int = { + var ds: String = null + def s = { + ds = "abs" + ds + } + s.length + } +}