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-less Scala 3 #321

Closed
wants to merge 1 commit into from
Closed

Mirror-less Scala 3 #321

wants to merge 1 commit into from

Conversation

KacperFKorban
Copy link
Collaborator

Draft of mirror-less implementation for Scala 3. Probably most of the code can be simplified a lot. But should be a good start for testing if recursive types can be handled properly this way.
Related to #310

@adamw
Copy link
Member

adamw commented Jun 28, 2021

Thanks!

I think it might turn out recursive types work as-is, but maybe we could support value classes and non-trivial sealed trait hierarchies, for which mirrors are currently not derived.

src/core/macro.scala Outdated Show resolved Hide resolved
src/core/magnolia.scala Outdated Show resolved Hide resolved
src/core/magnolia.scala Outdated Show resolved Hide resolved
@adamw
Copy link
Member

adamw commented Jul 13, 2021

FYI the base branch is now scala3

@KacperFKorban
Copy link
Collaborator Author

Ok, so this branch should now pass at least as many tests as the scala3 branch does.
So apart from the compatibility issues and overall being rough around the edges, it works.

@adamw will you be able to find some time to take a look at the changes and give some feedback? It would be great to test it on some real-world libraries that use magnolia - tapir?

@adamw
Copy link
Member

adamw commented Aug 29, 2022

So one thing that breaks is calling magnolia from another macro: Expr.summon[Schema[T]] returns a None.

Even in a simple case:

    case class Test1(f1: Int)
    implicitly[Schema[Test1]]
    val codec = implicitly[MultipartCodec[Test1]].codec

the implicitly[Schema[Test1]] works, while the same lookup done as part of the multipart codec derivation macro fails

@KacperFKorban
Copy link
Collaborator Author

But is it something that we actually care about? if Expr.summon[Schema[T]] returns None then we just use the same implicit method (Typeclass.derived) but by hand this time.
The only thing I can think of that may be affected here is the inline depth. Am I missing something?

@adamw
Copy link
Member

adamw commented Aug 29, 2022

Not sure I understand - Expr.summon used to work in the old implementation, returning a Some. It stopped with the new one (no idea why). Where would I use Typeclass.derived?

@adamw
Copy link
Member

adamw commented Aug 29, 2022

This is how to reproduce, inside magnolia/test:

// test1.scala
package magnolia1.tests

import magnolia1.examples._

case class Data(value: String)

object Test1 {
  summon[Show[String, Data]]
  Test2.summonShow[Data]
}

// test2.scala
package magnolia1.tests

import magnolia1.examples._
import scala.quoted.*

object Test2 {
  inline def summonShow[T]: Show[String, T] = ${ summonShowImpl[T] }

  def summonShowImpl[T: Type](using Quotes): Expr[Show[String, T]] = {
    import quotes.reflect.*

    Expr.summon[Show[String, T]].getOrElse(report.throwError(s"Cannot find a Show for: ${Type.show[T]}."))
  }
}

The direct summon works, the indirect one - fails.

@OlegYch
Copy link
Contributor

OlegYch commented Aug 29, 2022

there is a problem with current impl which i hope can be addressed as well with the rewrite
here is a showcase #421
basically, since subtypes contains only types directly inheriting root type, all of the logic relying on subtypes and choose breaks when you introduce non-leaf subtypes
as far as i can see the rewrite also suffers from this right now
can something be done about it?

@OlegYch
Copy link
Contributor

OlegYch commented Aug 29, 2022

actually the problem seems to be easy to fix with old implementation, i submitted a pr

@KacperFKorban
Copy link
Collaborator Author

@OlegYch You obviously can flatten the hierarchy in this rewrite too, but I'm not sure if that's what we want. Don't we lose some information, when we flatten the type hierarchy like that?

I'm pretty sure that there's a way to implement DecoderSafe derivation in a way that will work for those hierarchies. (choosing the instance that returns Right)

@OlegYch
Copy link
Contributor

OlegYch commented Aug 30, 2022

@KacperFKorban i thought so too, but it appears to be no way to make choose work, or enumerate all subtypes
trying out all instances will not work since multiple instances may succeed

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

Successfully merging this pull request may close these issues.

3 participants