Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

SI-6536 Generates super accessors X.super[Y].blah when Y is a class #1770

Merged
merged 2 commits into from

4 participants

@JamesIry

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.

review @paulp, @adriaanm

James Iry 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.
2124b9d
...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.")
@paulp
paulp added a note

Where I come from the word is "transform". NEWB

No, tansform. It's Californian.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...iler/scala/tools/nsc/typechecker/SuperAccessors.scala
@@ -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)
@paulp
paulp added a note

Can you please extract a condition of this length into a local method? This has no binary compatibility implications (it's private) and is far easier to read and maintain. I would maybe write it like this:

def requiresAccessor = name.isTermName && (clazz != currentClass) && (mix match {
  case tpnme.EMPTY => clazz.isTrait || !validCurrentOwner
  case _           => !mixtpe.typeSymbol.isTrait
})

Ha ha, "mix match".

@paulp
paulp added a note

I borked the logic of the above, but the gist is stil sound (especially the 2-case match on mix)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@JamesIry

Should have a better error if sup.tpe isn't SuperType.

...iler/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

Should have a better error if the assumption that sup.tpe is a SuperType turns out to be wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@scala-jenkins

Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1883/

@scala-jenkins

jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1883/

James Iry 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
af03afb
@scala-jenkins

Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1886/

@scala-jenkins

jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1886/

@adriaanm
Owner

LGTM

@adriaanm adriaanm merged commit 31518ee into scala:2.9.x
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 13, 2012
  1. SI-6536 Generates super accessors X.super[Y].blah when Y is a class

    James Iry authored
    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.
  2. SI-6536 Cleanup code around determining accessor requirement

    James Iry authored
    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
This page is out of date. Refresh to see the latest.
View
10 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 transformed 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)
View
2  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 {
View
32 src/compiler/scala/tools/nsc/typechecker/SuperAccessors.scala
@@ -118,8 +118,36 @@ 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))
- ensureAccessor(sel)
+
+ // 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.
+ //
+ // 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
}
View
27 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
View
297 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
+ }
+}
Something went wrong with that request. Please try again.