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

Pull semi-auto derivation into a trait, similar to auto derivation #220

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

dvgica
Copy link

@dvgica dvgica commented Mar 31, 2022

This is useful when you're trying to build an object that has a bunch of Circe config and implicits, such that you can use a single import to bring them all into scope. It avoids needing the extra io.circe.generic.extras.semiauto._ import.

Similar idea to https://tapir.softwaremill.com/en/latest/mytapir.html and consistent with AutoDerivation. I tried making semiauto into a package object for consistency with auto, but had a bit of trouble due to this line so left it as just an object.

Previous import usage will still work.

@dvgica
Copy link
Author

dvgica commented Jun 4, 2022

@zmccoy any thoughts here? Thanks!

@zarthross
Copy link
Member

@dvgica Sorry for the late review. The code itself looks fine, but we need to update with the latest from main, and fix the mima issues:

[error] genericExtras: Failed binary compatibility check against io.circe:circe-generic-extras_2.13:0.14.2 (e:info.versionScheme=early-semver)! Found 5 potential problems
[error]  * static method deriveConfiguredFor()io.circe.generic.extras.semiauto#DerivationHelper in class io.circe.generic.extras.semiauto has a different result type in current version, where it is io.circe.generic.extras.SemiAutoDerivation#DerivationHelper rather than io.circe.generic.extras.semiauto#DerivationHelper
[error]    filter with: ProblemFilters.exclude[IncompatibleResultTypeProblem]("io.circe.generic.extras.semiauto.deriveConfiguredFor")
[error]  * deprecated static method deriveFor()io.circe.generic.extras.semiauto#DerivationHelper in class io.circe.generic.extras.semiauto has a different result type in current version, where it is io.circe.generic.extras.SemiAutoDerivation#DerivationHelper rather than io.circe.generic.extras.semiauto#DerivationHelper
[error]    filter with: ProblemFilters.exclude[IncompatibleResultTypeProblem]("io.circe.generic.extras.semiauto.deriveFor")
[error]  * deprecated method deriveFor()io.circe.generic.extras.semiauto#DerivationHelper in object io.circe.generic.extras.semiauto has a different result type in current version, where it is io.circe.generic.extras.SemiAutoDerivation#DerivationHelper rather than io.circe.generic.extras.semiauto#DerivationHelper
[error]    filter with: ProblemFilters.exclude[IncompatibleResultTypeProblem]("io.circe.generic.extras.semiauto.deriveFor")
[error]  * method deriveConfiguredFor()io.circe.generic.extras.semiauto#DerivationHelper in object io.circe.generic.extras.semiauto has a different result type in current version, where it is io.circe.generic.extras.SemiAutoDerivation#DerivationHelper rather than io.circe.generic.extras.semiauto#DerivationHelper
[error]    filter with: ProblemFilters.exclude[IncompatibleResultTypeProblem]("io.circe.generic.extras.semiauto.deriveConfiguredFor")
[error]  * class io.circe.generic.extras.semiauto#DerivationHelper does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[MissingClassProblem]("io.circe.generic.extras.semiauto$DerivationHelper")
[error] stack trace is suppressed; run last genericExtrasJVM / mimaReportBinaryIssues for the full output
[error] (genericExtrasJVM / mimaReportBinaryIssues) Failed binary compatibility check against io.circe:circe-generic-extras_2.13:0.14.2 (e:info.versionScheme=early-semver)! Found 5 potential problems
[error] Total time: 0 s, completed Oct 21, 2022 6:27:54 PM

@dvgica
Copy link
Author

dvgica commented Oct 26, 2022

Thanks @zarthross. I merged main.

For the mima issues, do you mean just to filter them? I'm a little unfamiliar with mima but it seems like the issues it's flagging are maybe not legit. To the user, DerivationHelper can still be accessed as before, e.g.:

scala> new io.circe.generic.extras.semiauto.DerivationHelper
val res0: io.circe.generic.extras.semiauto.DerivationHelper[Nothing] = io.circe.generic.extras.SemiAutoDerivation$DerivationHelper@ef683ee6

I guess I'm not sure whether moving DerivationHelper actually changes its type in a meaningful or significant way, but I don't have a lot of expertise here.

@zarthross
Copy link
Member

Unfortunately, I don't have any reason to think Mima is wrong here... your change is 100% source compatible, but the way Scala tends to organize things into jvm bytecode differs sometimes depending on how the code is written. The real problem appears to be how DerivationHelper is moved from the object to the trait
io.circe.generic.extras.SemiAutoDerivation#DerivationHelper
io.circe.generic.extras.semiauto#DerivationHelper.

I'm not 100% sure what the best solution here is but,

  1. We could leave the affected methods and DerivationHelper in the object rather than moving them.
  2. Not mark the method and class as final and override them in the object.

@isomarcte Is also very good with binary compatibility issues and may have some alternative proposals.

*/
def deriveUnwrappedCodec[A](implicit codec: Lazy[UnwrappedCodec[A]]): Codec[A] = codec.value

final class DerivationHelper[A] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RE the bincompat issues.

If you define this class inside a non-static context, you are going to create a few issues. While it likely won't commonly come into play, every distinct subtype of trait SemiAutoDerivation will yield a distinct type DerivationHelper, because the type of type defined inside a class is scoped to the concrete instance of said class.

scala> :paste
// Entering paste mode (ctrl-D to finish)

trait Foo {
  case object Bar
}

object A extends Foo

object B extends Foo



// Exiting paste mode, now interpreting.

trait Foo
object A
object B

scala> implicitly[A.Bar.type =:= A.Bar.type]
val res3: A.Bar.type =:= A.Bar.type = generalized constraint

scala> implicitly[B.Bar.type =:= A.Bar.type]
                 ^
       error: Cannot prove that B.Bar.type =:= A.Bar.type.

=:= is how we can ask the compiler to prove type equivalence.

Also, with respect to the bincompat issues that @zarthross mentioned, many of them are caused by you moving this type into this trait.

For bincompat, you need to move DerivationHelper back into object semiauto.

My suggestion for this series would be to have this trait proxy to semiauto and then not have semiauto implement it. The in the next series, pull DerivationHelper into its own file, and reimplement this the way you have it now.

Sorry, bincompat stuff is always a pain. There are other much more verbose options we could do in this series, but I question their merit given this isn't a lot of code.

@zarthross @dvgica thoughts?

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

Successfully merging this pull request may close these issues.

3 participants