Emit all classes as public to avoid object deserialization issues #14686
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
For reference, all classes in Scala 2 are public in the backend because of this line: https://github.com/scala/scala/blob/ce95d6ca26179e5c189c2b1b40e68d4bc8738cfe/src/compiler/scala/tools/nsc/transform/ExplicitOuter.scala#L391 |
smarter
force-pushed
the
emit-class-public
branch
from
13434c0
to
2f717a4
Compare
|
Technically this isn't a forward-compatible ABI change, but since these classes aren't really part of our public API I think it's fine to ignore that: Error: scala3-library-bootstrapped: Failed binary compatibility check against org.scala-lang:scala3-library_3:3.1.1! Found 3 potential problems (filtered 697)
Error: * private class scala.quoted.FromExpr#PrimitiveFromExpr is inaccessible in other version, it must be public. |
smarter
force-pushed
the
emit-class-public
branch
from
2f717a4
to
fee60fb
Compare
smarter
added a commit
to dotty-staging/scalatest
that referenced
this pull request
Mar 14, 2022
After scala/scala3#14686, Scala 3 emits all classes (even local classes) as public, just like Scala 2. But unlike Scala 2, the local class TestSpec in XmlSocketReporterSpec will be emitted with a zero-argument constructor (in Scala 2 its constructor takes an XmlSocketReporterSpec outer parameter which is never used which seems like a bug). This matters because Scalatest considers a test suite "rerunnable" if it has a zero argument public constructor. So to get the test suite to pass, we need to check for the presence of this constructor just like SuiteRerunner does.
smarter
force-pushed
the
emit-class-public
branch
from
fee60fb
to
86b239a
Compare
sjrd
approved these changes
Mar 21, 2022
Comment on lines
299
to
302
| // Classes are always emitted as public. This matches the behavior of Scala 2 | ||
| // and is necessary for object deserialization to work properly, otherwise | ||
| // ModuleSerializationProxy may fail with an accessiblity error (see | ||
| // tests/run/serialize.scala). |
There was a problem hiding this comment.
Perhaps add a reference to the issue as well?
Yes, this is fine. It will also apply in libraries that have a forward compatibility policy, but in those libraries there should also only be a difference for Scala-private classes, which would be ignorable there as well. |
smarter
force-pushed
the
emit-class-public
branch
from
86b239a
to
05dfcff
Compare
smarter
added a commit
to smarter/scalatest
that referenced
this pull request
Mar 21, 2022
After scala/scala3#14686, Scala 3 emits all classes (even local classes) as public, just like Scala 2. But unlike Scala 2, the local class TestSpec in XmlSocketReporterSpec will be emitted with a zero-argument constructor (in Scala 2 its constructor takes an XmlSocketReporterSpec outer parameter which is never used which seems like a bug). This matters because Scalatest considers a test suite "rerunnable" if it has a zero argument public constructor. So to get the test suite to pass, we need to check for the presence of this constructor just like SuiteRerunner does.
smarter
added a commit
to dotty-staging/scalatest
that referenced
this pull request
Mar 21, 2022
After scala/scala3#14686, Scala 3 emits all classes (even local classes) as public, just like Scala 2. But unlike Scala 2, the local class TestSpec in XmlSocketReporterSpec will be emitted with a zero-argument constructor (in Scala 2 its constructor takes an XmlSocketReporterSpec outer parameter which is never used which seems like a bug). This matters because Scalatest considers a test suite "rerunnable" if it has a zero argument public constructor. So to get the test suite to pass, we need to check for the presence of this constructor just like SuiteRerunner does.
smarter
force-pushed
the
emit-class-public
branch
from
05dfcff
to
30961f5
Compare
This aligns our behavior with Scala 2 and fixes the issue encountered in typelevel/cats-effect#2360 (comment). Alternatively, we could change ModuleSerializationProxy upstream to call `setAccessible(true)` on the MODULE$ field, but this wouldn't work if the object in question is inside a Java 9+ module.
smarter
force-pushed
the
emit-class-public
branch
from
30961f5
to
5e6a3cf
Compare
nicolasstucki
pushed a commit
to dotty-staging/scalatest
that referenced
this pull request
Mar 24, 2022
After scala/scala3#14686, Scala 3 emits all classes (even local classes) as public, just like Scala 2. But unlike Scala 2, the local class TestSpec in XmlSocketReporterSpec will be emitted with a zero-argument constructor (in Scala 2 its constructor takes an XmlSocketReporterSpec outer parameter which is never used which seems like a bug). This matters because Scalatest considers a test suite "rerunnable" if it has a zero argument public constructor. So to get the test suite to pass, we need to check for the presence of this constructor just like SuiteRerunner does.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This aligns our behavior with Scala 2 and fixes the issue encountered in
typelevel/cats-effect#2360 (comment).
Alternatively, we could change ModuleSerializationProxy upstream to call
setAccessible(true)on the MODULE$ field, but this wouldn't work if the objectin question is inside a Java 9+ module.