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

Include sealed traits and classes as elements of Sum Mirrors #11851

Closed
wants to merge 1 commit into from

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Mar 22, 2021

Addresses feature request #161

lampepfl/dotty-feature-requests#161

This is just the immediate fix. We need more tests and use cases to see whether it works. @dwijnand if we can get some corrobation by Wednesday we might be able to roll this into 3.0. Otherwise it will have to wait.

Copy link
Contributor

@julienrf julienrf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick fix!

@@ -95,7 +95,7 @@ object SymUtils:
* - none of its children are anonymous classes
* - all of its children are addressable through a path from the parent class
* and also the location of the generated mirror.
* - all of its children are generic products or singletons
* - all of its children are generic products, singletons, or sealed traits ar classes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* - all of its children are generic products, singletons, or sealed traits ar classes
* - all of its children are generic products, singletons, or sealed traits or classes


@main def Test =
summon[Mirror.Of[A]]
summon[Mirror.Of[B]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we maybe add another test like the following?

val mirrorA = summon[Mirror.Of[A]]
summon[mirrorA.MirroredElemTypes <:< (C *: B *: EmptyTuple)]

@odersky
Copy link
Contributor Author

odersky commented Mar 23, 2021

As the failure of the Akka CB shows, I have overlooked something. The trait hierarchy can be an arbitrary graph. Consider:

sealed trait A
sealed trait B extends A
sealed trait C extends A
case class D() extends B, C
case class E() extends B, C

Now if we generate the ordinal method, we get unreachable cases. That's what we observe when trying to build akka.

I see currently three ways to address this:

  1. Continue with Synthesise Mirror.Sum for nested hierarchies #11686. This means that mirrors will show elements that are not in the children of the base trait. This is risky since it might break some assumptions of code that does typeclass derivation.
  2. Continue with the scheme in this PR, but only consider sealed child traits that dominate their children. I.e. their children are not at the same children of the parent trait or of another child trait. In other words the inheritance graph must be a tree. This is also risky since it might break an assumption in typeclass derivation code that elements of sum mirrors are always product mirrors.
  3. As a minimal solution, simply ignore sealed subtraits that don't introduce new children. That is, we would allow
    sealed trait A
    sealed trait B extends A
    case class C() extends A
    case class D() extends A, B
    Here, the children of A are B, C, and D, and the mirror elements are C and D. B is ignored since it adds no
    new children. This is also risky in that typeclass derivation code might assume that mirror elements are in a one to one correspondence with children. But it's probably less risky than (1) and (2)

@julienrf
Copy link
Contributor

julienrf commented Mar 23, 2021

  1. Continue with Synthesise Mirror.Sum for nested hierarchies #11686. This means that mirrors will show elements that are not in the children of the base trait. This is risky since it might break some assumptions of code that does typeclass derivation.

Which assumptions do you think about?

@odersky
Copy link
Contributor Author

odersky commented Mar 24, 2021

Which assumptions do you think about?

For instance, if I have typeclass derivation code that consults the children annotation at runtime I get a mismatch between that and what the mirror claims. Or if the code assumes that all mirror elements have the sum as direct parent type.

@lrytz
Copy link
Member

lrytz commented Mar 24, 2021

(Dale is OOO this week)

@japgolly
Copy link
Contributor

if we can get some corrobation by Wednesday we might be able to roll this into 3.0
(Dale is OOO this week)

I'm happy to put my hand up for that. Let me know what you need!

@japgolly
Copy link
Contributor

Regarding the approach to implementation, I have another one to add to the 3 ideas above.

(4) Have two means of summoning a mirror, both with simple algorithms:

  1. Gather all product children transitively. This is the most commonly useful case so could be the default for the existing Mirror.Of. In the Akka example above:
summon[Mirror.Of[A]].MirroredElemTypes =:= (C, D)
summon[Mirror.Of[B]].MirroredElemTypes =:= (D)
  1. Gather sub-types directly and non-transitively, regardless of whether the sub-type is a product or a sum. We could give this a different type Mirror.DirectlyOf and have users explicitly opt-in to this algorithm. In the Akka example above:
summon[Mirror.DirectlyOf[A]].MirroredElemTypes =:= (B, C, D)
summon[Mirror.DirectlyOf[B]].MirroredElemTypes =:= (D)

In my experience with automatic derivation, it's always one of the above two I want. Which one depends on what I'm deriving.

@julienrf
Copy link
Contributor

@japgolly Interesting. I’m not sure whether we can change this now but I agree with your analysis: those two ways of retrieving the subclasses are the most common in my experience as well.

@odersky
Copy link
Contributor Author

odersky commented Mar 25, 2021

I think it's probably best for now to continue with #11686. But since Dale is OOO this weak there's no chance for it to make it into RC2. In my opinion, even if we could work on this right now, it would be too big a change to make so short before the release date.

@odersky odersky closed this Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants