Emit bridge method forwarders with BRIDGE flag #7035

eed3si9n · 2018-08-10T04:15:58Z

Fixes scala/bug#11061
Ref scala/bug#10812

On 2.13.x branch #6531 removed the forwarder for bridge methods. I would like to do same in 2.12.x since Java 11-ea started to find them ambiguous as seen in akka/akka#25449 / scala/bug#11061.

The problem statement in scala/bug#10812 was:

Luckily, javac picks one of the two (the more specific), but IntelliJ seems to complain.

The situation has changed in Java 11-ea, which @chbatey reported in akka/akka#25449 / scala/bug#11061:

[error] /private/tmp/override-in-jdk11/src/main/java/example/Test.java:9:1: reference to get is ambiguous
[error]   both method get(akka.actor.ActorSystem) in example.TestKitExtension and method get(akka.actor.ActorSystem) in example.TestKitExtension match
[error]     TestKitExtension.get(a);

This shows that Java 11-ea thinks the forwarder to be ambiguous.

To keep binary compatibility, I am still emitting the forwarder for bridge methods, but with ACC_BRIDGE flag. I've locally built this backport and tested it in the repro build https://github.com/eed3si9n/override-in-jdk11.

2.12.6 on Java 11-ea:

sbt:akka-jdk11> compile
[info] Compiling 2 Scala sources and 1 Java source to /Users/eed3si9n/work/quicktest/override-in-jdk11/target/scala-2.12/classes ...
[info] Non-compiled module 'compiler-bridge_2.12' for Scala 2.12.6. Compiling...
[info]   Compilation completed in 6.463s.
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by sbt.internal.inc.javac.DiagnosticsReporter$PositionImpl$ (file:/Users/eed3si9n/.sbt/boot/scala-2.12.6/org.scala-sbt/sbt/1.2.1/zinc-compile-core_2.12-1.2.1.jar) to field com.sun.tools.javac.api.ClientCodeWrapper$DiagnosticSourceUnwrapper.d
WARNING: Please consider reporting this to the maintainers of sbt.internal.inc.javac.DiagnosticsReporter$PositionImpl$
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
[error] /Users/eed3si9n/work/quicktest/override-in-jdk11/src/main/java/example/Test.java:8:1: reference to get is ambiguous
[error]   both method get(akka.actor.ActorSystem) in example.TestKitExtension and method get(akka.actor.ActorSystem) in example.TestKitExtension match
[error]     TestKitExtension.get(a);
[error] (Compile / compileIncremental) javac returned non-zero exit code

2.12.7-bin-LOCAL20180810b (added BRIDGE flag to one of them)

sbt:akka-jdk11> compile
[info] Compiling 2 Scala sources and 1 Java source to /Users/eed3si9n/work/quicktest/override-in-jdk11/target/scala-2.12/classes ...
[info] Done compiling.
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by com.google.protobuf.UnsafeUtil (file:/Users/eed3si9n/.sbt/boot/scala-2.12.6/org.scala-sbt/sbt/1.2.1/protobuf-java-3.3.1.jar) to field java.nio.Buffer.address
WARNING: Please consider reporting this to the maintainers of com.google.protobuf.UnsafeUtil
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
[success] Total time: 6 s, completed Aug 10, 2018, 10:38:19 PM

javap:

  public static akka.testkit.TestKitSettings get(akka.actor.ActorSystem);
    descriptor: (Lakka/actor/ActorSystem;)Lakka/testkit/TestKitSettings;
    flags: (0x0009) ACC_PUBLIC, ACC_STATIC

  public static akka.actor.Extension get(akka.actor.ActorSystem);
    descriptor: (Lakka/actor/ActorSystem;)Lakka/actor/Extension;
    flags: (0x0049) ACC_PUBLIC, ACC_STATIC, ACC_BRIDGE

lrytz · 2018-08-10T06:09:14Z

Need to also backport #7017

lrytz · 2018-08-10T06:20:50Z

I'm a little worried about binary compatibility though. Do we have a reason to believe that these forwarders for bridges were never selected by the Java 8 compiler? It seems the extra forwarders don't have any special flags in bytecode (scala/bug#10812). Just removing the forwarders might break existing java classfiles.

As a workaround, we could try to mark forwarders for bridge methods with additional flags (SYNTHETIC and/or BRIDGE) and see how the java compilers (8, 11, intellij, eclipse) like that.

retronym · 2018-08-10T06:23:43Z

This does not look binary compatible.

For example, a call to List.empty, compiled with javac 10 or below links against the forwarder to the bridge method that this PR will remove.

$ scala
Welcome to Scala 2.12.4 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_162).
Type in expressions for evaluation. Or try :help.

scala> List.getClass.getMethods.filter(_.getName == "empty")
res1: Array[java.lang.reflect.Method] = Array(public scala.collection.immutable.List scala.collection.immutable.List$.empty(), public scala.collection.GenTraversable scala.collection.immutable.List$.empty())

scala> :quit

$ cat /tmp/Test.java
class Test {
    void test() {
        scala.collection.immutable.List.empty();
    }
}

$ javac -cp ~/scala/2.12.4/lib/scala-library.jar /tmp/Test.java -d /tmp

$ jardiff /tmp/Test.class

+++ Test.class.asm
// class version 52.0 (52)
// access flags 0x20
class Test {


  // access flags 0x0
  <init>()V
    ALOAD 0
    INVOKESPECIAL java/lang/Object.<init> ()V
    RETURN
    MAXSTACK = 1
    MAXLOCALS = 1

  // access flags 0x0
  test()V
    INVOKESTATIC scala/collection/immutable/List.empty ()Lscala/collection/GenTraversable;
    POP
    RETURN
    MAXSTACK = 1
    MAXLOCALS = 1
}

I was about the suggest the same as Lukas; we can be bug compatible in 2.12.x by marking these as BRIDGE.

eed3si9n · 2018-08-10T14:48:06Z

2.12.7-bin-LOCAL20180810 (added flags)

Simply adding flags didn't fix the problem.
It still emitted two get method both with flags.

  public static akka.testkit.TestKitSettings get(akka.actor.ActorSystem);
    descriptor: (Lakka/actor/ActorSystem;)Lakka/testkit/TestKitSettings;
    flags: (0x1049) ACC_PUBLIC, ACC_STATIC, ACC_BRIDGE, ACC_SYNTHETIC

  public static akka.actor.Extension get(akka.actor.ActorSystem);
    descriptor: (Lakka/actor/ActorSystem;)Lakka/actor/Extension;
    flags: (0x1049) ACC_PUBLIC, ACC_STATIC, ACC_BRIDGE, ACC_SYNTHETIC

2.12.7-bin-LOCAL20180809 (backport)

The patched version emits one forwarder for get:

  public static akka.testkit.TestKitSettings get(akka.actor.ActorSystem);
    descriptor: (Lakka/actor/ActorSystem;)Lakka/testkit/TestKitSettings;
    flags: (0x0009) ACC_PUBLIC, ACC_STATIC

2.12.6

2.12.6 emits two gets:

  public static akka.testkit.TestKitSettings get(akka.actor.ActorSystem);
    descriptor: (Lakka/actor/ActorSystem;)Lakka/testkit/TestKitSettings;
    flags: (0x0009) ACC_PUBLIC, ACC_STATIC

  public static akka.actor.Extension get(akka.actor.ActorSystem);
    descriptor: (Lakka/actor/ActorSystem;)Lakka/actor/Extension;
    flags: (0x0009) ACC_PUBLIC, ACC_STATIC

Does this mean that I have to figure out which condition is emitting the bad forwarder and put the flags only for those?

eed3si9n · 2018-08-11T02:10:00Z

2.12.7-bin-LOCAL20180810b (added BRIDGE flag only to one of them)

  public static akka.testkit.TestKitSettings get(akka.actor.ActorSystem);
    descriptor: (Lakka/actor/ActorSystem;)Lakka/testkit/TestKitSettings;
    flags: (0x0009) ACC_PUBLIC, ACC_STATIC

  public static akka.actor.Extension get(akka.actor.ActorSystem);
    descriptor: (Lakka/actor/ActorSystem;)Lakka/actor/Extension;
    flags: (0x0049) ACC_PUBLIC, ACC_STATIC, ACC_BRIDGE

Found a combination that seems to be working.

lrytz · 2018-08-14T06:55:14Z

src/compiler/scala/tools/nsc/backend/jvm/BCodeHelpers.scala

