Default values not working with @key annotation in Scala 3

[EindbaasExpress]

case class Person(
                     @key("first_name") firstName: String = "N/A",
                     @key("last_name") lastName: String)
object Person {
    implicit val rw: RW[Person] = macroRW
}

val json = """{"last_name": "Snow"}"""
val p = read[Person](json)

This code fragment executes with exception:

Exception in thread "main" java.lang.ExceptionInInitializerError
    at com.nitka.rb.RBSyncApp.main(RBSyncApp.scala)
Caused by: upickle.core.AbortException: missing keys in dictionary: first_name at index 20
    at ujson.CharParser$$anon$1.applyOrElse(CharParser.scala:343)
    at ujson.CharParser$$anon$1.applyOrElse(CharParser.scala:341)
    at scala.runtime.AbstractPartialFunction.apply(AbstractPartialFunction.scala:35)
    at ujson.CharParser.liftedTree1$1(CharParser.scala:496)
    at ujson.CharParser.tryCloseCollection(CharParser.scala:496)
    at ujson.CharParser.parseNested(CharParser.scala:462)
    at ujson.CharParser.parseTopLevel0(CharParser.scala:323)
    at ujson.CharParser.parseTopLevel(CharParser.scala:307)
    at ujson.CharParser.parse(CharParser.scala:59)
    at ujson.StringParser$.transform(StringParser.scala:28)
    at ujson.StringParser$.transform(StringParser.scala:28)
    at ujson.Readable$fromTransformer.transform(Readable.scala:13)
    at upickle.Api.read$$anonfun$1(Api.scala:37)
    at upickle.core.TraceVisitor$.withTrace(TraceVisitor.scala:18)
    at upickle.Api.read(Api.scala:37)
    at upickle.Api.read$(Api.scala:17)
    at upickle.default$.read(Api.scala:133)
    at com.nitka.rb.RBSyncApp$.<clinit>(RBSyncApp.scala:76)
    ... 1 more
Caused by: upickle.core.Abort: missing keys in dictionary: first_name
    at com.nitka.rb.RBSyncApp$$anon$1.make(RBSyncApp.scala:72)
    at com.nitka.rb.RBSyncApp$$anon$1.make(RBSyncApp.scala:72)
    at upickle.implicits.CaseClassReaderPiece$$anon$1.visitEnd(CaseClassReader.scala:30)
    ... 16 more

Reproduced with uPickle version 1.4.0

ID: 360 Original Author: pharod

[EindbaasExpress]

FWIW, this returns Person("N/A","Snow") with weepickle. I know we've refactored the macros, but haven't checked which is the crucial difference.

@pharod if you'd like to take a shot a fixing it, the problem is likely somewhere around here. Original Authorhtmldoug

[EindbaasExpress]

@htmldoug Do you think we can unify weepickle and upickle again? One of the two reasons for its existence can now go away because of mill-mima which I created to fix the binary compatibility problem in the com-lihaoyi ecosystem. Original Authorlolgab

[EindbaasExpress]

mill-mima is great to see! That was the biggest missing piece that finally drove weePickle from mill => sbt.

Do you think we can unify weepickle and upickle again?

Simple question, complicated answer. TL;DR: I'm not sure that unifying would be worth it for either project.

upickle is already getting the bug fixes from weepickle which are easily portable as well as notable features like #286. upickle also has access to the weepickle+jackson ecosystem of YAML, CBOR, Smile, XML (such that it is), etc. via the bridges in https://github.com/rallyhealth/weePickle/pull/87, if desired.

I understand there's a lot going on with the scala 2 macros. Regarding the fix for this issue, the weepickle's macros have diverged a bit. I'm not sure you would want all our changes to the scala 2 macros as they are. For example, the section to support case classes with > 64 fields and maintain binary backwards compatibility is particularly ugly. In the past, the upickle approach has been to make the breaking changes, but keep the implementation simple. So for this issue, I'd just port the part you care about, or just ignore weepickle entirely--whichever is easier.

