Check for forward reference in generated formats

playframework · Jun 26, 2021 · d3e15ce · d3e15ce
1 parent f1769bd
commit d3e15ce
Show file tree

Hide file tree

Showing 2 changed files with 105 additions and 12 deletions.
diff --git a/play-json/shared/src/main/scala-2/play/api/libs/json/JsMacroImpl.scala b/play-json/shared/src/main/scala-2/play/api/libs/json/JsMacroImpl.scala
@@ -170,8 +170,9 @@ class JsMacroImpl(val c: blackbox.Context) {
         selfRef: Boolean
     )
 
-    val optTpeCtor  = typeOf[Option[_]].typeConstructor
-    val forwardName = TermName(c.freshName("forward"))
+    val optTpeCtor = typeOf[Option[_]].typeConstructor
+    val forwardPrefix = "play_jsmacro"
+    val forwardName = TermName(c.freshName(forwardPrefix))
 
     // MacroOptions
     val options = config.actualType.member(TypeName("Opts")).asType.toTypeIn(config.actualType)
@@ -690,11 +691,10 @@ class JsMacroImpl(val c: blackbox.Context) {
       val (applyFunction, tparams, params, defaultValues) = utility.applyFunction match {
         case Some(info) => info
 
-        case _ =>
-          c.abort(
-            c.enclosingPosition,
-            s"No apply function found matching unapply parameters"
-          )
+        case _ => c.abort(
+          c.enclosingPosition,
+          "No apply function found matching unapply parameters"
+        )
       }
 
       // ---
@@ -731,24 +731,36 @@ class JsMacroImpl(val c: blackbox.Context) {
             val defaultValue = // not applicable for 'write' only
               defaultValueMap.get(name).filter(_ => methodName != "write")
 
+val resolvedImpl = {
+val implTpeName = Option(impl.tpe).fold("null")(_.toString)
+
+if (implTpeName.startsWith(forwardPrefix) ||
+(implTpeName.startsWith("play.api.libs.json") &&
+!implTpeName.contains("MacroSpec"))) {
+impl // Avoid extra check for builtin formats
+} else {
+q"""_root_.java.util.Objects.requireNonNull($impl, "Invalid implicit resolution (forward reference?) for '" + $cn + "': " + ${implTpeName})"""
+}
+}
+
             // - If we're an default value, invoke the withDefault version
             // - If we're an option with default value,
             //   invoke the WithDefault version
             (isOption, defaultValue) match {
               case (true, Some(v)) =>
                 val c = TermName(s"${methodName}HandlerWithDefault")
-                q"$config.optionHandlers.$c($jspathTree, $v)($impl)"
+                q"$config.optionHandlers.$c($jspathTree, $v)($resolveImpl)"
 
               case (true, _) =>
                 val c = TermName(s"${methodName}Handler")
-                q"$config.optionHandlers.$c($jspathTree)($impl)"
+                q"$config.optionHandlers.$c($jspathTree)($resolveImpl)"
 
               case (false, Some(v)) =>
                 val c = TermName(s"${methodName}WithDefault")
-                q"$jspathTree.$c($v)($impl)"
+                q"$jspathTree.$c($v)($resolveImpl)"
 
               case _ =>
-                q"$jspathTree.${TermName(methodName)}($impl)"
+                q"$jspathTree.${TermName(methodName)}($resolveImpl)"
             }
         }
         .reduceLeft[Tree] { (acc, r) =>
@@ -820,6 +832,7 @@ class JsMacroImpl(val c: blackbox.Context) {
             case _ =>
               q"$json.OFormat[${atpe}](instance.reads(_), instance.writes(_))"
           }
+
           val forwardCall =
             q"private val $forwardName = $forward"
 

diff --git a/play-json/shared/src/test/scala/play/api/libs/json/MacroSpec.scala b/play-json/shared/src/test/scala/play/api/libs/json/MacroSpec.scala
@@ -4,6 +4,8 @@
 
 package play.api.libs.json
 
+import scala.util.control.NonFatal
+
 import org.scalatest.matchers.must.Matchers
 import Matchers._
 import org.scalatest.wordspec.AnyWordSpec
@@ -141,11 +143,47 @@ class MacroSpec extends AnyWordSpec with Matchers with org.scalatestplus.scalach
         jsOptional.validate[Family].mustEqual(JsSuccess(optional))
       }
     }
+
+    "fails due to forward reference to Reads" in {
+      implicit def reads: Reads[Lorem[Simple]] =
+        InvalidForwardResolution.simpleLoremReads
+
+      val jsLorem = Json.obj("age" -> 11, "ipsum" -> Json.obj("bar" -> "foo"))
+
+      try {
+        jsLorem.validate[Lorem[Simple]]
+      } catch {
+        case NonFatal(npe: NullPointerException) => {
+          val expected = "Invalid implicit resolution"
+          npe.getMessage.take(expected.size) mustEqual expected
+        }
+
+        case NonFatal(cause) => throw cause
+      }
+    }
+
+    "fails due to forward reference to Format" in {
+      implicit def format: Format[Lorem[Simple]] =
+        InvalidForwardResolution.simpleLoremFormat
+
+      val jsLorem = Json.obj("age" -> 11, "ipsum" -> Json.obj("bar" -> "foo"))
+
+      try {
+        jsLorem.validate[Lorem[Simple]]
+      } catch {
+        case NonFatal(npe: NullPointerException) => {
+          val expected = "Invalid implicit resolution"
+          npe.getMessage.take(expected.size) mustEqual expected
+        }
+
+        case NonFatal(cause) => throw cause
+      }
+    }
   }
 
   "Writes" should {
     "be generated for simple case class" in {
-      Json.writes[Simple].writes(Simple("lorem")).mustEqual(Json.obj("bar" -> "lorem"))
+      Json.writes[Simple].writes(Simple("lorem")) mustEqual Json.obj("bar" -> "lorem")
     }
 
     "as Format for a generic case class" in {
@@ -483,6 +521,38 @@ class MacroSpec extends AnyWordSpec with Matchers with org.scalatestplus.scalach
       formatter.writes(Obj).mustEqual(jsObj)
       formatter.reads(jsObj).mustEqual(JsSuccess(Obj))
     }
+
+    "fails due to forward reference to Writes" in {
+      implicit def writes: Writes[Lorem[Simple]] =
+        InvalidForwardResolution.simpleLoremWrites
+
+      try {
+        Json.toJson(Lorem(age = 11, ipsum = Simple(bar = "foo")))
+      } catch {
+        case NonFatal(npe: NullPointerException) => {
+          val expected = "Invalid implicit resolution"
+          npe.getMessage.take(expected.size) mustEqual expected
+        }
+
+        case NonFatal(cause) => throw cause
+      }
+    }
+
+    "fails due to forward reference to Format" in {
+      implicit def format: Format[Lorem[Simple]] =
+        InvalidForwardResolution.simpleLoremFormat
+
+      try {
+        Json.toJson(Lorem(age = 11, ipsum = Simple(bar = "foo")))
+      } catch {
+        case NonFatal(npe: NullPointerException) => {
+          val expected = "Invalid implicit resolution"
+          npe.getMessage.take(expected.size) mustEqual expected
+        }
+
+        case NonFatal(cause) => throw cause
+      }
+    }
   }
 }
 
@@ -512,6 +582,16 @@ object MacroSpec {
    */
   }
 
+  object InvalidForwardResolution {
+    // Invalids as forward references to `simpleX`
+    val simpleLoremReads = Json.reads[Lorem[Simple]]
+    val simpleLoremWrites = Json.writes[Lorem[Simple]]
+    val simpleLoremFormat = Json.format[Lorem[Simple]]
+
+    implicit val simpleReads: Reads[Simple] = Json.reads
+    implicit val simpleWrites: OWrites[Simple] = Json.writes[Simple]
+  }
+
   object Foo {
     // https://github.com/lampepfl/dotty/issues/11054 Type aliasing breaks constValue
     // import shapeless.tag.@@