From 2124b9de64ff6e8b2f363bd062a63505d6824bc2 Mon Sep 17 00:00:00 2001 From: James Iry Date: Thu, 13 Dec 2012 08:32:14 -0800 Subject: SI-6536 Generates super accessors X.super[Y].blah when Y is a class The main change here is to add another case for generating super accessors - the case in X.super[Y].blah when X isn't the current class and Y is a class. The change is deliberately kept as minimal as possible to reduce the chance of breaking something in the 2.9.x line. Additionally GenICode now detects the case when we're trying to emit byte code that would be nonsense and warns about it. That can safely be made an assert for 2.11. Finally a related assert in RefChecks is beefed up to output a bit more useful information. --- .../scala/tools/nsc/backend/icode/GenICode.scala | 10 +- .../scala/tools/nsc/typechecker/RefChecks.scala | 2 +- .../tools/nsc/typechecker/SuperAccessors.scala | 16 +- test/files/run/t6536.check | 27 ++ test/files/run/t6536.scala | 297 +++++++++++++++++++++ 5 files changed, 347 insertions(+), 5 deletions(-) create mode 100644 test/files/run/t6536.check create mode 100644 test/files/run/t6536.scala diff --git a/src/compiler/scala/tools/nsc/backend/icode/GenICode.scala b/src/compiler/scala/tools/nsc/backend/icode/GenICode.scala index d9e8a791af..1d98a2936b 100644 --- a/src/compiler/scala/tools/nsc/backend/icode/GenICode.scala +++ b/src/compiler/scala/tools/nsc/backend/icode/GenICode.scala @@ -767,11 +767,15 @@ abstract class GenICode extends SubComponent { // to call super constructors explicitly and/or use their 'returned' value. // therefore, we can ignore this fact, and generate code that leaves nothing // on the stack (contrary to what the type in the AST says). - case Apply(fun @ Select(Super(_, mix), _), args) => + case Apply(fun @ Select(Super(qual, mix), _), args) => + + if (!qual.isInstanceOf[This]) { + log("WARNING: super call where selector isn't 'this'. May generate invalid bytecode. Previous phases should have tansformed away this form of super.") + } if (settings.debug.value) - log("Call to super: " + tree); + log("Call to super: " + tree) + val invokeStyle = SuperCall(mix) -// if (fun.symbol.isConstructor) Static(true) else SuperCall(mix); ctx.bb.emit(THIS(ctx.clazz.symbol), tree.pos) val ctx1 = genLoadArguments(args, fun.symbol.info.paramTypes, ctx) diff --git a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala index 921b242555..099503e685 100644 --- a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala +++ b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala @@ -1375,7 +1375,7 @@ abstract class RefChecks extends InfoTransform { def checkSuper(mix: Name) = // term should have been eliminated by super accessors - assert(!(qual.symbol.isTrait && sym.isTerm && mix == tpnme.EMPTY)) + assert(!(qual.symbol.isTrait && sym.isTerm && mix == tpnme.EMPTY), "The following selector should have been transformed by SuperAccessors:" + tree) transformCaseApply(tree, qual match { diff --git a/src/compiler/scala/tools/nsc/typechecker/SuperAccessors.scala b/src/compiler/scala/tools/nsc/typechecker/SuperAccessors.scala index 74e246e910..87a03648b8 100644 --- a/src/compiler/scala/tools/nsc/typechecker/SuperAccessors.scala +++ b/src/compiler/scala/tools/nsc/typechecker/SuperAccessors.scala @@ -110,6 +110,7 @@ abstract class SuperAccessors extends transform.Transform with transform.TypingT val Select(sup @ Super(_, mix), name) = sel val sym = sel.symbol val clazz = sup.symbol + val SuperType(_, mixtpe) = sup.tpe if (sym.isDeferred) { val member = sym.overridingSymbol(clazz); @@ -118,7 +119,20 @@ abstract class SuperAccessors extends transform.Transform with transform.TypingT unit.error(sel.pos, ""+sym.fullLocationString+" is accessed from super. It may not be abstract "+ "unless it is overridden by a member declared `abstract' and `override'"); } - if (name.isTermName && mix == tpnme.EMPTY && (clazz.isTrait || clazz != currentClass || !validCurrentOwner)) + + // we need an accessor to get to a super on an outer thing, but only if we can't call name more directly on + // a trait implementation class. So this complicated condition is leaving alone cases where we don't need to do + // anything special (i.e. we're getting a direct super class) or where a later transform will inject a call to + // a trait implementation method directly. + // So, we're looking for items of the form clazz.super[mix].name (or clazz.super.name wich is seen as clazz.super[EMPTY].name. + // with some limitations. First, name has to be a term rather than a type. + // Then there are a couple of cases. If mix is empty then we only need an accessor if clazz is a trait, it's not this current class, + // or the validCurentOwner setting is false...which...ugh, is a mess. + // If the mix is set then if it refers to a class and the clazz part isn't the current class + // it's not just super[mix].name then we need to generate an accessor. + // SI-6536 has more discussion about how this works. + if (name.isTermName && (mix == tpnme.EMPTY && (clazz.isTrait || clazz != currentClass || !validCurrentOwner )) + || (mix != tpnme.EMPTY && !mixtpe.typeSymbol.isTrait && clazz != currentClass)) ensureAccessor(sel) else sel } diff --git a/test/files/run/t6536.check b/test/files/run/t6536.check new file mode 100644 index 0000000000..8d6b1abec1 --- /dev/null +++ b/test/files/run/t6536.check @@ -0,0 +1,27 @@ +a1 a2 a3_t o1/o1 o2 outertrait.Outer/o1 List(a1, o1) +a1 a2 a3_c o1/o1 o2 outertrait.Outer/o1 List(a1, o1) +a1 a2 a3_o o1/o1 o2 outertrait.Outer/o1 List(a1, o1) +a1 a2 a3_tc o1/o1 o2 outertrait.Outer/o1 List(a1, o1) +a1 a2 a3_to o1/o1 o2 outertrait.Outer/o1 List(a1, o1) +a1 a2 a3_tco o1/o1 o2 outertrait.Outer/o1 List(a1, o1) + +a1 a2 a3_t o1/o1 o2 outerclass.Outer/o1 List(a1, o1) +a1 a2 a3_c o1/o1 o2 outerclass.Outer/o1 List(a1, o1) +a1 a2 a3_o o1/o1 o2 outerclass.Outer/o1 List(a1, o1) +a1 a2 a3_tc o1/o1 o2 outerclass.Outer/o1 List(a1, o1) +a1 a2 a3_to o1/o1 o2 outerclass.Outer/o1 List(a1, o1) +a1 a2 a3_tco o1/o1 o2 outerclass.Outer/o1 List(a1, o1) + +a1 a2 a3_t o1/o1 o2 withinmethod.Outer/o1 List(a1, o1) +a1 a2 a3_c o1/o1 o2 withinmethod.Outer/o1 List(a1, o1) +a1 a2 a3_o o1/o1 o2 withinmethod.Outer/o1 List(a1, o1) +a1 a2 a3_tc o1/o1 o2 withinmethod.Outer/o1 List(a1, o1) +a1 a2 a3_to o1/o1 o2 withinmethod.Outer/o1 List(a1, o1) +a1 a2 a3_tco o1/o1 o2 withinmethod.Outer/o1 List(a1, o1) +child super class +child super trait +child super class 2 +child super trait 2 +child super class 3 +child super trait 3a +child super trait 3a diff --git a/test/files/run/t6536.scala b/test/files/run/t6536.scala new file mode 100644 index 0000000000..397894e4ba --- /dev/null +++ b/test/files/run/t6536.scala @@ -0,0 +1,297 @@ + +// this was what was broken +class AClass { + def a = "super class" +} +class BClass extends AClass { + class M { + def z = "child " + BClass.super[AClass].a + } + override def a = "" +} + +// this variant would crash the compiler +class O3 { def f = "" } // O3 must be a class +class O4 extends O3 { + class T1 { def g = O4.super[O3].f } // ok + trait T2 { def g = O4.super[O3].f } // crash +} + +// make sure the fix didn't break this case, which wasn't broken +trait ATrait { + def a = "super trait" +} +class BTrait extends ATrait { + class M { + def z = "child " + BTrait.super[ATrait].a + } + override def a = "" +} + +// make sure the fix didn't break the simplest case +class AClass2 { + def a = "super class 2" +} +class BClass2 extends AClass2 { + override def a = "" + def z = "child " + super.a +} + +// make sure the fix didn't break this simplest case +class ATrait2 { + def a = "super trait 2" +} +class BTrait2 extends ATrait2 { + override def a = "" + def z = "child " + super.a +} + +// a more interesting example of the all that +// this was what was broken +class AClass3 { + def a = "super class 3" +} +trait ATrait3a { + def a = "super trait 3a" +} +trait ATrait3b { + def a = "super trait 3b" +} +class BClass3 extends AClass3 with ATrait3a with ATrait3b { + class M { + def zclass = "child " + BClass3.super[AClass3].a + def ztraita = "child " + BClass3.super[ATrait3a].a + def ztraitb = "child " + BClass3.super[ATrait3a].a + } + override def a = "" +} + +// here's a case where we call super from a trait +trait Root { + def a = "root" +} + +trait Mid extends Root { + override def a = "mid" + def b = super.a +} + +class Bottom extends Mid + +// and this is a bunch of other stuff we want to make sure doesn't explode +trait A1 { def m1 = "a1" } +trait A2 { def m1 = "a2" } + +trait O1 { def m2 = "o1" ; def o1 = "o1" } +trait O2 { def m2 = "o2" } + +class C1 { def m3 = "c1" } +trait C2 { def m3 = "c2" } + +package outertrait { + trait Outer extends O1 with O2 { + override def m2 = "outertrait.Outer" + + trait A3_T extends A1 with A2 { + override def m1 = "a3_t" + + def f1 = super[A1].m1 + def f2 = super[A2].m1 + def f3 = m1 + def f4 = Outer.super[O1].m2 + "/" + Outer.super[O1].o1 + def f5 = Outer.super[O2].m2 + def f6 = Outer.this.m2 + "/" + Outer.this.o1 + def f7 = () => List(super[A1].m1, Outer.super[O1].m2) + } + + class A3_C extends A1 with A2 { + override def m1 = "a3_c" + + def f1 = super[A1].m1 + def f2 = super[A2].m1 + def f3 = m1 + def f4 = Outer.super[O1].m2 + "/" + Outer.super[O1].o1 + def f5 = Outer.super[O2].m2 + def f6 = Outer.this.m2 + "/" + Outer.this.o1 + def f7 = () => List(super[A1].m1, Outer.super[O1].m2) + } + + object A3_O extends A1 with A2 { + override def m1 = "a3_o" + + def f1 = super[A1].m1 + def f2 = super[A2].m1 + def f3 = m1 + def f4 = Outer.super[O1].m2 + "/" + Outer.super[O1].o1 + def f5 = Outer.super[O2].m2 + def f6 = Outer.this.m2 + "/" + Outer.this.o1 + def f7 = () => List(super[A1].m1, Outer.super[O1].m2) + } + + class A3_TC extends A3_T { override def m1 = "a3_tc" } + object A3_TO extends A3_T { override def m1 = "a3_to" } + object A3_TCO extends A3_TC { override def m1 = "a3_tco" } + } +} + +package outerclass { + class Outer extends O1 with O2 { + override def m2 = "outerclass.Outer" + + trait A3_T extends A1 with A2 { + override def m1 = "a3_t" + + def f1 = super[A1].m1 + def f2 = super[A2].m1 + def f3 = m1 + def f4 = Outer.super[O1].m2 + "/" + Outer.super[O1].o1 + def f5 = Outer.super[O2].m2 + def f6 = Outer.this.m2 + "/" + Outer.this.o1 + def f7 = () => List(super[A1].m1, Outer.super[O1].m2) + } + + class A3_C extends A1 with A2 { + override def m1 = "a3_c" + + def f1 = super[A1].m1 + def f2 = super[A2].m1 + def f3 = m1 + def f4 = Outer.super[O1].m2 + "/" + Outer.super[O1].o1 + def f5 = Outer.super[O2].m2 + def f6 = Outer.this.m2 + "/" + Outer.this.o1 + def f7 = () => List(super[A1].m1, Outer.super[O1].m2) + } + + object A3_O extends A1 with A2 { + override def m1 = "a3_o" + + def f1 = super[A1].m1 + def f2 = super[A2].m1 + def f3 = m1 + def f4 = Outer.super[O1].m2 + "/" + Outer.super[O1].o1 + def f5 = Outer.super[O2].m2 + def f6 = Outer.this.m2 + "/" + Outer.this.o1 + def f7 = () => List(super[A1].m1, Outer.super[O1].m2) + } + + class A3_TC extends A3_T { override def m1 = "a3_tc" } + object A3_TO extends A3_T { override def m1 = "a3_to" } + object A3_TCO extends A3_TC { override def m1 = "a3_tco" } + } +} + +package withinmethod { + trait Outer extends O1 with O2 { + override def m2 = "withinmethod.Outer" + + def method1 = { + trait A3_T extends A1 with A2 { + override def m1 = "a3_t" + + def f1 = super[A1].m1 + def f2 = super[A2].m1 + def f3 = m1 + def f4 = Outer.super[O1].m2 + "/" + Outer.super[O1].o1 + def f5 = Outer.super[O2].m2 + def f6 = Outer.this.m2 + "/" + Outer.this.o1 + def f7 = () => List(super[A1].m1, Outer.super[O1].m2) + } + class A3_C extends A1 with A2 { + override def m1 = "a3_c" + + def f1 = super[A1].m1 + def f2 = super[A2].m1 + def f3 = m1 + def f4 = Outer.super[O1].m2 + "/" + Outer.super[O1].o1 + def f5 = Outer.super[O2].m2 + def f6 = Outer.this.m2 + "/" + Outer.this.o1 + def f7 = () => List(super[A1].m1, Outer.super[O1].m2) + } + object A3_O extends A1 with A2 { + override def m1 = "a3_o" + + def f1 = super[A1].m1 + def f2 = super[A2].m1 + def f3 = m1 + def f4 = Outer.super[O1].m2 + "/" + Outer.super[O1].o1 + def f5 = Outer.super[O2].m2 + def f6 = Outer.this.m2 + "/" + Outer.this.o1 + def f7 = () => List(super[A1].m1, Outer.super[O1].m2) + } + class A3_TC extends A3_T { override def m1 = "a3_tc" } + object A3_TO extends A3_T { override def m1 = "a3_to" } + object A3_TCO extends A3_TC { override def m1 = "a3_tco" } + + List[Test.Anything](new A3_T { }, new A3_C, A3_O, new A3_TC, A3_TO, A3_TCO) + } + } +} + +object Test { + type Anything = { + def f1: Any + def f2: Any + def f3: Any + def f4: Any + def f5: Any + def f6: Any + def f7: () => Any + } + + def show(x: Anything) { + import x._ + println(List(f1, f2, f3, f4, f5, f6, f7()) mkString " ") + } + def main(args: Array[String]): Unit = { + { + val o1 = new outertrait.Outer { } + show(new o1.A3_T { }) + show(new o1.A3_C) + show(o1.A3_O) + show(new o1.A3_TC) + show(o1.A3_TO) + show(o1.A3_TCO) + println("") + } + + { + val o1 = new outerclass.Outer { } + show(new o1.A3_T { }) + show(new o1.A3_C) + show(o1.A3_O) + show(new o1.A3_TC) + show(o1.A3_TO) + show(o1.A3_TCO) + println("") + } + + { + val o1 = new withinmethod.Outer { } + o1.method1 foreach show + } + + val bclass = new BClass + val bclassm = new bclass.M + println(bclassm.z) + + val btrait = new BTrait + val btraitm = new btrait.M + println(btraitm.z) + + val bclass2 = new BClass2 + println(bclass2.z) + + val btrait2 = new BTrait2 + println(btrait2.z) + + val bclass3 = new BClass3 + val bclass3m = new bclass3.M + println(bclass3m.zclass) + println(bclass3m.ztraita) + println(bclass3m.ztraitb) + + val bottom = new Bottom + bottom.a + bottom.b + } +} \ No newline at end of file -- cgit v1.2.3 From af03afba2596c3c3edfbf7b2a1d128a1d2fc412b Mon Sep 17 00:00:00 2001 From: James Iry Date: Thu, 13 Dec 2012 11:03:13 -0800 Subject: SI-6536 Cleanup code around determining accessor requirement Moves the logic for the condition on requiring an accessor into a local method with a match to make thing cleaner. Also determines the mix type using a pattern match that logs but does not fail if the symbol isn't of the expected type. Finally fixes a typo in an assertion in GenICode. review @paulp --- .../scala/tools/nsc/backend/icode/GenICode.scala | 2 +- .../tools/nsc/typechecker/SuperAccessors.scala | 36 +++++++++++++++------- 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/src/compiler/scala/tools/nsc/backend/icode/GenICode.scala b/src/compiler/scala/tools/nsc/backend/icode/GenICode.scala index 1d98a2936b..1f391f5b55 100644 --- a/src/compiler/scala/tools/nsc/backend/icode/GenICode.scala +++ b/src/compiler/scala/tools/nsc/backend/icode/GenICode.scala @@ -770,7 +770,7 @@ abstract class GenICode extends SubComponent { case Apply(fun @ Select(Super(qual, mix), _), args) => if (!qual.isInstanceOf[This]) { - log("WARNING: super call where selector isn't 'this'. May generate invalid bytecode. Previous phases should have tansformed away this form of super.") + log("WARNING: super call where selector isn't 'this'. May generate invalid bytecode. Previous phases should have transformed away this form of super.") } if (settings.debug.value) log("Call to super: " + tree) diff --git a/src/compiler/scala/tools/nsc/typechecker/SuperAccessors.scala b/src/compiler/scala/tools/nsc/typechecker/SuperAccessors.scala index 87a03648b8..94a6ce3c51 100644 --- a/src/compiler/scala/tools/nsc/typechecker/SuperAccessors.scala +++ b/src/compiler/scala/tools/nsc/typechecker/SuperAccessors.scala @@ -110,7 +110,6 @@ abstract class SuperAccessors extends transform.Transform with transform.TypingT val Select(sup @ Super(_, mix), name) = sel val sym = sel.symbol val clazz = sup.symbol - val SuperType(_, mixtpe) = sup.tpe if (sym.isDeferred) { val member = sym.overridingSymbol(clazz); @@ -120,20 +119,35 @@ abstract class SuperAccessors extends transform.Transform with transform.TypingT "unless it is overridden by a member declared `abstract' and `override'"); } + // determine if the mix in clazz.super[mix].name is a trait + def mixTpeIsTrait = sup.tpe match { + case SuperType(_, mixTpe) => mixTpe.typeSymbol.isTrait + case _ => + log("Warning: could not determine the type of mix " + mix + " by going through a Super node's "+ + "type because instead of a SuperType it was " + sup.tpe) + false + } + // we need an accessor to get to a super on an outer thing, but only if we can't call name more directly on // a trait implementation class. So this complicated condition is leaving alone cases where we don't need to do // anything special (i.e. we're getting a direct super class) or where a later transform will inject a call to // a trait implementation method directly. - // So, we're looking for items of the form clazz.super[mix].name (or clazz.super.name wich is seen as clazz.super[EMPTY].name. - // with some limitations. First, name has to be a term rather than a type. - // Then there are a couple of cases. If mix is empty then we only need an accessor if clazz is a trait, it's not this current class, - // or the validCurentOwner setting is false...which...ugh, is a mess. - // If the mix is set then if it refers to a class and the clazz part isn't the current class - // it's not just super[mix].name then we need to generate an accessor. - // SI-6536 has more discussion about how this works. - if (name.isTermName && (mix == tpnme.EMPTY && (clazz.isTrait || clazz != currentClass || !validCurrentOwner )) - || (mix != tpnme.EMPTY && !mixtpe.typeSymbol.isTrait && clazz != currentClass)) - ensureAccessor(sel) + // + // SI-6536 has more discussion about how this works. + // + // So, we're looking for items of the form clazz.super[mix].name (or clazz.super.name wich is seen as + // clazz.super[EMPTY].name with some limitations. First, name has to be a term rather than a type. + // Then there are a couple of cases. + def requiresAccessor = name.isTermName && (mix match { + // If mix is empty then we only need an accessor if clazz is a trait, it's not this current class, + // or the validCurentOwner setting is false...which...ugh, is a mess. + case tpnme.EMPTY => clazz.isTrait || clazz != currentClass || !validCurrentOwner + // If the mix is set then if it refers to a class and the clazz part isn't the current class + // it's not just super[mix].name then we need to generate an accessor. + case _ => clazz != currentClass && !mixTpeIsTrait + }) + + if (requiresAccessor) ensureAccessor(sel) else sel } -- cgit v1.2.3