Pretend that BoxedUnit does not extend java.io.Serializable. #9454
Conversation
There was a problem hiding this comment.
Hello, and thank you for opening this PR! 🎉
All contributors have signed the CLA, thank you! ❤️
Commit Messages
We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).
Please stick to these guidelines for commit messages:
- Separate subject from body with a blank line
- When fixing an issue, start your commit message with
Fix #<ISSUE-NBR>:- Limit the subject line to 72 characters
- Capitalize the subject line
- Do not end the subject line with a period
- Use the imperative mood in the subject line ("Add" instead of "Added")
- Wrap the body at 80 characters
- Use the body to explain what and why vs. how
adapted from https://chris.beams.io/posts/git-commit
Have an awesome day! ☀️
| val normalizedParents = | ||
| if (fromScala2) defn.adjustForTuple(cls, tparams, parents1) | ||
| else parents1 // We are setting the info of a Java class, so it cannot be one of the tuple classes | ||
| else defn.adjustForBoxedUnit(cls, parents1) |
There was a problem hiding this comment.
This adjustment also needs to be done in the Namer (like adjustForTuple) since we typecheck BoxedUnit.java when compiling the standard library
There was a problem hiding this comment.
Is this really how it works in Dotty? Because in scala/scala, BoxedUnit.java is compiled by Java first, and then its .class file is used when compiling the .scala sources of the stdlib. I don't think BoxedUnit.java is ever processed by scalac. Is the case with dotc?
There was a problem hiding this comment.
It's going to depend on how the project is built. I see the build.sbt of scala/scala sets compileOrder := CompileOrder.JavaThenScala but we don't do that in the dotty repo nor in our tests, so I wouldn't rely on it.
sjrd
force-pushed
the
scalajs-fix-unit-not-serializable-ir-error
branch
from
8675927 to
829368e
Compare
Unit does not extend Serializable, but scala.runtime.BoxedUnit does. Because of that, erasing `SomeSerializable | Unit` yields `java.io.Serializable` instead of `Object`. This is problematic for Scala.js, because the run-time representation of `()` in Scala.js is *not* `BoxedUnit` (which is a JVM artifact) but the JavaScript value `undefined`, the only instance of `java.lang.Void`. The latter however does not extend `java.io.Serializable`, so trying to assign `()` to a `SomeSerializable | Unit` will result in invalid IR that fails to pass the IR checker. We solve this issue by patching the parents of `BoxedUnit` during unpickling to remove `java.io.Serializable`. This is done in the same way as adding parents to Tuple classes.
sjrd
force-pushed
the
scalajs-fix-unit-not-serializable-ir-error
branch
from
829368e to
1bd7d2e
Compare
Unit does not extend Serializable, but scala.runtime.BoxedUnit does. Because of that, erasing
SomeSerializable | Unityieldsjava.io.Serializableinstead ofObject.This is problematic for Scala.js, because the run-time representation of
()in Scala.js is notBoxedUnit(which is a JVM artifact) but the JavaScript valueundefined, the only instance ofjava.lang.Void. The latter however does not extendjava.io.Serializable, so trying to assign()to aSomeSerializable | Unitwill result in invalid IR that fails to pass the IR checker.We solve this issue by patching the parents of
BoxedUnitduring unpickling to removejava.io.Serializable. This is done in the same way as adding parents to Tuple classes.