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

Improve derivation support for recursive ADTs #2187

Open
wants to merge 4 commits into
base: series/0.14.x
Choose a base branch
from

Conversation

MartinHH
Copy link
Contributor

Fixes #2101

@codecov-commenter
Copy link

codecov-commenter commented Aug 23, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.03% 🎉

Comparison is base (914d1d6) 84.84% compared to head (cfa9535) 84.87%.
Report is 2 commits behind head on series/0.14.x.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the GitHub App Integration for your organization. Read more.

Additional details and impacted files
@@                Coverage Diff                @@
##           series/0.14.x    #2187      +/-   ##
=================================================
+ Coverage          84.84%   84.87%   +0.03%     
=================================================
  Files                 76       76              
  Lines               2731     2731              
  Branches             371      374       +3     
=================================================
+ Hits                2317     2318       +1     
+ Misses               414      413       -1     
Flag Coverage Δ
2.12.18 84.59% <ø> (-0.12%) ⬇️
2.13.11 84.92% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zarthross
zarthross previously approved these changes Aug 30, 2023
zmccoy
zmccoy previously approved these changes Aug 30, 2023
@zarthross zarthross dismissed stale reviews from zmccoy and themself via 0b00646 August 30, 2023 22:52
@zarthross
Copy link
Member

@MartinHH, Looks like even if we get the compiler to generate these codecs at compile time, we will have a runtime issue with trees of sufficient size blowing up the stack. I pushed some test changes to limit the test size so we can address this later, but on my machine i could upwards of 10,000 depth fine... in CI that number is much smaller, making me somewhat concerned about the real world limits.

As it stands right now, users will be forced to do something other than derivation, which i think is better than merging this right now and possibly getting a stack overflow at runtime.

If we still wanted to get this in, we need to change the way the encoder/decoder works to be more like Defer/Fix #2123 or some other trick to trampoline on the stack rather than blow it up.

@MartinHH
Copy link
Contributor Author

MartinHH commented Sep 2, 2023

As it stands right now, users will be forced to do something other than derivation, which i think is better than merging this right now and possibly getting a stack overflow at runtime.

Not trying to talk you into merging this (I really don't mind as I wasn't planning to use this), but it might be worth pointing out that the following already compiles (resulting in a similarly unsafe codec):

sealed trait Tree derives Codec.AsObject
case class Branch(l: Tree, r: Tree) extends Tree
case object Leaf extends Tree

This as well:

sealed trait Tree
case class Branch(l: Tree, r: Tree) extends Tree
case object Leaf extends Tree
given Codec[Tree] = Codec.AsObject.derived[Tree]

@Maurycyt
Copy link

Maurycyt commented Oct 27, 2023

On the one hand, I cannot wait for this aspect of circe in Scala 3 to be improved (and I am saddened by the lack of activity for almost 2 months), but I think this is the right place to point out a weird peculiarity which may be worth considering before merging anything.

@MartinHH mentioned, that derivation for his Tree structure compiles. While Martin is right, that is the following compiles:

import io.circe.Codec

sealed trait Tree
case class Branch(l: Tree, r: Tree) extends Tree
case object Leaf extends Tree
given Codec[Tree] = Codec.AsObject.derived[Tree]

the following doesn't:

import io.circe.Codec

sealed trait Tree
case class Branch(l: Tree, r: Tree) extends Tree
case object Leaf extends Tree
case class TreeWrapper(tree: Tree)
given Codec[TreeWrapper] = Codec.AsObject.derived[TreeWrapper]

The latter piece of code fails to compile with the infamous Maximal number of successive inlines (32) exceeded error. And the only thing that changed was moving from deriving for a recursive type to deriving for a type which contained a recursive type.

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.

[Bug] Recursive ADTs fail to Derive
5 participants