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

Emit all bridge methods non-final (perhaps affecting serialization compat) #9976

Merged
merged 2 commits into from May 10, 2022

Conversation

lrytz
Copy link
Member

@lrytz lrytz commented Mar 17, 2022

Scala 3 does this too.

This change was at some point in Scala 2 as well, but then reverted (#7980, reverted in #8037).

The revert was for two PRs

It seems both changes remained in Scala 3, mixin forwarders are marked synthetic bridge, and bridges are always non-final. @smarter suggested (#8037 (review)) that we could keep all bridges non-final and just revert the first change, which is done here.

This change can affect serialization compatibility between 2.13.8 and 2.13.9 (#9976 (comment)).

Fixes scala/bug#12532

@scala-jenkins scala-jenkins added this to the 2.13.9 milestone Mar 17, 2022
@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Mar 17, 2022
@lrytz lrytz marked this pull request as ready for review March 17, 2022 20:26
@SethTisue
Copy link
Member

Travis-CI build is actually green; not sure why the status didn't update, here.

Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong feelings. A fix is a fix. Happy to land this?

@SethTisue
Copy link
Member

I see I marked this as "release notes", but I'm not sure what I was thinking at the time. So I'm removing the label again, but @lrytz let me know if you think it's actually note-able.

@SethTisue SethTisue removed the release-notes worth highlighting in next release notes label Apr 20, 2022
@lrytz
Copy link
Member Author

lrytz commented Apr 20, 2022

The risk is changes to synthetic serialVersionUIDs

@SethTisue
Copy link
Member

SethTisue commented Apr 20, 2022

I'm curious — a little, anyway — why the final or non-final status of a bridge method would affect serialization?

Sounds like the PR description is going to need some user-facing wording on that?

@lrytz
Copy link
Member Author

lrytz commented Apr 21, 2022

The computed SerialVersionUID is highly sensitive to changes that are actually safe in terms of serialization compatibility, such as changing the flags of a method.

Welcome to Scala 2.13.8 (OpenJDK 64-Bit Server VM, Java 17.0.1).
Type in expressions for evaluation. Or try :help.

scala> :pa -raw
// Entering paste mode (ctrl-D to finish)

class C extends Serializable { def f = 1; val x = 1 }

// Exiting paste mode, now interpreting.


scala> def serialize(obj: AnyRef): Array[Byte] = {
     |   import java.io._
     |   val buffer = new ByteArrayOutputStream
     |   val out = new ObjectOutputStream(buffer)
     |   out.writeObject(obj)
     |   buffer.toByteArray
     | }
def serialize(obj: AnyRef): Array[Byte]

scala> val a = serialize(new C())
val a: Array[Byte] = Array(-84, -19, 0, 5, 115, 114, 0, 1, 67, -76, -97, -7, -88, -63, -61, 55, 29, 2, 0, 1, 73, 0, 1, 120, 120, 112, 0, 0, 0, 1)

scala> def deserialize(a: Array[Byte]): AnyRef = {
     |   import java.io._
     |   val in = new ObjectInputStream(new ByteArrayInputStream(a))
     |   in.readObject
     | }
def deserialize(a: Array[Byte]): AnyRef

scala> deserialize(a)
val res0: AnyRef = C@56ed2baf

Make f final changes the SVUID:

Welcome to Scala 2.13.8 (OpenJDK 64-Bit Server VM, Java 17.0.1).
Type in expressions for evaluation. Or try :help.

scala> :pa -raw
// Entering paste mode (ctrl-D to finish)

class C extends Serializable { final def f = 1; val x = 1 }

// Exiting paste mode, now interpreting.


scala> def deserialize(a: Array[Byte]): AnyRef = {
     |   import java.io._
     |   val in = new ObjectInputStream(new ByteArrayInputStream(a))
     |   in.readObject
     | }
def deserialize(a: Array[Byte]): AnyRef

scala> deserialize(Array(-84, -19, 0, 5, 115, 114, 0, 1, 67, -76, -97, -7, -88, -63, -61, 55, 29, 2, 0, 1, 73, 0, 1, 120, 120, 112, 0, 0, 0, 1))
java.io.InvalidClassException: C; local class incompatible: stream classdesc serialVersionUID = -5431348122384910563, local class serialVersionUID = -4765401118249298305
  at java.base/java.io.ObjectStreamClass.initNonProxy(ObjectStreamClass.java:728)
  at java.base/java.io.ObjectInputStream.readNonProxyDesc(ObjectInputStream.java:2060)
  at java.base/java.io.ObjectInputStream.readClassDesc(ObjectInputStream.java:1907)
  at java.base/java.io.ObjectInputStream.readOrdinaryObject(ObjectInputStream.java:2209)
  at java.base/java.io.ObjectInputStream.readObject0(ObjectInputStream.java:1742)
  at java.base/java.io.ObjectInputStream.readObject(ObjectInputStream.java:514)
  at java.base/java.io.ObjectInputStream.readObject(ObjectInputStream.java:472)
  at deserialize(<console>:4)
  ... 32 elided

Deserialization works when the SVUID matches:

Welcome to Scala 2.13.8 (OpenJDK 64-Bit Server VM, Java 17.0.1).
Type in expressions for evaluation. Or try :help.

scala> :pa -raw
// Entering paste mode (ctrl-D to finish)

@SerialVersionUID(-5431348122384910563L) class C extends Serializable { final def f = 1; val x = 1 }

// Exiting paste mode, now interpreting.


scala> def deserialize(a: Array[Byte]): AnyRef = {
     |   import java.io._
     |   val in = new ObjectInputStream(new ByteArrayInputStream(a))
     |   in.readObject
     | }
def deserialize(a: Array[Byte]): AnyRef

scala> deserialize(Array(-84, -19, 0, 5, 115, 114, 0, 1, 67, -76, -97, -7, -88, -63, -61, 55, 29, 2, 0, 1, 73, 0, 1, 120, 120, 112, 0, 0, 0, 1))
val res0: AnyRef = C@44864ebe

scala> res0.asInstanceOf[C].x
val res2: Int = 1

@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Apr 21, 2022
@SethTisue
Copy link
Member

@lrytz can you either improve the PR description, or just hit "merge" if you feel that's unnecessary?

@SethTisue SethTisue added the prio:hi high priority (used only by core team, only near release time) label May 10, 2022
@lrytz
Copy link
Member Author

lrytz commented May 10, 2022

Done 👍

@lrytz lrytz merged commit 246e96e into scala:2.13.x May 10, 2022
@SethTisue SethTisue removed the prio:hi high priority (used only by core team, only near release time) label May 10, 2022
@SethTisue SethTisue changed the title Emit all bridge methods non-final Emit all bridge methods non-final (perhaps affecting serialization compat) Sep 1, 2022
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