From 92a5da0952b681b876696154e9cbcfb97a9587d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Chantepie?= Date: Wed, 16 Mar 2022 22:12:36 +0100 Subject: [PATCH 1/2] Failing test for issue #728 --- .../play/api/libs/json/QuotesSpec.scala | 63 ++++++++++++++++++- 1 file changed, 62 insertions(+), 1 deletion(-) diff --git a/play-json/shared/src/test/scala-3/play/api/libs/json/QuotesSpec.scala b/play-json/shared/src/test/scala-3/play/api/libs/json/QuotesSpec.scala index 0075df870..5697e1cdc 100644 --- a/play-json/shared/src/test/scala-3/play/api/libs/json/QuotesSpec.scala +++ b/play-json/shared/src/test/scala-3/play/api/libs/json/QuotesSpec.scala @@ -70,7 +70,7 @@ final class QuotesSpec extends AnyWordSpec with Matchers with org.scalatestplus. "from Foo" in { testWithTuple( Foo("1", 2) - ).mustEqual("scala.Tuple2[scala.Predef.String, scala.Int]/Foo(1,2)") + ) mustEqual "scala.Tuple2[scala.Predef.String, scala.Int]/Foo(1,2)" } "from generic Bar" in { @@ -87,6 +87,39 @@ final class QuotesSpec extends AnyWordSpec with Matchers with org.scalatestplus. ) } + "from BigFat" in { + val tooBig = BigFat( + a = 1, + b = 2D, + c = 3F, + d = "d", + e = Seq(1, 2, 3), + f = 6, + g = 7D, + h = 8F, + i = "i", + j = Seq(4, 5), + k = 10, + l = 11D, + m = 12F, + n = "n", + o = Seq(6, 7), + p = 13, + q = 14D, + r = 15F, + s = "s", + t = Seq(8), + u = 16F, + v = "v", + w = Seq(9, 10, 11), + x = 12, + y = Seq(13, 14), + z = 15D + ) + + testWithTuple[BigFat](tooBig).mustEqual("TODO") + } + "from non-case class" when { "fail when there is no Conversion[T, _ <: Product]" in { """testWithTuple(new TestUnion.UC("name", 2))""".mustNot(typeCheck) @@ -143,6 +176,34 @@ case class Foo(bar: String, lorem: Int) case class Bar[T](name: String, opt: Option[T], scores: Seq[Double]) +case class BigFat( + a: Int, + b: Double, + c: Float, + d: String, + e: Seq[Int], + f: Int, + g: Double, + h: Float, + i: String, + j: Seq[Int], + k: Int, + l: Double, + m: Float, + n: String, + o: Seq[Int], + p: Int, + q: Double, + r: Float, + s: String, + t: Seq[Int], + u: Float, + v: String, + w: Seq[Int], + x: Int, + y: Seq[Int], + z: Double) + object TestUnion: sealed trait UT case object UA extends UT From 4880314d8d959316e01451fbefba5751dba6c5dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Chantepie?= Date: Wed, 16 Mar 2022 23:05:13 +0100 Subject: [PATCH 2/2] Fix --- .scalafmt.conf | 2 +- .../play/api/libs/json/JsMacroImpl.scala | 3 +- .../play/api/libs/json/QuotesHelper.scala | 58 ++++---- .../main/scala/play/api/libs/json/Json.scala | 3 +- .../play/api/libs/json/MacroScala3Spec.scala | 45 +++++- .../play/api/libs/json/QuotesSpec.scala | 135 ++++++++++-------- .../play/api/libs/json/TestMacros.scala | 18 +-- 7 files changed, 153 insertions(+), 111 deletions(-) diff --git a/.scalafmt.conf b/.scalafmt.conf index 9a85e9e1d..e2a9e5d2b 100644 --- a/.scalafmt.conf +++ b/.scalafmt.conf @@ -22,7 +22,7 @@ rewrite.neverInfix.excludeFilters = [ # better for play-json dsl and, andKeep, andThen, # For scalatest - in, should, when, must mustEqual, mustBe, "must_===" + in, should, when, must mustEqual, mustNot, mustBe, "must_===" ] rewrite.sortModifiers.order = [ "private", diff --git a/play-json/shared/src/main/scala-3/play/api/libs/json/JsMacroImpl.scala b/play-json/shared/src/main/scala-3/play/api/libs/json/JsMacroImpl.scala index ee812c2d3..ddd4806b5 100644 --- a/play-json/shared/src/main/scala-3/play/api/libs/json/JsMacroImpl.scala +++ b/play-json/shared/src/main/scala-3/play/api/libs/json/JsMacroImpl.scala @@ -673,8 +673,7 @@ object JsMacroImpl { // TODO: debug ${ f('{ ok }) } } - val (tupleTpe, withTupled) = - withTuple[T, P, JsObject](tpr, toProduct, types) + val (tupleTpe, withTupled) = withTuple[T, P, JsObject](tpr, toProduct) def writeFields(input: Expr[T]): Expr[JsObject] = withTupled(input) { tupled => diff --git a/play-json/shared/src/main/scala-3/play/api/libs/json/QuotesHelper.scala b/play-json/shared/src/main/scala-3/play/api/libs/json/QuotesHelper.scala index 26594b58b..3434a903a 100644 --- a/play-json/shared/src/main/scala-3/play/api/libs/json/QuotesHelper.scala +++ b/play-json/shared/src/main/scala-3/play/api/libs/json/QuotesHelper.scala @@ -14,7 +14,6 @@ import scala.quoted.Expr import scala.quoted.Quotes import scala.quoted.Type -// TODO: Unit tests private[json] trait QuotesHelper { protected type Q <: Quotes @@ -94,14 +93,14 @@ private[json] trait QuotesHelper { @annotation.tailrec private def withElems[U <: Product]( tupled: Expr[U], - fields: List[(Symbol, TypeRepr, Symbol)], + fields: List[(Symbol, TypeRepr, Term => Term)], prepared: List[Tuple2[String, (Ref => Term) => Term]] ): Map[String, (Ref => Term) => Term] = fields match { case (sym, t, f) :: tail => { val elem = ValDef.let( Symbol.spliceOwner, - s"tuple${f.name}", - Typed(tupled.asTerm.select(f), Inferred(t)) + s"tuple${sym.name}", + Typed(f(tupled.asTerm), Inferred(t)) ) withElems(tupled, tail, (sym.name -> elem) :: prepared) @@ -121,16 +120,32 @@ private[json] trait QuotesHelper { decls: List[(Symbol, TypeRepr)], debug: String => Unit ): Map[String, (Term => Term) => Term] = { - val fields = decls.zipWithIndex.flatMap { case ((sym, t), i) => - val field = tupleTpe.typeSymbol.declaredMethod(s"_${i + 1}") + val tupleTpeSym = tupleTpe.typeSymbol - field.map { meth => - debug( - s"// Field: ${sym.owner.owner.fullName}.${sym.name}, type = ${t.typeSymbol.fullName}, annotations = [${sym.annotations.map(_.show).mkString(", ")}]" - ) + val fields = decls.zipWithIndex.map { case ((sym, t), i) => + debug( + s"// Field: ${sym.owner.owner.fullName}.${sym.name}, type = ${t.typeSymbol.fullName}, annotations = [${sym.annotations.map(_.show).mkString(", ")}]" + ) + + val fieldNme = s"_${i + 1}" + + val resolve: Term => Term = { + val field = tupleTpeSym.declaredField(fieldNme) - Tuple3(sym, t, meth) + if (field == Symbol.noSymbol) { + tupleTpeSym.declaredMethod(fieldNme) match { + case meth :: Nil => + (_: Term).select(meth) + + case _ => + report.errorAndAbort(s"Fails to resolve field: '${tupleTpeSym.fullName}.${fieldNme}'") + } + } else { + (_: Term).select(field) + } } + + Tuple3(sym, t, resolve) } withElems[U](tupled, fields, List.empty) @@ -143,7 +158,6 @@ private[json] trait QuotesHelper { * * @param tpr the type for which a `ProductOf` is provided * @param toProduct the function to convert the input value as product `U` - * @param types the types of the elements (fields) * * @return The tuple type + `{ v: Term => { tuple: Ref => ... } }` * with `v` a term of type `tpe`, and `tuple` the product created from. @@ -151,22 +165,11 @@ private[json] trait QuotesHelper { def withTuple[T, U <: Product, R: Type]( tpr: TypeRepr, toProduct: Expr[T => U], - types: List[TypeRepr] )(using - Type[T], - Type[U] - ): Tuple2[TypeRepr, Expr[T] => (Expr[U] => Expr[R]) => Expr[R]] = { - val unappliedTupleTpr: TypeRepr = { - if (types.isEmpty) { - TypeRepr.of[EmptyTuple] - } else { - TypeRepr.typeConstructorOf(Class.forName(s"scala.Tuple${types.size}")) - } - } - - val tupleTpr = unappliedTupleTpr.appliedTo(types) - - tupleTpr -> { + tt: Type[T], + ut: Type[U] + ): Tuple2[TypeRepr, Expr[T] => (Expr[U] => Expr[R]) => Expr[R]] = + TypeRepr.of(using ut) -> { (in: Expr[T]) => { (f: (Expr[U] => Expr[R])) => '{ @@ -175,7 +178,6 @@ private[json] trait QuotesHelper { } } } - } /** * Returns the elements type for `product`. diff --git a/play-json/shared/src/main/scala/play/api/libs/json/Json.scala b/play-json/shared/src/main/scala/play/api/libs/json/Json.scala index 7b07de236..6744e1341 100644 --- a/play-json/shared/src/main/scala/play/api/libs/json/Json.scala +++ b/play-json/shared/src/main/scala/play/api/libs/json/Json.scala @@ -227,7 +227,8 @@ object Json extends JsonFacade with JsMacros with JsValueMacros { JsValueWrapperImpl(w.writes(field)) def obj(fields: (String, JsValueWrapper)*): JsObject = JsObject(fields.map(f => (f._1, unwrap(f._2)))) - def arr(items: JsValueWrapper*): JsArray = JsArray(items.iterator.map(unwrap).toArray[JsValue]) + + def arr(items: JsValueWrapper*): JsArray = JsArray(items.iterator.map(unwrap).toArray[JsValue]) // Passed nulls will typecheck without needing the implicit conversion, so they need to checked at runtime private def unwrap(wrapper: JsValueWrapper) = wrapper match { diff --git a/play-json/shared/src/test/scala-3/play/api/libs/json/MacroScala3Spec.scala b/play-json/shared/src/test/scala-3/play/api/libs/json/MacroScala3Spec.scala index c5ec567fb..4870cf945 100644 --- a/play-json/shared/src/test/scala-3/play/api/libs/json/MacroScala3Spec.scala +++ b/play-json/shared/src/test/scala-3/play/api/libs/json/MacroScala3Spec.scala @@ -13,14 +13,47 @@ final class MacroScala3Spec with org.scalatestplus.scalacheck.ScalaCheckPropertyChecks { "Case class" should { "not be handled" when { - "no Product Conversion" in { - import MacroSpec.UsingAlias - - "Macros.writer[UsingAlias]".mustNot(typeCheck) + "no custom ProductOf" in { + "Json.writes[CustomNoProductOf]" mustNot typeCheck } + } - "no custom ProductOf" in { - "Macros.writer[CustomNoProductOf]".mustNot(typeCheck) + "be handled" when { + "is declared with more than 22 fields" in { + val format = Json.format[BigFat] + + format + .writes(BigFat.example) + .mustEqual( + Json.obj( + "e" -> Seq(1, 2, 3), + "n" -> "n", + "t" -> Seq(8), + "a" -> 1, + "m" -> 12, + "i" -> "i", + "v" -> "v", + "p" -> 13, + "r" -> 15, + "w" -> Seq(9, 10, 11), + "k" -> 10, + "s" -> "s", + "x" -> 12, + "j" -> Seq(4, 5), + "y" -> Seq(13, 14), + "u" -> 16, + "f" -> 6, + "q" -> 14, + "b" -> 2, + "g" -> 7, + "l" -> 11, + "c" -> 3, + "h" -> 8, + "o" -> Seq(6, 7), + "z" -> 15, + "d" -> "d" + ) + ) } } } diff --git a/play-json/shared/src/test/scala-3/play/api/libs/json/QuotesSpec.scala b/play-json/shared/src/test/scala-3/play/api/libs/json/QuotesSpec.scala index 5697e1cdc..3ca4d4ba3 100644 --- a/play-json/shared/src/test/scala-3/play/api/libs/json/QuotesSpec.scala +++ b/play-json/shared/src/test/scala-3/play/api/libs/json/QuotesSpec.scala @@ -70,54 +70,29 @@ final class QuotesSpec extends AnyWordSpec with Matchers with org.scalatestplus. "from Foo" in { testWithTuple( Foo("1", 2) - ) mustEqual "scala.Tuple2[scala.Predef.String, scala.Int]/Foo(1,2)" + ).mustEqual("play.api.libs.json.Foo/Foo(1,2)") } "from generic Bar" in { testWithTuple( Bar[Double]("bar1", None, Seq(1.2D, 34.5D)) ).mustEqual( - "scala.Tuple3[scala.Predef.String, scala.Option[scala.Double], scala.collection.immutable.Seq[scala.Double]]/Bar(bar1,None,List(1.2, 34.5))" + "play.api.libs.json.Bar[scala.Double]/Bar(bar1,None,List(1.2, 34.5))" ) testWithTuple( Bar[Int]("bar2", Some(2), Seq(3.45D)) ).mustEqual( - "scala.Tuple3[scala.Predef.String, scala.Option[scala.Int], scala.collection.immutable.Seq[scala.Double]]/Bar(bar2,Some(2),List(3.45))" + "play.api.libs.json.Bar[scala.Int]/Bar(bar2,Some(2),List(3.45))" ) } "from BigFat" in { - val tooBig = BigFat( - a = 1, - b = 2D, - c = 3F, - d = "d", - e = Seq(1, 2, 3), - f = 6, - g = 7D, - h = 8F, - i = "i", - j = Seq(4, 5), - k = 10, - l = 11D, - m = 12F, - n = "n", - o = Seq(6, 7), - p = 13, - q = 14D, - r = 15F, - s = "s", - t = Seq(8), - u = 16F, - v = "v", - w = Seq(9, 10, 11), - x = 12, - y = Seq(13, 14), - z = 15D - ) + List( + "play.api.libs.json.BigFat/BigFat(1,2.0,3.0,d,List(1, 2, 3),6,7.0,8.0,i,List(4, 5),10,11.0,12.0,n,List(6, 7),13,14.0,15.0,s,List(8),16.0,v,List(9, 10, 11),12,List(13, 14),15.0)", // JVM: With .0 for Double & Float + "play.api.libs.json.BigFat/BigFat(1,2,3,d,List(1, 2, 3),6,7,8,i,List(4, 5),10,11,12,n,List(6, 7),13,14,15,s,List(8),16,v,List(9, 10, 11),12,List(13, 14),15)" + ).contains(testWithTuple[BigFat](BigFat.example)).mustEqual(true) - testWithTuple[BigFat](tooBig).mustEqual("TODO") } "from non-case class" when { @@ -130,7 +105,7 @@ final class QuotesSpec extends AnyWordSpec with Matchers with org.scalatestplus. testWithTuple( new TestUnion.UC("name", 2) - ).mustEqual("scala.Tuple$package.EmptyTuple/(name,2)") + ).mustEqual("scala.Tuple2[scala.Predef.String, scala.Int]/(name,2)") } "be successful when conversion is provided" in { @@ -154,6 +129,16 @@ final class QuotesSpec extends AnyWordSpec with Matchers with org.scalatestplus. Bar("bar3", Some("opt2"), Seq(3.1D, 4.5D)) ).mustEqual("name=bar3,opt=Some(opt2),scores=List(3.1, 4.5)") } + + "BigFat" in { + val jvmToStr = + "e=List(1, 2, 3),n=n,t=List(8),a=1,m=12.0,i=i,v=v,p=13,r=15.0,w=List(9, 10, 11),k=10,s=s,x=12,j=List(4, 5),y=List(13, 14),u=16.0,f=6,q=14.0,b=2.0,g=7.0,l=11.0,c=3.0,h=8.0,o=List(6, 7),z=15.0,d=d" // With .0 for decimal + + val jsToStr = + "e=List(1, 2, 3),n=n,t=List(8),a=1,m=12,i=i,v=v,p=13,r=15,w=List(9, 10, 11),k=10,s=s,x=12,j=List(4, 5),y=List(13, 14),u=16,f=6,q=14,b=2,g=7,l=11,c=3,h=8,o=List(6, 7),z=15,d=d" + + Seq(jvmToStr, jsToStr).contains(testWithFields(BigFat.example)).mustEqual(true) + } } } @@ -177,32 +162,64 @@ case class Foo(bar: String, lorem: Int) case class Bar[T](name: String, opt: Option[T], scores: Seq[Double]) case class BigFat( - a: Int, - b: Double, - c: Float, - d: String, - e: Seq[Int], - f: Int, - g: Double, - h: Float, - i: String, - j: Seq[Int], - k: Int, - l: Double, - m: Float, - n: String, - o: Seq[Int], - p: Int, - q: Double, - r: Float, - s: String, - t: Seq[Int], - u: Float, - v: String, - w: Seq[Int], - x: Int, - y: Seq[Int], - z: Double) + a: Int, + b: Double, + c: Float, + d: String, + e: Seq[Int], + f: Int, + g: Double, + h: Float, + i: String, + j: Seq[Int], + k: Int, + l: Double, + m: Float, + n: String, + o: Seq[Int], + p: Int, + q: Double, + r: Float, + s: String, + t: Seq[Int], + u: Float, + v: String, + w: Seq[Int], + x: Int, + y: Seq[Int], + z: Double +) + +object BigFat { + def example = BigFat( + a = 1, + b = 2D, + c = 3F, + d = "d", + e = Seq(1, 2, 3), + f = 6, + g = 7D, + h = 8F, + i = "i", + j = Seq(4, 5), + k = 10, + l = 11D, + m = 12F, + n = "n", + o = Seq(6, 7), + p = 13, + q = 14D, + r = 15F, + s = "s", + t = Seq(8), + u = 16F, + v = "v", + w = Seq(9, 10, 11), + x = 12, + y = Seq(13, 14), + z = 15D + ) +} object TestUnion: sealed trait UT diff --git a/play-json/shared/src/test/scala-3/play/api/libs/json/TestMacros.scala b/play-json/shared/src/test/scala-3/play/api/libs/json/TestMacros.scala index a38113d85..3a0f93fb5 100644 --- a/play-json/shared/src/test/scala-3/play/api/libs/json/TestMacros.scala +++ b/play-json/shared/src/test/scala-3/play/api/libs/json/TestMacros.scala @@ -84,19 +84,10 @@ object TestMacros: } val tpe = TypeRepr.of[T] - val tpeElements = Expr - .summon[ProductOf[T]] - .map { - helper.productElements(tpe, _).get - } - .getOrElse(List.empty[(Symbol, TypeRepr)]) - - val types = tpeElements.map(_._2) - - val (tupleTpe, withTuple) = - helper.withTuple[T, P, String](tpe, toProduct, types) + val (tupleTpe, withTupled) = + helper.withTuple[T, P, String](tpe, toProduct) - withTuple(pure) { (tupled: Expr[P]) => + withTupled(pure) { (tupled: Expr[P]) => val a = Expr(tupleTpe.show) '{ @@ -127,10 +118,9 @@ object TestMacros: helper.productElements(tpe, _).get } .get - val types = tpeElements.map(_._2) val (tupleTpe, withTuple) = - helper.withTuple[T, T, String](tpe, '{ identity[T] }, types) + helper.withTuple[T, T, String](tpe, '{ identity[T] }) withTuple(pure) { (tupled: Expr[T]) => val fieldMap =