Add error reporting for mirror synthesis #15164
Conversation
szymon-rd
changed the title
| -- Error: tests/neg/mirror-synthesis-errors.scala:24:38 ---------------------------------------------------------------- | ||
| 24 |val testSubD = summon[Mirror.Of[SubD]] // error: takes more than one parameter list | ||
| | ^ | ||
| |No given instance of type deriving.Mirror.Of[SubD] was found for parameter x of method summon in object Predef. Failed to synthesize an instance of type deriving.Mirror.Of[SubD]: class SubD is not a sealed class |
There was a problem hiding this comment.
I find this one interesting - here Synthesiser.synthesizedMirror first checks the isGenericProduct test, which it fails, then it fails the isGenericSum test in synthesizedSumMirror, but the final error only says it is not a "sealed class". I think for it could be good to prefix the error message with it is not a product because <reason> and it is not a sum because class SubD is not a sealed class
There was a problem hiding this comment.
SubD is a case class. In that case, I would expect a message saying “class SubD is a case class but it …” explaining what was wrong with it.
On the other hand, if a type is neither a case class nor a sealed trait, then I would expect a message saying that “SubD should be either a case class or a sealed trait”.
There was a problem hiding this comment.
we have a lot of more fine grained reasons why it can fail so I think its good to capture the errors from both branches
There was a problem hiding this comment.
I made it so it collects errors from both branches. The error for synthesis for SubD is:
No given instance of type deriving.Mirror.Of[SubD] was found for parameter x of method summon in object Predef. Failed to synthesize an instance of type deriving.Mirror.Of[SubD]:
* class SubD is not a generic sum because it is not a sealed class
* class SubD is not a generic product because it takes more than one parameter list
I also removed the condition from synthesizedMirror, but synthesizedProductMirror makes an early exit anyway, so I think it's cleaner that way.
There was a problem hiding this comment.
I think this is very nice!, one point is that orElse combinator reverses the order of the errors, I think it should keep the order they were generated in
| val reason = if !cls.isGenericProduct then | ||
| i"because ${cls.whyNotGenericProduct}" | ||
| else if !canAccessCtor(cls) then | ||
| i"because constructor of $cls is unnaccessible from the calling scope." |
There was a problem hiding this comment.
| i"because constructor of $cls is unnaccessible from the calling scope." | |
| i"because the constructor of $cls is inaccessible from the calling scope." |
| private def orElse(treeWithErrors1: TreeWithErrors, treeWithErrors2: => TreeWithErrors): TreeWithErrors = treeWithErrors1 match | ||
| case (tree, errors) if tree eq genericEmptyTree => | ||
| val (tree2, errors2) = treeWithErrors2 | ||
| (tree2, errors2 ::: errors) |
There was a problem hiding this comment.
currently the the final error generated will be at the front of the list, when I think each new error should go to the back
| (tree2, errors2 ::: errors) | |
| (tree2, errors ::: errors2) |
| @@ -2,5 +2,5 @@ | |||
| 8 |val baz = summon[Mirror.Of[SubA[Int] | SubB[Int]]] // error | |||
| | ^ | |||
| |No given instance of type deriving.Mirror.Of[SubA[Int] | SubB[Int]] was found for parameter x of method summon in object Predef. Failed to synthesize an instance of type deriving.Mirror.Of[SubA[Int] | SubB[Int]]: | |||
| | * class Cov is not a generic sum because it is not a sealed class | |||
| | * class Cov is not a generic product | |||
There was a problem hiding this comment.
this is another interesting one, to the user seeing this error, it might not be obvious why the error mentions Cov, and not SubA or SubB, I think this can wait for a new PR but perhaps we should explain why the union collapses to its common superclass, which is why we reject it.
|
@szymon-rd would you like to add a small bit in this section to show an example of the error if a mirror can't be synthesised? |
|
@bishabosha I added the docs section in the last commit, could you take a quick look at it? |
|
very nice stuff! |
Fixes #15128
Added reporting for errors on mirror synthesis and modified the Synthesizer so adding errors for synthesis of another terms is also straightforward. Modified the error messages.