Satisfying rallyhealth/weepickle's needs would impose some additional constraints on upickle. For context, Rally has hundreds of internal scala repos using weepickle with an inter-dependency tree and different paces of development and staffing. Taking breaking changes with micro-repos is much more expensive than the databricks monorepo setup, since different modules may be compiled against different (binary incompatible) versions of the same library, resulting in runtime exceptions.

So, we shade (at publish time) all of our internal libraries that are meant to be used by other libraries and prefer third party libraries that do the same. MiMa + semver is not enough to avoid dependency hell, but shading is.

Is shading something that upickle would be willing to adopt on the next major release/mima failure? It's a big ask. Among other things, it might require a major version bump of Haoyi's book to update the examples.

weepickle has also made other minor tradeoffs in complexity/performance and behavior idiosyncrasies/ease of migration from play-json that we might be able work through to unify, but shading is the first major hurdle.

A second factor is that Rally has invested a fair amount in migrating from play-json to weepickle because: 1. it stopped wasting developer time on dependency hell, 2. stopped the OOMEs in prod apps, 3. improved response times. It'd be hard to make a business case to migrate 338 repos to another json library so soon, particularly without equally compelling benefits over weepickle.

I'm a huge fan of the com-lihaoyi ecosystem. It's a great fit for monorepos or leaf nodes of the repo dependency tree. It's just not a clear fit for our non-leaf libraries nodes due to our potential downstream costs of breaking changes in v1 (yay mill-mima!) or the threat of an unshaded v2.

I know this was a long answer. I'd love to hear your thoughts. Original Authorhtmldoug

[EindbaasExpress]

Hi @htmldoug, Thank you very much for the detailed answer! I'm happy you appreciate the effort put into porting MiMa to Mill, I really hope that will help us stabilize Mill's ecosystem. It is far from complete since it's hard to define complex scenarios for forward and backward compatibility, but it is a good starting point :) I did not know about weePickle at all until reviewing your PR. I really appreciate your effort in porting back some bug fixes to the upickle library. As you said, the two libraries diverged so much that a re-unification is very hard.. Also, only thinking about having to migrate 338 repositories makes me want to never have asked this question altogether 😄 But porting bug-fixes and improvements back would be really great for upickle.

For example, I spent some time trying to fix the Scala 3 macros (without success) to allow generating a ReadWriter for Scala 3 enums. The other day, after having given up, I tried the test I had on weePickle and it works correctly! This makes me want to port the fix back, but I think I will need to spend some time understanding how the Scala 3 macros work first :)

About shading, I understand your pain. Maybe it is something we could investigate for the Li Haoyi ecosystem, but as you said, it would require an update of the Hands on Scala book which is not cool.

Porting back bug fixes is probably the best strategy, for now, I hope I can help with that.

Original Authorlolgab

[EindbaasExpress]

Thanks the response and for understanding so well.

@russellremple took the upickle scala 3 macros and tweaked them a bit in https://github.com/rallyhealth/weePickle/pull/81, so they should be quite similar. It's exciting to hear that they work for scala 3 enums, particularly since we didn't even test for that yet! I wonder what the difference is between the implementations.

Many of our macro changes were aimed at runtime performance. We found that the scala 3 upickle macro implementations have something like 2% of the throughput of the scala 2 macros. There's some low hanging fruit (especially around default values), but IIRC, we hit a frustrating ceiling at ~50%-75% vs scala 2. inline is delightfully simple, but I suspect we'll need the lower level reflection APIs to reach performance parity. I'm considering the weepickle implementations temporary until I get around to exploring the new macro system deeper. We both have some reading to do. :)

porting bug-fixes and improvements back would be really great for upickle.

For sure. It's great for everyone. Besides minor behavior differences and the weepickle naming (Writer == From, Reader == To, ReadWriter == FromTo), all the bones are the same. I want to keep cross-porting as straightforward as it can be. Original Authorhtmldoug

