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.

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

synthesize mirrors for small generic tuples #15250

Merged
merged 2 commits into from
May 31, 2022

Conversation

bishabosha
Copy link
Member

@bishabosha bishabosha commented May 20, 2022

finally, summon[Mirror.Of[Int *: String *: EmptyTuple]],

has the nice benefit of also being possible to summon a mirror that is derived from the MirroredElemTypes of another mirror

fixes #14127
fixes #7049

@bishabosha
Copy link
Member Author

bishabosha commented May 20, 2022

to fix MiMa we probably need to exclude the scala.Tuple and scala.*: companion objects from themselves becoming mirrors

Edit: I don't know why the erasure of Tuple.apply would change due to this PR?

Perhaps it depends on either tupleArity or tupleElementTypes which I modified

@bishabosha
Copy link
Member Author

I think it'd probably be cleaner to make a variant of classSymbol that reduces to some union of classsymbol/pseudoGenericTupleClass that is ran first, and then in acceptable check that each part reduces to the same pseudoclass

@bishabosha bishabosha force-pushed the mirrors-generic-tuple branch 4 times, most recently from 3cc35fc to 0b90f35 Compare May 23, 2022 13:39
@bishabosha bishabosha added this to the 3.2.0-RC1 milestone May 23, 2022
@bishabosha bishabosha requested review from odersky and removed request for dwijnand May 23, 2022 13:42
@bishabosha bishabosha requested a review from dwijnand May 23, 2022 13:42
@bishabosha
Copy link
Member Author

@odersky I ask here for advice because the issue here is that typically we would extract a class symbol from a type, and judge if that class symbol is a generic product, but we can't make a class symbol for a generic tuple type.

an alternative would be to mirror the classSymbol operation but only extract to a Option of a generic tuple, (rather than the fusion here)

@bishabosha bishabosha force-pushed the mirrors-generic-tuple branch from 0b90f35 to 7299938 Compare May 23, 2022 15:09
@bishabosha
Copy link
Member Author

I have simplified the way to extract the tuple proxy, with a trade off that it traverses the type twice if necessary

@bishabosha

This comment was marked as outdated.

@bishabosha bishabosha force-pushed the mirrors-generic-tuple branch from 7299938 to b4a97d6 Compare May 24, 2022 12:05
@bishabosha bishabosha assigned dwijnand and unassigned odersky May 24, 2022
@bishabosha bishabosha removed the request for review from odersky May 24, 2022 13:59
@bishabosha bishabosha force-pushed the mirrors-generic-tuple branch 5 times, most recently from 5677126 to 888ec2b Compare May 30, 2022 15:00
@pweisenburger
Copy link
Contributor

Edit: I don't know why the erasure of Tuple.apply would change due to this PR?
Perhaps it depends on either tupleArity or tupleElementTypes which I modified

It probably does. In PR #14586, I found the following:

def test0: Int *: EmptyTuple.type = ???  // erases to `Tuple1`
def test1: Int *: EmptyTuple = ???       // erases to `Product`

It doesn't seem like these types should be erased differently but they are. That PR also fixes treating EmptyTuple and EmptyTuple.type differently in general (except for erasure for binary compatibility).

@bishabosha
Copy link
Member Author

bishabosha commented May 30, 2022

It doesn't seem like these types should be erased differently but they are. That PR also fixes treating EmptyTuple and EmptyTuple.type differently in general (except for erasure for binary compatibility).

Thanks for noticing these cases - I had a look after I made the comment and traced it earlier to the implementation of tupleArity which does not treat the widened EmptyTuple$ as EmptyTuple.type, which this PR addresses to make them be treated the same

@bishabosha bishabosha force-pushed the mirrors-generic-tuple branch 3 times, most recently from ac538f2 to 93e06be Compare May 30, 2022 22:42
@bishabosha bishabosha added the release-notes Should be mentioned in the release notes label May 30, 2022
@bishabosha bishabosha force-pushed the mirrors-generic-tuple branch 2 times, most recently from 0094836 to f82f8cb Compare May 31, 2022 08:56
- handles generic tuples of different arity
@bishabosha bishabosha force-pushed the mirrors-generic-tuple branch from f82f8cb to 352cc6f Compare May 31, 2022 08:59
@bishabosha
Copy link
Member Author

Last changes pushed were to treat only *: as a generic tuple, before I captured arguments of TupleN types which had some problems with type parameters.

@dwijnand dwijnand assigned bishabosha and unassigned dwijnand May 31, 2022
@bishabosha bishabosha enabled auto-merge May 31, 2022 10:57
@bishabosha bishabosha merged commit e583685 into scala:main May 31, 2022
@bishabosha bishabosha deleted the mirrors-generic-tuple branch May 31, 2022 12:56
@Kordyjan Kordyjan modified the milestones: 3.2.0-RC1, 3.2.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:typeclass-derivation release-notes Should be mentioned in the release notes
Projects
None yet
5 participants