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

"knownDirectSubclasses observed before subclass" when deriving for sealed trait #14

Open
nadavwr opened this issue Dec 23, 2019 · 1 comment

Comments

@nadavwr
Copy link

nadavwr commented Dec 23, 2019

Deriving for a case class works great:

@Semi(Show) case class Foo(a: Int) 

Derivation for ADT's is a bit brittle though:

@Semi(Show) sealed trait Tree
case class Node(nodes: List[Tree]) extends Tree
case class Leaf(label: String) extends Tree
// 👎Could not derive an instance of Show[Tree]
//implicit val _derived_cats_Show: cats.Show[Tree] = cats.derived.semi.show[Tree]
//knownDirectSubclasses of Tree observed before subclass Node registered
//knownDirectSubclasses of Tree observed before subclass Leaf registered

That's a pity—it's the first thing people will try (at least that's what I went for).

Naturally, the same error occurs with direct use of Kittens:

sealed trait Tree
object Tree {
  implicit val showTree: Show[Tree] = cats.derived.semi.show
}
case class Node(nodes: List[Tree]) extends Tree
case class Leaf(label: String) extends Tree
// 👎Could not derive an instance of Show[A]
//knownDirectSubclasses of Tree observed before subclass Node registered
//knownDirectSubclasses of Tree observed before subclass Leaf registered

Placing the companion object below the subclasses solves the problem:

sealed trait Tree
case class Node(nodes: List[Tree]) extends Tree
case class Leaf(label: String) extends Tree
object Tree {
  implicit val showTree: Show[Tree] = cats.derived.semi.show
}
// 👍 

But the @Semi annotation only works on the case class itself:

sealed trait Tree
case class Node(nodes: List[Tree]) extends Tree
case class Leaf(label: String) extends Tree
@Semi(Show) object Tree
// 👎@Semi can only annotate class, got: ...

How about allowing @Semi to be placed on the companion?
Adding a bit of docs around this pitfall might also help.


P.s. The first two things I tried failed, but the following alternatives work fine:

case class Node(nodes: List[Tree]) extends Tree
case class Leaf(label: String) extends Tree
@Semi(Show) sealed trait Tree
@Semi(Show) sealed trait Tree
object Tree {
  case class Node(nodes: List[Tree]) extends Tree
  case class Leaf(label: String) extends Tree
}
sealed trait Tree
case class Node(nodes: List[Tree]) extends Tree
case class Leaf(label: String) extends Tree
@Semi(Show) object Tree
@MateuszKubuszok
Copy link
Member

Hello!

Sorry for the late answer but I am on my vacations and I don't hang around GH, emails and other work-related stuff too often.

I'd say that the problem's you have described are real, but also they are something I had I think with virtually every other macro annotation:

  • knownDirectSubclasses is very reliant on compilation order and I usually resolved it by shuffling the other of classes and companions until it works (and I mean here also other annotations like @JsonCodec from Circe)
  • I had issues in the past when something started/stopped working locally after reordering and it was completely different on CI or after clean because incremental compiler's cache was a mess.

As for the suggestion of adding ability to annotate companion object directly I could take a look at this, but it could be difficult. There is basically a reason why not so many macro annotations for adding implicits works this way:

  • when you add annotation on class, in macro I receive a list of ASTs:
    • if class has no companion, it is classAST :: Nil,
    • if class has companion it is either classAST :: companionAST :: Nil or companionAST :: classAST :: Nil
  • then I do pattern matching on it, and use class to extract class information (e.g. FQCN) and I use companion to inject implicits (or create it if it wasn't there)
  • but if I annotate object I receive only object's AST. And it's type is different that classes type. So I would probably have to:
    • turn object's type to name , e.g MyCompanion.type
    • remove .type suffix
    • typecheck this string (which would increase the cost of compilation)
    • if type is found then use it to generate macro

It is definitely doable, but would require some hairy logic and a lot of testing. For the next few weeks I will stay off any work-related stuff but if you are interested in making a pull request and working on it I could log-in once in a while to help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants