Skip to content

SI-9080 Replace Cloneable/Serializable traits with type aliases #5288

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

Closed
wants to merge 1 commit into from

Conversation

soc
Copy link
Contributor

@soc soc commented Jul 17, 2016

No description provided.

@soc
Copy link
Contributor Author

soc commented Jul 17, 2016

Looks like we need to bootstrap the compiler between changing Definitions and removing the files...

@soc
Copy link
Contributor Author

soc commented Jul 18, 2016

@adriaanm adriaanm modified the milestones: 2.12.1, 2.12.0-RC1 Jul 26, 2016
@adriaanm
Copy link
Contributor

@soc, we like this, but RC1 is very close. Can you propose a plan to get this green by the end of next week without releasing another STARR?

@soc
Copy link
Contributor Author

soc commented Jul 30, 2016

I don't see one. Definitions assumes real, existing Scala classes. This has to be fixed before the Scala classes can be removed in favor of aliases. Another thing that I would like to see is dropping the package when printing the type names.

@retronym
Copy link
Member

I don't see one. Definitions assumes real, existing Scala classes

@soc What steps could I take to reproduce the problem?

@soc
Copy link
Contributor Author

soc commented Jul 30, 2016

@retronym Replace the classes with aliases in the same step, without building a new STARR. Compiler will crash.

@retronym
Copy link
Member

I see, the STARR compiler fails with:

[debug]     /Users/jason/code/scala/build/quick/classes/library
scala.reflect.internal.MissingRequirementError: class scala.Serializable in compiler mirror not found.
    at scala.reflect.internal.MissingRequirementError$.signal(MissingRequirementError.scala:17)
    at scala.reflect.internal.MissingRequirementError$.notFound(MissingRequirementError.scala:18)
    at scala.reflect.internal.Mirrors$RootsBase.$anonfun$getModuleOrClass$4(Mirrors.scala:54)
    at scala.reflect.internal.Mirrors$RootsBase.getModuleOrClass(Mirrors.scala:54)
    at scala.reflect.internal.Mirrors$RootsBase.getModuleOrClass(Mirrors.scala:66)
    at scala.reflect.internal.Mirrors$RootsBase.getClassByName(Mirrors.scala:102)
    at scala.reflect.internal.Mirrors$RootsBase.getRequiredClass(Mirrors.scala:105)
    at scala.reflect.internal.Mirrors$RootsBase.requiredClass(Mirrors.scala:108)
    at scala.reflect.internal.Definitions$DefinitionsClass.SerializableClass$lzycompute(Definitions.scala:371)
    at scala.reflect.internal.Definitions$DefinitionsClass.SerializableClass(Definitions.scala:371)

@soc
Copy link
Contributor Author

soc commented Jul 31, 2016

@retronym This one should compile, but as mentioned, I think the "package" in scala.package.Foo is really ugly, and we should not print it in general (not only in the case of Clonable/Serializable).
Didn't have the time to find the right place to eliminate it, though.

@smarter
Copy link
Member

smarter commented Aug 1, 2016

You probably need to get rid of scala.Serializable in https://github.com/scala/scala/blob/2.12.x/src/library/scala/runtime/LambdaDeserializer.scala too

@soc
Copy link
Contributor Author

soc commented Aug 1, 2016

I fixed the issue in scalap/caseClass, although I don't even want to know why it's scala.Serializable once and java.io.Serializable a few lines down:

case class CaseClass[A <: scala.Seq[scala.Int]](i: A, s: scala.Predef.String) extends scala.AnyRef with scala.Product with scala.Serializable {
...
object CaseClass extends scala.AnyRef with java.io.Serializable {
...

@soc
Copy link
Contributor Author

soc commented Aug 1, 2016

@retronym What do you think of the need for SerialVersionUIDs for the serialization tests?

I added a few of them to make things compile, but I think either all or none of the collection classes should get the annotations.
I think we don't guarantee serialization compatibility between major versions, but in this case I don't think that any breakage is possible (which would be in favor of adding annotations, and not changing the test values in the tests ...)

Opinions?

Edit: Forgot the test in question, it is https://github.com/scala/scala/blob/2.12.x/test/files/run/t8549.scala#L117
Many of the collection tests changed, and it shows very nicely that we don't have much test coverage for all those specialized 0/1/2/3/4-item collections.

// We could actually omit this marker interface as LambdaMetaFactory will add it if
// the FLAG_SERIALIZABLE is set.
// But the code is cleaner if we uniformly add a single marker, so I'm leaving it in place.
private val Serializable = "java.io.Serializable"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smarter Thanks! I simplified things, could you have a look whether I should go further and get rid of this completely? As I understand, there now no benefit anymore of doing it, as the only marker we add is already added automatically.

Copy link
Member

Choose a reason for hiding this comment

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

@retronym can probably answer this better than me

@soc
Copy link
Contributor Author

soc commented Aug 2, 2016

Can we disable scaladoc generation for this one?
My nerves won't survive adding another work-around just for one commit.

[error] /home/jenkins/workspace/scala-2.12.x-validate-test/src/library/scala/Cloneable.scala:14: Cloneable is already defined as type Cloneable in package object scala
[error] trait Cloneable extends java.lang.Cloneable
[error]       ^
[error] /home/jenkins/workspace/scala-2.12.x-validate-test/src/library/scala/Serializable.scala:14: Serializable is already defined as type Serializable in package object scala
[error] trait Serializable extends Any with java.io.Serializable
[error]       ^
[info] No documentation generated with unsuccessful compiler run
[error] two errors found

@adriaanm
Copy link
Contributor

adriaanm commented Aug 9, 2016

Let's continue work on this, but I think we should do this under a bigger umbrella of library cleanups in 2.13. 2.12.0-RC1 is too close.

@adriaanm adriaanm modified the milestones: WIP, 2.12.0-RC1 Aug 9, 2016
@soc
Copy link
Contributor Author

soc commented Aug 9, 2016

Well, it's just waiting for a merge. After that, we need a new bootstrap, and then the final change, removing the traits, can be merged, fixing the scaladoc error.

@soc
Copy link
Contributor Author

soc commented Aug 20, 2016

Any news on this?

@adriaanm
Copy link
Contributor

No, this will have to go into 2.13. I don't want the RC cycle to slip further.

@soc soc closed this Aug 22, 2016
@SethTisue SethTisue removed this from the WIP milestone Mar 6, 2018
smarter added a commit to smarter/scala that referenced this pull request Jun 5, 2018
Fixes scala/bug#9080.

This commit is adapted from scala#5288 and is thus:

Co-authored-by: Simon Ochsenreither <[email protected]>
smarter added a commit to smarter/scala that referenced this pull request Jun 6, 2018
Fixes scala/bug#9080.

This commit is adapted from scala#5288 and is thus:

Co-authored-by: Simon Ochsenreither <[email protected]>
smarter added a commit to smarter/scala that referenced this pull request Jun 6, 2018
Fixes scala/bug#9080.

This commit is adapted from scala#5288 and is thus:

Co-authored-by: Simon Ochsenreither <[email protected]>
smarter added a commit to smarter/scala that referenced this pull request Jun 6, 2018
Fixes scala/bug#9080.

This commit is adapted from scala#5288 and is thus:

Co-authored-by: Simon Ochsenreither <[email protected]>
smarter added a commit to smarter/scala that referenced this pull request Jun 6, 2018
Fixes scala/bug#9080.

This commit is adapted from scala#5288 and is thus:

Co-authored-by: Simon Ochsenreither <[email protected]>
smarter added a commit to smarter/scala that referenced this pull request Aug 29, 2018
Fixes scala/bug#9080.

This commit is adapted from scala#5288 and is thus:

Co-authored-by: Simon Ochsenreither <[email protected]>
smarter added a commit to smarter/scala that referenced this pull request Aug 29, 2018
Fixes scala/bug#9080.

This commit is adapted from scala#5288 and is thus:

Co-authored-by: Simon Ochsenreither <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants