Look up members at erasure phase for generating static forwarders

[lrytz]

Forward-ports scala/scala@b6d7e0c42c

Fixes #12753

[lrytz]

This is now green.

We did a similar change in 2.12.7 (scala/scala#7469) which also broke binary compatibility in almost exactly the same way. In the case of 2.12.5, there were two static forwarders for get in this example:

class A[T] { def get: T = ??? }
object T extends A[String] { override def get: String = "hi" }

When I compile a Java T.get(), it links to the ()String static forwarder.

Scala 3 generates the exact same static forwarders in T.class as 2.12.5, just in opposite order in the classfile. Compiling against that links to the ()Object overload. So the order of methods in the classfile seems to make a difference (javac 1.8.0_231).

Interestingly, javac only sees the ()Object variant, so compiling T.get().trim() fails (cannot find symbol: method trim()). This is good, because it prevents some (not all) usages of the method that's being removed by this PR.

I think this should go ahead. It's important for some Scala libraries that provide a Java API (e.g., akka, twitter/util).

[smarter]

In scala/scala#7469 you note:

This change breaks binary compatibility. However, in the examples we
tested, the Java compiler emits references to the non-bridge methods,
so compiled code continues to work if a library is replaced by a new
version that doesn't have forwarders for bridges

But based on your last comment, this reasoning is not true in Scala 3:

Scala 3 generates the exact same static forwarders in T.class as 2.12.5, just in opposite order in the classfile. Compiling against that links to the ()Object overload.

So unlike in Scala 2, this change would actually break binary-compatibility in practice (and @sjrd would say this means we need to release Scala 4). You also mention that:

PR #7035 fixed this for 2.12.7 by adding the ACC_BRIDGE flag to static
forwarders for bridges [...] However, the new flag causes the eclipse Java compiler (and apparently
also IntelliJ) to report ambiguity errors when using static forwarders.

Did you try adding ACC_SYNTHETIC too? I don't know about ejc/intellij but javac consistently ignores ACC_SYNTHETIC methods.

[lrytz]

The ACC_BRIDGE story is only a side-note, we added ACC_BRIDGE in 2.12.6, but then realized that doesn't work as intended. So if you look at the 2.12.5 -> 2.12.7 transition, it's the same as in Scala 3, up to the order of methods in the classfile. This order does have an effect and can cause an actual binary compatibility, at least for some javac compilers.

I haven't tried ACC_SYNTHETIC, can do that if desired. Personally I think the real fix is preferable; it's a niche case (Java interop, overriding), and the ecosystem is still young and will pick up new Scala 3 releases.

[smarter]

I haven't tried ACC_SYNTHETIC, can do that if desired.

If that works, I think that would be preferable. We made a commitment to binary-compatibility when releasing 3.0.0 and we shouldn't go back on that unless we absolutely need to.

[lrytz]

synthetic works, I updated the PR. It was a little tricky to know which methods to mark synthetic.

[smarter]

Awesome, were you able to check the behavior of eclipse or intellij with this change?

[lrytz]

Not yet, I'll follow up on that.

[lrytz]

ecj behaves the same as OpenJDK javac; it prefers the ()Object variant when the Scala object is compiled with 3.0.0, and it picks the ()String one after this PR.

I also tested javac on JDK 11, which reports an ambiguity for the 3.0.0-compiled version (scala/bug#11061), and works with this PR (one marked synthetic).

For IntelliJ, I created a fresh Java-only project and added a directory with classfiles as dependency. IntelliJ is quite confused about the dependency if it contains Scala-3-compiled classfiles, but that has nothing to do with this PR, it's the same before and after. Compiling this class into the directory

package p

class C {
  def hi = ""
}

IntelliJ doesn't see the class C

It works when compiling the class with Scala 2.13.

I created an IntelliJ ticket (https://youtrack.jetbrains.com/issue/SCL-19171).

[smarter]

Given:

class A[T] { def get: T = ??? }
object T extends A[String] { override def get: String = "hi" }

If I look at what we do before this PR, I see that sortedMembersBasedOnFlags returns two get methods, but only one of them .is(Bridge), can't we rely on that to know that its forwarder should be synthetic?

[lrytz]

I gave that a try, but

trait T { def m  = 1 }
object O extends T

looking up O$'s members at the backend phase we get a method O$.d that's marked Bridge. Also, for

trait T { def m: Object }
object O extends T { def m: String = "" }

the lookup returns both a bridge ()Object and the ()String method. However, 3.0.0 doesn't generate a static accessor for the bridge in this case, so it would be quite weird to start adding a new synthetic method now.

[smarter]

looking up O$'s members at the backend phase we get a method O$.d that's marked Bridge.

Ah, that's because we mark mixin forwarders as bridge too, so we could try excluding only bridges generated at erasure.

However, 3.0.0 doesn't generate a static accessor for the bridge in this case, so it would be quite weird to start adding a new synthetic method now.

But if we only add this check in the place where the original code called addForwarder we shouldn't end up adding more forwarders.

I've implemented this idea in #12860, let me know what you think.