Synthesise Mirror.Sum for nested hierarchies

[dwijnand]

Fixes lampepfl/dotty-feature-requests#161
aka https://github.com/lampepfl/dotty/issues/11050

[bishabosha]

I have minimised the Akka failure:

package akka.event

sealed trait LogEvent
object LogEvent

case class Error() extends LogEvent

class Error2() extends Error() with LogEventWithMarker

case class Warning() extends LogEvent

sealed trait LogEventWithMarker extends LogEvent

this generates the following warning:

-- [E030] Match case Unreachable Warning: sandbox/akka/Logging.scala:8:7 -------
8 |object LogEvent
  |       ^
  |       Unreachable case
1 warning found

the warning comes from the ordinal method, which looks like the following:

object LogEvent extends scala.deriving.Mirror.Sum { 
  type MirroredMonoType = akka.event.LogEvent

  def ordinal(x: akka.event.LogEvent.MirroredMonoType): Int = x match 
    case _:akka.event.Error => 0
    case _:akka.event.Warning => 1
    case _:akka.event.LogEventWithMarker => 2
}

The only instance of LogEventWithMarker is also an instance of Error, so it is a redundant case

[bishabosha]

the code gen for cases in the ordinal method needs to take into account multiple inheritance with traits
https://github.com/lampepfl/dotty/blob/a956774180d124adc98527863b919c35d5f9db92/compiler/src/dotty/tools/dotc/transform/SyntheticMembers.scala#L519

[bishabosha]

this is more strange, if we delete the Warning case class, then we get the ordinal method

def ordinal(x$0: akka.event.LogEvent.MirroredMonoType): Int = x$0 match
  case _:akka.event.Error => 0
  case _:akka.event.LogEventWithMarker => 1

but in this case we get no warning? cc/@liufengyun

[bishabosha]

its worth noting also that in this formulation, there is no warning as LogEventWithMarker comes first,

package akka.event

sealed trait LogEvent
object LogEvent

sealed trait LogEventWithMarker extends LogEvent

case class Error() extends LogEvent
class Error2() extends Error() with LogEventWithMarker

case class Warning() extends LogEvent

this means Error2 will get ordinal 0 but Error will get ordinal 1, should Sum children get lower priority in the match?

[liufengyun]

this is more strange, if we delete the Warning case class, then we get the ordinal method

def ordinal(x$0: akka.event.LogEvent.MirroredMonoType): Int = x$0 match
  case _:akka.event.Error => 0
  case _:akka.event.LogEventWithMarker => 1

but in this case we get no warning? cc/@liufengyun

Could you report a bug? I can have a look.

[dwijnand]

I have minimised the Akka failure:

package akka.event

sealed trait LogEvent
object LogEvent

case class Error() extends LogEvent

class Error2() extends Error() with LogEventWithMarker

case class Warning() extends LogEvent

sealed trait LogEventWithMarker extends LogEvent

this generates the following warning:

-- [E030] Match case Unreachable Warning: sandbox/akka/Logging.scala:8:7 -------
8 |object LogEvent
  |       ^
  |       Unreachable case
1 warning found

the warning comes from the ordinal method, which looks like the following:

object LogEvent extends scala.deriving.Mirror.Sum { 
  type MirroredMonoType = akka.event.LogEvent

  def ordinal(x: akka.event.LogEvent.MirroredMonoType): Int = x match 
    case _:akka.event.Error => 0
    case _:akka.event.Warning => 1
    case _:akka.event.LogEventWithMarker => 2
}

The only instance of LogEventWithMarker is also an instance of Error, so it is a redundant case

Just re-force pushed to make sure the test case correctly reproduces your findings. After trying different things I came to the conclusion that the "class extends case class" pattern should fail the sum/product game for the mirror, keeping things simple: that case isn't a part of the motivating use cases, and it only came up because they were failing to generate mirrors without warnings.

So I think this is good now, multi-level sealed traits now get sum mirrors without breaking the community projects.

[bishabosha]

It looks great apart from the comment about the declScope argument to whyNotGenericSum.

[bishabosha]

I have put this for 3.1 because some companion objects will now get scala.deriving.Sum parents and ordinal methods when they did not before

[bishabosha]

This PR still needs to update the ordinal method in SyntheticMembers,
the following will generate the "unreachable case" warning due to generating a branch for Bottom in the ordinal

sealed trait Top
object Top

final case class MiddleA() extends Top with Bottom
final case class MiddleB() extends Top with Bottom
final case class MiddleC() extends Top with Bottom

sealed trait Bottom extends Top

-- [E030] Match case Unreachable Warning: sandbox/mirrors/example.scala:2:7 ----
2 |object Top
  |       ^
  |       Unreachable case
1 warning found

... the generated ordinal mirror method here:

def ordinal(x$0: MirroredMonoType): Int = x$0 match
  case _:MiddleA => 0
  case _:MiddleB => 1
  case _:MiddleC => 2
  case _:Bottom => 3 // this case can't be reached

[dwijnand]

@bishabosha WDYT about just suppressing that? The fact that Top's first three children are also children of Bottom doesn't change the fact that Bottom is Top's fourth child (wat). The goal of unreachability warnings is to remove misleading cases, and seeing as this is generated code perhaps it's fine?

It's also a much easier fix but I can't tell how much my thinking above is biased by that...

[bishabosha]

@bishabosha WDYT about just suppressing that? The fact that Top's first three children are also children of Bottom doesn't change the fact that Bottom is Top's fourth child (wat). The goal of unreachability warnings is to remove misleading cases, and seeing as this is generated code perhaps it's fine?

It's also a much easier fix but I can't tell how much my thinking above is biased by that...

I think it would be fine to just add @unchecked on the match

[dwijnand]

@bishabosha WDYT about just suppressing that? The fact that Top's first three children are also children of Bottom doesn't change the fact that Bottom is Top's fourth child (wat). The goal of unreachability warnings is to remove misleading cases, and seeing as this is generated code perhaps it's fine?

It's also a much easier fix but I can't tell how much my thinking above is biased by that...

Might've been easier than I thought: #13414.