Incorrect bytecode generated for StringOps.minBy/maxBy

[scabug]

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

It looks like #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 #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");
					}
				}
			}
		}
	}
}

[scabug]

Imported From: https://issues.scala-lang.org/browse/SI-6554?orig=1
Reporter: Ramnivas Laddad (ramnivas)
Affected Versions: 2.10.0-RC1
See #3452

[scabug]

Ramnivas Laddad (ramnivas) said:
Looked a little further and it seems that the issue exists (in that the above program prints "Incompatible") all the way from 2.9.0. However, AspectJ weaving produces a VerifyError only for 2.9.2 and later.

[scabug]

@adriaanm said:
hmm, this is a tough call

the only workaround i can think of is to exclude the methods with generic signatures that trip up aspectj from your pointcuts

it's not technically a regression, and not a blocker (since it has a workaround), so not something we should fix in RC mode

unfortunately, it would break binary compatibility, so we'll have to defer until 2.11.x

[scabug]

@gkossakowski said:
I just checked and this issue is still present in latest master. I'll see if we can do something about it before 2.11.0-RC1.

[scabug]

@gkossakowski said:
Minimized:

trait Foo[A] {
  def minBy[B](b: B): A = ???
}

class Bar extends Foo[Int]

The generic signature of minBy in Bar is <B:Ljava/lang/Object;>(TB;)I;. However, the regular signature is public java.lang.Object minBy(java.lang.Object);. The problem is that we have a primitive type in position where erased type is Object.

It's interesting to observe what happens when we manually move implementation to Bar:

trait Foo[A] {
  def minBy[B](b: B): A
}

class Bar extends Foo[Int] {
  def minBy[B](b: B): Int = ???
}

In that case we get two minBy methods generated (bridge, and regular method) and only the regular method gets generic signature that is identical to the example above. However, regular method will have int as erased return type so generic signature and erased signature agree. It's also interesting to note that we don't generate generic signatures for bridge methods whereas Java does.

[scabug]

@retronym said:
Sounds a whole lot like #3452

[scabug]

@gkossakowski said:
I don't see connection with #3452. However, it's exactly #4214. The scala/scala@e42733e 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

[scabug]

@gkossakowski said:
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] {
def minBy[B](b: B): A = ???
}
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.

[scabug]

Ramnivas Laddad (ramnivas) said:
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 #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.

[scabug]

@retronym said:
I'm taking a swing at this for 2.11: scala/scala#3493

[scabug]

@retronym said:
Fixed, by selectively emitting weaker signatures trait- and static forwarders:

scala/scala#3493

More details:

https://issues.scala-lang.org/browse/SI-3452?focusedCommentId=68230&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-68230