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

Missing features for Scala 3 #296

Open
1 of 5 tasks
mbore opened this issue May 19, 2021 · 15 comments
Open
1 of 5 tasks

Missing features for Scala 3 #296

mbore opened this issue May 19, 2021 · 15 comments
Labels

Comments

@mbore
Copy link
Contributor

mbore commented May 19, 2021

@mbore mbore added the scala3 label May 19, 2021
@KacperFKorban
Copy link
Collaborator

Hi, could You elaborate on support for recursive types and accessing annotations?

@mbore
Copy link
Contributor Author

mbore commented May 19, 2021

Hi, sure:

  • support for recursive types
    I should actually call it recursive class. I see that, there are already defined tests for them (e.g. GPerson/RPerson and Recursive) but it seems that, for the following test case, the derivation doesn't work.
    sealed trait Node
    case class Edge(id: Long, source: Node) extends Node
    case class SimpleNode(id: Long) extends Node

    test("rerucsive class") {
      val res = summon[Show[String, Node]].show(Edge(1, SimpleNode(2)))
      println(res)
    }
  • accessing annotations
    I see, annotations are already supported, but the following test case from legacy branch doesn't work
    case class MyAnnotation(order: Int) extends StaticAnnotation
    case class MyTypeAnnotation(order: Int) extends StaticAnnotation
    
    sealed trait AttributeParent
    @MyAnnotation(0) case class Attributed(
      @MyAnnotation(1) p1: String @MyTypeAnnotation(0),
      @MyAnnotation(2) p2: Int @MyTypeAnnotation(1)
    ) extends AttributeParent @MyTypeAnnotation(2)

    test("capture attributes against params") {
      val res = summon[Show[String, Attributed]].show(Attributed("xyz", 100))
      assertEquals(res, "Attributed{MyAnnotation(0)}{MyTypeAnnotation(2)}(p1{MyAnnotation(1)}{MyTypeAnnotation(0)}=xyz,p2{MyAnnotation(2)}{MyTypeAnnotation(1)}=100)")
    }

diff:

=> Diff (- obtained, + expected)
-Attributed{MyAnnotation(0)}(p1{MyAnnotation(1)}{MyTypeAnnotation(0)}=xyz,p2{MyAnnotation(2)}{MyTypeAnnotation(1)}=100)
+Attributed{MyAnnotation(0)}{MyTypeAnnotation(2)}(p1{MyAnnotation(1)}{MyTypeAnnotation(0)}=xyz,p2{MyAnnotation(2)}{MyTypeAnnotation(1)}=100)

@KacperFKorban
Copy link
Collaborator

Hi, recursive types are indeed supported. See tests for Tree here.
Your snippet with a small tweak works:

sealed trait Node
case class Edge(id: Long, source: Node) extends Node
case class SimpleNode(id: Long) extends Node

object Node:
  given Show[String, Node] = Show.derived

test("rerucsive class") {
  val res = summon[Show[String, Node]].show(Edge(1, SimpleNode(2)))
  println(res)
}

In order for recursive structures to work with magnolia for Scala 3, an explicit given is needed.
Fortunately for enums and type classes with single parameters, it can be neatly expressed with just derives clauses. Just like in the readme. So using enums is the preferred way of defining those structures in Scala 3.
I think that it's more of a documentation issue. I'll try to make it more explicit in readme.

@adamw
Copy link
Member

adamw commented May 26, 2021

Re: recursive types - I think we also need to have them supported if the given is defined externally, not on the companion object. This is needed so that we can work with externally defined types.

E.g. I think typeclasses for List should be derivable given given [T] Show[T] = Show.derived

@KacperFKorban
Copy link
Collaborator

RE: a simple way to pass configuration - since Scala 3 adds Trait Parameters wouldn't it be enough to just add a configuration parameter to the derivation trait?

@joroKr21
Copy link
Contributor

joroKr21 commented May 26, 2021

RE: a simple way to pass configuration - since Scala 3 adds Trait Parameters wouldn't it be enough to just add a configuration parameter to the derivation trait?

That could work, or it could be a method that you can override (depending on ergonomics). It would have to be inline.
Edit: In fact it would have to be a method, because trait/class parameters cannot be inline.

@adamw
Copy link
Member

adamw commented May 27, 2021

@joroKr21 @KacperFKorban Maybe I'm misunderstanding your proposal, but once the companion object extends the Derivation trait, the parameters need to be fixed. Here, we are looking at parametrising the derivation for specific .derived invocations.

I think we wanted to try a two-level solution, where the .derived method is available via an extension which requires the implicit configuration; the extension then implements the Derivation trait and has access to the parameter. But not sure if it's done in tapir, @mbore ?

@joroKr21
Copy link
Contributor

Maybe I'm misunderstanding your proposal, but once the companion object extends the Derivation trait, the parameters need to be fixed.

Yeah I thought that's what we wanted (to configure the derivation itself).

I think we wanted to try a two-level solution, where the .derived method is available via an extension which requires the implicit configuration; the extension then implements the Derivation trait and has access to the parameter.

So you mean passing runtime configuration, not compile-time? I guess that is another axis of configuration but it would have to be customisable.

@adamw
Copy link
Member

adamw commented May 27, 2021

So you mean passing runtime configuration, not compile-time? I guess that is another axis of configuration but it would have to be customisable.

Yes, e.g. in tapir when deriving the Schema, we pass in configuration which specifies how field names should be transformed - this can be from camelCase to snake_case etc.

@mbore
Copy link
Contributor Author

mbore commented May 27, 2021

I think we wanted to try a two-level solution, where the .derived method is available via an extension which requires the implicit configuration; the extension then implements the Derivation trait and has access to the parameter. But not sure if it's done in tapir, @mbore ?

Yes, you right, but we haven't implemented it yet.

@kpodsiad
Copy link

kpodsiad commented Sep 6, 2022

Can someone elaborate about support for values classes? What differs from Scala 2, what needs to done to support it, etc.

@KacperFKorban
Copy link
Collaborator

The problem is that value classes don't have generated Mirrors in Scala 3. I'm not sure if there is anything to be done here. If I remember correctly, the problem was that one of the generated methods conflicted after erasure.

The most obvious fix would be finishing #321, though I'm not sure how to tackle some of the new problems posted there.

@kpodsiad
Copy link

kpodsiad commented Sep 6, 2022

Thanks for answer @KacperFKorban.

Is mirror-less approach needed? I wonder if something similar to this
can be done here. It's a hybrid approach that piggybacks on mirrors when they can be generated and provides necessary information for other types for which there are no mirrors, like value classes.

@adamw
Copy link
Member

adamw commented Sep 13, 2022

@kpodsiad that could probably work :) maybe you'd like to try implementing this in magnolia?

@adamw
Copy link
Member

adamw commented Dec 13, 2022

Partial support for value classes (case classes, without type parameters) has been added in #435

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

No branches or pull requests

5 participants