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.

Sign up for GitHub

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

Jump to bottom

NullPointerException trying to pickle Coursier case class #120

Open
lihaoyi opened this issue Nov 1, 2017 · 8 comments · May be fixed by #136
Open

NullPointerException trying to pickle Coursier case class #120

lihaoyi opened this issue Nov 1, 2017 · 8 comments · May be fixed by #136

Comments

@lihaoyi
Copy link
Contributor

lihaoyi commented Nov 1, 2017 

$ amm
Loading...
Welcome to the Ammonite Repl 1.0.3
(Scala 2.12.4 Java 1.8.0_152)
If you like Ammonite, please support our development at www.patreon.com/lihaoyi

@ import $ivy.`com.typesafe.play::play-json:2.6.6`
import $ivy.$

@ import play.api.libs.json._
import play.api.libs.json._

@ {
    implicit val modFormat: Format[coursier.Module] = Json.format
    implicit val depFormat: Format[coursier.Dependency] = Json.format
    implicit val attrFormat: Format[coursier.Attributes] = Json.format
  }
modFormat: Format[coursier.package.Module] = play.api.libs.json.OFormat$$anon$1@295d8852
depFormat: Format[coursier.package.Dependency] = play.api.libs.json.OFormat$$anon$1@142409b4
attrFormat: Format[coursier.package.Attributes] = play.api.libs.json.OFormat$$anon$1@7445fdb2

@ Json.toJson(coursier.Dependency(coursier.Module("org.scala-lang", "scala-reflect"), "2.12.4"))
java.lang.NullPointerException
  play.api.libs.json.PathWrites.$anonfun$at$3(JsConstraints.scala:175)
  play.api.libs.json.OWrites$$anon$3.writes(Writes.scala:112)
  play.api.libs.json.OFormat$$anon$2.writes(Format.scala:46)
  play.api.libs.json.OWrites$MergedOWrites$.mergeIn(Writes.scala:76)
  play.api.libs.json.OWrites$MergedOWrites$$anon$1.writeFields(Writes.scala:68)
  play.api.libs.json.OWrites$MergedOWrites$$anon$1.writeFields(Writes.scala:64)
  play.api.libs.json.OWrites$OWritesFromFields.writes(Writes.scala:96)
  play.api.libs.json.OWrites$OWritesFromFields.writes$(Writes.scala:94)
  play.api.libs.json.OWrites$MergedOWrites$$anon$1.writes(Writes.scala:64)
  play.api.libs.json.OFormat$$anon$2.writes(Format.scala:46)
  play.api.libs.json.OWrites$MergedOWrites$.mergeIn(Writes.scala:76)
  play.api.libs.json.OWrites$MergedOWrites$$anon$1.writeFields(Writes.scala:67)
  play.api.libs.json.OWrites$MergedOWrites$$anon$1.writeFields(Writes.scala:64)
  play.api.libs.json.OWrites$OWritesFromFields.writes(Writes.scala:96)
  play.api.libs.json.OWrites$OWritesFromFields.writes$(Writes.scala:94)
  play.api.libs.json.OWrites$MergedOWrites$$anon$1.writes(Writes.scala:64)
  play.api.libs.json.OFormat$$anon$2.writes(Format.scala:46)
  play.api.libs.json.OWrites$MergedOWrites$.mergeIn(Writes.scala:76)
  play.api.libs.json.OWrites$MergedOWrites$$anon$1.writeFields(Writes.scala:67)
  play.api.libs.json.OWrites$MergedOWrites$$anon$1.writeFields(Writes.scala:64)
  play.api.libs.json.OWrites$OWritesFromFields.writes(Writes.scala:96)
  play.api.libs.json.OWrites$OWritesFromFields.writes$(Writes.scala:94)
  play.api.libs.json.OWrites$MergedOWrites$$anon$1.writes(Writes.scala:64)
  play.api.libs.json.OFormat$$anon$2.writes(Format.scala:46)
  play.api.libs.json.OFormat$$anon$5.$anonfun$inmap$2(Format.scala:29)
  play.api.libs.json.OFormat$$anon$1.writes(Format.scala:39)
  ammonite.$sess.cmd7$.$anonfun$depFormat$4(cmd7.sc:2)
  play.api.libs.json.OFormat$$anon$1.writes(Format.scala:39)
  play.api.libs.json.OFormat$$anon$1.writes(Format.scala:35)
  play.api.libs.json.Json$.toJson(Json.scala:189)
  ammonite.$sess.cmd8$.<init>(cmd8.sc:1)
  ammonite.$sess.cmd8$.<clinit>(cmd8.sc)
@lihaoyi
Copy link
Contributor Author

lihaoyi commented Nov 1, 2017

