Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: Scala 2.9.2, Scala 2.10.0
    • Component/s: None
    • Labels:
      None

      Description

      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
      }
      

        Activity

        Hide
        James Iry added a comment -

        Here's the latest and greatest news.

        Changing the condition to the eminently reasonable

              if (name.isTermName && (clazz.isTrait || !currentClass.isSubClass(clazz) || !validCurrentOwner)) 
        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.

        Show
        James Iry added a comment - Here's the latest and greatest news. Changing the condition to the eminently reasonable if (name.isTermName && (clazz.isTrait || !currentClass.isSubClass(clazz) || !validCurrentOwner)) 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.
        Hide
        James Iry added a comment - - edited

        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.

        Show
        James Iry added a comment - - edited 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.
        Hide
        Paul Phillips added a comment -

        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)
        
        ****/
        
        Show
        Paul Phillips added a comment - 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) ****/
        Hide
        James Iry added a comment -

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

        Show
        James Iry added a comment - Yeah, my current round of changes seems to deal with that as well. Just buttoning up comments to explain what's happening.
        Show
        James Iry added a comment - https://github.com/scala/scala/pull/1770

          People

          • Assignee:
            James Iry
            Reporter:
            Paul Phillips
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Due:
              Created:
              Updated:
              Resolved:

              Development