Circe: ConfiguredJsonEncoder broken on upgrade from 0.8.0 to 0.9.1.

[nathankleyn]

I'm attempting to upgrade a large enterprise Scala project from Circe 0.8.0 to 0.9.1 and getting a compiler error on the following code:

object Parsing {
  // This is where the implicit `Configuration` comes from.
  import JsonWithTypeField.implicits._

  // Note: Circe needs empty companion objects after any extending case classes for all sealed traits below.
  // This is to work around SI-7046 issues in a stable and repeatable way.
  // See https://circe.github.io/circe/codec.html#warnings-and-known-issues, point 5, for more information.

  @ConfiguredJsonCodec
  sealed trait ValidationErrorHandling

  object AttributeValidation {
    @ConfiguredJsonCodec
    sealed trait AttributeValidationErrorHandling extends ValidationErrorHandling
    case object Strict extends AttributeValidationErrorHandling
    case object EmptyAsDelete extends AttributeValidationErrorHandling
    case object EmptyAndFailureAsDelete extends AttributeValidationErrorHandling
    object AttributeValidationErrorHandling
  }

  object ActionValidation {
    @ConfiguredJsonCodec
    sealed trait ActionValidationErrorHandling extends ValidationErrorHandling
    case object Strict extends ActionValidationErrorHandling
    case object AllowEmpty extends ActionValidationErrorHandling
    case object AllowEmptyAndFailure extends ActionValidationErrorHandling
    object ActionValidationErrorHandling
  }

  object ValidationErrorHandling

  // ...
}

This was compiling happily on circe 0.8.0 but fails to compile on circe 0.9.1:

[info] Compiling 29 Scala sources to [...]/target/scala-2.11/classes ...
[error] [...]/Parsing.scala:19:4: could not find Lazy implicit value of type io.circe.generic.extras.decoding.ConfiguredDecoder[com.foo.ValidationErrorHandling]
[error]   @ConfiguredJsonCodec
[error]    ^
[error] one error found
[error] (foo / Compile / compileIncremental) Compilation failed
[error] Total time: 18 s, completed 12-Mar-2018 16:12:12

After exhausting all the options of moving around wrapper object types and ensuring I wasn't being hit by the ghosts of SI-7046, I managed to track down that I _could_ still have this example compile on 0.9.0-M2 but not 0.9.0-M3 - a quick git bisect later revealed that the issue was introduced somewhere in the change at https://github.com/circe/circe/commit/5193b5b8bcc3910535d036f09adee259493c7cb8.

Unfortunately, I can't seem to make a reproducible test case from just the above code. It's worth noting that there are other decoders that reference these above too, I'm just posting the code snippet that _actually fails compilation_.

I'm not really able to say exactly what about this change causes the compilation in this project to fail, mainly because the commit in question is way over my head, but I'm happy to assist with reproducing the error in any way needed!

[travisbrown]

Thanks for the report, @nathankleyn—unfortunately this failure doesn't sound familiar (my first thought was that you might be missing the now-required Configuration, but you say you have one). Would it be possible for you to try compiling with the splain plugin added and enabled (which should give a more useful error message)?

[nathankleyn]

Thanks so much for the super-speedy reply @travisbrown - you're an OSS inspiration!

I spent a while struggling to get the splain plugin to work, as I was hitting the problem that as soon as I enabled it, the macro expansion failed to work everywhere. It turns out this is a known issue if you're also including macro paradise (which I am because i'm using the annotations). The fix was simple enough in the end: add splain before paradise - it's SI-7046 compiler plugin edition! 😀

That's finally led me to get this expanded version of the actual underlying error:

[error] [...]/Parsing.scala:19:4: implicit error;
[error] !I decode: ConfiguredDecoder[ValidationErrorHandling]
[error] ――ConfiguredDecoder.decodeIncompleteCaseClass invalid because
[error]   !I ffp: FnFromProduct.Aux[P => A, ValidationErrorHandling]
[error] ――ConfiguredDecoder.decodeCaseClass invalid because
[error]   !I gen: LabelledGeneric.Aux[ValidationErrorHandling, R]
[error] ――――LabelledGeneric.materializeProduct invalid because
[error]     !I gen: Generic.Aux[ValidationErrorHandling, V]
[error]
[error] ――ConfiguredDecoder.decodeAdt invalid because
[error]   !I decodeR: ReprDecoder[R]
[error] ――――LabelledGeneric.materializeCoproduct invalid because
[error]     !I gen: Generic.Aux[AllowEmpty.type, V]
[error]
[error] ――――――――Decoder.importedDecoder invalid because
[error]         !I exported: Exported[Decoder[AllowEmpty.type]]
[error] ――――――――――LabelledGeneric.materializeCoproduct invalid because
[error]           !I gen: Generic.Aux[AllowEmptyAndFailure.type, V]
[error]
[error] ――――――――Decoder.importedDecoder invalid because
[error]         !I exported: Exported[Decoder[AllowEmptyAndFailure.type]]
[error] ――――――――――LabelledGeneric.materializeCoproduct invalid because
[error]           !I gen: Generic.Aux[Strict.type, V]
[error]
[error] ――――――――Decoder.importedDecoder invalid because
[error]         !I exported: Exported[Decoder[Strict.type]]
[error] ――――――――――LabelledGeneric.materializeCoproduct invalid because
[error]           !I gen: Generic.Aux[EmptyAndFailureAsDelete.type, V]
[error]
[error] ――――――――Decoder.importedDecoder invalid because
[error]         !I exported: Exported[Decoder[EmptyAndFailureAsDelete.type]]
[error] ――――――――――LabelledGeneric.materializeCoproduct invalid because
[error]           !I gen: Generic.Aux[EmptyAsDelete.type, V]
[error]
[error] ――――――――Decoder.importedDecoder invalid because
[error]         !I exported: Exported[Decoder[EmptyAsDelete.type]]
[error] ――――――――――LabelledGeneric.materializeCoproduct invalid because
[error]           !I gen: Generic.Aux[Strict.type, V]
[error]
[error] ――――――――Decoder.importedDecoder invalid because
[error]         !I exported: Exported[Decoder[Strict.type]]
[error]   @ConfiguredJsonCodec
[error]    ^
[error] one error found
[error] (foo / Compile / compileIncremental) Compilation failed

[nathankleyn]

It appears to be related to the fact that two of the derivative case objects have the same name, in this case Strict. Renaming one of them to StrictX makes the problem go away. I suspect the commit in question at https://github.com/circe/circe/commit/5193b5b8bcc3910535d036f09adee259493c7cb8 perhaps means these two names clash now?

[travisbrown]

Ugh, that sounds plausible but unfortunate. Luckily we could fix it in 0.9.2 (which I've been meaning to release for the validate fix for the past few days, anyway).

[travisbrown]

Hmm, I've been thinking a little more about this, and I'm not sure we want it to compile.

Take the following:

import io.circe.generic.extras.{ Configuration, ConfiguredJsonCodec }

implicit val configCirce: Configuration = Configuration.default

@ConfiguredJsonCodec sealed trait Base
object A {
  @ConfiguredJsonCodec sealed trait BaseA extends Base
  case object X extends BaseA
}
object B {
  @ConfiguredJsonCodec sealed trait BaseB extends Base
  case object X extends BaseB
} 

…and then:

scala> io.circe.jawn.decode[Base]((B.X: Base).asJson.noSpaces).map(_ == A.X)
res0: scala.util.Either[io.circe.Error,Boolean] = Right(true)

scala> io.circe.jawn.decode[Base]((B.X: Base).asJson.noSpaces).map(_ == B.X)
res1: scala.util.Either[io.circe.Error,Boolean] = Right(false)

…which is clearly kind of bad, or at least against our principle that derived codecs always round-trip values.

To be honest I'm not sure I've ever stopped to think about this case (i.e. ADTs where Shapeless's generic representation isn't actually a union).

The internal change you noted assumed that the generic representation is always a union (with unique keys). This isn't the case, and code that previously compiled doesn't compile any more, but my feeling is that that's the way things should be. You'd have to write the top-level instances manually, but that seems preferable to breaking our round-trip invariant.

What do you think, @nathankleyn?

[travisbrown]

This comment is a minimization for the sake of voting / discussion. The question is whether the following code should compile (I'm using @JsonCodec here, but you'd see the same thing with either auto or semiauto):

import io.circe.generic.JsonCodec

@JsonCodec sealed trait Base

object Foo { case object Qux extends Base }
object Bar { case object Qux extends Base }

It doesn't compile on 0.9.x, but does on 0.8 and earlier, where it has the following goofy behavior:

scala> import io.circe.Decoder, io.circe.syntax._
import io.circe.Decoder
import io.circe.syntax._

scala> Decoder[Base].decodeJson((Foo.Qux: Base).asJson).map(_ == Bar.Qux)
res0: scala.util.Either[io.circe.DecodingFailure,Boolean] = Right(true)

Specifically, Shapeless's "union" doesn't have unique keys, so either case object will get encoded in such a way that it will be decoded as the one whose wrapper object name is lexicographically first.

I'm inclined to think not compiling is the right thing to do here, since there are no other (known) cases where a generically derived encoder-decoder pair won't round-trip values (at least for circe-generic), but I'm open to counter-arguments, or you can just vote:

:+1:: it should compile (the pre-0.9 behavior); :-1:: it should not compile (the current behavior).

[nathankleyn]

@travisbrown Massive thanks for distilling the issue down into a reproducible test case! I completely agree that this should not compile, as it makes no sense whatsoever IMO.

Once I understood what was going on here two things really stood out as reasons to not just blindly fix this and sweep it under the carpet:

• I can't understand why the original code I posted has two levels of sealed trait hierarchies where _both_ are marked with @JsonCodec. The top level one doesn't seem useful. By using that one, you lose the typesafety you were trying to gain by having the nested options split in sub-traits in the first place, and if you wanted them to be treated as one group you wouldn't want two with the same name anyway? I would argue only the two sealed traits within the nested objects should actually be marked with the annotation, or you should just have one level of trait with non-overlapping names.
• JSON is meant as a human readable format, but when you're using it like this where it's bound to some typesafe definition/schema I think it becomes really important to be able to see at a glance where you can find the definition for something. Having two things with the same name makes this much more difficult, as then you have to understand the full context of the containing scope. Perhaps this is opinionated behaviour, but I think a library like circe should be setting guide-rails for people here to do the right thing™️.

One unfortunate thing though is that the error message is very opaque, although that may just be impossible to fix - I wish I understood the internals more to say for sure! I think perhaps the best way forward there is just to document this issue under the warnings and known issues section of the docs (which I am super happy to raise a PR for!)