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

Synthesise Mirror.Sum for nested hierarchies #11686

Merged
merged 1 commit into from
Aug 17, 2021

Conversation

dwijnand
Copy link
Member

tests/run/i11050.scala Outdated Show resolved Hide resolved
tests/run/i11050.scala Outdated Show resolved Hide resolved
@japgolly

This comment has been minimized.

@julienrf

This comment has been minimized.

@bishabosha

This comment has been minimized.

@bishabosha bishabosha self-requested a review March 30, 2021 09:37
@dwijnand

This comment has been minimized.

@dwijnand

This comment has been minimized.

@bishabosha
Copy link
Member

bishabosha commented May 21, 2021

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

Copy link
Member

@bishabosha bishabosha left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

bishabosha commented May 21, 2021

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member Author

dwijnand commented Jun 2, 2021

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.

Copy link
Member

@bishabosha bishabosha left a comment

Choose a reason for hiding this comment

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

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

compiler/src/dotty/tools/dotc/transform/SymUtils.scala Outdated Show resolved Hide resolved
tests/run/i11050.scala Outdated Show resolved Hide resolved
@bishabosha bishabosha added this to the 3.1.0 milestone Jun 2, 2021
@bishabosha bishabosha added the needs-minor-release This PR cannot be merged until the next minor release label Jun 2, 2021
@bishabosha bishabosha self-requested a review June 2, 2021 13:19
@bishabosha
Copy link
Member

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

Copy link
Member

@bishabosha bishabosha left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

dwijnand commented Jun 7, 2021

@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
Copy link
Member

@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
Copy link
Member Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-minor-release This PR cannot be merged until the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Synthesise Mirror.Sum for Nested hierarchies
6 participants