Flakiness in macro success

[hughsimpson]

Using:
scala 2.13.16
"com.github.plokhotnyuk.jsoniter-scala" %% "jsoniter-scala-core" % "2.35.3"
"com.github.plokhotnyuk.jsoniter-scala" %% "jsoniter-scala-macros" % "2.35.3" % Provided
(and previous versions of jsoniter-scala going back quite some time -- I don't think this is in anyway a recently-introduced issue)

When compiling the code generated by the tapir openapi generator (example output), I will occasionally hit an error looking as follows:

  [error] /home/runner/work/myapp/target/scala-2.13/src_managed/main/sbt-openapi-codegen/TapirGeneratedEndpointsJsonSerdes.scala:12:186: Cannot evaluate a parameter of the 'make' macro call for type 'sttp.tapir.generated.myapp.TapirGeneratedEndpoints.Payload'. It should not depend on code from the same compilation module where the 'make' macro is called. Use a separated submodule of the project to compile all such dependencies before their usage for generation of codecs. Cause:
  [error] java.lang.AssertionError: assertion failed: com package com <none>
  [error]   implicit lazy val payloadJsonCodec: com.github.plokhotnyuk.jsoniter_scala.core.JsonValueCodec[Payload] = com.github.plokhotnyuk.jsoniter_scala.macros.JsonCodecMaker.make(com.github.plokhotnyuk.jsoniter_scala.macros.CodecMakerConfig.withAllowRecursiveTypes(true).withTransientEmpty(false).withTransientDefault(false).withRequireCollectionFields(true))

I have seen this happen on a relatively minimal file:

  implicit def seqCodec[T: com.github.plokhotnyuk.jsoniter_scala.core.JsonValueCodec]: com.github.plokhotnyuk.jsoniter_scala.core.JsonValueCodec[List[T]] =
    com.github.plokhotnyuk.jsoniter_scala.macros.JsonCodecMaker.make[List[T]]
  implicit def optionCodec[T: com.github.plokhotnyuk.jsoniter_scala.core.JsonValueCodec]: com.github.plokhotnyuk.jsoniter_scala.core.JsonValueCodec[Option[T]] =
    com.github.plokhotnyuk.jsoniter_scala.macros.JsonCodecMaker.make[Option[T]]

  implicit lazy val payloadJsonCodec: com.github.plokhotnyuk.jsoniter_scala.core.JsonValueCodec[Payload] = com.github.plokhotnyuk.jsoniter_scala.macros.JsonCodecMaker.make(com.github.plokhotnyuk.jsoniter_scala.macros.CodecMakerConfig.withAllowRecursiveTypes(true).withTransientEmpty(false).withTransientDefault(false).withRequireCollectionFields(true))

  implicit lazy val otherModelJsonCodec: com.github.plokhotnyuk.jsoniter_scala.core.JsonValueCodec[OtherModel] = com.github.plokhotnyuk.jsoniter_scala.macros.JsonCodecMaker.make(com.github.plokhotnyuk.jsoniter_scala.macros.CodecMakerConfig.withAllowRecursiveTypes(true).withTransientEmpty(false).withTransientDefault(false).withRequireCollectionFields(true))
  implicit lazy val innerBodySchemaJsonCodec: com.github.plokhotnyuk.jsoniter_scala.core.JsonValueCodec[InnerBodySchema] = com.github.plokhotnyuk.jsoniter_scala.macros.JsonCodecMaker.make(com.github.plokhotnyuk.jsoniter_scala.macros.CodecMakerConfig.withAllowRecursiveTypes(true).withTransientEmpty(false).withTransientDefault(false).withRequireCollectionFields(true))

  implicit lazy val anotherModelJsonCodec: com.github.plokhotnyuk.jsoniter_scala.core.JsonValueCodec[AnotherModel] = com.github.plokhotnyuk.jsoniter_scala.macros.JsonCodecMaker.make(com.github.plokhotnyuk.jsoniter_scala.macros.CodecMakerConfig.withAllowRecursiveTypes(true).withTransientEmpty(false).withTransientDefault(false).withRequireCollectionFields(true).withDiscriminatorFieldName(scala.None).withDiscriminatorFieldName(scala.None))

  implicit lazy val anotherModel2JsonCodec: com.github.plokhotnyuk.jsoniter_scala.core.JsonValueCodec[AnotherModel2] = com.github.plokhotnyuk.jsoniter_scala.macros.JsonCodecMaker.make(com.github.plokhotnyuk.jsoniter_scala.macros.CodecMakerConfig.withAllowRecursiveTypes(true).withTransientEmpty(false).withTransientDefault(false).withRequireCollectionFields(true).withDiscriminatorFieldName(scala.None).withDiscriminatorFieldName(scala.None))
  implicit lazy val innerBodyCodec: com.github.plokhotnyuk.jsoniter_scala.core.JsonValueCodec[InnerBody] = new com.github.plokhotnyuk.jsoniter_scala.core.JsonValueCodec[InnerBody] {
    def decodeValue(in: com.github.plokhotnyuk.jsoniter_scala.core.JsonReader, default: InnerBody): InnerBody = {
      List(
        innerBodySchemaJsonCodec)
        .foldLeft(Option.empty[InnerBody]) {
          case (Some(v), _) => Some(v)
          case (None, next) =>
            in.setMark()
            scala.util.Try(next.asInstanceOf[com.github.plokhotnyuk.jsoniter_scala.core.JsonValueCodec[InnerBody]].decodeValue(in, default))
              .fold(_ => { in.rollbackToMark(); None }, Some(_))
        }.getOrElse(throw new RuntimeException("Unable to decode json to untagged ADT type InnerBody"))
    }
    def encodeValue(x: InnerBody, out: com.github.plokhotnyuk.jsoniter_scala.core.JsonWriter): Unit = x match {
      case x: InnerBodySchema => InnerBodySchemaJsonCodec.encodeValue(x, out)
    }

    def nullValue: InnerBody = InnerBodySchemaJsonCodec.nullValue
  }
  implicit lazy val messageJsonCodec: com.github.plokhotnyuk.jsoniter_scala.core.JsonValueCodec[Message] = com.github.plokhotnyuk.jsoniter_scala.macros.JsonCodecMaker.make(com.github.plokhotnyuk.jsoniter_scala.macros.CodecMakerConfig.withAllowRecursiveTypes(true).withTransientEmpty(false).withTransientDefault(false).withRequireCollectionFields(true))

The relevant models seem to be

  case class Payload (
    body: InnerBody,
    some: String,
    other: String,
    params: Seq[String]
  )

  sealed trait InnerBody

  case class InnerBodySchema (
    has: String,
    some: Int,
    more: Seq[DateTime],
    params: String 
  ) extends InnerBody

although I haven't been able to produce a minimal version that definitely triggers this yet, I'm afraid - in part because it happens so rarely. I can't even get the failure locally on the exact file that's caused it before. It seems to happen more often when building in CI docker for some mysterious reason 😅

Anyway I'm not sure if this is really a jsoniter-scala bug or a scala bug or what, but thought I'd raise it here in case @plokhotnyuk had any ideas.

Perhaps there might be some workaround we could do in the tapir codegen to ameliorate this flakiness?

[plokhotnyuk]

Hi @hughsimpson,

It appears you are encountering a known issue #2.

Could you please try the suggested workarounds to see if they stabilize your CI builds?

The most effective solution, which isn't listed there, is migrating to Scala 3. This version does not use the powerful but sometimes unstable Eval.eval from Scala 2.

If migrating to Scala 3 isn't feasible at the moment, I can add an additional macro with pre-configured derivation options, similar to these examples, and cut a release for you.

[hughsimpson]

@plokhotnyuk Thank you for this. If you don't mind, I'll take a little time to delve into this a little -- a work colleague has managed to stumble across a reliable reproduction of the issue, and I want to check a few things.

The serdes are constructed by the tapir codegen in a single object that hosts only jsoniter serde declarations (the snippet above is the whole of the object contents), and I don't know how practical it would really be to pull these out into a separate object for each, especially given that mutually-recursive schemas would then need even more care and attention than they currently have.

The two configurations most used by the tapir codegen are found here.
They expand to:
CodecMakerConfig.withAllowRecursiveTypes(true).withTransientEmpty(false).withTransientDefault(false).withRequireCollectionFields(true)
And
CodecMakerConfig.withAllowRecursiveTypes(true).withTransientEmpty(false).withTransientDefault(false).withRequireCollectionFields(true).withDiscriminatorFieldName(scala.None)
and I've just realised that there's a path that expands to
CodecMakerConfig.withAllowRecursiveTypes(true).withTransientEmpty(false).withTransientDefault(false).withRequireCollectionFields(true).withDiscriminatorFieldName(scala.None).withDiscriminatorFieldName(scala.None)
which is obviously mad and a bug in the codegen 🤦 (duplicate method calls for withDiscriminatorFieldName)
Anyway, that excludes the config for ADTs but perhaps it would help to have those pre-configured 🤔. I will try to spend some more time on this in light of your response over the next few days. Really appreciate you getting back to me on this.

EDIT: ,In fact I wonder if that duplicate method call is a part of the reason for the flakiness. Perhaps this can be mitigated at the level of tapir codegen. Like I say, I will run some tests on this... doesn't seem to be relevant, fixing the duplicate method call doesn't change anything for the replication example I have. Sadly said example is too large and $private to share as it stands, though I'll try to minimise.

[hughsimpson]

I have something that reliably triggers the bug every time which I'll upload today, and it seems that the issue is specifically triggered by multi-module builds, so there seems to be something at compile time that isn't thread-safe. Will keep poking at this and check if predefining those options sets helps. More to follow...

[hughsimpson]

OK I have tested locally with introducing 2 new custom macros for use by the tapir codegen:

    def makeOpenapiLike[A: c.WeakTypeTag](c: blackbox.Context): c.Expr[JsonValueCodec[A]] =
      make(c)(CodecMakerConfig.withTransientEmpty(false).withTransientDefault(false)
        .withRequireCollectionFields(true).withAllowRecursiveTypes(true))

    def makeOpenapiEnumLike[A: c.WeakTypeTag](c: blackbox.Context): c.Expr[JsonValueCodec[A]] =
      make(c)(CodecMakerConfig.withTransientEmpty(false).withTransientDefault(false)
        .withRequireCollectionFields(true).withAllowRecursiveTypes(true).withDiscriminatorFieldName(scala.None))

-- this seems to have entirely fixed the problem, and covers almost all cases where the codegen would otherwise use the make method (exception here being for adts with discriminators, which I guess absolutely require the use of c.eval to permit passing in the mapping and discriminator name). It does seem a bit of a shame that there isn't a more general fix here, but them's the breaks I suppose. I'm not sure any of the other solutions are going to cut the mustard here 😞

I don't know if it would still be helpful to have a replication of the bug, but I can push it up if you think it's worth it, just need to clean up a bit first.

EDIT AGAIN: Ah. Whilst this helps for defns that don't include ADTs, with ADTs it still goes wrong... Have a reproduction up here. It really does seem to all come down to multi-module builds for some reason.

Adding two more macros:

    def makeOpenapiADTLikeDefaultMapping[A: c.WeakTypeTag](c: blackbox.Context)(discriminator: c.Expr[String]): c.Expr[JsonValueCodec[A]] =
      make(c)(CodecMakerConfig.withTransientEmpty(false).withTransientDefault(false)
        .withRequireCollectionFields(true).withAllowRecursiveTypes(true)
        .withRequireDiscriminatorFirst(false).withDiscriminatorFieldName(Some(c.eval(c.Expr[String](c.untypecheck(discriminator.tree.duplicate))))))
    def makeOpenapiADTLike[A: c.WeakTypeTag](c: blackbox.Context)(discriminator: c.Expr[String], mapping: c.Expr[PartialFunction[String, String]]): c.Expr[JsonValueCodec[A]] =
      make(c)(CodecMakerConfig.withTransientEmpty(false).withTransientDefault(false)
        .withRequireCollectionFields(true).withAllowRecursiveTypes(true)
        .withRequireDiscriminatorFirst(false).withDiscriminatorFieldName(Some(c.eval(c.Expr[String](c.untypecheck(discriminator.tree.duplicate))))).withAdtLeafClassNameMapper(x => c.eval(c.Expr[PartialFunction[String, String]](c.untypecheck(mapping.tree.duplicate))).apply(com.github.plokhotnyuk.jsoniter_scala.macros.JsonCodecMaker.simpleClassName(x))))

seems to fix it even for ADT types and I have absolutely no idea why that would be, since they both still use eval. Really weird and mysterious.

[hughsimpson]

I strongly suspect that the pr you just merged will fix this, at least for code generated by tapir, so I'm closing this for now. Thanks again for the help and guidance 🙏

[plokhotnyuk]

@hughsimpson I've renamed added macros and their parameter to be more consistent with names of already existing ones in this commit

Please check if all is fine and I will cut a release for you.

[hughsimpson]

@plokhotnyuk absolutely no issues with the renaming, all looks good to me. Thanks for bearing with me on all this, excited about the release

[hughsimpson]

Also run the local test case I have (sorry about the delay, was eating dinner) and that's still fine and passing, so think this is grand 👍

[plokhotnyuk]

@hughsimpson Please peek the latest v2.36.0 release with your contribution.

[hughsimpson]

@plokhotnyuk Thanks! Running tests now.

• Regression testing behaviour for tapir codegen with new methods passed on this pr here
• Reproduction passed here (still failing here with default make method, as expected)

All looks good! I'm just about to run on $privateCodebaseThatTriggeredAllOfThis, but I'm feeling positive!

Final edit (?): Yep, even passes on the private codebase. Awesome.