Scala Programming Language
  1. Scala Programming Language
  2. SI-4214

Incorrect bytecode generated for StringBuilder.max

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Misc Compiler
    • Labels:
      None

      Description

      === Background ===
      While trying to weave a simple AspectJ aspect is a Scala compiler jar,
      we got a puzzling error that fails the AspectJ weaver if we use
      Scala version 2.8.1 or 2.9.0-SNAPSHOT, but not 2.8.0.
      Upon closer inspection (see below) there appears to be a problem
      due to inconsistent bytecode generated by the Scala compiler.
      This not only fails with AspectJ, but also
      crashes the Eclipse compiler in a plain (non-AspectJ) Java project.

      Here is the analysis based on input from AspectJ project lead Andy Clement.

      The issue is with StringBuilder.max (with 2.8.1)

      === What steps will reproduce the problem ? ===
      Here is the bytecode generated by the Scala compiler:

      public java.lang.Object max(scala.math.Ordering);
       Code:
        Stack=2, Locals=2, Args_size=2
        0:   aload_0
        1:   aload_1
        2:   invokestatic    SI-1024; //Method
      scala/collection/TraversableOnce$$class.max:(Lscala/collection/TraversableOnce;Lscala/math/Ordering;)Ljava/lang/Object;
        5:   areturn
       Signature: length = 0x2
        03 FFFFFFFD
      

      So it calls another method then finishes with an ARETURN (i.e.
      returning an object, not a primitive). Now it is generic so we can
      lookup the original signature using that attribute, the generic
      signature is:

       <B:Ljava/lang/Object;>(Lscala/math/Ordering<TB;>;)C;
      

      and that says that the method returns a char (the trailing C), so it
      should have been declared 'char max(Ordering)' and finish with an
      IRETURN.

      Now, the compiler may be being clever and looking for the boxing to a
      Character, but it seems odd that the generic signature disagrees
      with the bytecode signature in that way.

      In Scala 2.8.0 it was not a generic method so there is no mismatch in
      signatures.

      === Seeing the problem without involving AspectJ ===
      If you want to see the Eclipse compiler crashing, create a pure java project,
      give it a dependency on the 2.8.1 scala library then try to compile
      this:

      import scala.collection.mutable.StringBuilder;
      
      public class CC {
             public void m() {
                     StringBuilder sb = new StringBuilder();
             }
       }
      

      Eclipse throws up an error message (an Internal compiler error)

      Internal compiler error: java.lang.ClassCastException: 
      org.eclipse.jdt.internal.compiler.lookup.BaseTypeBinding cannot be cast to 
      org.eclipse.jdt.internal.compiler.lookup.ReferenceBinding at 
      org.eclipse.jdt.internal.compiler.lookup.BinaryTypeBinding.initializeTypeVariable(BinaryTypeBinding.java:944)	CC.java	/scala-library-bug/src/bug	
      line 0
      

      I can attach the project if you wish, but as you can see,
      there isn't much needed to get the Eclipse compiler to crash.

      === What versions of the following are you using? ===

      • Scala: 2.8.1 and 2.9.0-SNAPSHOT (2.8.0 works fine)
      • Java: 1.6.0_22
      • Operating system: Mac Snow Leopard

        Activity

        Hide
        Paul Phillips added a comment -

        The following program generates an invalid signature. It is again the primitive type parameter problem.

        import scala.collection.mutable
        
        object Test {
          def f[A, B] = new mutable.HashMap[A, B] { }
          def g[A] = new mutable.HashMap[A, Int] { }
          def h[A] = new mutable.HashMap[A, Int] { override def default(k: A) = 0 }
        }
        

        Methods f and g are marked bridge as expected.

        public B scala.collection.mutable.HashMap.default(A) (bridge) in Test$$$$anon$$2
        public B scala.collection.mutable.HashMap.default(A) (bridge) in Test$$$$anon$$3
        

        The third class gets these:

        bridge=false public int Test$$$$anon$$3.default(A)
        bridge=true  public java.lang.Object Test$$$$anon$$3.default(java.lang.Object)
        

        But Int is not a valid return type for the non-bridge method, because all code based on HashMap will be looking for an object on the stack.

        As far as I can see, there isn't any valid way to implement a generically defined method and have primitives appearing in the signature in type parameter positions. My only idea (this is what I was talking about at villars) is to implement additional bridges, like:

        public java.lang.Integer Test$$$$anon$$3.default(java.lang.Object)
        

        But that may be infeasible.

        So the signature bombs are still in there. I found this one in SeqLike.

        private def occCounts[B](sq: Seq[B]): mutable.Map[B, Int] = {
          val occ = new mutable.HashMap[B, Int] { override def default(k: B) = 0 }
          for (y <- sq.seq) occ(y) += 1
          occ
        }
        
        Show
        Paul Phillips added a comment - The following program generates an invalid signature. It is again the primitive type parameter problem. import scala.collection.mutable object Test { def f[A, B] = new mutable.HashMap[A, B] { } def g[A] = new mutable.HashMap[A, Int] { } def h[A] = new mutable.HashMap[A, Int] { override def default(k: A) = 0 } } Methods f and g are marked bridge as expected. public B scala.collection.mutable.HashMap.default(A) (bridge) in Test$$$$anon$$2 public B scala.collection.mutable.HashMap.default(A) (bridge) in Test$$$$anon$$3 The third class gets these: bridge=false public int Test$$$$anon$$3.default(A) bridge=true public java.lang.Object Test$$$$anon$$3.default(java.lang.Object) But Int is not a valid return type for the non-bridge method, because all code based on HashMap will be looking for an object on the stack. As far as I can see, there isn't any valid way to implement a generically defined method and have primitives appearing in the signature in type parameter positions. My only idea (this is what I was talking about at villars) is to implement additional bridges, like: public java.lang.Integer Test$$$$anon$$3.default(java.lang.Object) But that may be infeasible. So the signature bombs are still in there. I found this one in SeqLike. private def occCounts[B](sq: Seq[B]): mutable.Map[B, Int] = { val occ = new mutable.HashMap[B, Int] { override def default(k: B) = 0 } for (y <- sq.seq) occ(y) += 1 occ }
        Hide
        Paul Phillips added a comment -

        See also SI-4388.

        Show
        Paul Phillips added a comment - See also SI-4388 .
        Hide
        Martin Odersky added a comment -

        Can you elaborate? What goes wrong? I did not understand:

        "But Int is not a valid return type for the non-bridge method, because all code based on HashMap? will be looking for an object on the stack."

        I thought, that's what the bridge method is for?

        – Martin

        Show
        Martin Odersky added a comment - Can you elaborate? What goes wrong? I did not understand: "But Int is not a valid return type for the non-bridge method, because all code based on HashMap? will be looking for an object on the stack." I thought, that's what the bridge method is for? – Martin
        Hide
        Ramnivas Laddad added a comment -

        Just to report that the original issue I was seeing is now gone. So from my point of view, this issue may be closed.

        Thank you Paul and Martin. Paul, I emailed you separately around the AspectJ issue that had been taken care of.

        Show
        Ramnivas Laddad added a comment - Just to report that the original issue I was seeing is now gone. So from my point of view, this issue may be closed. Thank you Paul and Martin. Paul, I emailed you separately around the AspectJ issue that had been taken care of.
        Hide
        Martin Odersky added a comment -

        Any further input on that, or can we close it?

        Show
        Martin Odersky added a comment - Any further input on that, or can we close it?

          People

          • Assignee:
            Unassigned
            Reporter:
            Ramnivas Laddad
            TracCC:
            Ismael Juma, Paul Phillips, Seth Tisue, Simon Ochsenreither
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development