I've added a second commit that removes cloneBeforeErasure and the tests still pass as expected. However, I also tried turning off the bridge flag and it looks like the t8905 test I added is the only one directly testing that javac gets the correct signature, so the lack of good test coverage makes it difficult to evaluate this PR, hopefully the community build will help ?
Another problem is that if we get the second commit in, then #7752 can no longer go in, thus making @sjrd very sad :). I think for his usecase what's really needed is a way to go from the forwarder symbol to the original symbol, perhaps this could be done by adding an annotation on the forwarder?

I like it :-) One area to look for issues this may cause is overloading. See scala/bug#11271 where adding the bridge flag to static forwarder methods generated in companion classes of top-level modules caused problems.

I'd like to add that we're in unspecified territory here, as far as I can tell. The jvm spec just says "The ACC_BRIDGE flag is used to indicate a bridge method generated by a compiler for the Java programming language". In the language spec I only found "Implementations can enforce these semantics by creating bridge methods".

So it's not really clear which methods can be marked bridge, and how different tools react to that. So we'd have to test a lot.

See scala/bug#11271 where adding the bridge flag to static forwarder methods generated in companion classes of top-level modules caused problems.

I did see that issue but was a bit confused by the back-and-forth, tell me if my understanding of things is wrong here:

1. scalac was emitting methods that javac considered ambiguous
2. Emitting these methods as BRIDGE fixed the issue for javac
3. ... but it didn't fix them for ejc and the intellij java compiler
4. So you ended up completely removing these methods

So my question is: were the methods considered ambiguous by ejc and intellij when they were emitted without the BRIDGE flag ? If so, then adding the BRIDGE flag somewhere shouldn't break these compilers.

The methods only became ambiguous in ejc/IntelliJ once we added the flag. In the case of static forwarders, the end goal was to remove bridge versions anyway, we just didn't do it initially in 2.12 because it's (potentially) binary incompatible. But due to the overloading issues, we finally back-ported the change from 2.13 which removes the bridge overloads. The story is also summarized here: #7469.

For mixin methods this cannot be done obviously, we need the mixin methods.

My guess is that things started to go wrong because you were emitting bridge methods that don't override anything which javac would never do, mixin forwarders by definition override something, and moreover they forward to at least one of the method they override, so that seems closer to being correct.

That's a promising guess.

Another data point is what javac produces. Example:

public class A {
  public static abstract class U<T> {
    public abstract <P> T foo(P p);
  }
  public static class V extends U<String> {
    public <P> String foo(P p) { return ""; }
  }
}

We get (ASM syntax):

// access flags 0x421
// signature <T:Ljava/lang/Object;>Ljava/lang/Object;
// declaration: A$U<T>
public abstract class A$U {

  // access flags 0x401
  // signature <P:Ljava/lang/Object;>(TP;)TT;
  // declaration: T foo<P>(P)
  public abstract foo(Ljava/lang/Object;)Ljava/lang/Object;
}

// access flags 0x21
// signature LA$U<Ljava/lang/String;>;
// declaration: A$V extends A$U<java.lang.String>
public class A$V extends A$U  {

  // access flags 0x1
  // signature <P:Ljava/lang/Object;>(TP;)Ljava/lang/String;
  // declaration: java.lang.String foo<P>(P)
  public foo(Ljava/lang/Object;)Ljava/lang/String;

  // access flags 0x1041
  public synthetic bridge foo(Ljava/lang/Object;)Ljava/lang/Object;
}

So the bridge also doesn't have a signature.

I've pushed an extra commit to exercise overload resolution, and not only do things appear to still work correctly, the test now passes on ecj whereas it considers the overload ambiguous on master!

As an aside, I think It'd be really useful and not much work to run all the tests involving .java files with both javac and ecj.

More good news: I was able to take a few existing test cases and verify that javac and ecj now see better types for them, see latest commit (static forwarders are still screwed up, but I think this really ought to be solved by generating them pre-erasure as has been suggested a few times in the past).

Besides conflicting with #7752, this PR already breaks Scala.js: @Test methods in traits are not recognized anymore, because their bridges are ignored by Scala.js, due to
https://github.com/scala-js/scala-js/blame/f524bbeff87e888c21a3d7cc1a9e75ea669bc4ce/junit-plugin/src/main/scala/org/scalajs/junit/plugin/ScalaJSJUnitPlugin.scala#L263
which was introduced to fix another issue, the latter was caused by scalac starting to copy annotations to mixin forwarders.

@sjrd So I just checked and JUnit will run test methods with the BRIDGE flag set, so I'd say Scala.js is the one doing something weird here, do you have a test case that demonstrates the issue that lead you to implement this check ?

do you have a test case that demonstrates the issue that lead you to implement this check ?

Yes, pretty much every test. Without that check, the test suite cannot be compiled without crashing the compiler using Scala >= 2.12.5.

Well ... "retournement de situation", as we say in French: it turns out this PR also fixes scala-js/scala-js#3538, which was the issue that #7752 was meant to fix on Scala.js' side. So this PR does not conflict with #7752 as much as superseding it.

And for the issue with @Test methods which I mentioned earlier, it only happens for one shady test method in our test suite whose signature is

@Test def toString()(): Unit = ...

which somehow gets a bridge generated for it. (Yes, the double ()() is in the code.)

So I'll 👍 this PR.

@SethTisue Could you run the community build on this PR ? If possible it would also be interesting to run it on 46cda53 which is this PR + don't emit the bridge flag for mixin forwarders, to see if this actually influences the community build at all.

EDIT: Started a community build for this PR: https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-community-build/1843/console
And one to check the coverage: https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-community-build/1845/console

The commit only says that this "has deleterious effects since many tools
ignore bridge methods" which is too vague for me to do anything about.

Spring Framework skips ACC_BRIDGE methods when reflectively compiling a list of annotated methods.

Here's analagous logic in Hibernate.

Graal seems to assume certain bytecode shapes within bridge methods (must delegage to a method with the same name). I don't know the context of this.

Spring Framework skips ACC_BRIDGE methods when reflectively compiling a list of annotated methods.

It looks like things are more complicated than that, I found https://github.com/spring-projects/spring-framework/blob/master/spring-core/src/main/java/org/springframework/core/BridgeMethodResolver.java which in particular lead me to the notion of "visibility bridge method" described in http://stas-blogspot.blogspot.com/2010/03/java-bridge-methods-explained.html which is very interesting since it describes a case where javac emits a bridge from a subclass to a superclass!, and the way Spring checks for that should cover the bridges in this PR too.

So arguably, any annotation framework that doesn't handle this is broken for Java too, but since it's a fairly rare thing, to be conservative we could choose to treat mixin methods with runtime-visible annotations specially:

• If they don't need a forwarder for semantics, just don't emit one (since we would only be emitting it for cold performance reasons, which you probably don't care about if you're using an annotation-heavy Java framework)
• If they do need a forwarder for semantics, either output an error, or emit the method without the bridge flag (so the generic signature will be missing, but that's not worse than the status-quo where some of the mixin forwarders we generate also have missing generic signatures).

The community build run looks OK to me given that the "unexpected failures" match the failures in a recent commit from master.

@lrytz @retronym What about the special case for methods with runtime-visible annotations I suggested in #7843 (comment) ? Emitting less forwarders would be binary incompatible so there's not much time left to experiment with that.

I'd keep it as it is now and see if we can avoid special cases like this..

OK. I guess there's always the option of keeping these forwarders for binary-compatibility but dropping the BRIDGE flag for them.

The code in Graal is used mostly for things in our core which are all in Java and was written for the bytecode that is emitted by Eclipse and javac. We can easily change this to support the new pattern as well.

For this particular case, I think it is more time-efficient to fix this issue if someone encounters it by using Scala for core Graal things.

@retronym thanks for letting us know about the change.