lloydmeta / enumeratum

A type-safe, reflection-free, powerful enumeration implementation for Scala with exhaustive pattern match warnings and helpful integrations.
MIT License
1.19k stars 148 forks source link

Derived `CirceEnum` codecs fail roundtrip on `case class` values #386

Closed VKFisher closed 10 months ago

VKFisher commented 10 months ago

Tested on 1.7.3, scala 2.13.10

Given this type definition

sealed trait SomeType extends EnumEntry

object SomeType extends Enum[SomeType] with CirceEnum[SomeType] {
  val values: IndexedSeq[SomeType] = findValues

  case class SomeValue(value: Int) extends SomeType
}

Running this example


private val example = {
  val initialValue      = SomeType.SomeValue(value = 5)
  val encodedJson       = SomeType.circeEncoder(initialValue)
  val encodedJsonString = encodedJson.spaces2
  val decodedJson       = parser.parse(encodedJsonString).toOption.get
  val decodedJsonString = decodedJson.spaces2
  val decodingResult    = SomeType.circeDecoder.decodeJson(decodedJson)

  println("=== initialValue ===")
  println(initialValue)
  println("=== encodedJsonString ===")
  println(encodedJsonString)
  println("=== decodedJsonString ===")
  println(decodedJsonString)
  println("=== decodingResult ===")
  println(decodingResult)
}

produces this output

=== initialValue ===
SomeValue(5)
=== encodedJsonString ===
"SomeValue(5)"
=== decodedJsonString ===
"SomeValue(5)"
=== decodingResult ===
Left(DecodingFailure('SomeValue(5)' is not a member of enum example.SomeType$@4d910fd6, List()))
lloydmeta commented 10 months ago
  1. Depending on where you get the definition of an enum, you might get different answers, but ultimately, they are either defined as "a closed/finite set of values" or set of constant values, etc. With compiled static langs, the additional implication that this set is defined at compile time.

    An instance of a case class created at compile time would not be part of the enum set. Enumeratum (deliberately) ignores any inherited class that is not an object

  2. Circe encoding-to-JSON for a type with an encoder is non-fallible, hence the ability to encode non-members of an enum type

I would need a lot of convincing to budge from (1) (ie. SomeValue case class will remain not an enum member for the foreseeable future), and I don't really know how we can simply solve for (2), either, since it's a Circe thing (and I would likewise need heavy convincing to code something in the encoder that checks for members throws)

VKFisher commented 10 months ago

Huh, I see. I was mistaken about enumeratum's scope then.

In any case, I think that codecs that fail a roundtrip at runtime on some subset of possible values is an issue that needs some sort of resolution. What do you think about these?

lloydmeta commented 10 months ago

Updating the documentation with an explicit statement that enumeratum enums are just simple sets of constant values and not full-fledged ADTs (some languages like Rust or Swift name their ADTs Enums so this is a possible source of confusion)

While I can appreciate both Rust and the potential confusion, I think it's sufficiently clear that enums != ADTs when it comes to the Scala context and enumeratum: we have a values: ImmutableSeq[EnumType] as a requirement/main feature, and ADTs are implemented via sealed. That Rust and Swift decided to use enum as a keyword for ADTs/tagged unions (all enums are tagged unions but not all tagged unions are enums) is "interesting", but I don't feel that should require all other enumeration implementations to explicitly call out that they are , in fact, implementing enumerable enumerations.

Emitting a warning or an error at compile time if there's a non-object value type that enumeratum or circe won't be able to handle

This would break a lot of usages of intermediate classes or case classes that are used to construct valid object members.

IMO it would be more sensible to update the findValue macros to emit a warning that there were no members found (though this should probably be a minor version bump).

VKFisher commented 10 months ago

@lloydmeta fair enough. Closing this issue since no changes are warranted. Thanks for your responses!