+      // Without it, Java 11 finds them to be ambiguous. (scala/bug#11061)
+      val membersAfterPickler = exitingPickler(moduleClass.info.membersBasedOnFlags(BCodeHelpers.ExcludedForwarderFlags, symtab.Flags.METHOD)).toList
+      val members = moduleClass.info.membersBasedOnFlags(BCodeHelpers.ExcludedForwarderFlags, symtab.Flags.METHOD)
+      for (m <- members) {


could we, instead, get the method symbols in the same way as before, and just make sure we copy over the bridge flags to the forwareder methods? this would be the least risky change.

You mean like:

- isBridge = !membersAfterUncurry.contains[Symbol](m), + isBridge = m.isBridge,

? If I did that the test fails:

[info] Test scala.lang.annotations.BytecodeTest.bridgeFlag started [error] Test scala.lang.annotations.BytecodeTest.bridgeFlag failed: java.lang.AssertionError: expected:<List(f()Ljava/lang/String;0x9, f()Ljava/lang/Object;0x49)> but was:<List(f()Ljava/lang/String;0x9, f()Ljava/lang/Object;0x9)>, took 2.672 sec [error] at scala.lang.annotations.BytecodeTest.$anonfun$bridgeFlag$1(BytecodeTest.scala:30) [error] at scala.lang.annotations.BytecodeTest.$anonfun$bridgeFlag$1$adapted(BytecodeTest.scala:24) [error] at scala.collection.immutable.List.foreach(List.scala:388) [error] at scala.lang.annotations.BytecodeTest.bridgeFlag(BytecodeTest.scala:24)

so maybe isBridge is not set at the symbol?

@eed3si9n ah, that was because bridges were not included in the member search... so instead, the member search returned the method from the superclass (which the bridge would then override / implement), and this one doesn't have the bridge flag of course. Here's a fix: https://github.com/scala/scala/compare/2.12.x...lrytz:pr7035?expand=1. I think you can squash all these commits into one and label it [nomerge]

Gotcha. Thanks for the hand holding :) A squashed commit is pushed with [nomerge].

lrytz

Thinking about it, the tests should be in https://github.com/scala/scala/blob/2.12.x/test/junit/scala/tools/nsc/backend/jvm/BytecodeTest.scala. Otherwise LGTM!

Fixes scala/bug#11061 Ref scala/bug#10812 On 2.13.x branch scala#6531 removed the mirror class forwarders for bridge methods. I would like to do same in 2.12.x since Java 11-ea started to find them ambiguous as seen in akka/akka#25449 / scala/bug#11061. To keep binary compatibility, I am still emitting the forwarders for bridge methods, but with `ACC_BRIDGE` flag.

eed3si9n · 2018-08-20T20:21:58Z

@lrytz Yea. That makes sense.

eed3si9n · 2018-08-21T14:58:00Z

/rebuild

lrytz · 2018-08-21T18:43:29Z

Thanks, @eed3si9n!

lrytz · 2018-10-18T11:53:59Z

src/compiler/scala/tools/nsc/backend/jvm/BCodeHelpers.scala

