Make uPickle derivation in Scala 3 call `apply` method instead of `new` #607

lihaoyi · 2024-07-12T02:45:47Z

fixes #552

Bumps minimum Scala 3 version to 3.4.2 because 3.3.3 crashes with weird errors

lihaoyi · 2024-07-12T12:37:00Z

upickle/test/src-3/upickle/DerivationTests.scala

-      assert(rError.msg.contains("No given instance of type ReadersVersionSpecific_this.Reader[(A.B : A)] was found"))
-      assert(wError.msg.contains("No given instance of type WritersVersionSpecific_this.Writer[(A.B : A)] was found"))
+      //      assert(rError.msg.contains("No given instance of type ReadersVersionSpecific_this.Reader[(A.B : A)] was found"))
+      //      assert(wError.msg.contains("No given instance of type WritersVersionSpecific_this.Writer[(A.B : A)] was found"))


Not sure why this test broke, but this PR fixes happy-path behavior and this test covers failure-mode error message, so making the happy path work takes precedence

these can also be restored with 3.3.4-RC1

bishabosha · 2024-07-12T12:43:43Z

build.sc

@@ -17,7 +17,7 @@ import com.github.lolgab.mill.mima._
 val scala212  = "2.12.18"
 val scala213  = "2.13.11"

-val scala3   = "3.3.3"
+val scala3   = "3.4.2"


any reason for this?

Yeah, 3.3.3 fails with this exception

[error] |Exception occurred while executing macro expansion. [error] |java.lang.ClassCastException: class dotty.tools.dotc.ast.Trees$This cannot be cast to class dotty.tools.dotc.ast.Trees$RefTree (dotty.tools.dotc.ast.Trees$This and dotty.tools.dotc.ast.Trees$RefTree are in unnamed module of loader mill.api.ClassLoader$$anon$1 @4f12ea86) [error] | at scala.quoted.runtime.impl.QuotesImpl.scala$quoted$runtime$impl$QuotesImpl$reflect$Ref$$$_$apply$$anonfun$6(QuotesImpl.scala:446) [error] | at scala.quoted.runtime.impl.QuotesImpl$reflect$.scala$quoted$runtime$impl$QuotesImpl$reflect$$$withDefaultPos(QuotesImpl.scala:3002) [error] | at scala.quoted.runtime.impl.QuotesImpl$reflect$Ref$.apply(QuotesImpl.scala:446) [error] | at scala.quoted.runtime.impl.QuotesImpl$reflect$Ref$.apply(QuotesImpl.scala:444) [error] | at upickle.implicits.macros.macros$package$.apply$1(macros.scala:221) [error] | at upickle.implicits.macros.macros$package$.applyConstructorImpl(macros.scala:255)

This is the code in question

