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

Incorrect bytecode generated for StringOps.minBy/maxBy

    Details

      Description

      (This is against 2.10.0-RC1, but JIRA doesn't offer that choice)

      It looks like SI-4214 is resurrected.

      While trying to weave in an AspectJ aspect, I got a VerifyError. Upon closer examination, it looks like the bytecode generated for StringOps.minBy (and StringOps.maxBy) is incorrect. Here is a simple Java program that shows the issue (a small variation of one reported in SI-4214).

      package bug;
      
      import java.lang.reflect.Method;
      import java.lang.reflect.Type;
      
      import scala.collection.immutable.StringOps;
      
      public class Main {
      	public static void main(String[] args) {
      		Method[] ms = StringOps.class.getDeclaredMethods();
      		for (Method m : ms) {
      			if (m.getName().equals("minBy")) {
      				Class<?> c = m.getReturnType();
      				System.out.println("Bytecode return type: " + c);
      				Type t = m.getGenericReturnType();
      				System.out.println("Generic return type : " + t);
      				if (t instanceof Class) {
      					if (((Class) t).isPrimitive() && !c.isPrimitive()) {
      						System.out.println("Incompatible");
      					}
      				}
      			}
      		}
      	}
      }
      

        Issue Links

          Activity

          Hide
          Grzegorz Kossakowski added a comment -

          I don't see connection with SI-3452. However, it's exactly SI-4214. The https://github.com/scala/scala/commit/e42733e9fe1f3af591976fbb48b66035253d85b9 was meant to fix this problem but I don't understand (yet) the fix to really see what got overlooked in that commit.

          I have a hard time understanding how Erasure.javaSig can work without considering whether method with given symbol overrides some other method. I wish javaSig documented some of its assumptions (like what kind of information does it need to take into account).

          Moreover, I learnt about Ycheck:jvm which reports the problem:

          Foo.scala:5: warning: compiler bug: created generic signature for method minBy in Bar that does not conform to its erasure
          signature: <B:Ljava/lang/Object;>(TB;)I
          original type: [B](b: Object)Int
          normalized type: [B](b: Object)Int
          erasure type: (b: Object)Object
          if this is reproducible, please report bug at http://issues.scala-lang.org/
          class Bar extends Foo[Int] {
                ^
          [Now checking: jvm]
          one warning found
          
          Show
          Grzegorz Kossakowski added a comment - I don't see connection with SI-3452 . However, it's exactly SI-4214 . The https://github.com/scala/scala/commit/e42733e9fe1f3af591976fbb48b66035253d85b9 was meant to fix this problem but I don't understand (yet) the fix to really see what got overlooked in that commit. I have a hard time understanding how Erasure.javaSig can work without considering whether method with given symbol overrides some other method. I wish javaSig documented some of its assumptions (like what kind of information does it need to take into account). Moreover, I learnt about Ycheck:jvm which reports the problem: Foo.scala:5: warning: compiler bug: created generic signature for method minBy in Bar that does not conform to its erasure signature: <B:Ljava/lang/Object;>(TB;)I original type: [B](b: Object)Int normalized type: [B](b: Object)Int erasure type: (b: Object)Object if this is reproducible, please report bug at http://issues.scala-lang.org/ class Bar extends Foo[Int] { ^ [Now checking: jvm] one warning found
          Hide
          Grzegorz Kossakowski added a comment -

          After talking to Jason I got convinced that this is very hairy issue and we won't be able to fix it in 2.11 due to limited time. I'll quote Jason's summary:

          trait Foo[A]

          Unknown macro: { def minBy[B](b}

          class Bar extends Foo[Int]

          // no bridge, just a trait forwarder
          // signature matches generic signature exactly, accepts an Object, not an Int
          //
          scala> classOf[Bar].getDeclaredMethods.filter(_.getName == "minBy")
          res7: Array[java.lang.reflect.Method] = Array(public java.lang.Object Bar.minBy(java.lang.Object))

          //
          // But we emit a Java generic signature based on the member type of `minBy` in `Bar`
          //
          scala> res7.head.toGenericString
          res8: String = public <B> int Bar.minBy(B)

          Which is invalid, as `int` doesn't erase to `Object`, so you get LinkageErrors if you call this from Java, or AspectJ crashes.

          The logic to control the pre- and post-erasure signatures of the forwarder method is in Mixin::cloneBeforeErasure.

          I see two choice here:

          1. Weaken the Java generic signature of the method to reflect the reality of the erased signature
          2. Emit a bridge method to match the generic signature, and give the mixin method a more precise signature.

          Pros/ConsThe problems with 1.

          Cons of #1:

          • If we do this naively, we break source compatibility with Java code. See, for example, test/files/run/t3452e in by branch [1]. I thought we could find a middle ground that would weaken the java generic signature iff the current approach violates the invariant that the generic sig must erase to the erased sig.

          Pros of #1:

          • Simpler to implement
          • Avoid further bytecode bloat with bridges

          Cons of #2

          • Adding more bridges creates opportunities for additional "double definition after erasure" errors in previously legal code. I think I hit such a corner case when I tried to implement this, or I might have been foiled by...
          • Spreading the logic to generate bridges between Erasure and Mixin makes it really hard to implement this approach with certainty about correctness
          • more bridges means more bytecode.

          SI-3452 also highlights the same problem in a different context: static forwarder methods also have the wrong generic signatures. This one is actually much easier to fix, as we can freely change the erased signature to match the precise generic signature.

          [1] https://github.com/retronym/scala/compare/ticket/3452-rebased

          I think generating additional bridges is the way to go but we'll have to be careful about breaking existing code. I'm guessing we would need to introduce the logic behind a switch and try it out on various projects to asses how likely it is we are breaking existing code. This is 2.12 material.

          Show
          Grzegorz Kossakowski added a comment - After talking to Jason I got convinced that this is very hairy issue and we won't be able to fix it in 2.11 due to limited time. I'll quote Jason's summary: trait Foo [A] Unknown macro: { def minBy[B](b} class Bar extends Foo [Int] // no bridge, just a trait forwarder // signature matches generic signature exactly, accepts an Object, not an Int // scala> classOf [Bar] .getDeclaredMethods.filter(_.getName == "minBy") res7: Array [java.lang.reflect.Method] = Array(public java.lang.Object Bar.minBy(java.lang.Object)) // // But we emit a Java generic signature based on the member type of `minBy` in `Bar` // scala> res7.head.toGenericString res8: String = public <B> int Bar.minBy(B) Which is invalid, as `int` doesn't erase to `Object`, so you get LinkageErrors if you call this from Java, or AspectJ crashes. The logic to control the pre- and post-erasure signatures of the forwarder method is in Mixin::cloneBeforeErasure. I see two choice here: 1. Weaken the Java generic signature of the method to reflect the reality of the erased signature 2. Emit a bridge method to match the generic signature, and give the mixin method a more precise signature. Pros/ConsThe problems with 1. Cons of #1: If we do this naively, we break source compatibility with Java code. See, for example, test/files/run/t3452e in by branch [1] . I thought we could find a middle ground that would weaken the java generic signature iff the current approach violates the invariant that the generic sig must erase to the erased sig. Pros of #1: Simpler to implement Avoid further bytecode bloat with bridges Cons of #2 Adding more bridges creates opportunities for additional "double definition after erasure" errors in previously legal code. I think I hit such a corner case when I tried to implement this, or I might have been foiled by... Spreading the logic to generate bridges between Erasure and Mixin makes it really hard to implement this approach with certainty about correctness more bridges means more bytecode. SI-3452 also highlights the same problem in a different context: static forwarder methods also have the wrong generic signatures. This one is actually much easier to fix, as we can freely change the erased signature to match the precise generic signature. [1] https://github.com/retronym/scala/compare/ticket/3452-rebased I think generating additional bridges is the way to go but we'll have to be careful about breaking existing code. I'm guessing we would need to introduce the logic behind a switch and try it out on various projects to asses how likely it is we are breaking existing code. This is 2.12 material.
          Hide
          Ramnivas Laddad added a comment -

          Thanks for taking time to investigate this.

          It appears that any fix for this will take at least two releases--one to introduce a fix behind a flag and then make that flag default. If this doesn't get any kind of fix in 2.11, it will be at least another couple of years before there is a final fix. Given how long this has stayed open (and the original SI-4214 that was reported three years back) with no definitive approach in sight and backwards compatibility argument only getting stronger with (hopefully) increased Scala adoption, come 2.12, I fear it may not have any better chance of getting fixed.

          Show
          Ramnivas Laddad added a comment - Thanks for taking time to investigate this. It appears that any fix for this will take at least two releases--one to introduce a fix behind a flag and then make that flag default. If this doesn't get any kind of fix in 2.11, it will be at least another couple of years before there is a final fix. Given how long this has stayed open (and the original SI-4214 that was reported three years back) with no definitive approach in sight and backwards compatibility argument only getting stronger with (hopefully) increased Scala adoption, come 2.12, I fear it may not have any better chance of getting fixed.
          Hide
          Jason Zaugg added a comment -

          I'm taking a swing at this for 2.11: https://github.com/scala/scala/pull/3493

          Show
          Jason Zaugg added a comment - I'm taking a swing at this for 2.11: https://github.com/scala/scala/pull/3493
          Show
          Jason Zaugg added a comment - Fixed, by selectively emitting weaker signatures trait- and static forwarders: https://github.com/scala/scala/pull/3493 More details: https://issues.scala-lang.org/browse/SI-3452?focusedCommentId=68230&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-68230

            People

            • Assignee:
              Jason Zaugg
              Reporter:
              Ramnivas Laddad
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development