@@ -1161,7 +1170,12 @@ object BCodeHelpers {
  val ExcludedForwarderFlags = {
    import scala.tools.nsc.symtab.Flags._
    // Should include DEFERRED but this breaks findMember.
-    SPECIALIZED | LIFTED | PROTECTED | STATIC | EXPANDEDNAME | BridgeAndPrivateFlags | MACRO
+    // Note that BRIDGE is *not* excluded. Trying to exclude bridges by flag doesn't work, findMembers
+    // will then include the member from the parent (which the bridge overrides / implements).


This comment is true. Unfortunately there was an unintended side-effect: before this PR, the findMembers call for forwarder methods would return the member from the parent overridden by the bridge. This member is potentially abstract, in which case the forwarder is not emitted (https://github.com/scala/scala/blob/v2.12.7/src/compiler/scala/tools/nsc/backend/jvm/BCodeHelpers.scala#L892).

After this PR, findMembers returns the bridge, which is concrete, so a forwarder is added. This is the cause for scala/bug#11207.

In 2.12.7, scala#7035 added the `bridge` flag to static forwarders that are generated for bridge methods. (2.13 geneartes no forwarders for bridges, but we wanted to stay binary compatible in 2.12.) Unfortunately the change caused even more bridges to be generated, namely for bridge methods that implement an abstract member. Now we exclude them again, which brings the binary interface back to the state of 2.12.6. Fixes scala/bug#11207

In 2.12.6 and before, the Scala compiler emits static forwarders for bridge methods in top-level modules. These forwarders are emitted by mistake, the filter to exclude bridges did not work as expected. These bridge forwarders make the Java compiler on JDK 11 report ambiguity errors when using static forwarders (scala/bug#11061). PR scala#7035 fixed this for 2.12.7 by adding the `ACC_BRIDGE` flag to static forwarders for bridges. We decided to keep these bridges for binary compatibility. However, the new flag causes the eclipse Java compiler (and apparently also IntelliJ) to report ambiguity errors when using static forwarders (scala/bug#11271). In 2.13.x the Scala compiler no longer emits static forwarders for bridges (PR scala#6531). This PR brings the same behavior to 2.12.8. 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: ``` $> cat T.scala class A[T] { def get: T = ??? } object T extends A[String] { override def get: String = "hi" } $> ~/scala/scala-2.12.7/bin/scalac T.scala ``` Generates two forwarders in `T.class` ``` // access flags 0x49 public static bridge get()Ljava/lang/Object; // access flags 0x9 public static get()Ljava/lang/String; ``` ``` $> javac -version javac 1.8.0_181 $> cat Test.java public class Test { public static void main(String[] args) { System.out.println(T.get()); } } $> javac Test.java ``` Generates in Test.class ``` INVOKESTATIC T.get ()Ljava/lang/String; ```

eed3si9n requested review from retronym and lrytz August 10, 2018 04:15

scala-jenkins added this to the 2.12.7 milestone Aug 10, 2018

SethTisue added the prio:blocker release blocker (used only by core team, only near release time) label Aug 10, 2018

eed3si9n force-pushed the bport/b10812 branch from 01a4bf8 to 537deb0 Compare August 11, 2018 02:33

eed3si9n changed the title ~~Don't emit forwarder in mirror class for bridge methods~~ Emit bridge method forwarders with BRIDGE flag Aug 11, 2018

smarter mentioned this pull request Aug 11, 2018

Release 2.11.13 scala/scala-dev#451

Closed

1 task

lrytz reviewed Aug 14, 2018

View reviewed changes

eed3si9n force-pushed the bport/b10812 branch from edecc7b to 2b664b9 Compare August 20, 2018 10:31

lrytz requested changes Aug 20, 2018

View reviewed changes

eed3si9n force-pushed the bport/b10812 branch from 2b664b9 to 0763168 Compare August 20, 2018 20:21

lrytz approved these changes Aug 21, 2018

View reviewed changes

lrytz merged commit 4c9780f into scala:2.12.x Aug 21, 2018

eed3si9n deleted the bport/b10812 branch August 21, 2018 18:46

lrytz mentioned this pull request Oct 15, 2018

case class companion object generates two apply methods scala/bug#11207

Closed

lrytz reviewed Oct 18, 2018

View reviewed changes

lrytz mentioned this pull request Nov 1, 2018

No static forwarders for bridges implementing abstract methods #7383

Merged

lrytz mentioned this pull request Nov 23, 2018

eclipse java compiler gets confused by the new ACC_BRIDGE flag in forwarders scala/bug#11271

Closed

lrytz mentioned this pull request Nov 27, 2018

[backport] Don't emit forwarder in mirror class for bridge methods #7469

Merged

eed3si9n mentioned this pull request Jul 11, 2019

Static forwarders are sometimes missing generic signatures scala/bug#11305

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit bridge method forwarders with BRIDGE flag #7035

Emit bridge method forwarders with BRIDGE flag #7035

eed3si9n commented Aug 10, 2018 •

edited

Loading

lrytz commented Aug 10, 2018

lrytz commented Aug 10, 2018

retronym commented Aug 10, 2018 •

edited

Loading

eed3si9n commented Aug 10, 2018

eed3si9n commented Aug 11, 2018

lrytz Aug 14, 2018

eed3si9n Aug 17, 2018

lrytz Aug 20, 2018 •

edited

Loading

eed3si9n Aug 20, 2018

lrytz left a comment •

edited

Loading

eed3si9n commented Aug 20, 2018

eed3si9n commented Aug 21, 2018

lrytz commented Aug 21, 2018

lrytz Oct 18, 2018 •

edited

Loading

Emit bridge method forwarders with BRIDGE flag #7035

Emit bridge method forwarders with BRIDGE flag #7035

Conversation

eed3si9n commented Aug 10, 2018 • edited Loading

2.12.6 on Java 11-ea:

2.12.7-bin-LOCAL20180810b (added BRIDGE flag to one of them)

lrytz commented Aug 10, 2018

lrytz commented Aug 10, 2018

retronym commented Aug 10, 2018 • edited Loading

eed3si9n commented Aug 10, 2018

2.12.7-bin-LOCAL20180810 (added flags)

2.12.7-bin-LOCAL20180809 (backport)

2.12.6

eed3si9n commented Aug 11, 2018

2.12.7-bin-LOCAL20180810b (added BRIDGE flag only to one of them)

lrytz Aug 14, 2018

Choose a reason for hiding this comment

eed3si9n Aug 17, 2018

Choose a reason for hiding this comment

lrytz Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

eed3si9n Aug 20, 2018

Choose a reason for hiding this comment

lrytz left a comment • edited Loading

Choose a reason for hiding this comment

eed3si9n commented Aug 20, 2018

eed3si9n commented Aug 21, 2018

lrytz commented Aug 21, 2018

lrytz Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

eed3si9n commented Aug 10, 2018 •

edited

Loading

retronym commented Aug 10, 2018 •

edited

Loading

lrytz Aug 20, 2018 •

edited

Loading

lrytz left a comment •

edited

Loading

lrytz Oct 18, 2018 •

edited

Loading