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

summon Mirror of a union type has incorrect children #13493

Closed
bishabosha opened this issue Sep 9, 2021 · 5 comments · Fixed by #15279
Closed

summon Mirror of a union type has incorrect children #13493

bishabosha opened this issue Sep 9, 2021 · 5 comments · Fixed by #15279
Assignees
Milestone

Comments

@bishabosha
Copy link
Member

If I summon a Mirror for Option[Int] | Option[String] then the children in MirroredElemTypes are None.type and Some[Int]

Compiler version

3.0.2

Minimized code

summon[deriving.Mirror.Of[Option[Int] | Option[String]]]

Output

scala> summon[deriving.Mirror.Of[Option[Int] | Option[String]]]                                                         
val res0: 
  (
    deriving.Mirror{
      MirroredType = (Option[Int] | Option[String]); 
        MirroredMonoType = (Option[Int] | Option[String])
      ; MirroredElemTypes <: Tuple
    }
   & 
    scala.deriving.Mirror.Sum{
      MirroredMonoType = (Option[Int] | Option[String]); 
        MirroredType = (Option[Int] | Option[String])
      ; MirroredLabel = "Option"
    }
  ){
    MirroredElemTypes = (None.type, Some[Int]); 
      MirroredElemLabels = ("None$", "Some")
  } = anon$1@36618b3b

Expectation

I would expect that this should be illegal, or at least the mirrored elem types should mention String somewhere

@dwijnand
Copy link
Member

I guess seeing as Option is injective (right?) that the elem types should be None.type and Some[Int | String].

@bishabosha
Copy link
Member Author

@odersky just asking for your opinion

@odersky
Copy link
Contributor

odersky commented Sep 14, 2021

Yes, mirrored elem types should not just mention Int. If I try in the reverse order I get:

scala> summon[deriving.Mirror.Of[Option[String] | Option[Int]]]
val res1: 
  (
    deriving.Mirror{
      MirroredType = (Option[String] | Option[Int]); 
        MirroredMonoType = (Option[String] | Option[Int])
      ; MirroredElemTypes <: Tuple
    }
   & 
    scala.deriving.Mirror.Sum{
      MirroredMonoType = (Option[String] | Option[Int]); 
        MirroredType = (Option[String] | Option[Int])
      ; MirroredLabel = "Option"
    }
  ){
    MirroredElemTypes = (None.type, Some[String]); 
      MirroredElemLabels = ("None$", "Some")
  } = anon$1@4edf1051

So it depends on what comes first, which looks wrong.

@odersky odersky assigned bishabosha and unassigned odersky Sep 14, 2021
@Lasering
Copy link

Mirrors of union types are only generated when all the types of the union are of the same higher-kinded type (and even then things are not consistent look at the summon of Map | Map):

import scala.deriving.Mirror
import java.io.File

// All of these fail to compile with `no implicit argument of type ... was found for ...`
summon[Mirror.Of[Int | Double]]
summon[Mirror.Of[String | Unit]]
summon[Mirror.Of[String | File]]
summon[Mirror.Of[Option[Int] | File]]
summon[Mirror.Of[5 | 6]]
summon[Mirror.Of["Foo" | "Bar"]]
summon[Mirror.Of[Map[Int, String] | Option[String]]]
summon[Mirror.Of[Map[String, Int] | Option[String]]]
summon[Mirror.Of[List[String] | Option[String]]]
summon[Mirror.Of[List["Foo"] | Option["Foo"]]]
summon[Mirror.Of[Map[Int, String] | Map[String, Int]]]
summon[Mirror.Of[Map[String, Int] | Map[String, Double]]]

// These compile
summon[Mirror.Of[Option[Int] | Option[String]]]
summon[Mirror.Of[List[Int] | List[String]]]
summon[Mirror.Of[List["Foo"] | List["Foo"]]]
summon[Mirror.Of[List["Foo"] | List["Bar"]]]
summon[Mirror.Of[List["Foo"] | List[5]]]

https://scastie.scala-lang.org/mXMVGHWwTBuB8C5tpsqqGw

@bishabosha
Copy link
Member Author

bishabosha commented Apr 21, 2022

The alternatives I have come up with are either Some[Int] | Some[String] or Some[Int & String] as valid subtypes of Option[Int] | Option[String] which should be preferred?

Edit: I think it should be the first option, because for an Encoder/Decoder derived from a mirror it makes the most sense to me to say "this encoder accepts either a Some[Int], or a Sum[String]"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment