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

Bridge methods prevent forward-compatible library evolution #11804

Open
szeiger opened this issue Nov 16, 2019 · 11 comments
Open

Bridge methods prevent forward-compatible library evolution #11804

szeiger opened this issue Nov 16, 2019 · 11 comments

Comments

@szeiger
Copy link

@szeiger szeiger commented Nov 16, 2019

While we are going to abandon forward binary compatibility in the long run, we still need to preserve it for the 3.0 transition (probably until we switch to a 3.x library in 3.1 and tooling has been adapted to the new compatibility model).

Currently bridge methods are a big problem for forward compatibility. Example:

trait A[T] {
  protected[this] def x: T
  def foo: T = x
}

class B[T <: String](protected[this] val x: T) extends A[T] {
  //override def foo: T = super.foo
}

Enabling the override in B creates two separate methods:

  public T foo();
    descriptor: ()Ljava/lang/String;
    flags: ACC_PUBLIC

  public java.lang.Object foo();
    descriptor: ()Ljava/lang/Object;
    flags: ACC_PUBLIC, ACC_BRIDGE, ACC_SYNTHETIC

Any caller that sees an instance of B as B will call the "real" method with the refined signature (due to the tighter upper bound of T compared to the original version), callers that only see an A call the bridge method (which overrides the inherited signature) instead. This is not forward compatible because the "real" method did not exist in the old version.

There is no good reason for the bridge method to exist in such a case, except to potentially avoid some casts due to the tighter bounds. Note that without the explicit override the class still overrides foo in order to delegate to the interface's default implementation but this method uses the original bounds so there is no bridge method.

This case frequently comes up in the collections library where the C and CC[_] type parameters are refined through the hierarchy. There is no workaround via casts or @unsafSomething annotations so we have to resort to hacking the override into the original implementation in some superclass using isInstanceOf. Depending on access restrictions of other parts of the class this is not always possible. Where it is possible the extra type checks make all uses slower (even when not related to the class that should be changed) and create more work down the line to switch to the proper design the next time we can break forward compatibility.

We should add an annotation @nobridge than can be added to an overridden method to prevent the generation of a bridge method. There are cases where a bridge method is really needed (when access restrictions get relaxed) but in most cases it could be suppressed. The only work required when breaking forward compatibility would be the removal of these annotations.

@sjrd

This comment has been minimized.

Copy link
Member

@sjrd sjrd commented Nov 16, 2019

Just to ease understanding: I think you are swapping which method is a bridge and which one is the real method. The real method is the one with the refined type. The bridge is the one with the exact same signature as in the parent class, and which at run-time delegates to the real method. It is you are arguing for removing the real method, not the bridge. Not generating the bridge and keeping only the real method would not have the correct semantics at all.

@szeiger

This comment has been minimized.

Copy link
Author

@szeiger szeiger commented Nov 18, 2019

Yes, the real method needs to get the inherited signature, therefore no bridge method will be generated. After reading your comment I see how the terminology is confusing. @nobridge basically means "don't create a situation where you need a bridge method".

@dwijnand

This comment has been minimized.

Copy link
Member

@dwijnand dwijnand commented Nov 18, 2019

Yeah, the signature of the bridge method with the body of the real method. The annotation suppresses the covariant return type in the method signature, thus alleviating the need for a bridging method.

Having this fixes the issue like in the example, where the return type is really being refined elsewhere via a type parameter. But if the return type is being refined in a more independent way (e.g. a method in Seq returns Seq, without using C or CC[_], and Vector wants to override it and return Vector) then this annotation would mean you only get the original type at compile time (Seq).

@szeiger

This comment has been minimized.

Copy link
Author

@szeiger szeiger commented Nov 18, 2019

No change is visible from Scala, only erased types are affected. The original motivation (in Java) appears to be keeping the raw types better aligned with the generic types. This was a fair point at the time (covariant return types were introduced in 5.0 together with generics; interoperability with pre-5.0 Java was a big issue) but doesn't look very useful anymore in a 2019 Scala context.

@lrytz

This comment has been minimized.

Copy link
Member

@lrytz lrytz commented Nov 18, 2019

Sounds good! Do you have a list of PRs that we could merge / re-open with this fixed?

@dwijnand

This comment has been minimized.

Copy link
Member

@dwijnand dwijnand commented Nov 18, 2019

@szeiger The erased type being affected matters, but I realise my example would behave differently: @nobridge should compile error the method. But your C/CC[_] case is fine.

@szeiger

This comment has been minimized.

Copy link
Author

@szeiger szeiger commented Nov 18, 2019

@dwijnand It doesn't matter if you refine the return type explicitly or through tighter type bounds. In both cases the compiler knows the overriding pair and generates an erased signature and potentially a bridge method accordingly. This is the part that needs to be changed (i.e. erase to the overridden method's erasure instead).

@szeiger

This comment has been minimized.

Copy link
Author

@szeiger szeiger commented Nov 18, 2019

@lrytz I can't recall any PR that we couldn't do at all. There is a comment in mapValuesInPlaceImpl in mutable.HashMap that demands cleaning up the binary compatibility hack (and this example also shows how to work around access restrictions). Another override in immutable.TreeSeqMap is commented out because the cost of putting the workaround into the base class would have outweighed the benefit.

I remember several other cases where we used these hacks but they were in 2.12. 2.13 is still too young to have accumulated a large armount of them.

@dwijnand

This comment has been minimized.

Copy link
Member

@dwijnand dwijnand commented Nov 18, 2019

@szeiger Here's the test case I'm thinking:

class Foo {
  def create: Foo = new Foo
}
class Bar extends Foo {
  @nobridge override def create: Bar = new Bar
}

If we make Bar's create have a Foo in the method descriptor (so no bridge method needed) but Bar in Signature and ScalaSignature (to keep all the info) then I think val bar: Bar = new Bar().create wouldn't work without a cast, which I think is confusing given the source code. So better if @nobridge just rejects it.

@szeiger

This comment has been minimized.

Copy link
Author

@szeiger szeiger commented Nov 18, 2019

The erased signatures don't matter. The compiler will automatically insert the cast. It does that all the time when you use type members and type variables. If the return type was an abstract type that you refined in Bar without overriding the method you would get exactly the same situation.

@odd

This comment has been minimized.

Copy link

@odd odd commented Nov 27, 2019

Another override in immutable.TreeSeqMap is commented out because the cost of putting the workaround into the base class would have outweighed the benefit.

@szeiger The special handling is in MapFactoryDefaults at line 999.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.