FWIW this seems to work fine

  implicit val depFormat: Format[coursier.Dependency] =  new Format[coursier.Dependency] {
    def writes(o: Dependency) = {
      Json.obj(
        "module" -> Json.toJson(o.module),
        "version" -> Json.toJson(o.version),
        "configuration" -> Json.toJson(o.configuration),
        "exclusions" -> Json.toJson(o.exclusions),
        "attributes" -> Json.toJson(o.attributes),
        "optional" -> Json.toJson(o.optional),
        "transitive" -> Json.toJson(o.transitive)
      )
    }

    def reads(json: JsValue) = json match{
      case x: JsObject =>
        JsSuccess(coursier.Dependency(
          Json.fromJson[coursier.Module](x.value("module")).get,
          Json.fromJson[String](x.value("version")).get,
          Json.fromJson[String](x.value("configuration")).get,
          Json.fromJson[coursier.Attributes](x.value("attributes")).get,
          Json.fromJson[Set[(String, String)]](x.value("exclusions")).get,
          Json.fromJson[Boolean](x.value("optional")).get,
          Json.fromJson[Boolean](x.value("transitive")).get
        ))

      case _ => JsError("Dep must be an object")
    }
  }

@cchantep
Copy link
Member

cchantep commented Nov 1, 2017

Hi, thanks for reporting.

Please note that the following codes are working:

{
  implicit val modFormat: Format[coursier.Module] = Json.format
  implicit val attrFormat: Format[coursier.Attributes] = Json.format
  implicit val depFormat: Format[coursier.Dependency] = Json.format

  Json.toJson(coursier.Dependency(coursier.Module("org.scala-lang", "scala-reflect"), "2.12.4"))
}

With instance of Format[Attributes] declared before the one for Dependency, which makes sense as the Dependency type depends on the Attributes one.

{
  implicit val modFormat: Format[coursier.Module] = Json.format
  implicit val depFormat: Format[coursier.Dependency] = Json.format
  implicit lazy val attrFormat: Format[coursier.Attributes] = Json.format

  Json.toJson(coursier.Dependency(coursier.Module("org.scala-lang", "scala-reflect"), "2.12.4"))
}

With a lazy declaration.

From there I would say that the issue, not specific to Play JSON, is a forward declaration of implicit instances.

Best regards.

@francisdb
Copy link

We had a similar issue (pasting stack because it points to a different line)

[error]    java.lang.NullPointerException: null (JsConstraints.scala:70)
[error] play.api.libs.json.PathReads.$anonfun$nullableWithDefault$5(JsConstraints.scala:70)
[error] play.api.libs.json.JsSuccess.fold(JsResult.scala:16)
[error] play.api.libs.json.PathReads.$anonfun$nullableWithDefault$3(JsConstraints.scala:68)
[error] play.api.libs.json.PathReads.$anonfun$nullableWithDefault$1(JsConstraints.scala:66)
[error] play.api.libs.json.Reads$$anon$8.reads(Reads.scala:132)
[error] play.api.libs.json.Reads$$anon$3$$anon$4.reads(Reads.scala:110)
[error] play.api.libs.json.Reads.$anonfun$map$1(Reads.scala:43)
[error] play.api.libs.json.Reads$$anon$8.reads(Reads.scala:132)

The case class Foo it was trying to read contains an Option[Bar] value with default None. The Json.reads[Foo] macro that depends on an implicit Reads[Bar] that was indeed declared before the implicit Reads[Bar].
As explained above moving the implicit Reads[Bar] in front of this one fixed the Nullpointer.

Would it be possible to detect this case in the macro and throw a compile-time exception?

@cchantep
Copy link
Member

cchantep commented Nov 2, 2017

@francisdb not sure it's possible. Btw the same kind of NPE is raise for val using forward declaration (not specific to macro).

@francisdb
Copy link

francisdb commented Nov 2, 2017 •
edited
Loading

A simple null check on the reads after implicit resolution could throw an exception indicating that this forward declaration might be the cause? Anything is better than a NullpointerException deep in the play-json code.

@cchantep
Copy link
Member

cchantep commented Nov 2, 2017

Unfortunately, it doesn't seem possible for a macro to check the actual position of an resolved implicit instance.

On the other side, as such forward reference raises a compilation warning as bellow, it can be handled (or blocked by making the warning fatal).

...: Reference to uninitialized value ...

@lihaoyi
Copy link
Contributor Author

lihaoyi commented Nov 5, 2017 •
edited
Loading

@cchantep Thanks, it seems you are right that the initialization order is the problem.

Perhaps one mitigation would be to change the examples in the readme to use lazy val instead of val. That seems to resolve the initialization order problems for me, and shouldn't result in a noticeable performance impact

Also, while it may be unavoidable to throw a NPE, I wonder if it is possible to tweak the macro to throw an NPE as early as possible? Ideally I'd like the NPE to be thrown when defining the vals, rather than when using them. Perhaps sprinkling a few Object.requireNotNulls in the macro-generated code would be enough to surface these errors at the implicit declaration site, which would make them far easier to recognize and debug.

@cchantep cchantep mentioned this issue Nov 25, 2017
Closed
@cchantep cchantep linked a pull request Jan 20, 2018 that will close this issue
Draft
@b-gyula
Copy link

b-gyula commented Apr 14, 2021 •
edited
Loading

It still exists in version 2.9.0. It is really annoying, since it is a runtime error + there is absolutely no sign where the problem is or what type is missing the required Writes...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Forward ref check cchantep/play-json
5 participants
@francisdb @cchantep @dwijnand @lihaoyi @b-gyula