plokhotnyuk / jsoniter-scala

Scala macros for compile-time generation of safe and ultra-fast JSON codecs + circe booster
MIT License
751 stars 99 forks source link

case object doesn't encode correctly #1114

Open DesZhang opened 10 months ago

DesZhang commented 10 months ago

Recently I upgrade to 2.27.7 in my scala 3 project, the minimal example is:

final case class Error[+E <: ErrCode](code: E, msg: String)
given [E <: ErrCode](using JsonValueCodec[E]): JsonValueCodec[Error[E]] = JsonCodecMaker.make

sealed trait ErrCode
given JsonValueCodec[ErrCode] = JsonCodecMaker.make(
  CodecMakerConfig.withDiscriminatorFieldName(None)
)

@named("error type 1")
case object errType1 extends ErrCode
given JsonValueCodec[errType1.type] = JsonCodecMaker.make

@named("error type 2")
case object errType2 extends ErrCode

val err                  = Error(errType1, "msg")
val gErr: Error[ErrCode] = err

// {"code":{},"msg":"msg"}
val json1 = writeToString(err)
// {"code":"error type 1","msg":"msg"}
val json2 = writeToString(gErr)

obviously json1 is incorrect.

BTW: the library is amazing, thank you.

plokhotnyuk commented 10 months ago

Hi, @DesZhang! Thanks for your feedback and support!

I would say that jsoniter-scala-macros derives right codec according to types derived by scala compiler.

The root cause of issue that types are different for err and gErr values.

I've pasted your code to the module tests and got the following type hint for err:

image

Possible other workarounds to help scalac derive a proper type are using an explicit type when creating value or writing it :

writeToString(Error[ErrCode](errType1, "msg"))
writeToString[Error[ErrCode]](err)

Yet another solution could be removing (or commenting out) of a redundant codec for errType1.type type:

//given JsonValueCodec[errType1.type] = JsonCodecMaker.make

Then compiler will say that writeToString require an implicit parameter for JsonValueCodec[Error[errType1.type]]:

image

DesZhang commented 10 months ago

@plokhotnyuk Thank you for quick response, the last part is exactly what I was trying to deal with. Do I have to widen the type explicitly every time so the compiler could use JsonValueCodec[ErrCode] to synthesize the instance for encoding?

and I'm still confused why value of code is empty in json1, I already had an instance of JsonValueCodec[errType1.type], and the compiler didn't complain, something like "error type 1" should be expected.

plokhotnyuk commented 10 months ago

The best option would be using of non-generic Error class and derivation of just one top-level codec for it:

final case class Error(code: ErrCode, msg: String)
given codec: JsonValueCodec[Error] = JsonCodecMaker.make(CodecMakerConfig.withDiscriminatorFieldName(None))

sealed trait ErrCode

@named("error type 1")
case object errType1 extends ErrCode

@named("error type 2")
case object errType2 extends ErrCode

val err         = Error(errType1, "msg")
val gErr: Error = err

// {"code":"error type 1","msg":"msg"}
println(writeToString(err))
// {"code":"error type 1","msg":"msg"}
println(writeToString(gErr))
DesZhang commented 10 months ago

@plokhotnyuk thank you

plokhotnyuk commented 10 months ago

@DesZhang I have no confidence if proposed solutions were acceptable for you.

Yes, for your version of code snippet widening is required because the compiler tends to derive the narrowest type.

The codec generated by given JsonValueCodec[errType1.type] = JsonCodecMaker.make is out of any context and just serializes an empty JSON object to be distinguished from the default null value.

Sometime users report minimized code for their issue without description of the context, so my willing to get the simplest solution could be unacceptable or hard to grasp.

Have you migrated from Scala 2 or your code was initially created for other json library?

As example, if you migrate your code from circe or upickle codecs replacing them by jsoniter-scala codecs step by step, than it could be a long journey. The ideal approach would be to start from tests for top-level messages and derive only required jsoniter-scala codecs.

DesZhang commented 9 months ago

@plokhotnyuk thank you and sorry for late response. I used type parameter to make different types of Errors so that I could make precise error declaration in ZIO's error channel, like ZIO[Any, NotFoundError | PageError | NetworkError, A] .

I've started a new project with jsoniter-scala(2.28.3) , and type widening is ok for me, recently I found another compiler warning I don't know how to deal with.

sealed trait Error[+E <: Code] {
    val code: E
    val msg: String
}

@named("basic")
final case class BasicError[E <: Code](
    code: E,
    msg: String
) extends Error[E]

@named("line")
final case class LineError[E <: Code](
    code: E,
    ln: LineNumber,
    msg: String
) extends Error[E]

sealed trait Code

@named("0001")
case object notFound extends Code
type NotFound = Error[notFound.type]

@named("0002")
case object incomplete extends Code
type Incomplete = Error[incomplete.type]

// compiler warning: the type test for BasicError[Code] cannot be checked at runtime because
// its type arguments can't be determined from Error[Code]
given JsonValueCodec[Error[Code]] = JsonCodecMaker.make(
    CodecMakerConfig
          .withDiscriminatorFieldName(Some("tpe"))
          .withRequireDiscriminatorFirst(true)
)

The compiler(scala 3.4.0) complains about type test problem caused by type erasure, my guess is that pattern match happened somewhere in code generated by jsoniter-scala macro, since E has upper bound, can I safely assume the warning could be ignored? or what can I do to eliminate it?

Beside the question above, another strange thing is, I have almost the same error coding pattern in another project, by "almost" I mean they only have different class/trait names, and I got no warning at all, both projects have the same versions of jsoniter-scala and scala compiler.

plokhotnyuk commented 9 months ago

@DesZhang Thanks for your feedback!

Please peek the latest release with a fix for unwanted warning: https://github.com/plokhotnyuk/jsoniter-scala/releases/tag/v2.28.4

DesZhang commented 9 months ago

@plokhotnyuk wow,that was lightning fast, I'll try and report back asap

DesZhang commented 8 months ago

@plokhotnyuk 2.28.4 works as expected, thanks.