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

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

Merged
merged 1 commit into from
Nov 28, 2018

Conversation

lrytz
Copy link
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
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
Member

@lrytz
Copy link
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
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
4 participants