Fix #4430: Disallow access to Java bridge methods before erasure

[odersky]

Scala bridge methods only come to existence during erasure, so are not visible
before. But Java bridge methods come with their class files. It first I thought
we should get parity, so have to ignore Java bridge methods before erasure. But
that did not work, because unlike for Scala, some Java bridges are generated
to allow access to inherited methods. The situation can be described using
StringBuilder. It inherits a class AbstractStringBuilder which is not accessible
from Scala. Here's a minimized version highlighting the problem:

private class AbstractStringBuilder {
  def length: Int = ...
  def append(xs: Array[Char]): AbstractStringBuilder = ...
}
class StringBuilder extends AbstractStringBuilder {
  override def append(xs: Array[Char]): StringBuilder = ...
  <bridge> def length: Int = super.length
  <bridge> def append(xs: Array[Char]): AbstractStringBuilder = this.append(xs)
}

User code

val sb = new StringBuilder
sb.length      // needs to access bridge in StringBuilder
sb.append(arr) // should not access bridge in StringBuilder

In the first case, sb.length needs to access the bridge method, as
the inherited method is inaccssible. So we cannot simply hide bridge
methods before erasure, as we do for Scala bridges. But in the second
case, we should not access the bridge append, because it refers to an
inaccessible class in its result type.

The fix is simple, but it took me a long time to find it: When merging denotations
with the same signature (which is the case for the two appends, since results are
ignored), always prefer a non-bridge over a bridge as the symbol of a JointRefDenotation.

[smarter]

This should be a DottyBytecodeTests that checks the call signature.

[odersky]

Can somebody else do the bytecode test? It's one of the few things I don't want to get into.

[smarter]

@retronym do you know how scalac deals with Java bridges?

[Blaisorblade]

Makes perfect sense to me (apart from @smarter’s comment of course).

[odersky]

Turns out, accessibleFrom was the wrong way to do it since the compiler sometimes selected a bridge and then complained it was not accessible. A more precise way is in the current commit 1bef624: Mark them as private before erasure, and drop the private flag as part of the erasure transform. That brings them more in line with normal bridge methods. Also, the issue is contained better, needing changes in just ClassfileParser and Erasure.

[odersky]

Still wrong. This is a lot trickier than I thought. Turns out some Java bridges need to be accessible from the start.

[smarter]

I think something else is going on here. If I run the added test case with -Xprint-types -Xprint:all, the type of append is initially correct (the result type is StringBuilder), but after Erasure this changes (the result type is now AbstractStringBuilder), so it looks like Erasure is redoing overloading resolution somehow and getting it wrong.

[odersky]

@allanrenucci Can I ask you to add the bytecode test? Thanks!

[odersky]

@smarter I believe it is mergeDenot that is the culprit. Erasure does not perform any overloading resolution to my knowledge.

[retronym]

By default, scalac excludes members that had the ACC_BRIDGE flag from Type.member etc.

scala> typeOf[java.lang.String].memberBasedOnName(TermName("compareTo"), excludedFlags = 0L).alternatives.map(_.defString)
res11: List[String] = List(def compareTo(x$1: Any): Int, def compareTo(x$1: String): Int)

scala> typeOf[java.lang.String].memberBasedOnName(TermName("compareTo"), excludedFlags = reflect.internal.Flags.BridgeFlags).alternatives.map(_.defString)
res12: List[String] = List(def compareTo(x$1: String): Int)

[odersky]

@retronym

By default, scalac excludes members that had the ACC_BRIDGE flag from Type.member etc.

That's what I tried as well, but then new StringBuilder().length resolves to the one in AbstractStringBuilder, which gives an illegal access error at runtime. How does scalac handle this?

[retronym]

The backend uses the qualifier type as the class of CONSTANT_Methodref in the INVOKEVIRTUAL, rather than the owner of the invoked method. The method descriptor does come directly from the symbol itself.

CONSTANT_Methodref_info {
    u1 tag;
    u2 class_index;
    u2 name_and_type_index;
}

There are other corner cases handled in erasure:

https://github.com/scala/scala/blob/69d60cb54d787a90c74de092cc5173e12a1087fb/src/compiler/scala/tools/nsc/transform/Erasure.scala#L1220-L1264

https://github.com/scala/scala/blob/69d60cb54d787a90c74de092cc5173e12a1087fb/src/compiler/scala/tools/nsc/transform/Erasure.scala#L730-L749

@lrytz was the last to work on these, adding some documentation in the code in https://github.com/scala/scala/pull/6023/files

[odersky]

The backend uses the qualifier type as the class of CONSTANT_Methodref in the INVOKEVIRTUAL, rather than the owner of the invoked method. The method descriptor does come directly from the symbol itself.

Ah, that would explain it. We have to check in the backend to see whether it does the right thing and to fix it if not.

Outdated.

[odersky]

I believe the change in this PR is harmless: when in doubt it is better to select a non-bridge over a bridge. Ideally, the backend would compensate. But is somebody ready to do this right now? Or should we wait for the merged backend. In the latter case I propose to merge this PR, so that we have one important issue less to deal with.