scala / scala3

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

Mirror.Sum not synthesised for base class without companion #10997

Closed jtjeferreira closed 3 years ago

jtjeferreira commented 3 years ago

Minimized code

class Test {
  sealed trait Role
  case object Admin extends Role
  case class Contributor(organization: String) extends Role

  println(summon[Mirror.Of[Role]])
}

Output

// no implicit argument of type deriving.Mirror.Of[Test.this.Role] was found for parameter x of method summon in object Predef

Expectation

It should be able to derive the mirror as if it was inside the companion object.

object Test {
  sealed trait Role
  case object Admin extends Role
  case class Contributor(organization: String) extends Role

  println(summon[Mirror.Of[Role]])
}

I found this while I was port play-json to dotty (https://github.com/playframework/play-json/pull/555/files).

jtjeferreira commented 3 years ago

If this is not considered a bug, at least would be nice to have a better error message explaining this is a nested class

bishabosha commented 3 years ago

I investigated by printing the internal reporting and the reason given for not generating a mirror is that its child object Admin is not accessible within its owner class Test.

This accessibility test is defined as the child being contained within the companion of Role. Edit: This accessibility test is defined as the companion object of Role having either itself, or any of its enclosing scopes, match the child's enclosing scope, or repeating that test following the outer enclosing scopes of the child when they are objects. The code can be seen here: https://github.com/lampepfl/dotty/blob/2be0707cae91bc394338676cd6fdad48af148d31/compiler/src/dotty/tools/dotc/transform/SymUtils.scala#L105

So the example can be fixed by instead defining the child cases within a companion object, such as this:

class Test {
  sealed trait Role
  object Role {
    case object Admin extends Role
    case class Contributor(organization: String) extends Role
  }

  println(summon[deriving.Mirror.Of[Role]])
}

I will update with why changing Test to an object is also a fix

bishabosha commented 3 years ago

Basically, following the code of the accessibility test in the above comment, I think it is a bug that it works when Test is an object: Following the chain of owners of object Test you reach the root package and then it is a pure coincidence that the owner of the root package is the empty symbol and the the companion of Role is also the empty symbol, so the accessibility test will pass.

jtjeferreira commented 3 years ago

Hi Jamie thanks for the fast feedback, but allow me to disagree...

In the scaladoc of that method:

https://github.com/lampepfl/dotty/blob/2be0707cae91bc394338676cd6fdad48af148d31/compiler/src/dotty/tools/dotc/transform/SymUtils.scala#L90-L96

The important bit is "all of its children are addressable through a path from its companion object". And if we try that "manually" it compiles:

class Main {
  sealed trait Role
  case object Admin extends Role
  case class Contributor(organization: String) extends Role

  object Role {
    val r: Role = ???
    val a: Admin.type = ???
    val c: Contributor = ???

  }
}

So all the children are addressable from the companion object...

bishabosha commented 3 years ago

Ok right yes that would make sense, so only defining the companion object is required:

class Test {
  sealed trait Role
  case object Admin extends Role
  case class Contributor(organization: String) extends Role
  object Role

  println(summon[deriving.Mirror.Of[Role]])
}

because object Role is now defined, the enclosing scope of object Role (i.e. class Test) also declares object Admin and class Contributor

jtjeferreira commented 3 years ago

oh thats a very nice workaround. should we close this? or should this be fixed? maybe there is a way that the compiler could generate the companion object behind the scenes?

bishabosha commented 3 years ago

In my opinion, based on the test cases in #6531, it is a bug that Mirror.Sum can be summoned when the companion object does not exist

bishabosha commented 3 years ago

One reason this may have gone unnoticed for so long is that using a derives clause will cause a companion object to be generated if one does not exist. But certainly you may want to use a Mirror outside of derivation purposes

bishabosha commented 3 years ago

tests https://github.com/lampepfl/dotty/blob/master/tests/run/deriving.scala and https://github.com/lampepfl/dotty/blob/master/tests/run/deriving-constructor-order.scala

work on the assumption that a companion is not required, which makes the motivation for this accessible from companion test confusing

bishabosha commented 3 years ago

I have found this commit: https://github.com/lampepfl/dotty/commit/275a0a96bdb1f28300ca8e0f045226e1e99b3df1 i.e. the message is "Generate sum mirrors for sealed traits that do not have a companion"

so it would seem pretty clear that it is infact the original code example which is a bug:

class Test {
  sealed trait Role
  case object Admin extends Role
  case class Contributor(organization: String) extends Role

  println(summon[Mirror.Of[Role]]) // should be able to find the Mirror but can't
}