Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VerifyError with qualified super #6536

Closed
scabug opened this issue Oct 17, 2012 · 16 comments
Closed

VerifyError with qualified super #6536

scabug opened this issue Oct 17, 2012 · 16 comments
Milestone

Comments

@scabug
Copy link

scabug commented Oct 17, 2012

class A { // Will work with trait.
  def a = "super"
}
class B extends A {
  class M {
    def z = "child" + B.super[A].a // B.super[A], not super[A]
  }
  override def a = ""
}

object Test {
  def main(args: Array[String]): Unit = {
    val b = new B
    val z = new b.M
    println(z.z)
  }
}
scala3 Test
java.lang.VerifyError: (class: B$M, method: z signature: ()Ljava/lang/String;) Illegal use of nonvirtual function call
	at Test$.main(a.scala:17)
	at Test.main(a.scala)

Note, there are two work-arounds that solve this problem. One specific to this bug report and one more general.

The specific one is

class B extends A {
  class M {
    def z = "child" + B.super.a // B.super, not B.super[A]
 }
  override def a = ""
}

That works because this bug is only triggered when there is a type qualifier on a super access to an outer class. Get rid of the type qualifier and it's all good. But it's not fully general if you need to decide among several different sources for 'a'.

The more general work around is to manually do your own accessor

class B extends A {
  class M {
    def z = "child" + a_a // access using a hand-written accessor
 }
  override def a = ""
  private[this] def a_a = super[A].a
}
@scabug
Copy link
Author

scabug commented Oct 17, 2012

Imported From: https://issues.scala-lang.org/browse/SI-6536?orig=1
Reporter: @paulp
Assignee: @JamesIry
Affected Versions: 2.9.2, 2.10.0
Other Milestones: 2.10.1-RC1

@scabug
Copy link
Author

scabug commented Nov 13, 2012

@szeiger said:
Related Typesafe ticket: https://support.typesafe.com/tickets/1856

@scabug
Copy link
Author

scabug commented Dec 12, 2012

@adriaanm said:
James, we've been requested to fix this by 2.9.3. Could you have a look?

@scabug
Copy link
Author

scabug commented Dec 12, 2012

@JamesIry said:

public class B$M extends java.lang.Object implements scala.ScalaObject{
public final B $outer;

public java.lang.String z();
  Code:
   0:	new	#9; //class scala/collection/mutable/StringBuilder
   3:	dup
   4:	invokespecial	#13; //Method scala/collection/mutable/StringBuilder."<init>":()V
   7:	ldc	#16; //String child
   9:	invokevirtual	#20; //Method scala/collection/mutable/StringBuilder.append:(Ljava/lang/Object;)Lscala/collection/mutable/StringBuilder;
   12:	aload_0
   13:	invokespecial	#25; //Method A.a:()Ljava/lang/String;
   16:	invokevirtual	#20; //Method scala/collection/mutable/StringBuilder.append:(Ljava/lang/Object;)Lscala/collection/mutable/StringBuilder;
   19:	invokevirtual	#28; //Method scala/collection/mutable/StringBuilder.toString:()Ljava/lang/String;
   22:	areturn

public B B$M$$$outer();
  Code:
   0:	aload_0
   1:	getfield	#34; //Field $outer:LB;
   4:	areturn

public B$M(B);
  Code:
   0:	aload_1
   1:	ifnonnull	12
   4:	new	#37; //class java/lang/NullPointerException
   7:	dup
   8:	invokespecial	#38; //Method java/lang/NullPointerException."<init>":()V
   11:	athrow
   12:	aload_0
   13:	aload_1
   14:	putfield	#34; //Field $outer:LB;
   17:	aload_0
   18:	invokespecial	#41; //Method java/lang/Object."<init>":()V
   21:	return

}

@scabug
Copy link
Author

scabug commented Dec 12, 2012

@JamesIry said:
The instructions

   12:	aload_0
   13:	invokespecial	#25; //Method A.a:()Ljava/lang/String;

would be correct if z were defined directly on B. But the intervening class M means it should be going through the M instance's outer field to get to the enclosing B instance first.

@scabug
Copy link
Author

scabug commented Dec 12, 2012

@JamesIry said:
The trees look sound all the way through

  class B$M extends java.lang.Object with ScalaObject {
    def z(): java.lang.String = "child".+(B$M.this.B$M$$$outer().super[A].a());

But the icode isn't right

  def z(): java.lang.String {
  locals: 
  startBlock: 1
  blocks: [1]
  
  1: 
    6	CALL_PRIMITIVE(StartConcat)
    6	CONSTANT("child")
    6	CALL_PRIMITIVE(StringConcat(REF(class String)))
    6	THIS(B$M)
    6	CALL_METHOD A.a (super(A))
    6	CALL_PRIMITIVE(StringConcat(REF(class String)))
    6	CALL_PRIMITIVE(EndConcat)
    6	RETURN(REF(class String))
    
  }

@scabug
Copy link
Author

scabug commented Dec 12, 2012

@JamesIry said:
Line 770 of GenICode does a pattern match like so

        case Apply(fun @ Select(Super(_, mix), _), args) =>

It ignores the tree that's the left argument of Super, assuming it will be a This.

@scabug
Copy link
Author

scabug commented Dec 12, 2012

@JamesIry said:
Making it pay attention to that left argument doesn't work either. The JVM requires that invokespecial's target class be this class or a super class. So the problem is earlier in tree transformation after all. In order to make this code work we need a super accessor - a method generated in B that can forward to super.a() and B#M needs to call that accessor method. Now investigating the SuperAccessor phase.

@scabug
Copy link
Author

scabug commented Dec 12, 2012

@JamesIry said:
Line 121 of SuperAccessors has a condition

{code} if (name.isTermName && mix == tpnme.EMPTY && (clazz.isTrait || clazz != currentClass || !validCurrentOwner)) {code}

which this call fails because mix is not empty, and thus the tree isn't transformed with an accessor.

Changing the condition to

{code} if (name.isTermName && ((mix == tpnme.EMPTY && clazz.isTrait) || clazz != currentClass || !validCurrentOwner)) {code}

seems to solve this bug and doesn't break the case where A is a trait. However, this is some subtle stuff so I'm digging in to understand exactly what the condition is detecting/preventing. For instance, the name "mix" suggests that it actually SHOULD be empty in this case so maybe the real problem lies upstream.

@scabug
Copy link
Author

scabug commented Dec 12, 2012

@JamesIry said:
Okay, mix shouldn't be empty because it's the type in brackets on super (e.g. 'A' in B.super[A]). So the question now is why is mix == EMPTY ever something we care about? We should only care about whether the target of the super is a super class of this class or not - not means we need an accessor.

@scabug
Copy link
Author

scabug commented Dec 12, 2012

@retronym said:
It would still be prudent to harden GenICode to ensure that only tpnme.EMPTY makes it that far.

@scabug
Copy link
Author

scabug commented Dec 12, 2012

@JamesIry said:
Here's the latest and greatest news.

Changing the condition to the eminently reasonable {code} if (name.isTermName && (clazz.isTrait || !currentClass.isSubClass(clazz) || !validCurrentOwner)) {code} fixes this bug. But it reveals another problem. If you need to do B.super[A].a and B.super[C].a (i.e. you've got a second trait in the mix) then the current name mangler tries to generate the same method name twice - it doesn't take into account the fact that the super accessors go to different things. Fixing that is easy enough but requires a binary breaking change or a giant hack-ball.

I'm going to see if I can narrowly fix this bug without creating any more breakage related to name mangling and then separately fix the name mangling in master where the binary incompatibility is okay. If not we may have to defer this bug.

@scabug
Copy link
Author

scabug commented Dec 12, 2012

@JamesIry said (edited on Dec 13, 2012 5:17:26 AM UTC):
Okay, now I've got it. The reason the existing code doesn't have a name mangling problem is because if you do B.super[A].a and A is trait then instead of a super accessor it just becomes a call to the A's impl class. That means B.super[C].a can't collide. My attempts to fix resulted in name collisions because I started generating accessors for everything. Long story short, my initial rewrite of the condition is more nearly right than later variants. If the target clazz is a trait we only need a super accessor if mix is EMPTY. The problem with the old code is the if mix isn't EMPTY we still need an accessor for classes.

@scabug
Copy link
Author

scabug commented Dec 13, 2012

@paulp said:
Poking around I found a a variation which crashes the compiler in mixin rather than generating a VerifyError. I think this will naturally be scooped up by the fix you're already on, but for completeness.

class O1 { def f = "" } // O1 must be a class
class O2 extends O1 {
  class T1 { def g = O2.super[O1].f } // ok
  trait T2 { def g = O2.super[O1].f } // crash
}
/****

class O2$T2$class
  at scala.tools.nsc.transform.Mixin$MixinTransformer.scala$tools$nsc$transform$Mixin$MixinTransformer$$postTransform(Mixin.scala:1202)
  at scala.tools.nsc.transform.Mixin$MixinTransformer$$anonfun$transform$1.apply(Mixin.scala:1248)
  at scala.tools.nsc.transform.Mixin$MixinTransformer$$anonfun$transform$1.apply(Mixin.scala:1248)
  at scala.reflect.internal.SymbolTable.atPhase(SymbolTable.scala:201)

****/

@scabug
Copy link
Author

scabug commented Dec 13, 2012

@JamesIry said:
Yeah, my current round of changes seems to deal with that as well. Just buttoning up comments to explain what's happening.

@scabug
Copy link
Author

scabug commented Dec 13, 2012

@JamesIry said:
scala/scala#1770

@scabug scabug closed this as completed Dec 14, 2012
@scabug scabug added this to the 2.9.3-RC1 milestone Apr 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant