Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 13 commits into from
Jul 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions build.sc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason for this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

val scalaNative = "0.5.0"
val acyclic = "0.3.12"

Expand Down Expand Up @@ -98,8 +98,8 @@ trait CommonPublishModule
}

def scalacOptions = T {
Seq("-unchecked", "-deprecation", "-encoding", "utf8", "-feature", "-Xfatal-warnings") ++
Agg.when(!isScala3(scalaVersion()))("-opt:l:method").toSeq
Seq("-unchecked", "-encoding", "utf8", "-feature") ++
Agg.when(!isScala3(scalaVersion()))("-opt:l:method", "-Xfatal-warnings", "-deprecation").toSeq
}

trait CommonTestModule0 extends ScalaModule with TestModule.Utest {
Expand Down
13 changes: 6 additions & 7 deletions upickle/implicits/src-3/upickle/implicits/Readers.scala
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ trait ReadersVersionSpecific

abstract class CaseClassReader3[T](paramCount: Int,
missingKeyCount: Long,
allowUnknownKeys: Boolean) extends CaseClassReader[T] {
allowUnknownKeys: Boolean,
construct: Array[Any] => T) extends CaseClassReader[T] {

def visitors0: Product
lazy val visitors = visitors0
def fromProduct(p: Product): T
Expand Down Expand Up @@ -45,11 +47,7 @@ trait ReadersVersionSpecific
if (this.checkErrorMissingKeys(missingKeyCount))
this.errorMissingKeys(paramCount, allKeysArray)

fromProduct(new Product {
def canEqual(that: Any): Boolean = true
def productArity: Int = params.length
def productElement(i: Int): Any = params(i)
})
construct(params)
}
override def visitObject(length: Int,
jsonableKeys: Boolean,
Expand All @@ -63,7 +61,8 @@ trait ReadersVersionSpecific
val reader = new CaseClassReader3[T](
macros.paramsCount[T],
macros.checkErrorMissingKeysCount[T](),
macros.extractIgnoreUnknownKeys[T]().headOption.getOrElse(this.allowUnknownKeys)
macros.extractIgnoreUnknownKeys[T]().headOption.getOrElse(this.allowUnknownKeys),
params => macros.applyConstructor[T](params)
){
override def visitors0 = compiletime.summonAll[Tuple.Map[m.MirroredElemTypes, Reader]]
override def fromProduct(p: Product): T = m.fromProduct(p)
Expand Down
48 changes: 48 additions & 0 deletions upickle/implicits/src-3/upickle/implicits/macros.scala
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,54 @@ def tagKeyImpl[T](using Quotes, Type[T])(thisOuter: Expr[upickle.core.Types with
case None => '{${thisOuter}.tagName}
}

inline def applyConstructor[T](params: Array[Any]): T = ${ applyConstructorImpl[T]('params) }
def applyConstructorImpl[T](using quotes: Quotes, t0: Type[T])(params: Expr[Array[Any]]): Expr[T] =
import quotes.reflect._
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
Comment on lines +207 to +209

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

val rhs = params0.zipWithIndex.map {
case (sym0, i) =>
val lhs = '{$params(${ Expr(i) })}
val tpe0 = constructorTpe.memberType(sym0)

typeApply.map(tps => tpe0.substituteTypes(tparams0, tps)).getOrElse(tpe0) 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 => Select.overloaded(Ref(companion), "apply", Nil, rhs).asExprOf[T]
case Some(args) =>
Select.overloaded(Ref(companion), "apply", args, rhs).asExprOf[T]
Copy link
Contributor

@bishabosha bishabosha Jul 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@bishabosha bishabosha Jul 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@bishabosha bishabosha Jul 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

}
}

TypeRepr.of[T] match{
case t: AppliedType => apply(Some(t.args))
case t: TypeRef => apply(None)
case t: TermRef => '{${Ref(t.classSymbol.get.companionModule).asExprOf[Any]}.asInstanceOf[T]}
}

inline def tagName[T]: String = ${ tagNameImpl[T] }
def tagNameImpl[T](using Quotes, Type[T]): Expr[String] =
tagNameImpl0(identity)
Expand Down
4 changes: 2 additions & 2 deletions upickle/test/src-3/upickle/DerivationTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,8 @@ object DerivationTests extends TestSuite {
val rwError = compileError("""given rw: upickle.default.ReadWriter[A] = upickle.default.macroRW""")
val rError = compileError("""given r: upickle.default.Reader[A] = upickle.default.macroR""")
val wError = compileError("""given w: upickle.default.Writer[A] = upickle.default.macroW""")
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"))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these can also be restored with 3.3.4-RC1

}
test("issue469"){
// Ensure that `import upickle.default.given` doesn't mess things up by
Expand Down
15 changes: 15 additions & 0 deletions upickle/test/src/upickle/AdvancedTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,14 @@ object AdvancedTests extends TestSuite {
val written = upickle.default.write(result)
assert(written == input)
}
test("customApply") {
val input = """{"x":"a","y":-102}"""
val expected = CustomApply("A", +102)
val result = upickle.default.read[CustomApply](input)
assert(result == expected)
val written = upickle.default.write(result)
assert(written == """{"x":"A","y":102}""")
}
}
}

Expand All @@ -352,5 +360,12 @@ object AdvancedTests extends TestSuite {
class Thing[T: upickle.default.Writer, V: upickle.default.Writer](t: Option[(V, T)]) {
implicitly[upickle.default.Writer[Option[(V, T)]]]
}
case class CustomApply(x: String, y: Int)

object CustomApply {
def apply(x: String, y: Int) = new CustomApply(x.toUpperCase, math.abs(y))

implicit val nodeRW: ReadWriter[CustomApply] = macroRW[CustomApply]
}
}

Loading