Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Emit bridge method forwarders with BRIDGE flag #7035

Merged
merged 1 commit into from Aug 21, 2018

Conversation

Projects
None yet
5 participants
@eed3si9n
Copy link
Member

eed3si9n commented Aug 10, 2018

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

@eed3si9n eed3si9n requested review from retronym and lrytz Aug 10, 2018

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

@lrytz

This comment has been minimized.

Copy link
Member

lrytz commented Aug 10, 2018

Need to also backport #7017

@lrytz

This comment has been minimized.

Copy link
Member

lrytz commented Aug 10, 2018

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

This comment has been minimized.

Copy link
Member

retronym commented Aug 10, 2018

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

This comment has been minimized.

Copy link
Member Author

eed3si9n commented Aug 10, 2018

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

This comment has been minimized.

Copy link
Member Author

eed3si9n commented Aug 11, 2018

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.

@eed3si9n eed3si9n force-pushed the eed3si9n:bport/b10812 branch from 01a4bf8 to 537deb0 Aug 11, 2018

@eed3si9n 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 smarter referenced this pull request Aug 11, 2018

Open

Release 2.11.13 #451

// 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) {

This comment has been minimized.

@lrytz

lrytz Aug 14, 2018

Member

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.

This comment has been minimized.

@eed3si9n

eed3si9n Aug 17, 2018

Author Member

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?

This comment has been minimized.

@lrytz

lrytz Aug 20, 2018

Member

@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]

This comment has been minimized.

@eed3si9n

eed3si9n Aug 20, 2018

Author Member

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

@eed3si9n eed3si9n force-pushed the eed3si9n:bport/b10812 branch from edecc7b to 2b664b9 Aug 20, 2018

@lrytz
Copy link
Member

lrytz left a comment

[nomerge] Emit bridge method forwarders with BRIDGE flag
Fixes scala/bug#11061
Ref scala/bug#10812

On 2.13.x branch #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 eed3si9n force-pushed the eed3si9n:bport/b10812 branch from 2b664b9 to 0763168 Aug 20, 2018

@eed3si9n

This comment has been minimized.

Copy link
Member Author

eed3si9n commented Aug 20, 2018

@lrytz Yea. That makes sense.

@eed3si9n

This comment has been minimized.

Copy link
Member Author

eed3si9n commented Aug 21, 2018

/rebuild

@lrytz

lrytz approved these changes Aug 21, 2018

@lrytz

This comment has been minimized.

Copy link
Member

lrytz commented Aug 21, 2018

Thanks, @eed3si9n!

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

3 checks passed

cla @eed3si9n signed the Scala CLA. Thanks!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
validate-main [939] SUCCESS. Took 82 min.
Details

@eed3si9n eed3si9n deleted the eed3si9n:bport/b10812 branch Aug 21, 2018

@@ -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 has been minimized.

@lrytz

lrytz Oct 18, 2018

Member

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.

lrytz added a commit to lrytz/scala that referenced this pull request Nov 1, 2018

No static forwarders for bridges implementing abstract methods
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

lrytz added a commit to lrytz/scala that referenced this pull request Nov 1, 2018

[nomerge] No static forwarders for bridges implementing abstract methods
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

lrytz added a commit to lrytz/scala that referenced this pull request Nov 1, 2018

[nomerge] No static forwarders for bridges implementing abstract methods
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

lrytz added a commit to lrytz/scala that referenced this pull request Nov 27, 2018

[backport] Don't emit forwarder in mirror class for bridge methods
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;
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.