Using Scala 3 seriously breaks default value handing

[EindbaasExpress]

Minimal example:

case class A(a: Int = 1)

import upickle.default._
implicit val rw: ReadWriter[A] = macroRW[A]

println(read[A]("{\"a\":2}"))
println(read[A]("{}"))

Expected (upickle 1.4.0, Scala 2.13.6):

A(2)
A(1)

Got (upickle 1.4.0, Scala 3.0.1):

A(2)
A(2)

God knows how many projects are silently corrupting their databases. Or leaking data from another user in their API calls. Since the parser is keeping and leaking state, everything is possible. ID: 362 Original Author: strelec

[EindbaasExpress]

There seems to be some state capture involved. It appears that the default value depends on the previously parsed value.

Extending the example above:

case class A(a: Int = 1)

import upickle.default._
implicit val rw: ReadWriter[A] = macroRW[A]

@main def run() =
  println(read[A]("{}"))
  println(read[A]("{\"a\":2}"))
  println(read[A]("{}"))
  println(read[A]("{\"a\":3}"))
  println(read[A]("{}"))
  println(read[A]("{\"a\":4}"))
  println(read[A]("{\"a\":5}"))
  println(default.read[A]("{}"))
  println(default.read[A]("{}"))

prints:

A(1)
A(2)
A(2)
A(3)
A(3)
A(4)
A(5)
A(5)
A(5)
A(5)

I can confirm that it has nothing to do with the wildcard import or the use of implicit (given has the same behavior). Original Authorjodersky

[EindbaasExpress]

Until we find and fix the root cause of this, the workaround is to use an implicit def, not a val.

Changing: implicit val rw: ReadWriter[A] = macroRW[A] to implicit def rw: ReadWriter[A] = macroRW[A] works as expected. Original Authorjodersky

[EindbaasExpress]

I think this issue is so serious we should unpublish Scala 3 support (and/or all affected versions).

Let me know what you think. Original Authorstrelec

[EindbaasExpress]

Maven doesn't work that way, you can't unpublish something. Also, that would break even more people's deployments. That being said, this is a very high prio issue of course. Original Authorjodersky

[EindbaasExpress]

I havent looked at the code, but from the discussion it sounds like we're putting the mutable variables inside the (possibly long lived) Visitor body when we should be putting them in the (short loved) ObjVisitor/ArrVisitor bodirs instead

On Fri, 27 Aug 2021 at 2:25 AM, jodersky @.***> wrote:

Maven doesn't work that way, you can't unpublish something. Also, that would break even more people's deployments. That being said, this is a very high prio issue of course.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/com-lihaoyi/upickle/issues/362#issuecomment-906640281, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHEB7BADTGUIZVDVI2VD4LT62BJJANCNFSM5CZV6SGQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

Original Authorlihaoyi

[EindbaasExpress]

Yes it looks exactly like that. I saw that there's a "builder" val in the parent Visitor, when, as you said, I reckon it should be scoped to the ObjVisitor.

When I move it into the ObjVisitor, the problem described here is fixed, however some other tests fail. I'll need to look into it further. Original Authorjodersky

[EindbaasExpress]

Why did you release a new version with Scala 3 support, despite at least 2 serious serialization bugs? 1.4.1 should have been released without Scala 3 support at the time. Original Authorstrelec

[EindbaasExpress]

There is a known issue indeed, but please keep in mind that we're all volunteers here and have other activities in our lives. Sometimes we can't immediately fix a bug when one is reported. Pull requests with fixes are welcome any time however.

As for this issue in particular, note that upickle was one of the first macro-heavy libraries ported to Scala 3 while it was still called Dotty. We uncovered some bugs in the Dotty compiler itself during the effort, and sometimes had to adapt test suites in order to move forward. The goal of course is always to write correct code, but sometimes things slip through.

As for the release, it is managed by an automatic process. Furthermore, I don't think anyone is served by stopping to provide a library which has already been published and is functional in almost all regards except for a few issues, regardless of their severity. Original Authorjodersky

[EindbaasExpress]

I know you are volunteers. I also think mr. Li took too much burden to carry. The result is a lot of sporadically supported libraries, which is unfortunate. :(

The only thing I was suggesting is to omit Scala 3 support from the future releases, until the issues are fixed. Above we talked about unpublishing, which is not possible. But not publishing more broken versions is reasonable.

I have already written my own Scala 3 macros for JSON pickling, so no support needed for me. I am merely on this thread for other peoiple, as this bug could have disastrous consequences (privacy leaks and database corruption) and I have discovered it. Original Authorstrelec

[EindbaasExpress]

@jodersky if I'm not mistaken I think this is the fix

lihaoyi upickle$ git diff
diff --git a/build.sc b/build.sc
index 133b30f..8664795 100644
--- a/build.sc
+++ b/build.sc
@@ -14,7 +14,7 @@ import com.github.lolgab.mill.mima._
 val scala211  = "2.11.12"
 val scala212  = "2.12.13"
 val scala213  = "2.13.4"
-val scala3    = "3.0.0"
+val scala3    = "3.0.2"
 val scalaJS06 = "0.6.33"
 val scalaJS1  = "1.5.1"
 val scalaNative = "0.4.0"
diff --git a/implicits/src-3/upickle/implicits/CaseClassReader.scala b/implicits/src-3/upickle/implicits/CaseClassReader.scala
index de66b16..e21a6b1 100644
--- a/implicits/src-3/upickle/implicits/CaseClassReader.scala
+++ b/implicits/src-3/upickle/implicits/CaseClassReader.scala
@@ -11,9 +11,8 @@ trait CaseClassReaderPiece extends MacrosCommon:

     def visitorForKey(currentKey: String): Visitor[_, _]

-    private val builder = collection.mutable.Map.empty[String, Any]
-
     override def visitObject(length: Int, index: Int) = new ObjVisitor[Any, T] {
+      private val builder = collection.mutable.Map.empty[String, Any]
       var currentKey: String = null

       def subVisitor: Visitor[_, _] = visitorForKey(currentKey)
diff --git a/implicits/src-3/upickle/implicits/CaseClassWriter.scala b/implicits/src-3/upickle/implicits/CaseClassWriter.scala
index f2802c0..f49368c 100644
--- a/implicits/src-3/upickle/implicits/CaseClassWriter.scala
+++ b/implicits/src-3/upickle/implicits/CaseClassWriter.scala
@@ -14,8 +14,8 @@ trait CaseClassWriterPiece extends MacrosCommon:
       var n = 0
       for
         (name, _, value) <- elemsInfo(v)
-        defaultValue <- defaultParams.get(name)
-        if defaultValue != value || serializeDefaults
+        defaultValue = defaultParams.get(name)
+        if serializeDefaults || defaultValue.isEmpty || defaultValue.get != value
       do n += 1
       n
     end length

Looking at the Scala 3 macro code, it diverges from the Scala 2 implementation considerably; I haven't benched it, but I bet the Scala 3 performance won't be as good, as it seems to allocate a ton of stuff (it uses for-loops!? collections!?!? hashtables!?!?!?) that the Scala 2 version painstakingly avoids. We should definitely look into unifying them. They may do the same thing, but for a performance critical piece of code the Scala 2 version is what we want

Regardless, this small patch should fix this particular issue Original Authorlihaoyi