[EindbaasExpress]

I'm not sure if I'm doing something wrong here, but I'm trying to repro this on latest master and getting no joy. It seems to me like it's doing the thing that it should be doing?

lihaoyi upickle$ ./mill -i upickle.jvm[2.13.4].console
[154/154] upickle.jvm[2.13.4].console
Welcome to Scala 2.13.4 (OpenJDK 64-Bit Server VM, Java 11.0.7).
Type in expressions for evaluation. Or try :help.

scala> case class Person(@upickle.implicits.key("first_name") firstName: String = "N/A",
     |                   @upickle.implicits.key("last_name") lastName: String)
class Person

scala> implicit val rw: upickle.default.ReadWriter[Person] = upickle.default.macroRW
val rw: upickle.default.ReadWriter[Person] = upickle.core.Types$ReadWriter$$anon$3@1d40f597

scala> val json = """{"last_name": "Snow"}"""
val json: String = {"last_name": "Snow"}

scala> val p = upickle.default.read[Person](json)
val p: Person = Person(N/A,Snow)

Original Authorlihaoyi

[EindbaasExpress]

Looking at the stack trace, this looks like a Scala3 specific issue CC @jodersky Original Authorlihaoyi

[EindbaasExpress]

@jodersky here's a fix that seems to make the given test cases pass:

lihaoyi upickle$ git diff
diff --git a/implicits/src-3/upickle/implicits/macros.scala b/implicits/src-3/upickle/implicits/macros.scala
index fcbf4a3..f03d41e 100644
--- a/implicits/src-3/upickle/implicits/macros.scala
+++ b/implicits/src-3/upickle/implicits/macros.scala
@@ -10,9 +10,11 @@ def getDefaultParmasImpl[T](using Quotes, Type[T]): Expr[Map[String, AnyRef]] =

   if (sym.isClassDef) {
     val comp = if (sym.isClassDef) sym.companionClass else sym
-    val names =
-      for p <- sym.caseFields if p.flags.is(Flags.HasDefault)
-      yield p.name
+    val hasDefaults =
+      for p <- sym.caseFields
+      yield p.flags.is(Flags.HasDefault)
+    val names = fieldLabelsImpl0[T].zip(hasDefaults).collect{case (n, true) => n}
+
     val namesExpr: Expr[List[String]] =
       Expr.ofList(names.map(Expr(_)))

@@ -45,20 +47,24 @@ def extractKey[A](using Quotes)(sym: quotes.reflect.Symbol): Option[String] =
 end extractKey

 inline def fieldLabels[T] = ${fieldLabelsImpl[T]}
-def fieldLabelsImpl[T](using Quotes, Type[T]): Expr[List[String]] =
+
+def fieldLabelsImpl0[T](using Quotes, Type[T]): List[String] =
   import quotes.reflect._
   val fields: List[Symbol] = TypeTree.of[T].symbol
     .primaryConstructor
     .paramSymss
     .flatten

-  val names = fields.map{ sym =>
+  fields.map{ sym =>
     extractKey(sym) match {
       case Some(name) => name
       case None => sym.name
     }
   }
-  Expr.ofList(names.map(Expr(_)))
+end fieldLabelsImpl0
+
+def fieldLabelsImpl[T](using Quotes, Type[T]): Expr[List[String]] =
+  Expr.ofList(fieldLabelsImpl0[T].map(Expr(_)))
 end fieldLabelsImpl

 inline def isMemberOfSealedHierarchy[T]: Boolean = ${ isMemberOfSealedHierarchyImpl[T] }

I'm not familiar with Scala 3 macros at all, so perhaps this can be done in a better way, but this seems to work at least enough for my cursory testing Original Authorlihaoyi

[EindbaasExpress]

should be fixed in 1.4.2 Original Authorlihaoyi