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

Let mirrors support default parameters #17979

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

EugeneFlesselle
Copy link
Contributor

@EugeneFlesselle EugeneFlesselle commented Jun 15, 2023

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 of defaultArgument in Mirror.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

@EugeneFlesselle EugeneFlesselle marked this pull request as draft June 15, 2023 11:55
@EugeneFlesselle EugeneFlesselle marked this pull request as ready for review June 16, 2023 06:36
@mbovel mbovel requested a review from bishabosha June 19, 2023 11:06
@bishabosha
Copy link
Member

bishabosha commented Jun 19, 2023

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.

@bishabosha bishabosha added the needs-minor-release This PR cannot be merged until the next minor release label Jun 19, 2023
@bishabosha
Copy link
Member

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)

@EugeneFlesselle
Copy link
Contributor Author

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 defaultArgument right ?

*/
def mirrorSupportsDefaultArguments(using Context): Boolean = self.isClass && {
val d = self.asClass.classDenot
TastyFormat.isVersionCompatible(28, 4, 1, d.tastyMajorVersion, d.tastyMinorVersion, d.tastyExperimentalVersion)
Copy link
Member

@bishabosha bishabosha Oct 19, 2023

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)

Copy link
Member

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

@EugeneFlesselle EugeneFlesselle marked this pull request as ready for review November 29, 2023 21:57
@bishabosha
Copy link
Member

This needs another rebase

@nicolasstucki
Copy link
Contributor

At this point, it might also be a good idea to squash the commits.

@EugeneFlesselle
Copy link
Contributor Author

EugeneFlesselle commented Feb 1, 2024

To give an idea of the amount of additional generated code the changes introduce:

A case class WithDefault(x: Int, y: Int = 1) results in the following method being added to its companion object:

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.

@Gedochao
Copy link
Contributor

Gedochao commented Jul 3, 2024

Note: a SIP has to be documented and raised to the SIP committee for this to go forward.

@Gedochao Gedochao added the needs-sip A SIP needs to be raised to move this issue/PR along. label Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-minor-release This PR cannot be merged until the next minor release needs-sip A SIP needs to be raised to move this issue/PR along.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants