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

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

Closed
lrytz opened this Issue Nov 23, 2018 · 15 comments

Comments

Projects
None yet
4 participants
@lrytz
Member

lrytz commented Nov 23, 2018

This bug affects Akka users that write Java code in eclipse. They get error messages in the IDE ("The method get(ActorSystem) is ambiguous").

To reproduce, download ecj-4.9.jar from http://download.eclipse.org/eclipse/downloads/drops4/R-4.9-201809060745/#JDTCORE

T.scala

class A[T] {
  def get: T = ???
}

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

Test.java

public class Test {
  public static void main(String[] args) {
    System.out.println(T.get());
  }
}

With 2.12.6:

$> ~/scala/scala-2.12.6/bin/scalac T.scala
$> javac -version
javac 1.8.0_172
$> javac -cp . Test.java
$> java -jar ~/Downloads/ecj-4.9.jar -8 -cp . Test.java
$>

2.12.6 generates the following static methods in T.class:

  // access flags 0x9
  public static get()Ljava/lang/String;

  // access flags 0x9
  public static get()Ljava/lang/Object;

With 2.12.7 and current 2.12.x (2.12.8-bin-25c7215):

$> ../build/quick/bin/scalac T.scala
$> javac -cp . Test.java
$> java -jar ~/Downloads/ecj-4.9.jar -8 -cp . Test.java
----------
1. ERROR in /Users/luc/scala/scala12/sandbox/Test.java (at line 3)
	System.out.println(T.get());
	                     ^^^
The method get() is ambiguous for the type T
----------
1 problem (1 error)
$>

Generated methods in T.class:

  // access flags 0x49
  public static bridge get()Ljava/lang/Object;

  // access flags 0x9
  public static get()Ljava/lang/String;

The bytecode change is exactly as intended by PR scala/scala#7035 that went to 2.12.7. The follow-up PR scala/scala#7383 that went into 2.12.x fixes a different bug and does not change anything here.

Someone predicted that something like this could happen.

@lrytz lrytz added the blocker label Nov 23, 2018

@lrytz lrytz added this to the 2.12.8 milestone Nov 23, 2018

@patriknw

This comment has been minimized.

patriknw commented Nov 23, 2018

Same problem exists in IntelliJ

@adriaanm

This comment has been minimized.

Member

adriaanm commented Nov 26, 2018

I think the same reasoning that motivated us to go back to 2.12.6 in scala/scala#7383 should also apply here? Is it feasible to revert fully to the 2.12.6 behavior?

@patriknw

This comment has been minimized.

patriknw commented Nov 26, 2018

It will not work in JDK 11 if going back to exactly the same as in 2.12.6.

It's also broken in IntelliJ with 2.12.6, and I'm surprised that we haven't heard more complaints about that.

@adriaanm

This comment has been minimized.

Member

adriaanm commented Nov 26, 2018

Ah right, thanks, rock and hard place.

@adriaanm

This comment has been minimized.

Member

adriaanm commented Nov 26, 2018

So, if we backport the 2.13 fix to 2.12.8, we can make both Java 11 and alternative Java 8 compilers happy, but it will mean new versions of akka compiled with 2.12.8 would not be drop in compatible. Java users would have to recompile against the new jars.

@patriknw, what do you prefer?

@adriaanm

This comment has been minimized.

Member

adriaanm commented Nov 26, 2018

The 2.13 fix being scala/scala#6531

@patriknw

This comment has been minimized.

patriknw commented Nov 26, 2018

In above example there are two methods

def get: T
def get: String

Is the change in 2.13 to remove the forwarder for def get: T and keep def get: String? The latter corresponds to what is used in Akka extensions, so hopefully that will not be a binary incompatible change in practice? I would be happy to try out a preview.

@lrytz

This comment has been minimized.

Member

lrytz commented Nov 26, 2018

Yes, the change would remove the erased version of get: T (which has signature get: Object in bytecode).

De-compiling Test.class in the example, the Java 8 compiler emits a call to get: String. So it would be binary compatible. Not sure if there are other examples when the Java compiler would pick the "bridge" overload.

@viktorklang

This comment has been minimized.

viktorklang commented Nov 26, 2018

@viktorklang

This comment has been minimized.

viktorklang commented Nov 26, 2018

@lrytz

This comment has been minimized.

Member

lrytz commented Nov 26, 2018

It doesn't genarte anything as the compiler issues an error. Actually, without the -8 flag, ECJ compiles Test.java and also emits a call to get: String.

lrytz added a commit to lrytz/scala that referenced this issue 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;
```
@lrytz

This comment has been minimized.

Member

lrytz commented Nov 27, 2018

@patriknw I created a PR with the change to remove forwarders for bridges.

Could you test with version 2.12.8-bin-3edeaac-SNAPSHOT from this resolver: https://scala-ci.typesafe.com/artifactory/scala-pr-validation-snapshots

If you have a way to build akka with that version and drop in the jars into some project (which uses the static forwarders) compiled against a previous version of akka, that would give us some confidence for the assumption that the bridge forwarders are not actually used.

@patriknw

This comment has been minimized.

patriknw commented Nov 27, 2018

I will try it, thanks.

@patriknw

This comment has been minimized.

patriknw commented Nov 27, 2018

I have tested 2.12.8-bin-3edeaac-SNAPSHOT and it works.

For binary compatibility verification I used akka-sample-cluster-java, which is using Akka extensions.

  • First build the sample with Akka 2.5.18 (which was built with Scala 2.12.7)
  • Used sbt-native-packager to create a staging directory with all jars
  • Run the app
  • Replaced the 2.5.18 jars in lib directory with 2.5-SNAPSHOT (built with 2.12.8-bin-3edeaac-SNAPSHOT), and modified classpath in launch script
  • Run the app
  • Success

Tried same thing with Akka 2.5.17 (which was built with Scala 2.12.6).

Tried https://github.com/patriknw/akka-compile-ide-java in both Eclipse and IntelliJ (no Scala plugins installed) and they are both happy.

@lrytz

This comment has been minimized.

Member

lrytz commented Nov 27, 2018

Thank you for testing!

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