scala / scala3

The Scala 3 compiler, also known as Dotty.
https://dotty.epfl.ch
Apache License 2.0
5.84k stars 1.05k forks source link

Mirror.SumOf not being provided when 0 inhabitants #11764

Open japgolly opened 3 years ago

japgolly commented 3 years ago

Compiler version

3.0.0-RC1

Minimized code

import scala.deriving.*
sealed trait X
summon[Mirror.Of[X]]

Output

[error] 59 |  summon[Mirror.Of[X]]
[error]    |                      ^
[error]    |no implicit argument of type deriving.Mirror.Of[X] was found for parameter x of method summon in object Predef

Expectation

X is a sealed trait. I expect a Mirror.SumOf[X] { type MirroredElemTypes = EmptyTuple } to be provided.

bishabosha commented 3 years ago

I am going to put this as an enhancement request, as we see this behaviour is by design:

Is this a sealed class or trait for which a sum mirror is generated?
It must satisfy the following conditions:
- it has at least one child class or object
  ...

taken from https://github.com/lampepfl/dotty/blob/5bb0e92b33cd96ac96d394068165b38e4ce5b035/compiler/src/dotty/tools/dotc/transform/SymUtils.scala#L100

In which case is it useful to have a Mirror for a sealed trait with no children?

joroKr21 commented 3 years ago

In which case is it useful to have a Mirror for a sealed trait with no children?

It's just an induction base - it seems weird to treat this as a special case and exclude it

bishabosha commented 3 years ago

I think its fine to try this out

japgolly commented 3 years ago

In which case is it useful to have a Mirror for a sealed trait with no children?

I can try to did them out if you like, but I've also had a bunch of legitimate cases where I want to derive a valid type-class for a sealed trait with no concrete children. Also having no concrete children is also good for compile-time-only computation/logic.

bishabosha commented 3 years ago

I can try to did them out if you like, but I've also had a bunch of legitimate cases where I want to derive a valid type-class for a sealed trait with no concrete children. Also having no concrete children is also good for compile-time-only computation/logic.

And using Mirror.Sum is more appropriate in this case rather than some custom IsSealed typeclass? because there will still be no mirror generated if a single child is a plain class

japgolly commented 3 years ago

And using Mirror.Sum is more appropriate in this case rather than some custom IsSealed typeclass? because there will still be no mirror generated if a single child is a plain class

Yeah I think that does make sense and is most appropriate for two reasons:

  1. In terms of the logic/math, treating a sealed trait with 0 children differently than sealed traits with >0 children means we're special-casing 0 and adding an exception to the general rule. Removing 0 as a special case makes the concept simpler. I kind of view it like us having a single List type in stdlib, as opposed to having two separate types: EmptyList and NonEmptyList
  2. Re "no mirror generated if a single child is a plain class":
    • unfortunately that problem isn't completely solved because you could have one case class A child and one plain class B child and it sounds like the mirroring stuff will silently ignore B. So I'd say that's a more general problem because it affects the >0 existing cases too.
    • I wonder if it would make sense to prohibit or at least warn, when a non-(effectively)-sealed child extends a sealed parent?
bishabosha commented 2 years ago

@japgolly after trying to enable mirrors for sealed traits with 0 children, we have observed in the community build that there are already libraries that assume the sum mirror implies there must be a child. We suggest that if you want to deal with zero children sealed-types you could make a wrapper around Mirror that will be provided in the case where there are no children (and the type is still sealed).

joroKr21 commented 2 years ago

@japgolly after trying to enable mirrors for sealed traits with 0 children, we have observed in the community build that there are already libraries that assume the sum mirror implies there must be a child. We suggest that if you want to deal with zero children sealed-types you could make a wrapper around Mirror that will be provided in the case where there are no children (and the type is still sealed).

That seems like a very weird thing to rely on. If they test that something does not compile we can ask them to remove those tests. Compiling more stuff should not really break anyone's code. Esp. considering that sealed traits with no children cannot be instantiated. Do you think there is a use case that could break by allowing mirrors for these kind of traits?

bishabosha commented 2 years ago

That seems like a very weird thing to rely on. If they test that something does not compile we can ask them to remove those tests. Compiling more stuff should not really break anyone's code. Esp. considering that sealed traits with no children cannot be instantiated. Do you think there is a use case that could break by allowing mirrors for these kind of traits?

One actually crashed at runtime due to their implementation calling reduce on the empty list, (plus it is documented in a comment in the code of the compiler that a mirror is not provided for a sum type without children - making it part of the API) - so if this change was included in a release then clients could use the current version of the library with the new compiler to cause the crash

joroKr21 commented 2 years ago

Thanks for explaining - I guess we are stuck with it then. Although a bit unsatisfactory, perhaps it won't be a big issue in practice. I'm not sure what you mean with making a wrapper around Mirror though 🤔

som-snytt commented 2 years ago

If they test that something does not compile we can ask them to remove those tests.

This is the joke where the patient says, "Doc, it hurts when I do this," and the doctor replies, "Then don't do that."

japgolly commented 2 years ago

Hello. As I understand it, which ever library you're talking about currently doesn't support sum types with no inhabitants (both because Scala 3 isn't providing a mirror and because they've used reduce instead of foldLeft), so fixing up the Scala 3 side of the equation will result in it going from "not working" (via compilation error due to no implicit mirror) to "not working" (due to a crash). This doesn't seem like a great reason to hold back the entire language. If it didn't work before and wont work immediately after this change, we're not breaking anything, we're just discussing what the break looks like.

I really think we should add this to a minor release. That some library used reduce instead of foldLeft shouldn't hold back Scala itself. Plus it would be trivial to fix such a library, no? 0 inhabitants isn't some special case here, it's just a natural part of the mathematics. Expecting at least one inhabitant is the artificial special case here.

bishabosha commented 2 years ago

Personally, I would like to see this change go through, but there's other points to consider - we will have to build more special cases into the compiler to support backwards compatibility.

I can try to persuade the team again, but to quote what @odersky said the first time "1 is fine for a base case of induction"

bishabosha commented 2 years ago

Thanks for explaining - I guess we are stuck with it then. Although a bit unsatisfactory, perhaps it won't be a big issue in practice. I'm not sure what you mean with making a wrapper around Mirror though 🤔

this would be a bit like OptManifest vs Manifest, see https://github.com/lampepfl/dotty/issues/9482#issuecomment-668838161 - except your implicit definition should do an extra check in the "empty" case that the type is sealed (and not a case class)

japgolly commented 2 years ago

to quote what @odersky said the first time "1 is fine for a base case of induction"

I'm glad he's ok with it but it's not universally true. I have legitimate cases where this assumption fails.

Would it be helpful if I put my hand up to fix whichever libraries in the community build break?

som-snytt commented 2 years ago

An example would be, "Fool me once, shame on you. Fool me twice, shame on me." There is no fool me zeroth. I guess we are always already fooled, which sounds about right to me.

bishabosha commented 2 years ago

@japgolly I raised this again and from core dotty team it seems to be a hard no, would you like to volunteer an example where this would be much more convenient than the status quo? perhaps we can figure out a nicer way to express the same thing