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

Use macros to fix stack overflow when deriving for local classes #2287

Merged
merged 3 commits into from
Sep 5, 2024

Conversation

mrdziuban
Copy link
Contributor

Fixes #2263
Supersedes and closes #2278 if merged

@hamnis this is an alternate approach to what I did in #2278. By using macros for derivation we can match on the Mirror and extract the MirroredLabel, MirroredElemLabels, and MirroredElemTypes types without causing a stack overflow.

There are two caveats (the tests will fail if either of these things change in the future):

  1. Any methods that call the macros must mark their Mirror.Of as inline
  2. Any parameters derived from the Mirror in the macro must be passed by-name, e.g. fromProduct in ConfiguredDecoder.ofProduct and ConfiguredEncoder.ofSum

hamnis
hamnis previously approved these changes Jul 2, 2024
Copy link
Collaborator

@hamnis hamnis left a comment

Choose a reason for hiding this comment

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

I found this easier to understand

@hamnis
Copy link
Collaborator

hamnis commented Jul 2, 2024

Letting @zarthross decide to merge

import io.circe.{ Codec, Decoder, Encoder, HCursor, JsonObject }

trait ConfiguredCodec[A] extends Codec.AsObject[A], ConfiguredDecoder[A], ConfiguredEncoder[A]
object ConfiguredCodec:
private def of[A](nme: String, decoders: => List[Decoder[?]], encoders: => List[Encoder[?]], labels: List[String])(
Copy link
Member

Choose a reason for hiding this comment

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

I would love to be shown wrong on this, but removing this method concerns me.

I know that when you use inline on a private method, (the original derived below calls this of), that there is a public accessor created, in this case it should be inner$of.

If we have some library, lets call if A that used theses macros, and some end user app called B.
What happens if circe is updated to ofProduct, built a built with of... and B updates the version of circe to no longer include inner#of? I feel like we will get a NoSuchMethodError in application B?

I'm worried we'll have a similar problem to #2089

Copy link
Member

Choose a reason for hiding this comment

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

Not sure why mima doesn't catch this situation....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow I didn't know about that behavior but you're definitely right, I confirmed in a fresh project. I'll put the of methods back

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh wait actually, just putting of back doesn't fix the issue. The inline$of method only exists in the bytecode if of is called from an inline method, so we could also need to add a dummy inline method that still calls it. Given that, what do you think the best course of action is @zarthross?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is certainly an awkward part of scala3 inlines... we could try adding a private method with the name inline$of? I'm not sure if scala 3 will allow that, or we could try a dummy inline method as you suggested... we'll just have to try a few things and check the generated bytecode....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up adding inline$of methods. They can't be private otherwise they cause a java.lang.IllegalAccessError instead of a binary incompatibility, but I made them private[derivation] and marked them as deprecated

@hamnis hamnis dismissed their stale review August 9, 2024 11:29

Some changes were needed, dissmissed to avoid premature merge

@mrdziuban mrdziuban force-pushed the fix-2263-with-macro branch from c993023 to 00e8fe9 Compare August 12, 2024 19:32
Copy link
Member

@zarthross zarthross left a comment

Choose a reason for hiding this comment

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

Thanks for the hard work on this!

@hamnis hamnis merged commit 4978327 into circe:series/0.14.x Sep 5, 2024
18 checks passed
@mrdziuban mrdziuban deleted the fix-2263-with-macro branch September 5, 2024 17:54
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.

StackOverflowError if local class since 0.14.7 with Scala 3
3 participants