Skip to content

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

Merged
adriaanm merged 1 commit intoscala:2.12.xfrom
lrytz:b10812-2.12
Nov 28, 2018
Merged

[backport] Don't emit forwarder in mirror class for bridge methods#7469
adriaanm merged 1 commit intoscala:2.12.xfrom
lrytz:b10812-2.12

Conversation

@lrytz
Copy link
Copy Markdown
Member

@lrytz lrytz commented Nov 27, 2018

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 #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 #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;

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;
```
@scala-jenkins scala-jenkins added this to the 2.12.9 milestone Nov 27, 2018
@lrytz lrytz modified the milestones: 2.12.9, 2.12.8 Nov 27, 2018
@lrytz
Copy link
Copy Markdown
Member Author

lrytz commented Nov 27, 2018

See scala/bug#11271 (comment) for an integration test with Akka.

@lrytz lrytz requested a review from adriaanm November 27, 2018 11:10
@SethTisue
Copy link
Copy Markdown
Member

@lrytz
Copy link
Copy Markdown
Member Author

lrytz commented Nov 28, 2018

enumeratum in the community build, during tests, not sure if / how that's related / spurious:

[enumeratum] [info] EnumJVMSpec:
[enumeratum] [info] findValues Vector
[enumeratum] [info] - should be in the same order that the objects were declared in *** FAILED ***
[enumeratum] [info]   java.lang.RuntimeException: output directory /home/jenkins/workspace/scala-2.12.x-integrate-community-build/target-0.9.16/project-builds/enumeratum-c71d2a973fcdaa37d6030b29bd483c95a9093de4/macros/.jvm/target/scala-2.12.8-SNAPSHOT/classes does not exist.
[enumeratum] [info]   at scala.sys.package$.error(package.scala:30)
[enumeratum] [info]   at enumeratum.Eval$.$anonfun$macroToolboxClassPath$1(Eval.scala:25)

and a few more of the same. I started a rebuild which failed in the same way (https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/3819/consoleFull)

@adriaanm
Copy link
Copy Markdown
Contributor

The same test fail happened before. If it does turn out to be related, we can follow-up with a PR later this week, before we cut the release (hopefully on Monday?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes worth highlighting in next release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants