softwaremill / magnolia

Easy, fast, transparent generic derivation of typeclass instances
https://softwaremill.com/open-source/
Apache License 2.0
760 stars 116 forks source link

Accumulate errors for all missing implicits #140

Open ahoy-jon opened 5 years ago

ahoy-jon commented 5 years ago

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"""))
joroKr21 commented 4 years ago

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 commented 4 years ago

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

ahoy-jon commented 4 years ago

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

joroKr21 commented 4 years ago

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.

ahoy-jon commented 4 years ago

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?

joroKr21 commented 4 years ago

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.

ahoy-jon commented 4 years ago

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.

joroKr21 commented 4 years ago

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.

ahoy-jon commented 4 years ago

👍 ! 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"""))
joroKr21 commented 4 years ago

@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.