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

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

Closed
jtjeferreira opened this issue Jan 4, 2021 · 10 comments · Fixed by #11039
Closed

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

jtjeferreira opened this issue Jan 4, 2021 · 10 comments · Fixed by #11039
Assignees
Milestone

Comments

@jtjeferreira
Copy link
Contributor

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

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

bishabosha commented Jan 5, 2021

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

bishabosha commented Jan 5, 2021

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.

@bishabosha bishabosha changed the title Mirror derivation for ADT inside a class Mirror derivation for flat ADT inside a class Jan 5, 2021
@jtjeferreira
Copy link
Contributor Author

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

bishabosha commented Jan 5, 2021

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

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

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 bishabosha changed the title Mirror derivation for flat ADT inside a class Mirror.Sum synthesised for parent without companion Jan 5, 2021
@bishabosha bishabosha changed the title Mirror.Sum synthesised for parent without companion Mirror.Sum synthesised for base class without companion Jan 5, 2021
@bishabosha bishabosha self-assigned this Jan 7, 2021
@bishabosha
Copy link
Member

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

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 bishabosha changed the title Mirror.Sum synthesised for base class without companion Mirror.Sum not synthesised for base class without companion Jan 8, 2021
@bishabosha
Copy link
Member

I have found this commit: 275a0a9 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
}

bishabosha added a commit to dotty-staging/dotty that referenced this issue Jan 8, 2021
bishabosha added a commit to dotty-staging/dotty that referenced this issue Jan 8, 2021
bishabosha added a commit that referenced this issue Jan 13, 2021
fix #10997: check accessible from sealed parent not companion
@Kordyjan Kordyjan added this to the 3.0.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants