magnolia icon indicating copy to clipboard operation
magnolia copied to clipboard

Accumulate errors for all missing implicits

Open ahoy-jon opened this issue 7 years ago • 10 comments

When we use derivation on a user-defined case class, some type-class instances might be missing. Magnolia at the moment may only report on the first missing instance.

Is it possible to change the way errors are collected so we could see all the missing instances?

test("show error for multiple missing implicits") {
      scalac"""
        case class Alpha(dbl:Double, str:String)
        Show.gen[Alpha]
      """
    }.assert(_ == TypecheckError(
      txt"""magnolia: could not find Show.Typeclass for type Double
           |    in parameter 'dbl' of product type Alpha
           |magnolia: could not find Show.Typeclass for type String
           |    in paramater 'str' of product type String"""))

ahoy-jon avatar Sep 23 '18 21:09 ahoy-jon

It may be possible. I don't know if it's desirable.

Check how Deferred is used. In a similar fashion you could use a Failed dummy to collect all errors and report them at the end.

joroKr21 avatar Sep 29 '19 17:09 joroKr21

The approach outlined above might also simplify the error handling logic in the stack.

joroKr21 avatar Jan 13 '20 01:01 joroKr21

@joroKr21 I don't really understand how Deferred could help. Can you tell me a bit more?

ahoy-jon avatar Jan 15 '20 08:01 ahoy-jon

Whenever we need to have a recursive reference to a lazy val we put a Deferred("name of the val") instead and there is a final stage that replaces each Deferred with the actual value reference. So when get an error instead of aborting immediately we could put a Failure("error message") instead and then the final stage would collect all errors and display them to the user.

joroKr21 avatar Jan 15 '20 09:01 joroKr21

I don't think so, errors are already available, something like Validated instead of raising directly would work: https://typelevel.org/cats/datatypes/validated.html

test("show error for multiple missing implicits") {
      scalac"""
        case class Alpha(dbl:Double, str:String)
        Show.gen[Alpha]
      """
    }.assert(_ == TypecheckError(
      txt"""magnolia: could not find Show.Typeclass for type Double
           |    in parameter 'dbl' of product type Alpha
           |magnolia: could not find Show.Typeclass for type String
           |    in paramater 'str' of product type String"""))

Do you think the spec is valid?

ahoy-jon avatar Jan 15 '20 10:01 ahoy-jon

We can't use Validated. We can only do two things in a macro:

  1. Abort immediately on the first error
  2. Return a valid AST that typechecks

It might seem hacky but it's a common technique to stub out things with a marker method like:

def magnoliaError(message: String): Nothing = ???

And have the latest stage go over the entire AST and act on such marker methods.

joroKr21 avatar Jan 15 '20 10:01 joroKr21

I could make a PR on this issue then.

That being said, I will wear a mean hat 🎩 for the rest of the message.

I find the current situation being very annoying. I report an issue, with a spec compatible with the test suite. Your current line is :

It may be possible. I don't know if it's desirable.

I know it is possible, macros have been out since 2012, potential contributors (me or someone that find this issue valuable) need to know if it is desirable.

My question is again : Do you think the spec is valid?

If the spec is valid, we can work on the implementation. I will definitely ask for guidance if I am working on it.

If not, It would be nice to explain why it is not desirable and close the issue.

ahoy-jon avatar Jan 15 '20 10:01 ahoy-jon

Right, sorry for the confusion. Often I'm on the phone when answering on GitHub and my messages tend to be too terse and not explanatory.

So I think it's both possible and desirable, but we don't need to put too much weight on my every word. Keep in mind that when I wrote this reply I was not a collaborator so I didn't feel it was my place to say whether it is desirable.

About the spec you mean the error message you suggested, right?

We don't have to agree on a spec in advance, for me it's enough to say that you want to work on creating a PR. Since macros are a complex matter, even an incomplete PR would be progress.

For the error message itself I think it would be ideal if it would have some kind of tree structure like this:

magnolia: could not derive Show.Typeclass for type Alpha:
  - in parameter 'dbl' of product type Alpha
    - could not find Show.Typeclass for type Double
  - in paramater 'str' of product type String
    - could not find Show.Typeclass for type String

But the error message you suggested is also an improvement over the current state.

joroKr21 avatar Jan 15 '20 13:01 joroKr21

👍 ! The tree structure is better, so let's go for :

test("show error for multiple missing implicits") {
      scalac"""
        case class Alpha(dbl:Double, str:String)
        Show.gen[Alpha]
      """
    }.assert(_ == TypecheckError(
      txt"""magnolia: could not derive Show.Typeclass for type Alpha:
  - in parameter 'dbl' of product type Alpha
    - could not find Show.Typeclass for type Double
  - in paramater 'str' of product type String
    - could not find Show.Typeclass for type String"""))

ahoy-jon avatar Jan 15 '20 15:01 ahoy-jon

@ahoy-jon did get somewhere with this?

I would be interested to help you if you're stuck or try it myself if you haven't started yet.

joroKr21 avatar Feb 22 '20 11:02 joroKr21