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

sum mirror is available when mirror for child can't be summoned #15222

Closed
bishabosha opened this issue May 18, 2022 · 7 comments · Fixed by #15231
Closed

sum mirror is available when mirror for child can't be summoned #15222

bishabosha opened this issue May 18, 2022 · 7 comments · Fixed by #15231
Assignees
Labels
itype:bug regression This worked in a previous version but doesn't anymore
Milestone

Comments

@bishabosha
Copy link
Member

bishabosha commented May 18, 2022

The fix for #14986 does not consider when a sum type has a child for which the mirror can not be summoned.

Compiler version

3.2.0-RC1-bin-20220517-e5abec0-NIGHTLY

First bad commit d6e17a2 in #15008

Minimized code

import scala.deriving.Mirror

package lib {
  sealed trait Foo
  object Foo // Mirror.Of[Foo] cached in companion
  case class Bar private[lib] () extends Foo
  case object Bar // force mirror for Bar to be anonymous.
}

package app {
  val mFoo = summon[Mirror.SumOf[lib.Foo]]
  val mBar = summon[Mirror.ProductOf[lib.Bar]] // error
}

Output

-- Error: test.scala:11:49 -------------------------------------------------------------
11 |  val mBar = summon[Mirror.ProductOf[lib.Bar]] // error
   |                                                 ^
   |No given instance of type deriving.Mirror.ProductOf[lib.Bar] was found for parameter x of method summon in object Predef. Failed to synthesize an instance of type deriving.Mirror.ProductOf[lib.Bar]: class Bar is not a generic product because the constructor of class Bar is innaccessible from the calling scope.

Expectation

summon[Mirror.SumOf[lib.Foo]] should also fail because in the calling scope, the child of Foo, i.e. Bar can not have a mirror - this affects signatures of Foo, because object Foo should not cache the mirror.

@bishabosha bishabosha added itype:bug regression This worked in a previous version but doesn't anymore labels May 18, 2022
@bishabosha bishabosha added this to the 3.2.0-RC1 milestone May 18, 2022
@bishabosha bishabosha changed the title hierarchical sum mirror is available when mirror for child can't be summoned sum mirror is available when mirror for child can't be summoned May 18, 2022
@bishabosha
Copy link
Member Author

because of this bug, object Foo will unnecessarily extend Mirror.Sum when it shouldn't even be a valid mirror

@bishabosha
Copy link
Member Author

bishabosha commented May 19, 2022

this example is quite problematic - from within lib, Bar is accessible, so from Foo's perspective Bar should have a mirror, however the moment we move outside of lib, there is no longer a mirror for Bar.

So what to do?

  1. mirror for Foo is anonymous, therefore it will exist in lib, but not outside. This will remove Mirror.Sum subclass and ordinal method from the companion compared to if it were compiled by 3.1. - so a bincompat issue
  2. cache Mirror for Foo in its companion, but make it inaccessible outside of lib. (i.e. do not synthesize a mirror in app) This is completely compatible

@nicolasstucki
Copy link
Contributor

nicolasstucki commented May 19, 2022

First bad commit d6e17a2 in #15008

@joroKr21
Copy link
Member

Why does the child also need to have a mirror?

@bishabosha
Copy link
Member Author

bishabosha commented May 23, 2022

Why does the child also need to have a mirror?

A class is only a "generic sum" if all of its children are also either "generic sum" or "generic product".

That has been the rule forever, so we should be quite careful to consider this recursive case whenever we change the rules for mirror generation

@joroKr21
Copy link
Member

Hmm I didn't know that - what is the reason for this limitation?

@bishabosha
Copy link
Member Author

bishabosha commented May 23, 2022

mirrors are meant to represent an algebraic data type, and that all parts mentioned by the mirror should be reachable.
The mirror for Foo will mention Bar in its MirroredElemTypes. the user should then be able to iterate through MirroredElemTypes (with inline method or macro) to summon the mirror for Bar programatically. This operation is meant to be total

bishabosha added a commit that referenced this issue May 27, 2022
fix #15222 recursively check for product ctor accessibility
@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
itype:bug regression This worked in a previous version but doesn't anymore
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants