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.

Sign up for GitHub

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

Jump to bottom

Replace Cloneable/Serializable traits with type aliases #6729

Merged
merged 2 commits into from
Aug 29, 2018
Merged

Replace Cloneable/Serializable traits with type aliases #6729

merged 2 commits into from
Aug 29, 2018

Conversation

smarter
Copy link
Member

@smarter smarter commented Jun 5, 2018 •
edited
Loading

Fixes scala/bug#9080.

This commit is adapted from #5288 and is thus:

Co-authored-by: Simon Ochsenreither simon@ochsenreither.de

@scala-jenkins scala-jenkins added this to the 2.13.0-M5 milestone Jun 5, 2018
@smarter smarter force-pushed the replace/Cloneable-Serializable branch from de9ca64 to ed2ab05 Compare June 5, 2018 21:52
@smarter
Copy link
Member Author

smarter commented Jun 5, 2018

This PR doesn't actually compile currently, I get:

[debug]         /home/smarter/opt/scala/build/quick/classes/reflect:/home/smarter/opt/scala/build/quick/classes/library
scala.reflect.internal.MissingRequirementError: class scala.Serializable 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.ensureClassSymbol(Mirrors.scala:96)
        at scala.reflect.internal.Mirrors$RootsBase.getClassByName(Mirrors.scala:101)
        at scala.reflect.internal.Mirrors$RootsBase.getRequiredClass(Mirrors.scala:104)
        at scala.reflect.internal.Mirrors$RootsBase.requiredClass(Mirrors.scala:107)
        at scala.reflect.internal.Definitions$DefinitionsClass.SerializableClass$lzycompute(Definitions.scala:375)
        at scala.reflect.internal.Definitions$DefinitionsClass.SerializableClass(Definitions.scala:375)
        at scala.reflect.internal.Definitions$DefinitionsClass.isPossibleSyntheticParent$lzycompute(Definitions.scala:1378)
        at scala.reflect.internal.Definitions$DefinitionsClass.isPossibleSyntheticParent(Definitions.scala:1378)

This also happened at some point in #5288 but was apparently resolved: #5288 (comment). Assuming it actually worked then I don't see why it's failing now. Is it possible that the resolving rules have changed such that scala.package.Serializable is now preferred to scala.Serializable?

@smarter
Copy link
Member Author

smarter commented Jun 5, 2018

Opened #6730 to try to solve the bootstrapping conundrum.

smarter added a commit to smarter/scala that referenced this pull request Jun 6, 2018
@smarter
Preparations for making Cloneable/Serializable aliases
1faa53f 
In the future (scala#6729) scala.Serializable and scala.Cloneable will
be type aliases. To be able to bootstrap this change, we need a STARR
that does not crash when this change is made.
@smarter smarter force-pushed the replace/Cloneable-Serializable branch 2 times, most recently from 57fef38 to d65ecca Compare June 6, 2018 01:00
@lrytz
Copy link
Member

lrytz commented Jun 6, 2018

I think we should merge #6730 and do the change here after M5.

Why doesn't this PR remove the Serializable / Cloneable traits?

smarter added a commit to smarter/scala that referenced this pull request Jun 6, 2018
@smarter
In the future (scala#6729) scala.Serializable will be a type alias to
a41cb31 
java.io.Serializable. To be able to bootstrap this change, we need a
STARR that does not crash when this change is made.
@smarter smarter force-pushed the replace/Cloneable-Serializable branch from d65ecca to a846fa4 Compare June 6, 2018 13:39
@smarter
Copy link
Member Author

smarter commented Jun 6, 2018

Why doesn't this PR remove the Serializable / Cloneable traits?

Done. This required tweaking #6730 more.

@smarter smarter mentioned this pull request Jun 6, 2018
Merged
@smarter smarter force-pushed the replace/Cloneable-Serializable branch from a846fa4 to 8b34b6c Compare June 6, 2018 15:40
@lrytz
Copy link
Member

lrytz commented Jun 7, 2018

Looks good! Let's reopen this after releasing M5. I marked scala/bug#9080 as blocker and assigned it to RC1 to make sure we don't forget.

@lrytz lrytz closed this Jun 7, 2018
@lrytz lrytz mentioned this pull request Jun 7, 2018
Closed
@smarter smarter modified the milestones: 2.13.0-M5, 2.13.0-RC1 Jun 7, 2018
@SethTisue SethTisue removed this from the 2.13.0-RC1 milestone Aug 9, 2018
@smarter smarter reopened this Aug 29, 2018
@scala-jenkins scala-jenkins added this to the 2.13.0-RC1 milestone Aug 29, 2018
@smarter smarter force-pushed the replace/Cloneable-Serializable branch from 8b34b6c to 39d5c16 Compare August 29, 2018 04:30
@smarter
Replace Cloneable/Serializable traits with type aliases
82fd591 
Fixes scala/bug#9080.

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

Co-authored-by: Simon Ochsenreither <simon@ochsenreither.de>
@smarter smarter force-pushed the replace/Cloneable-Serializable branch from 39d5c16 to 82fd591 Compare August 29, 2018 05:45
@smarter
Copy link
Member Author

smarter commented Aug 29, 2018

Reopened and rebased now that M5 is out.

@smarter smarter requested a review from lrytz August 29, 2018 06:54
lrytz
lrytz approved these changes Aug 29, 2018
View reviewed changes
Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

👍

@lrytz lrytz merged commit faee586 into scala:2.13.x Aug 29, 2018
@SethTisue SethTisue mentioned this pull request Aug 30, 2018
Closed
46 tasks
@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Aug 31, 2018
@milessabin milessabin mentioned this pull request Sep 6, 2018
Closed
@milessabin
Copy link
Contributor

milessabin commented Sep 6, 2018 •
edited
Loading

Somewhat to my surprise, git bisect is blaming this PR for shapeless's current brokeness in the community build: milessabin/shapeless#857. Reverting 82fd591 confirms it.

I'll try and work out what's going on.

@milessabin
Copy link
Contributor

It appears that the problem is that an intersection previously inferred as Product with Serializable with Fruit is now being inferred as Product with Fruit with java.io.Serializable which is being used in an invariant context where the two are incompatible.

I can't see any obvious way of tweaking the test which will cross build before and after this PR. It's not a huge deal in this case, and I've commented out the test without any real loss, but it suggests that this change might cause bigger problems elsewhere in the community build.

@SethTisue
Copy link
Member

thanks, I'll keep an eye out for it.

@milessabin milessabin mentioned this pull request Sep 26, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lrytz lrytz lrytz approved these changes

Assignees
No one assigned
Labels
release-notes worth highlighting in next release notes
Projects
None yet
Milestone
2.13.0-RC1
5 participants
@smarter @lrytz @milessabin @SethTisue @scala-jenkins