val companion: Symbol = tpe.classSymbol.get.companionModule val constructorParamSymss = tpe.typeSymbol.primaryConstructor.paramSymss def isPrimaryApplyMethod(syms1: Seq[Seq[Symbol]], syms2: Seq[Seq[Symbol]]) = { // try to guess the primary apply method based on the parameter counts // not sure why comparing the types doesn't seem to work // println(syms1.flatten.zip(syms2.flatten).map{case (s1, s2) => (s1.typeRef.simplified, s2.typeRef.simplified)}) syms1.map(_.length) == syms2.map(_.length) } val applyMethods = companion .methodMember("apply") .filter(s => isPrimaryApplyMethod(s.paramSymss, constructorParamSymss)) val lhs = Select(Ref(companion), applyMethods.head)

It's the Ref.apply call that is failing. It was beyond my abilities to debug why, but somehow going onto 3.4.2 fixes it

ok, seems to be because of scala/scala3#19732, fixed in 3.4

it might not be easy to workaround that in 3.3.3 (like reimplementing Ref.apply)

oh it looks like the fix is backported to 3.3.4-RC1

lihaoyi · 2024-07-12T13:14:08Z

upickle/implicits/src-3/upickle/implicits/macros.scala

+    def isPrimaryApplyMethod(syms1: Seq[Seq[Symbol]], syms2: Seq[Seq[Symbol]]) = {
+      // try to guess the primary apply method based on the parameter counts
+      // not sure why comparing the types doesn't seem to work
+      // println(syms1.flatten.zip(syms2.flatten).map{case (s1, s2) => (s1.typeRef.simplified, s2.typeRef.simplified)})


@bishabosha do you have any idea how I can find the apply method with a signature matching the primary constructor? In Scala 2 I can just call apply in a quasiquote and it somehow resolves to the right now, but in Scala 3 it seems pickier and asks me to select which apply method I want to call ahead of time

I think you would still want to capture the MirroredElemTypes from the mirror - cast each element of the array to the type from MirroredElemTypes, and then call Select.overloaded(ref, "apply", args) - however then that makes it hard to adapt the varargs if needed - I wonder if its not needed

or yeah you maybe can manually resolve the right apply method by comparing the types of the mirrored elem types

I don't think you need the mirrored elem types - cast away with the primary constructor signatures

How do I compare the primary constructor signatures though? println(syms1.flatten.zip(syms2.flatten).map{case (s1, s2) => (s1.typeRef, s2.typeRef)}) seems to give me singleton TypeRefs or something for each of the parameters, and when I try to =:= them I get false. Also, if the case class i generic, then presumably the T type param for the primary constructor is different from the T type param from the apply method

there's the substituteTypes method, where you can substitute references to the constructor type parameters, with the type arguments of the typeclass

I added a suggestion that passes the tests

upickle/implicits/src-3/upickle/implicits/macros.scala

Co-authored-by: Jamie Thompson <[email protected]>

bishabosha · 2024-07-12T14:52:01Z

upickle/implicits/src-3/upickle/implicits/macros.scala

+    typeApply match{
+      case None => Select.overloaded(Ref(companion), "apply", Nil, rhs).asExprOf[T]
+      case Some(args) =>
+        Select.overloaded(Ref(companion), "apply", args, rhs).asExprOf[T]


just thought of this nasty example though (not tested) -

case class CustomApplyGenVararg[T](x: String, ys: T*) object CustomApplyGenVararg { def apply(x: String, ys: Int*) = new CustomApplyGenVararg[Int](x.toUpperCase, ys.map(math.abs(_)): _*) }

so there is no apply that matches the constructor

Is there some way to say "call apply, whichever one ends up neing resolved"? That's basically what it does in Scala 2, and it works great including edge cases such as this one

that should be what this overloaded does - runs overload resolution with the provided arguments

~~in which case, maybe you can always leave type arguments empty because the value arguments should be correct~~ - tried that and type arguments are needed :/

ok, looks like you can "try both" :)

typeApply match { case None => Select.overloaded(Ref(companion), "apply", Nil, rhs, tpe).asExprOf[T] case Some(args) => val term = try Select.overloaded(Ref(companion), "apply", Nil, rhs, tpe) // try in case there are no type parameters catch case scala.util.control.NonFatal(e) => // assume apply method needs type parameters Select.overloaded(Ref(companion), "apply", args, rhs, tpe) term.asExprOf[T] }

however it somehow still resolves to the synthetic "universal" apply method rather than the user defined one

case class CustomApplyGen2[T](x: String, y: T) object CustomApplyGen2 { def apply(x: String, y: Int) = new CustomApplyGen2[Int](x.toUpperCase, math.abs(y)) implicit def nodeRW[T: ReadWriter]: ReadWriter[CustomApplyGen2[T]] = macroRW[CustomApplyGen2[T]] } ... test("customApplyGen2") { val input = """{"x":"a","y":-102}""" val expected = CustomApplyGen2("A", +102) val result = upickle.default.read[CustomApplyGen2[Int]](input) assert(result == expected) val written = upickle.default.write(result) assert(written == """{"x":"A","y":102}""") } ... X upickle.AdvancedTests.issues.customApplyGen2 3ms utest.AssertionError: result == expected result: upickle.AdvancedTests.CustomApplyGen2[Int] = CustomApplyGen2(a,-102) expected: upickle.AdvancedTests.CustomApplyGen2[Int] = CustomApplyGen2(A,102) utest.asserts.Asserts$.assertImpl(Asserts.scala:30) upickle.AdvancedTests$.$init$$$anonfun$1$$anonfun$5$$anonfun$7(AdvancedTests.scala:108)

tschuchortdev · 2024-07-12T18:18:32Z

upickle/implicits/src-3/upickle/implicits/macros.scala

+    val companion: Symbol = tpe.classSymbol.get.companionModule
+    val constructorSym = tpe.typeSymbol.primaryConstructor
+    val constructorParamSymss = constructorSym.paramSymss


Are you sure that those always exist? Quotes API has a bad habit of returning Symbol.noSymbol instead of None. It is not obvious from the type signature that those methods can fail.

bishabosha · 2024-07-12T20:07:04Z

I think I have the right solution for matching the signatures - and rejects when there isn't a single match (seems to work well in the repl)

def apply(typeApply: Option[List[TypeRepr]]) = {
    val tpe = TypeRepr.of[T]
    val companion: Symbol = tpe.classSymbol.get.companionModule
    val constructorSym = tpe.typeSymbol.primaryConstructor
    val constructorParamSymss = constructorSym.paramSymss

    val (tparams0, params0) = constructorParamSymss.flatten.partition(_.isType)
    val constructorTpe = tpe.memberType(constructorSym).widen

    val companionRef = companion.termRef

    val sub: (tpe: TypeRepr, tparams: List[Symbol]) => TypeRepr =
      typeApply match
        case Some(tps) => (tpe, tparams) => tpe.substituteTypes(tparams, tps)
        case None => (tpe, tparams) => tpe

    val apps = for
      app <- companion.methodMember("apply")
      info = companionRef.memberType(app).widen
      (tps, ps) = app.paramSymss.flatten.partition(_.isType)
      if tps.length == tparams0.length
      if ps.length == params0.length
      if tps.corresponds(tparams0)((t0, t1) =>
        sub(info.memberType(t0), tps) =:= sub(constructorTpe.memberType(t1), tparams0)
      )
      if ps.corresponds(params0)((p0, p1) =>
        sub(info.memberType(p0), tps) =:= sub(constructorTpe.memberType(p1), tparams0)
      )
    yield app

    val matching = apps match
      case app :: Nil => app
      case _ => report.errorAndAbort(s"Cannot find a uniquely matching apply method in companion ${companionRef.show}")

    val lhs = Select(Ref(companion), matching)

    val rhs = params0.zipWithIndex.map {
      case (sym0, i) =>
        val lhs = '{$params(${ Expr(i) })}
        sub(constructorTpe.memberType(sym0), tparams0) match {
          case AnnotatedType(AppliedType(base, Seq(arg)), x)
            if x.tpe =:= defn.RepeatedAnnot.typeRef =>
            arg.asType match {
              case '[t] =>
                Typed(
                  lhs.asTerm,
                  TypeTree.of(using AppliedType(defn.RepeatedParamClass.typeRef, List(arg)).asType)
                )
            }
          case tpe =>
            tpe.asType match {
              case '[t] => '{ $lhs.asInstanceOf[t] }.asTerm
            }
        }

    }

    typeApply match{
      case None => lhs.appliedToArgs(rhs).asExprOf[T]
      case Some(args) => lhs.appliedToTypes(args).appliedToArgs(rhs).asExprOf[T]
    }

lihaoyi · 2024-07-13T00:47:33Z

Thanks @bishabosha ! I refactored the code a bit and updated the PR

lihaoyi · 2024-07-13T08:00:42Z

I don't think this quite follows the exact behavior of Scala 2. In particular, in Scala 2 the argument lists do not even need to match AFAIK; you can have an extra implicit parameter list the gets filled in by typechecking, or extra parameters with default values. But it will have to do for now, and we can refine the implementation further in future

DamianReeves · 2024-08-02T17:50:21Z

Is it possible to add a chore to revert the minimum Scala 3 version back down to Scala 3.3.4 once that becomes available with the backported changes?

It seems if a library depends on upickle 4.x it is forced to update to using Scala 3.4.x and it seems the intention of Scala Center is for library authors to avoid this, though the guidance does address needing new language features.

As upickle is a part of the toolkit and is used in many places in the ecosystem, it seems forcing the 3.4.2 here for the entirety of the upickle 4.x version will either limit users from upgrading or force the library authors using upickle to upgrade and thus produce a ripple effect.

I admit I may be dead wrong in my analysis here as the Scala LTS versioning strategy is still confusing to me. Also it seems we are around 2 years removed from the announcement of LTS, is a new LTS version perhaps on the horizon that means this is no big deal? 🤷🏾

lihaoyi · 2024-08-02T17:54:46Z

@DamianReeves yes, that's fine. We can add back support for 3.3.4 or 3.3.5 or whatever version has our bug fix backported. I just didn't want to hold up uPickle 4 for the folks on 2.12/2.13/3.4, but once we can compile on 3.3 we can publish a version supporting it

nafg · 2024-10-01T13:22:06Z

Did 3.3.4 fix the errors?

nafg · 2024-10-01T13:23:30Z

I guess it did #635

lihaoyi added 11 commits July 12, 2024 10:44

wip

6af7c4e

.

12faf00

.

50ac93b

.

dd5106a

wip

4519b78

.

98b1188

.

1b914e4

.

5e7785c

.

bc3c1ad

.

228f938

.

47a8347

lihaoyi commented Jul 12, 2024

View reviewed changes

.

3864e4d

bishabosha reviewed Jul 12, 2024

View reviewed changes

lihaoyi commented Jul 12, 2024

View reviewed changes

bishabosha reviewed Jul 12, 2024

View reviewed changes

upickle/implicits/src-3/upickle/implicits/macros.scala Outdated Show resolved Hide resolved

Update upickle/implicits/src-3/upickle/implicits/macros.scala

9c69d3f

Co-authored-by: Jamie Thompson <[email protected]>

bishabosha reviewed Jul 12, 2024

View reviewed changes

tschuchortdev reviewed Jul 12, 2024

View reviewed changes

lihaoyi merged commit 1683583 into main Jul 13, 2024
8 checks passed

lihaoyi deleted the upickle-apply branch July 13, 2024 08:01

Andrapyre mentioned this pull request Aug 21, 2024

Update upickle lumidion/sonatype-central-client#8

Closed

nox213 mentioned this pull request Oct 14, 2024

Use primary constructor instead of apply when create case class #637

Closed

nox213 mentioned this pull request Oct 14, 2024

Doesn't work well with custom apply on case class #624

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make uPickle derivation in Scala 3 call `apply` method instead of `new` #607

Make uPickle derivation in Scala 3 call `apply` method instead of `new` #607

lihaoyi commented Jul 12, 2024 •

edited

Loading

lihaoyi Jul 12, 2024

bishabosha Jul 12, 2024

bishabosha Jul 12, 2024

lihaoyi Jul 12, 2024

bishabosha Jul 12, 2024

bishabosha Jul 12, 2024

bishabosha Jul 12, 2024

lihaoyi Jul 12, 2024

bishabosha Jul 12, 2024

bishabosha Jul 12, 2024

bishabosha Jul 12, 2024

lihaoyi Jul 12, 2024

bishabosha Jul 12, 2024

bishabosha Jul 12, 2024

bishabosha Jul 12, 2024 •

edited

Loading

lihaoyi Jul 12, 2024

bishabosha Jul 12, 2024 •

edited

Loading

bishabosha Jul 12, 2024 •

edited

Loading

bishabosha Jul 12, 2024

tschuchortdev Jul 12, 2024

bishabosha commented Jul 12, 2024

lihaoyi commented Jul 13, 2024

lihaoyi commented Jul 13, 2024

DamianReeves commented Aug 2, 2024

lihaoyi commented Aug 2, 2024

nafg commented Oct 1, 2024

nafg commented Oct 1, 2024

Make uPickle derivation in Scala 3 call apply method instead of new #607

Make uPickle derivation in Scala 3 call apply method instead of new #607

Conversation

lihaoyi commented Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bishabosha Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bishabosha Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

bishabosha Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bishabosha commented Jul 12, 2024

lihaoyi commented Jul 13, 2024

lihaoyi commented Jul 13, 2024

DamianReeves commented Aug 2, 2024

lihaoyi commented Aug 2, 2024

nafg commented Oct 1, 2024

nafg commented Oct 1, 2024

Make uPickle derivation in Scala 3 call `apply` method instead of `new` #607

Make uPickle derivation in Scala 3 call `apply` method instead of `new` #607

lihaoyi commented Jul 12, 2024 •

edited

Loading

bishabosha Jul 12, 2024 •

edited

Loading

bishabosha Jul 12, 2024 •

edited

Loading

bishabosha Jul 12, 2024 •

edited

Loading