Don't generate Mirror.Sum for simple enum cases

[andrzejressel]

From my understanding simple enum cases are not real classes, but instances of enum object class. Because of that they inherit their Mirror.Sum which may confuse tools like Shapeless (and also I feel it's not valid, because class cannot have both Product and Sum at the same time).

[joroKr21]

@andrzejressel is that causing the problems with enums in shapeless and kittens?
That's a really good find!

[andrzejressel]

@joroKr21 Hi, I've just verified this and yes, this commit fixes typelevel/shapeless-3#51 and typelevel/kittens#449

[joroKr21]

isGenericSum is used in one more place and I wonder if it should be modified as well.

[andrzejressel]

In SyntheticMembers::addMirrorSupport? It's not an issue, because this method will never be called: as far as I understand branch else if (impl.removeAttachment(ExtendsSingletonMirror).isDefined) will be invoked.

[bishabosha]

it's fine to keep isGenericSum implementation unchanged, it should only be called on a class symbol, perhaps it could be more informative to rename to isGenericSumClass, along with isGenericProduct to isGenericProductClass

[bishabosha]

Hey, thank you for noticing this and opening the PR!

I think this could go further, for example, it's still possible to summon a sum mirror for the union of two value cases (that is generated like the mirror for Foo):

enum Foo { case A, B, C }

summon[scala.deriving.Mirror.SumOf[Foo.A.type | Foo.B.type]]
/*
val res6: 
  (
    deriving.Mirror.Sum{
      MirroredType = (Foo.A.type | Foo.B.type); 
        MirroredMonoType = (Foo.A.type | Foo.B.type)
      ; MirroredElemTypes <: Tuple
    }
   & 
    scala.deriving.Mirror.Sum{
      MirroredMonoType = (Foo.A.type | Foo.B.type); 
        MirroredType = (Foo.A.type | Foo.B.type)
      ; MirroredLabel = "Foo"
    }
  ){
    MirroredElemTypes = (Foo.A.type, Foo.B.type, Foo.C.type); 
      MirroredElemLabels = ("A", "B", "C")
  } = anon$1@7b205eaa
*/

my guess for a workable heuristic would be any top level part of the type being a term ref is not allowed.

[joroKr21]

Maybe the right approach is to use typeSymbol and isClass instead. It looks like classSymbol is trying really hard to find a class symbol but this is not what we want here.

[bishabosha]

Maybe the right approach is to use typeSymbol and isClass instead. It looks like classSymbol is trying really hard to find a class symbol but this is not what we want here.

classSymbol is the usual approach to looking through type aliases, Unions and Intersection types,typeSymbol would not work for union types, which should still be supported (just not unions of TermRef)

[joroKr21]

classSymbol is the usual approach to looking through type aliases, Unions and Intersection types,typeSymbol would not work for union types, which should still be supported (just not unions of TermRef)

This doesn't make much sense to me - if we want union types to work properly their "children" should be only the elements of the union type. What happens right now is that it finds the LUB class and takes its children?

scala> {
     | sealed trait Foo extends Product, Serializable
     | case object A extends Foo
     | case object B extends Foo
     | case object C extends Foo
     | }
// defined trait Foo
// defined case object A
// defined case object B
// defined case object C

scala> summon[scala.deriving.Mirror.SumOf[A.type | B.type]]
val res4:
  (
    deriving.Mirror.Sum{
      MirroredType = (A.type | B.type); MirroredMonoType = (A.type | B.type);
        MirroredElemTypes <: Tuple
    }
   &
    scala.deriving.Mirror.Sum{
      MirroredMonoType = (A.type | B.type); MirroredType = (A.type | B.type);
        MirroredLabel = "Foo"
    }
  ){
    MirroredElemTypes = (A.type, B.type, C.type);
      MirroredElemLabels = ("A", "B", "C")
  } = anon$1@2b749404

Note that Product and Serializable are required in this case.
Luckily if I use classes pattern matching catches my mistake:

scala> {
     | sealed trait Foo extends Product, Serializable
     | case class A() extends Foo
     | case class B() extends Foo
     | case class C() extends Foo
     | }
// defined trait Foo
// defined case class A
// defined case class B
// defined case class C

scala> summon[scala.deriving.Mirror.SumOf[A | B]]
-- Error: ----------------------------------------------------------------------
1 |summon[scala.deriving.Mirror.SumOf[A | B]]
  |                                          ^
  |this case is unreachable since type MirroredMonoType and class C are unrelated
1 error found

[bishabosha]

A union type can only be allowed where all of its branches are type applications of a covariant class, i.e. there can still be a mirror for Mirror.SumOf[Option[Int] | Option[String]] where its MirroredElemTypes would be (Some[Int | String], None.type). (see #13493) But we should not allow a sum for a union of singleton term references.

Elaborating:
The way a sum type is defined for the purpose of mirror generation is "a single sealed abstract class/trait, with optional type parameters, where all its immediate subclasses/enum-cases are generic products", so we can not create ad-hoc sum types out of unrelated classes joined with a union.

[joroKr21]

I think it's fine disallowing Mirror.SumOf[Option[Int] | Option[String]] because Option[Int] | Option[String] is only a subtype of Option[Int | String] but Mirror.SumOf is invariant. What is happening instead is there is a sort of widening step (via classSymbol) and we get the mirror for the "widened" type. If anything it should be Some[Int] | Some[String] and None.type. But it's going in edge case territory. However a rule "there is a mirror for classes or type aliases thereof" sounds simpler to explain than - "there is no mirror when a top-level symbol is a term symbol". Even though if we enumerate all cases in the union like A.type | B.type | C.type from the example above that would be a valid mirror currently.

[bishabosha]

@andrzejressel hey, I'm just wondering if are you blocked, or do not have time to work on this, or if we can help in any way?

[andrzejressel]

@andrzejressel hey, I'm just wondering if are you blocked, or do not have time to work on this, or if we can help in any way?

To be honest I was wondering where your discussion would lead. I guess my current task is disallowing mirror of union of singletons: #14525 (comment)

[bishabosha]

I think we should also want to block SumOf[Bar.A | Bar.B] for the following:

enum Bar:
  case A(i: Int)
  case B(b: Boolean)
  case C(s: String)

so the algorithm for this would be that we extract the leaves of union types, and then check that either no branch is some singleton value, or that no two branches refer to different class types

[andrzejressel]

Hi, I don't know If I understood you correctly, but I found method TypeComparer.provablyDisjoint which seems to be doing what you said - at least for my understanding.

I addressed my own critiques

[dwijnand]

Nicely done

[joroKr21]

I still think it would have been simpler to use typeSymbol instead of classSymbol but I'm fairly happy with this and I'm looking forward to this fix a lot 😄

[bishabosha]

I simplified a bit so it is more resistant to change