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

Ability to not use auto-derivation #166

Closed
pjrt opened this issue May 4, 2020 · 5 comments · Fixed by #357
Closed

Ability to not use auto-derivation #166

pjrt opened this issue May 4, 2020 · 5 comments · Fixed by #357
Milestone

Comments

@pjrt
Copy link

pjrt commented May 4, 2020

Auto derivation, though convenient, can cause increases in compile time (since the macro runs on a per-call-site basis), as well as make implicit instances of Transformers placed in the companion object of a type useless (causes an implicit resolution conflict).

A lot of libraries (like Pureconfig and Scanamo) have moved away from auto derivation because of it, and instead allowing the user to call a semi and auto package, which they can use as they please (semi usually used in the companion of a type).

It would be great if Chimney followed this pattern as well.

@mrobakowski
Copy link

mrobakowski commented Dec 1, 2020

I don't know if I should open an additional issue for that, but since I believe this issue is very related, I'll just add a comment here. Transformers in companion objects sometimes work in some version of chimney, which is very annoying. For example this works in 0.5.3, but doesn't work in 0.6.0.

case class Id(value: String)
object Id {
  implicit val idToLong: TransformerF[Option[+*], Id, Long] = id => Either.catchOnly[NumberFormatException](id.value.toLong).toOption
}

case class Foo(id: Id)
case class Bar(id: Long)

val foo = Foo(Id("1"))
val bar = foo.transformIntoF[Option[+*], Bar]

https://scastie.scala-lang.org/sMQz9zqzTemTMwJgeJNwWw

Curiously, if we try to transformF an Id into Long, it doesn't work in both versions. It's a bit of a mess 😂

@Dr-Nikson
Copy link

Any updates on it?
With auto-derivation it's almost impossible to catch bugs at compile time, it feels like implicit type conversions :(

@MateuszKubuszok
Copy link
Member

MateuszKubuszok commented Aug 29, 2023

I am willing to fix it, but none of the obvious solutions I find to be satisfying.

The problem is that Chimney attempts optimizations no other library (besides Jsoniter) attempts to do and that it allows customizations that no other derivation-based library has. What do I mean by that?

Let's say we have:

case class InnerFrom(a: Int)
case class OuterFrom(inner: InnerFrom)

case class InnerTo(a: String)
case class OuterTo(inner: InnerTo)

implicit val intToString: Transformer[Int, String] = _.toString

Let's say Chimney behaves the same way as any other way as any other library, e.g. Circe, when it comes to automatic derivation. Then when we'd do:

OuterFrom(InnerFrom(10)).transformInto[OuterTo]

compiler would generate something like:

OuterFrom(InnerFrom(10)).transformInto[OuterTo]({
  val anon1 = new Transformer[InnerFrom, InnerTo] {
    def transform(src: InnerFrom): InnerTo = new InnerTo(
      intToString.transform(src)
    )
  }

  val anon2 = new Transformer[OuterFrom, OuterTo] {
    def transform(src: OuterFrom): OuterTo = new OuterTo(
      anon1.transform(src.inner)
    )  
  }
})

But that's not what Chimney generates. For:

OuterFrom(InnerFrom(10)).transformInto[OuterTo]

and

OuterFrom(InnerFrom(10)).into[OuterTo].transform

it generates respectively:

// notice only 1, outermost, Transformer
OuterFrom(InnerFrom(10)).transformInto[OuterTo](
  new Transformer[OuterFrom, OuterTo] {
    def transformer(src: OuterFrom): OuterTo = new OuterTo(
      new InnerTo(intToString.transform(src.inner.a))
    ) 
  }
)

and

{
  // notice 0 transformers, only the transforming expression
  val outerfrom = OuterFrom(InnerFrom(10))
  new OuterTo(
    new InnerTo(intToString.transform(outerfrom.inner.a))
  )
}

which drastically limits amount of code generated and allocations. This matter a lot for people who use Chimney in large data pipelines (many of our users are Spark developers).

But putting performance aside, when the transformation expression is generated all the settings provided through enableJavaBeanSetters, enableJabaBeanGetters, enableMethodAccessors (and in the future withFieldConst(_.foo.bar.baz, value)) have to be propagated recurively. If they aren't it is not only a matter of optimization but also a matter of lost correctness and unexpected behavior.

We could assume that user might use an implicit to override derivation, but we also cannot easily distinguish between user-provided implicits and autoderived ones (it makes sense for libraries like Circe where there is no customization, or where all customization can be handled by a single, shared implicit or annotation, but that design would not suit Chimney's use cases). Prior to 0.8.0-M1 we used to always summon implicit and check if it was created by autoderivation, and reject if it was (so that we could derive the expression without a type-class wrapper AND be sure that all overrides were propagated) but that resulted in these infamous long compile times (basically each branching - on field or subtype - could trigger a whole macro derivation which was then discarded).

Temporarily (0.8.0-M1), we addressed that by creating 2 types e.g. Transformer (for users) and Transformer.Autoderived where the former is intended for users to provide their own logic OR to be returned by Transformer.derive/Transformer.define.buildTransformer, and the later is only used in .transformInto if there is no other Transformer provided.

But even if we used 2 different methods to summon either only Transformer (safer) or Transformer/Transformer.Autoderived, that is still not a separation between auto and semiauto. There is still an issue that for some people recursive derivation in macros, always enabled, would be considered autoderivation. Which means that semiauto vs auto is no longer a matter of a simple import but also a matter of changing the behavior of macros, to disable the recursive derivation in macros by default and re-enable it with a flag.

The problem that I have with that is at this point the library would become safer to 10% of users by becoming useless to the remaining 90%. What is the suggested approach to recursive derivation in e.g. Circe?

val encoder = {
  import auto.*
  deriveEncoder 
}

If you used that approach in Chimney (remove Transformer vs Transformer.Autoderived) you'd end up with a suboptimal code (since everything would be based on summone implicits -> lost opportunity to inline) which could additionally ignore your overrides, so it's neither safe nor fast. As much as I want to find a way to make Chimney safer for users I strongly believe that, due to the difference in use cases, simply copy-pasting solutions from other libraries could break it rather than improve it.

Additionally:

With auto-derivation it's almost impossible to catch bugs at compile time, it feels like implicit type conversions :(

I saw similar sentiment on Reddit when someone reasoned that if they have to write unit tests for Chimney, why use it?

I find it to be a backward reasoning - Chimney's goal is to reduce manually writing stupid code. It has never attempted to be some compile-time-correctness prover. I write unit tests for any code that uses Chimney, as if it didn't use it, because I used Chimney to have less code to maintain, rather than to write less unit tests.

For the same reason I am not assuming that I don't have to test for Circe codecs, or code using Cats type classes - each of these derivations compiling proves merely that there exist one example of an implementation which would turn one value into another. But there might be multiple possible such implementation and without runtime tests you cannot claim that the obtained instance does what you think it should do. Perhaps if you used Refined/newtype for every single value which is not case class not enum you could have ~90% of certainty but it still would not be 100%. So I am always writing tests for code using type classes derived by Cats, JSON codecs derived by Circe/Jsoniter/Play JSON, and business logic with dumbest parts were derived by Chimney.

Summarizing, as much as I intend to make Chimney less error prone, and I would happily hear some suggestions on how to do it, I do not believe that "removing the need for writing unit tests" was ever in the scope of this library. I am open though to some other design suggestions which could address the same problems that solves semiauto-auto-separations in other libraries (and I am also curious if solutions like this #328 would be acceptable, as it simply creates a separate extension method which only uses Transformers explicitly imported/defined in Scope).

@MateuszKubuszok
Copy link
Member

Some additional note to the above:

What are the uses cases for automatic and semiautomatic?

  • automatic
    • yolo derivation, "fast and dirty" solution, often for a single usage, non-reused piece of code
      import auto.*
      implicitly[TypeClass[A]] // recursive derivation
    • usually doesn't work with recursive data types, they would require
      implicit val typeclass: TypeClass[A] = implicitly[TypeClass[A]]
      to allow using typeclass inside derivation... but which results in a circular dependency on initialization
  • semiautomatic
    • allow working with recursion
      // allow using typeclass somewhere inside but is defined in such a way
      // that TypeClass[A] is not summoned as semiTypeClass[A] directly
      //  - either it summons some wrapper (different type!) and unwraps it
      //  - or it uses macro which remembers to not summon itself (like Chimney does)
      implicit val typeclass: TypeClass[A] = semiTypeClass[A]
    • usually used to derive something only once and make it easier to ensure that the same (tested) instance is used everywhere
      class A
      object A {
        implicit val typeclass = semiTypeClass[A]
      }

However if we look at the use cases and what Chimney does already we'll see that:

  • Transformer.derive[From, To] or Transformer.define[From, To].buildTransformer already act like a semiautomatic derivation
  • source.into[Target].transform tries to inline result - if it won't find an implicit Transformer, it will attempt to derive inlined expression
  • only source.transformInto[Target] will require an actual implicit - it's type is defined in such a way that when there is no user-defined Transformer it will use Transformer.AutoDerived which is derived automatically

It is VERY important to understand that there is a difference (that we should document) between foo.into[Bar].transform and foo.transformInto[Bar]:

  • into.transform summons always summons implicit:

    foo.transformInto[Bar] // is the same as
    foo.transformInto(implicitly[Transformer.AutoDerived[Foo, Bar]])
  • meanwhile

    foo.into[Bar].transform // calls macro which could be expanded to e.g.
    new Bar(baz = foo.baz) // if there is no user-provided implicit it generates
                           // INLINED expression with no typeclasses instantiated

To support all cases tackled by auto-vs-semi - but not implemented in the same way as other libraries (for reasons explained above) we need to solve 2 problems:

  • how to let transform Source into Target using only Transformer existing in scope - one way would be to create a separate method like transformIntoSafe
  • how to allow mixing automatic derivation with user-provided implicits which also use Transformers
    implicit def something[A, B](implicit t: Transformer[A, B]): Transformer[Type[A], Type[B]] = ...
    typeA.transformInto[Type[B]]
    // ^ will not work, automatically derived Transformer has a signature Transformer.AutoDerived

Let's assume that we can break compatibility (and performance) for everyone. How could we design the API to make closer to other libraries?

import io.scalaland.chimney.syntax.*

implicit val transformer = Transformer.derive[Foo, Bar] // no change here - value derived

foo.transformInto[Bar] // also, no change here - value transformed with implicit

foo.transformInto[Baz] // should NOT compile - no implicit in scope

locally {
  io.scalaland.chimney.auto.*
  foo.transformInto[Baz] // should compile - implicit derived with auto
}

foo.into[Bar].transform // what about this? what has a precedence: implicit or config?

foo.into[Baz].transform // no implicit - but currently this syntax always:
                        //   - look for implicit
                        //   - fallback on deriving inlined expression
                        // should it not derive expression unless auto is imported?

This should show that even with breaking change we have some issues. To make this syntax work with current codebase we would have to:

  • move implicit def deriving to this auto module but also
  • create a flag allowDerivation inside macros which would
    • control whether or not implicit summoning is the only allowed thing to do in a macro
    • be disabled in into.transform if import auto.* is absent
    • be always on when Transformer.define would be used or for summoned implicits when auto is enabled

This makes it even harder to reason what the library does. So, for now issue still has no satisfying solution.

@MateuszKubuszok
Copy link
Member

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