From a9019996483bb775164ecd05e0c682e1fdb7fb0c Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Fri, 2 Dec 2016 20:12:26 +0100 Subject: [PATCH 1/2] Illustrate issue with TreeTypeMap and miniphases When running TreeTypeMap in a miniphase, the tree we're transforming has already been transformed by all the miniphases in the same group, but the symbols we're looking at might be older than the trees, this means that we can miss transforming some symbols as illustrated in details by the added testcase which currently fails. --- tests/pos/treetypemap-miniphase.scala | 38 +++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 tests/pos/treetypemap-miniphase.scala diff --git a/tests/pos/treetypemap-miniphase.scala b/tests/pos/treetypemap-miniphase.scala new file mode 100644 index 000000000000..01d85d446268 --- /dev/null +++ b/tests/pos/treetypemap-miniphase.scala @@ -0,0 +1,38 @@ +class Test { + def byName(param: => Any) = {} + + def test: Unit = { + + // 1. In ResolveSuper#transformTemplate we add "def <(that: Int): Boolean = + // Foo.super[Ordered].<(that)", etc to Foo, these methods are inserted + // into a new decls scope using enteredAfter in MixinOps#implementation + + // 2. In ElimByName#transformApply we call `changeOwner` on the tree + // containing Foo, which will use a TreeTypeMap + + // 3. In TreeTypeMap#withMappedSyms we call `cls.info.decls` on every mapped + // class to map its symbols, one of these mapped class is Foo, but at + // this point we are before the ResolveSuper phase, therefore the decls + // scope does not contain the methods added in ResolveSuper like `<`, so + // we never map them and their owner is still the old symbol for Foo, + // this is wrong! But the compiler will not realize this until much later + + // 4. In LinkScala2ImplClasses we replace: + // def <(that: Int): Boolean = Foo.super[Ordered].<(that) + // by: + // def <(that: Int): Boolean = scala.math.Ordered$class#<(this, scala.Int.box(that)) + // This is fine, except that the owner of `<` is incorrect since 3., so the `this` tree + // gets an incorrect symbol + + // 5. In the backend, the compiler finally realizes that something has gone very wrong: + // assertion failed: Trying to access the this of another class: + // tree.symbol = class Test$~Foo#4444, + // class symbol = class Test$Foo$1#6723 + + byName({ + class Foo extends Ordered[Int] { + def compare(that: Int): Int = 0 + } + }) + } +} From 56f658a31587af9d2a97ee940f3dc1dbacc99291 Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Fri, 2 Dec 2016 20:14:52 +0100 Subject: [PATCH 2/2] Hacky fix for the TreeTypeMap issue from the previous commit With this commit, tests/pos/treetypemap-miniphase.scala now compiles, but this is only a hack: there are other instances of TreeTypeMap in the compiler and they would all need to be run with "atGroupEnd", this is tricky since we are not always calling TreeTypeMap in a context where we have access to the current TreeTransformer (for example when we create a TreeTypeMap inside TypeMap to deal with annotations), ideas and alternative fixes welcome. --- compiler/src/dotty/tools/dotc/transform/ElimByName.scala | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/compiler/src/dotty/tools/dotc/transform/ElimByName.scala b/compiler/src/dotty/tools/dotc/transform/ElimByName.scala index 1922272612a2..6663fdb9d880 100644 --- a/compiler/src/dotty/tools/dotc/transform/ElimByName.scala +++ b/compiler/src/dotty/tools/dotc/transform/ElimByName.scala @@ -80,7 +80,9 @@ class ElimByName extends MiniPhaseTransform with InfoTransformer { thisTransform val inSuper = if (ctx.mode.is(Mode.InSuperCall)) InSuperCall else EmptyFlags val meth = ctx.newSymbol( ctx.owner, nme.ANON_FUN, Synthetic | Method | inSuper, MethodType(Nil, Nil, argType)) - Closure(meth, _ => arg.changeOwner(ctx.owner, meth)) + atGroupEnd { implicit ctx: Context => + Closure(meth, _ => arg.changeOwner(ctx.owner, meth)) + } } ref(defn.dummyApply).appliedToType(argType).appliedTo(argFun) case _ =>