-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Support Mirrors for value classes #7023
Conversation
val v0 = m0.fromProduct(Tuple1(23.0)) | ||
assert(v0 == B(23.0)) | ||
|
||
case class C[T](v: T) extends AnyVal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens for value classes which don't extend product (so don't have case
) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently only case classes (value or otherwise) have product mirrors generated automatically, so that case doesn't arise. Someone could provide a mirror for a non-case value class by hand, and then they'd have to deal with the bridge collision themselves.
@smarter any thoughts on moving this forward? |
@smarter It would be good to get this resolved before the release. |
Oops, forgot about that one. Earlier I asked:
And Miles replied:
So all the mirrors generated automatically are for classes that extend Product, therefore the On another note, what happens if a value class underlying type is scala.Product, do we get a clash again ? Would be good to have a test for it either way. |
Do we still want to support this? |
I would have thought so, but I'm afraid that I don't have time to do any more work on it. |
Coming back to this PR...
I'm pretty sure the answer is yes, so this PR as-is doesn't fix the fundamental problem that we cannot always generate a fromProduct for value classes. I think the fix is to just check if we can in fact generate such a method without getting a clash at erasure: if we can, great; if we can't then just don't do it; this would exclude some value classes from having Mirrors, but fewer than today. |
We are currently trying to clean open PR's. I will close this due to the apparent need for a redesign |
This is a pretty painful limitation when attempting to migrate over Scala 2 codebases that rely heavily on value classes -- is there an ETA on when a fix/resolution will land?? |
No ETA since no one is actively working on it unfortunately. |
For anyone willing to contribute, I outlined a possible way forward in #7023 (comment) |
Wow, so every Scala 3 application that depends on converting value classes to/from underlying primitive type has to come up with reams of boilerplate to workaround what were 1-liners in Scala 2 (think REST api endpoints, database mappings, etc.) I would think that this issue would have been a major priority -- Scala 3 migration is proving to be quite a pain, despite the myriad language improvements. Hopefully issues like this are resolved in the not too distant future. |
This PR adds support for
Mirrors
for value classes, fixing #7000.As discussed on that issue, the reason for value classes being prohibited from having
Mirrors
was a subtle issue with the bridges for thefromProduct
method which are generated due to its result type being the value class type. In the case of value classes with polymorphic contents which erase toObject
, the bridge method collides with the underlying method as in #1905.@smarter suggested putting a bound of
<: Product
onMirroredMonoType
to prevent the erasures being the same. This can't be done in general (in general the type need not be<: Product
), but we can create a subtype ofMirror.Product
for value classes which can have that additional constraint.Intuitively, the following ought to have been sufficient,
however, the bridges transform still sees a collision in this case. The following does work though,
where for value classes we now implement
wrapValue
on the companion.It's not clear to me if this really is necessary, or if the simpler fix ought to have worked as I had expected. The collision reported in that case appears to be between the bridge method and the
fromProduct
method fromProduct
(ie. further up the hierarchy than the refinement which introduces the<: Product
bound). I'm a bit out of my depth here, but it seems at least possible that the bridge code is missing someasSeenFrom
somewhere and so isn't seeing the refinement which would remove the collision.