Add Defer instances for Decoder and Encoder #2123

morgen-peschke · 2023-04-01T23:13:43Z

Implements #2122

zarthross · 2023-04-06T20:56:07Z

modules/tests/shared/src/test/scala/io/circe/DecoderSuite.scala

@@ -763,4 +764,6 @@ class DecoderSuite extends CirceMunitSuite with LargeNumberDecoderTestsMunit {
    assert(result.isInvalid)
    assertEquals(result.swap.toOption.map(_.size), Some(2))
  }
+
+  checkAll("Defer[Decoder]", DeferTests[Decoder].defer[MiniInt])


Can you add some property tests for recursive datastructures like what was used in the example? Just to verify further the stack safety and serve as an example for users on how to user defer?

I added non-property test, as generating recursive data structures are kind of annoying to write Gens for.

Not impossible though, so if you feel strongly that they need to be property tests, I can take a pass at converting them.

I think we need this test to have some depth to it. We need to have the test ensure the stack safety of the implementation of defer. Its probably fine to use List as the type the way you did, but using a generated list of some significant length should do the trick.

Clarification question: are you concerned about the stack safety of the Defer instance, or the Decoder it's used to write?

The former is tested by the Defer laws:

+ Defer[Decoder]: defer.defer Identity 0.156s + Defer[Decoder]: defer.defer does not evaluate 0.001s + Defer[Decoder]: defer.defer is stack safe 0.319s + Defer[Decoder]: defer.defer matches fix 0.429s

The later I think may be out of our control.

modules/core/shared/src/main/scala/io/circe/Decoder.scala

zmccoy · 2023-04-06T23:40:34Z

Could we add some documentation as well to the docs for this? You could easily transform what you have in the accommodating issue and it would be great for discoverability of this.

- Cache the resolution loop so we don't have to do it every time the instance is called. - Preserve short-circuit decoding behavior when not accumulating errors

zarthross · 2023-04-12T22:41:02Z

@morgen-peschke Thanks for adding the tests. It looks like this build failed MIMA for some reason, do you mind taking a look?

armanbilge · 2023-04-12T22:45:26Z

modules/core/shared/src/main/scala/io/circe/Decoder.scala

+  implicit final val decoderInstances
+    : SemigroupK[Decoder] with MonadError[Decoder, DecodingFailure] with Defer[Decoder] =
+    new SemigroupK[Decoder] with MonadError[Decoder, DecodingFailure] with Defer[Decoder] {


I think you'll need to do something like this to fix bincompat

Suggested change

implicit final val decoderInstances

: SemigroupK[Decoder] with MonadError[Decoder, DecodingFailure] with Defer[Decoder] =

new SemigroupK[Decoder] with MonadError[Decoder, DecodingFailure] with Defer[Decoder] {

@deprecated

def decoderInstances: MonadError[Decoder, DecodingFailure] = decoderInstancesBinCompat

implicit final val decoderInstancesBinCompat =

: SemigroupK[Decoder] with MonadError[Decoder, DecodingFailure] with Defer[Decoder] =

new SemigroupK[Decoder] with MonadError[Decoder, DecodingFailure] with Defer[Decoder] {

Alternatively, we could just make a separate val for the Defer couldn't we? Defer has no parents and no children so there shouldn't be any real need for it to be part of decoderInstances right?

Sure, good point!

Also, does the order matter on those with clauses? Maybe moving Defer[Decoder] to before MonadError[Decoder, DecodingFailure] instead of after?

I'm not sure. But I do know that Scala 2 and Scala 3 have different algorithms for this, so it may be impossible to reconcile in a way to make all versions happy. We had a similar issue in Cats (actually we accidentally broke bincompat on Scala 3 since weren't checking it back then ... )

Ref:

cats 2.7.0 broke binary compatibility for Scala 3 (because mima wasn't run) typelevel/cats#4062 (comment)

Hopefully this should fix the bincompat issue

hamnis · 2024-04-17T07:13:22Z

would you mind fixing the merge conflics here?

hamnis · 2024-05-14T11:17:09Z

failure is genuine: https://github.com/circe/circe/actions/runs/9078292566/job/24945019131?pr=2123#step:13:2785

johnynek · 2024-05-14T17:26:20Z

modules/tests/shared/src/test/scala/io/circe/DecoderSuite.scala

+    implicit def uglyListDecoder[A: Decoder]: Decoder[List[A]] =
+      Decoder.recursive[List[A]] { implicit recurse =>
+        Decoder.instance { c =>
+          (c.downField("car").as[A], c.downField("cdr").as[Option[List[A]]]).mapN(_ :: _.getOrElse(Nil))


I don't think this is stack safe, and it looks like the tests fail.

So, recurse is doing to be used at c.downField("cdr").as[Option[List[A]]] but that's nested inside of a mapN. While nesteding Defer is stack safe in the construction you have here, composing with map2, which is what is happening here, isn't. Something like: lazy val d = map2(a, defer(d))(_ :: _), map2 would need to somehow unroll that loop. It's not easy to see how that could happen.

So, I think the stack depth will be the same depth as the object graph.

cats-parse has the same issue on parsing (which is the same basic typeclass as Decoder).

If you ask me, we should merely add a comment to recursive and remove this test. Many structures have depth = log(size), so they are perfectly safe to parse with this. If you have a linear depth object (such as a linked list), this isn't suitable if the lists are big.

Something like this?

Note: while a Decoder written using Decoder.recursive can prevent unneeded creation of Decoder instances when recursing, the resulting Decoder cannot be guaranteed to be stack-safe.

yeah, something like that seems right.

… stack stafe

johnynek · 2024-05-14T23:37:53Z

modules/core/shared/src/main/scala/io/circe/Encoder.scala

+import java.time.ZoneOffset
+import java.time.ZonedDateTime
+import java.time.format.DateTimeFormatter
+import java.time.format.DateTimeFormatter.ISO_LOCAL_DATE_TIME


personally, I don't like these import reformats. I think grouped imports make it easier to see the package level dependencies and I would rather use { } to the maximum vs the minimum.

I don't have strong opinions on it, other than "let the tooling handle it" 😅

In this case, it was the result of running scalafixAll and scalafmtAll (then reverting the changes to the files I hadn't touched).

I can revert the changes manually if you find it particularly objectionable, but it might be worth revisiting the formatting config if that is the case 🤷🏻

Also, in my experience, unfolded imports tend to trigger less conflicts on merges and rebases. I guess this could one of the reasons why it was made a default behavior for the OrganizeImports rule in scalafix.

I needed to re-run the CI, so I've rolled back the import reformat

Add Defer instances for Decoder and Encoder

1f0ca13

morgen-peschke requested review from zarthross and zmccoy as code owners April 1, 2023 23:13

zarthross requested changes Apr 6, 2023

View reviewed changes

morgen-peschke added 3 commits April 12, 2023 09:45

Defer[Decoder/Encoder] performance improvements.

5a26637

- Cache the resolution loop so we don't have to do it every time the instance is called. - Preserve short-circuit decoding behavior when not accumulating errors

Add tests for Defer[Decoder/Encoder]

02ac799

Scalafmt catchup

3318fd2

armanbilge reviewed Apr 12, 2023

View reviewed changes

morgen-peschke added 4 commits April 13, 2023 11:26

Move Defer[Decoder] to a separat instance

a621f81

Hopefully this should fix the bincompat issue

Merge branch 'series/0.14.x' into provide-defer-instances

f91f14d

Add extra test for Decoder.recursive stack safety

70122c7

Add first pass of Defer.recursive docs

208e352

zarthross mentioned this pull request Aug 30, 2023

Improve derivation support for recursive ADTs #2187

Open

Merge branch 'series/0.14.x' into provide-defer-instances

19d2f0c

morgen-peschke requested a review from hamnis as a code owner May 14, 2024 10:40

formatting

3f8abcf

johnynek reviewed May 14, 2024

View reviewed changes

Remove test incorrectly asserting result of Decoder.recursive will be…

50657e8

… stack stafe

johnynek reviewed May 14, 2024

View reviewed changes

morgen-peschke added 3 commits May 30, 2024 16:03

Clean up mdoc errors

77b2736

Rollback changes to imports

eae979e

Merge branch 'series/0.14.x' into provide-defer-instances

11ec075

morgen-peschke requested a review from zarthross May 31, 2024 23:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Defer instances for Decoder and Encoder #2123

Add Defer instances for Decoder and Encoder #2123

morgen-peschke commented Apr 1, 2023

zarthross Apr 6, 2023

morgen-peschke Apr 12, 2023

zarthross Apr 12, 2023

morgen-peschke Apr 13, 2023

zmccoy commented Apr 6, 2023

zarthross commented Apr 12, 2023

armanbilge Apr 12, 2023 •

edited

zarthross Apr 12, 2023

armanbilge Apr 12, 2023

zarthross Apr 12, 2023

armanbilge Apr 12, 2023 •

edited

armanbilge Apr 13, 2023

hamnis commented Apr 17, 2024

hamnis commented May 14, 2024

johnynek May 14, 2024

johnynek May 14, 2024

morgen-peschke May 14, 2024

johnynek May 14, 2024

johnynek May 14, 2024

morgen-peschke May 15, 2024

morgen-peschke May 15, 2024

satorg May 29, 2024

morgen-peschke May 31, 2024

Add Defer instances for Decoder and Encoder #2123

Are you sure you want to change the base?

Add Defer instances for Decoder and Encoder #2123

Conversation

morgen-peschke commented Apr 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmccoy commented Apr 6, 2023

zarthross commented Apr 12, 2023

armanbilge Apr 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanbilge Apr 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamnis commented Apr 17, 2024

hamnis commented May 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanbilge Apr 12, 2023 •

edited

armanbilge Apr 12, 2023 •

edited