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

[DEMO] Circe Encoder/Decoder derivation issues #86

Closed
wants to merge 2 commits into from

Conversation

vpavkin
Copy link
Contributor

@vpavkin vpavkin commented Apr 10, 2018

This branch is a demo of prototypical circe encoder/decoder derivations.

It also contains tests for scenarios, where it's hard/impossible for magnolia to follow shapeless-based circe derivation semantics.
For each test case there is a separate issue (#87, #88, #89), this branch just serves playground purposes.

To run the tests:

tests/runMain CirceRecursiveTypeTest
tests/runMain IntermediateTraitsTest
tests/runMain PriorityIssueTest

@vpavkin
Copy link
Contributor Author

vpavkin commented Apr 10, 2018

@propensive If there's a better way I can set this up ergonomically (e.g. redirect the PR in some specially created branch) - just tell :)

@propensive
Copy link
Collaborator

Thanks, @vpavkin! I will take a look in about a week. :)

@vpavkin
Copy link
Contributor Author

vpavkin commented Jun 7, 2018

@propensive I created a full-blown repo to continue making magnolia and circe good friends :)

https://github.com/vpavkin/circe-magnolia

Is this PR of any help to you, or I can close it?

@propensive
Copy link
Collaborator

It's very useful, though I've been too short on time recently. (I'm a bit freer now, though...) I want to take a closer look at this issue because it's something I've seen sometimes in my own projects. Sorry for the delay... but I'm getting round to it!

@vpavkin
Copy link
Contributor Author

vpavkin commented Jun 8, 2018

Sure, no worries! Then just feel free to close it whenever it fits :)

@fommil
Copy link
Contributor

fommil commented Aug 3, 2018

btw #120 should help with circe decoding

@propensive
Copy link
Collaborator

What was the status of these tests before @fommil made his changes? They seem to be passing now, apart from one which shouldn't...

@vpavkin
Copy link
Contributor Author

vpavkin commented Aug 4, 2018

@fommil cool, thanks! @propensive I'll check everything out in the following days, but you can close this at any moment, since I already have a separate repo.
If you happen to be able to publish a new jar with all these fixes, that would be just perfect!

@propensive
Copy link
Collaborator

Thanks, @vpavkin! I will wait for @fommil's current flurry of activity to come to an end, then publish a new version, so hopefully that will be out there in the next couple of days. But I will leave this open as a reminder for myself. :)

@propensive
Copy link
Collaborator

@vpavkin Version 0.9.1 has been out for a couple of days, so it might be worth checking if that fixes your issues. If it does, I can close this.

@Igosuki
Copy link

Igosuki commented May 7, 2019

@propensive It doesn't, I'm using 0.9.1 on an API and the encoder of immutable.List still encodes the constructor (::) instead of picking up the List encoder already in scope.

@Igosuki
Copy link

Igosuki commented May 7, 2019

I think magnolia solves all our scala compilation time problems with the typer so if we can help fix this we'll gladly do it.

@vpavkin
Copy link
Contributor Author

vpavkin commented May 7, 2019

@Igosuki try 0.10.0, I think it was fixed there

@Igosuki
Copy link

Igosuki commented May 7, 2019

@vpavkin Using circe 0.11.1, magnolia 0.10.0, circe-magnolia 0.4.0, mercator 0.1.1 I'm still facing problems (I also checked that none of these dependencies are evicted due to conflicts).

Empty immutable lists still get encoded with { Nil } and lists with { "::": [ ] } (since the default configuration uses the resolved constructor name).

@Igosuki
Copy link

Igosuki commented May 7, 2019

Encoders for recursive types probably get properly generated, however circe implicits for certain types don't get resolved properly during macro generation such as immutable.List

@Igosuki
Copy link

Igosuki commented May 7, 2019

I'm trying to go from your CirceRecursiveTypeTest to create another failing test

@Igosuki
Copy link

Igosuki commented May 7, 2019

This test : Igosuki@648ab74#diff-017018123cf09df13d64d6c1e428e923R35
fails if io.circe.Encoder._ isn't imported in scope last, which is totally awkward (or even crashes compilation) when you're already importing these :

import io.circe.magnolia.derivation.decoder.auto._
import io.circe.magnolia.derivation.encoder.auto._

As vpavkin/circe-magnolia#2 describes, bringing io.circe.Encoder._ in scope solves the problem of priority resolution where magnolia needs to be improved but it's rather cumbersome and may cause other problems when expanding macros.

And as expected, in my endpoint's code if I import io.circe.Encoder._ I get diverging implicit expansions conflicts on some types so it's a snake biting its tail.

@fommil
Copy link
Contributor

fommil commented May 7, 2019

Shameless plug: my book has en entire chapter (and accompanying code) with a demo for doing JSON derivation with Magnolia. It may serve as inspiration. https://leanpub.com/fpmortals

@Igosuki
Copy link

Igosuki commented May 7, 2019

Ok my divergence problem are due to expansion of self types with http4s.Rho.

@plokhotnyuk
Copy link

@Igosuki yet another option is to use jsoniter-scala instead of circe.

It derives safe and efficient codecs for for primitives, boxed primitives, enums, tuples, String, BigInt, BigDecimal, Option, Either, java.util.UUID, java.time.* (to/from ISO-8601 representation only), Scala collections, arrays, module classes, value classes, sealed traits (coproducts) and case classes (products) with values/fields having any of types listed here

@Igosuki
Copy link

Igosuki commented May 7, 2019

@plokhotnyuk Thanks, I saw your repo, looks great, I'll try it.
Still, I'd like to solve the problem with implicit scope prioritization in Magnolia if possible since circe is used in most FP stacks nowadays.

@joroKr21
Copy link
Contributor

joroKr21 commented May 7, 2019

I think there is a lot of confusion floating around. If you import an implicit (including an implicit macro, which delegates to Magnolia), it will take priority over implicits which are not imported. That's just how regular Scala works.

You can check how we solve this problem in kittens for reference:
https://github.com/typelevel/kittens/blob/master/core/src/main/scala/cats/derived/package.scala

We first assert that there is no Tc[A] in scope and then trigger the derivation which returns a subclass (MkTc[A]) in order to avoid polluting the implicit scope of Tc.

@Igosuki
Copy link

Igosuki commented May 9, 2019

Thanks

@vpavkin vpavkin closed this Jan 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants