Skip to content

Revert "Inline _N case class product members under -Yscala2-stdlib" #17928

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

Merged

Conversation

nicolasstucki
Copy link
Contributor

These product members are only in the bytecode.

This reverts commit 8c58cbf.

@nicolasstucki nicolasstucki requested a review from smarter June 7, 2023 04:28
@nicolasstucki nicolasstucki marked this pull request as ready for review June 7, 2023 06:11
@nicolasstucki nicolasstucki force-pushed the revert-stdlib-product-members branch from e9eab1a to f515f52 Compare June 23, 2023 14:49
@smarter
Copy link
Member

smarter commented Jun 23, 2023

Sorry, can you explain in more details why it's ok to leave these members?

@nicolasstucki nicolasstucki force-pushed the revert-stdlib-product-members branch from f515f52 to 9547198 Compare June 30, 2023 15:48
@nicolasstucki
Copy link
Contributor Author

These members should not be added at all. The inline version only removes them from the bytecode, but not from the pickles. I still need to find the correct solution to this issue. This PR only reverts the old, not quite right attempt to fix this.

I do not have a proper fix yet because this one is a bit more complicated than the others. Ideally we would just need to remove these members. But unfortunately, many other transformations depend on the existence of these members. One approach would be to patch all of the places where we use these member accessors and use the names of the fields directly. I need to see if this is viable. Otherwise, we could try to find another way, maybe remove them in the pickler.

@smarter
Copy link
Member

smarter commented Jun 30, 2023

But unfortunately, many other transformations depend on the existence of these members.

Does that happen in other transforms than pattern matching? The idea was that case classes wouldn't be handled specially for pattern matching in Scala 3 and instead rely on name-based pattern matching, but the logic for handling things by name still exists for Scala 2 case classes.

@nicolasstucki
Copy link
Contributor Author

I just managed to make a proper fix for this in another branch. I will open another PR for that. I want to revert to this state as it uncovers a hole in TASTy MiMa that we should investigate.

It does not seem to affect pattern matching because all unapplys are adapted to return Options. They are handled as usual. We only need to patch desugaring and synthetic members.

@nicolasstucki nicolasstucki merged commit 574cff1 into scala:main Jul 3, 2023
@nicolasstucki nicolasstucki deleted the revert-stdlib-product-members branch July 3, 2023 09:58
@nicolasstucki
Copy link
Contributor Author

Followed by #18117

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.

3 participants