-
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
Let mirrors support default parameters #17979
base: main
Are you sure you want to change the base?
Let mirrors support default parameters #17979
Conversation
efdf0b6
to
01eda60
Compare
One thing that we must get right is that Mirrors should still be able to be summoned correctly for case classes that were compiled by 3.0.0 - i.e. in that case we probably still want to reuse the original companion object. What should we do in this case? we certainly don't want to be forced to generate anonymous mirror instances to protect against using an old library. Maybe for old code we generate an anonymous class that wraps the companion, and keep current behaviour for new code? I think introducing a new class to house the default mirror functionality would be bad as it would require macro libraries such as upickle to break compatibility to adopt the new API, - but perhaps still they could deprecate and add a new method (for a minor release)? Another problem - generating a mirror for a case class compiled by scala 3.0.0 that has a private constructor and default parameters - previously we would have used the companion object as the mirror, and now we would not be able to generate a mirror as we cant access the default parameters from the old code - but we still need to be able to summon a mirror for backwards compatibility. |
had an offline discussion, we could create a warning whenever we would not be able to match the new API (by reading an old artefact) |
Thanks Jamie for the follow up. Should we have the warning anytime the new method is used on an old mirror, or only for those where we can not generate an implementation with a proxy ? I.e. should we support the new API for old mirrors with public constructors ? If we use the same warning for the new abstract type, are we able to report the warning before we attempt match type reduction (I'm thinking of the experimental annotation of which we were losing the warning when the type was fully erased) ? Also just checking I understood correctly, the cases where we emit the warning would keep the inherited implementation of |
3857210
to
b0e542c
Compare
b0e542c
to
f163bdd
Compare
*/ | ||
def mirrorSupportsDefaultArguments(using Context): Boolean = self.isClass && { | ||
val d = self.asClass.classDenot | ||
TastyFormat.isVersionCompatible(28, 4, 1, d.tastyMajorVersion, d.tastyMinorVersion, d.tastyExperimentalVersion) |
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.
the thing that's complex is that eventually this will need to be a comparison to 28, 4, 0
, aka a stable version, but then we can't test that until TastyFormat changes (because experimental tasty versions are only equal to themselves)
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.
potentially you could relax the comparison in this case to only check the major/minor
256b000
to
3ebf926
Compare
This needs another rebase |
At this point, it might also be a good idea to squash the commits. |
9853f76
to
15c5e58
Compare
To give an idea of the amount of additional generated code the changes introduce: A final module class WithDefault() extends AnyRef(),
scala.deriving.Mirror.Product { this: Test.WithDefault.type =>
...
@experimental override def defaultArgument(x$0: Int): Any =
x$0 match
{
case 1 => WithDefault.$lessinit$greater$default$2:Any
case _ => throw new NoSuchElementException(x$0.toString())
}
} If the mirror needs to be anonymous, then each use-site will generate a definition, analogously to the other product mirror members in that case. |
Note: a SIP has to be documented and raised to the SIP committee for this to go forward. |
Access to default parameters can be provided for type class derivation with two experimental additions to
Mirror.Product
:type MirroredElemHasDefaults
indicating which case class fields have default values. Its refinement is added during the typer in the Synthesizer.def defaultArgument(index: Int): Any
which provides the default value for the given index if there is one. The method body is added in SyntheticMembers, it dispatches to the default getter methods which already exist in the class companion. Note that said accessors may be private to the class and hence that the new interface in a way extends their scope. Mirrors are generated for all case classes, so to minimise the generated code since, the base implementation ofdefaultArgument
inMirror.Product
which always throws an exception is only overridden in synthesised mirrors of classes with at least one default parameter.Refs #17893
Review by @bishabosha