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

Serialization macros intermittently fail #68

Closed
tealsoftware opened this issue Feb 12, 2015 · 20 comments
Closed

Serialization macros intermittently fail #68

tealsoftware opened this issue Feb 12, 2015 · 20 comments

Comments

@tealsoftware
Copy link

I have a relatively simple case class graph to serialize, with one instance of 'sealed trait' implemented by multiple case classes.

It fails when compiled with my application, as described below. However, when I remove all other code and only compile the case classes and Json.scala, everything works fine!

The thing needed to make it work is to remove the code (Scala Android app) not related to upickle in any way!

When I compile the whole code through sbt, I get very many messages such as:

The referenced trait [[Answer]] does not have any sub-classes. This may happen due to a limitation of scalac (SI-7046) given that the trait is not in the same package. If this is the case, the hierarchy may be defined using integer constants.

and then a few compile messages such as

[error] C:\projects\myproject\android\src\main\scala\package\SynchronizationActivity.scala:28: exception during macro expansion:
[error] java.lang.AssertionError: assertion failed
[error]         at scala.Predef$.assert(Predef.scala:151)
[error]         at upickle.Macros$.macroWImpl(Macros.scala:49)
[error]         at sun.reflect.GeneratedMethodAccessor3.invoke(Unknown Source)
[error]         at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
[error]         at java.lang.reflect.Method.invoke(Method.java:606)
[error]         at scala.reflect.macros.runtime.JavaReflectionRuntimes$JavaReflectionResolvers$$anonfun$resolveJavaReflectionRuntime$2.apply(JavaReflectionRuntimes.scala:34)
[error]         at scala.reflect.macros.runtime.JavaReflectionRuntimes$JavaReflectionResolvers$$anonfun$resolveJavaReflectionRuntime$2.apply(JavaReflectionRuntimes.scala:22)
[error]         at scala.tools.nsc.typechecker.Macros$class.macroExpandWithRuntime(Macros.scala:755)
[error]     info(s"This would be sent to server: ${Serialize(qr)}")
[error]                                                     ^
[error] 15 errors found
[error] (compile:compile) Compilation failed

The trait it has a problem with is roughly

sealed trait Answer 
case class AnswerWithDefects(defects: Option[Seq[Defect]] = None) extends Answer
case class AnswerYesNo(expectedAnswer: Boolean, answer: Option[Boolean] = None) extends Answer 

The code doing the serialization is

package mypackage.json
import upickle._

object Serialize {
  def apply[T : Writer](value: T) = write(value)
}

object Deserialize {
  def apply[T : Reader](json: String) = read[T](json)
}

I also tried adding the following in there after the upickle import, but the results are the same:

import mypackage.{AnswerYesNo, AnswerWithDefects, Answer}
object JsonImplicits {
  val classNameKey = "className"
  val dataKey = "data"

  implicit val answerWriter = Writer[Answer] {
        case answer =>
          val data = answer match {
            case x: AnswerWithDefects => writeJs[AnswerWithDefects](x)
            case x: AnswerYesNo => writeJs[AnswerYesNo](x)
          }

          Js.Obj(
            classNameKey -> Js.Str(answer.getClass.getName),
            dataKey -> data
          )
      }

      implicit val answerRead = Reader[Answer] {
        case o @ Js.Obj(_) =>
          o(classNameKey) match {
            case Js.Str(className) =>
              val dataJson = o(dataKey)

              val data = if (className == classOf[AnswerWithDefects].getName) {
                readJs[AnswerWithDefects](dataJson)
              } else if (className == classOf[AnswerYesNo].getName) {
                readJs[AnswerYesNo](dataJson)
              } else {
                throw new RuntimeException(s"Did not recognize $className")
              }

              data
          }
      }
}

import JsonImplicits._

I appreciate the root cause is the scalac defect SI-7046, and that it's not getting fixed any time soon, but do you have any workaround ideas to make it work?

I had hoped that me providing the implicits would make it not resort to reflection when analyzing the Answer class, but it still fails.

@tindzk
Copy link
Contributor

tindzk commented Feb 12, 2015

This is related to #31.

Defining your own implicits will definitely work. In the code you provided it is not entirely visible where the import JsonImplicits._ actually happens. Make sure you import the implicits before the write() and read() calls. Both functions internally use implicitly which will look for a viable pickler in the scope.

I'm not entirely sure whether it makes a difference, but I always define my picklers using implicit def instead of implicit val. You may want to test that as well.

In #31 @japgolly found an interesting workaround for SI-7046: If the package containing the type you want to serialise defines any kind of implicit (which is evaluated), then uPickle's macros generate the picklers correctly. This approach works reliably for me.

@tealsoftware
Copy link
Author

In the code you provided it is not entirely visible where the import JsonImplicits._ actually happens.

Full code

package mypackage.json
import upickle._

object JsonImplicits {
  val classNameKey = "className"
  val dataKey = "data"

  implicit val answerWriter = Writer[Answer] {
    case answer =>
      val data = answer match {
        case x: AnswerWithDefects => writeJs[AnswerWithDefects](x)
        case x: AnswerYesNo => writeJs[AnswerYesNo](x)
      }

      Js.Obj(
        classNameKey -> Js.Str(answer.getClass.getName),
        dataKey -> data
      )
  }

  implicit val answerRead = Reader[Answer] {
    case o @ Js.Obj(_) =>
      o(classNameKey) match {
        case Js.Str(className) =>
          val dataJson = o(dataKey)

          val data = if (className == classOf[AnswerWithDefects].getName) {
            readJs[AnswerWithDefects](dataJson)
          } else if (className == classOf[AnswerYesNo].getName) {
            readJs[AnswerYesNo](dataJson)
          } else {
            throw new RuntimeException(s"Did not recognize $className")
          }

          data
      }
  }
}

import JsonImplicits._

object Serialize {
  def apply[T : Writer](value: T) = write(value)
}

object Deserialize {
  def apply[T : Reader](json: String) = read[T](json)
}

I'm not entirely sure whether it makes a difference, but I always define my picklers using implicit def instead of implicit val. You may want to test that as well.

This made no difference.

If the package containing the type you want to serialise defines any kind of implicit (which is evaluated), then uPickle's macros generate the picklers correctly.

I'm not sure how to do this.

I moved the Json object to the same package where all my case classes reside, but it made no difference.

@tindzk
Copy link
Contributor

tindzk commented Feb 12, 2015

I moved the Json object to the same package where all my case classes reside, but it made no difference.

The declarations AnswerWithDefects and AnswerYesNo should be in the same package as Serialize and Deserialize (not in a sub-package thereof). The macros must work if this is the case.

The bug only affects sealed traits, so writeJs[AnswerWithDefects](...) should define the implicits correctly. If your writeJs and readJs calls cause the compilation to fail with the SI-7046 error, then a workaround will be to give each case class a separate implicit as in: https://github.com/lihaoyi/upickle/blob/master/upickle/shared/src/main/scala/upickle/Implicits.scala#L142-149

@tealsoftware
Copy link
Author

Thanks for your help, @tindzk. I cannot quite get it working tonight, but maybe I will later on.

@kyleu
Copy link

kyleu commented Feb 12, 2015

Could you explain that workaround a little more clearly, @tindzk ? I have a sealed trait in my "models" package, and I'm calling upickle's read/write method from a class in the "services" package. I've tried defining bogus implicit defs in both package objects, but I still have the "uPickle does not know how to read" issue.

@tindzk
Copy link
Contributor

tindzk commented Feb 12, 2015

Sure, here is a short excerpt from my code. It comes from a client-server application that uses Autowire with nested protocols:

package app.shared

sealed trait Response[+T]
object Response {
  case class Failure(error: String) extends Response[Nothing]
  case class Success[+T](value: T) extends Response[T]
}

object Picklers {
  import upickle.Js
  import upickle.Aliases._

  implicit def ResponseW[T: W]: W[Response[T]] = W[Response[T]] {
    case Response.Failure(s) => Js.Arr(Js.Num(0), upickle.writeJs(s))
    case Response.Success(t) => Js.Arr(Js.Num(1), upickle.writeJs(t))
  }

  /* Some more custom implicits for Response */
}

object Dashboard {
  ...
}

trait Dashboard {
  def summary(session: Session): Response[Dashboard.Summary]
}

trait Protocol {
  val dashboard: Dashboard
  ...
}

Response cannot be generated automatically by uPickle (due to its generic parameter). Every operation on the server uses it as its return type. The case classes are defined in the same package.

ServerOps is an object that wraps every single operation in a function so that nothing needs to be imported when a protocol function is called:

package app.client

object ServerOps {
  import autowire._
  import app.shared._

  /* IntelliJ erroneously declares this as 'unused import statement'. */
  import upickle._
  import Picklers._

  object Dashboard {
    def summary(session: Session) = Server[Protocol].dashboard.summary(session).call()
  }

  ...
}

I slightly simplified the structure, though. Actually, the shared package has two sub-packages model and protocol. For each operation, there is a separate file encapsulating its sealed traits and case classes in an object. Finally, for each endpoint there is a file containing one trait in the sub-package protocol. Then, in the shared package the trait Protocol aggregates the protocols from the sub-packages (as shown above).

I got it working to generate an AST (consisting of several sealed trait hierarchies) on the server and use the same code to deserialise it on the client.

Please let me know whether this helps you. If not, I may work out a small demo application over the weekend.

@kyleu
Copy link

kyleu commented Feb 12, 2015

Maybe I'm having a different issue? From a clean compile, both my server and client code report that "uPickle does not know how to write [models.RequestMessage]s;". If I remove the read/write calls, my code compiles without issue. At that point, adding the read/write calls back still compiles fine, until the next clean.

I have the following class for my models:

package models

sealed trait RequestMessage

case class MalformedRequest(content: String) extends RequestMessage
case class Ping(timestamp: Long = System.currentTimeMillis) extends RequestMessage

Here's my client code:

package connection

import models.{RequestMessage, ResponseMessage}
import org.scalajs.dom.{Event, MessageEvent, WebSocket}
import upickle._
import utils.{ClientSettings, Logging}

class WebsocketConnection(onOpen: (WebsocketConnection) => Unit, onMessage: (ResponseMessage) => Unit) {
  private val url = "ws://" + ClientSettings.domain + "/websocket"
  private val ws = new WebSocket(url, "ws")

  def send(request: RequestMessage) {
    val s = write(request)
    ws.send(s)
  }

And for my server code:

package models

import play.api.mvc.WebSocket.FrameFormatter
import upickle._

object MessageFrameFormatter {
  private def requestToString(r: RequestMessage): String = write(r)
  private def requestFromString(s: String): RequestMessage = read[RequestMessage](s)
  implicit val requestFormatter = FrameFormatter.stringFrame.transform(requestToString, requestFromString)
}

@tindzk
Copy link
Contributor

tindzk commented Feb 13, 2015

No, actually it's the same problem. It worked for you in certain situations because the results of the required phase was already cached, so knownDirectSubclasses was behaving correctly.

@kyleu
Copy link

kyleu commented Feb 13, 2015

OK, that makes sense. You alluded to @japgolly 's fix in #31 - is that a valid option for me? I've tried defining and calling implicits everywhere I can think of, with no success.

@tindzk
Copy link
Contributor

tindzk commented Feb 13, 2015

Yes. This is the hack that I used in the past.

I tried to reproduce your problem, but I don't even need any custom implicits to get working here (works fine after a clean, too). Could either of you please provide a minimal project so that I could have a look?

@kyleu
Copy link

kyleu commented Feb 13, 2015

I've added you to my repo. I'll post to this thread if we find a workaround. Thanks in advance.

@tindzk
Copy link
Contributor

tindzk commented Feb 13, 2015

I am not entirely sure, why I couldn't reproduce your problems in my test project, but I assume it's because I have set up Scala.js 0.6 with a CrossProject configuration. sbt automatically detects the shared folder. CrossProject enforces a certain build order, compiling the shared resources first. So the server and client projects will always access the compilation cache, whereby knownDirectSubclasses is populated correctly and no further workarounds become necessary.

Also, I wrongly stated that it's sufficient to declare just any implicit. It turns out that my AST model was actually defining manual picklers, but only for the top-level AST trait. For the children in the trait hierarchy I was relying on uPickle, though. As uPickle's macros work fine in the package where the traits are declared, the obvious solution would be to bind their result to implicits:

sealed trait Answer 
/* ... */
object Picklers {
  import upickle._
  implicit def AnswerR: Reader[Answer] = Reader.macroR[Answer]
  implicit def AnswerW: Writer[Answer] = Writer.macroW[Answer]
}

This is probably the shortest we can get without defining the picklers manually. Please let me know whether this works for you.

@kyleu
Copy link

kyleu commented Feb 13, 2015

Could you share your manual pickler for the top-level trait? I managed to
get one working, but it pulls the class name from the Js.Arr, and matches
on strings (ick).

On Fri, Feb 13, 2015 at 12:56 PM, Tim Nieradzik notifications@github.com
wrote:

I am not entirely sure, why I couldn't reproduce your problems in my test
project, but I assume it's because I have set up Scala.js 0.6 with a
CrossProject configuration. sbt automatically detects the shared folder.
CrossProject enforces a certain build order, compiling the shared
resources first. So the server and client projects will always access the
compilation cache, whereby knownDirectSubclasses is populated correctly
and no further workarounds become necessary.

Also, I wrongly stated that it's sufficient to declare just any
implicit. It turns out that my AST model was actually defining manual
picklers, but only for the top-level AST trait. For the children in the
trait hierarchy I was relying on uPickle, though. As uPickle's macros work
fine in the package where the traits are declared, the obvious solution
would be to bind their result to implicits:

sealed trait Answer /* ... */object Picklers {
import upickle._
implicit def AnswerR: Reader[Answer] = Reader.macroR[Answer]
implicit def AnswerW: Writer[Answer] = Writer.macroW[Answer]
}

This is probably the shortest we can get without defining the picklers
manually. Please let me know whether this works for you.


Reply to this email directly or view it on GitHub
#68 (comment).

@tindzk
Copy link
Contributor

tindzk commented Feb 13, 2015

Sure. The tree is specified as follows (Prop is a sealed trait hierarchy):

sealed trait Tree
object Tree {
  case class Node(id: Int, prop: Prop, children: Seq[Tree]) extends Tree
  case class Leaf(id: Int, prop: Prop, text: String) extends Tree
}

Before I figured out the above workaround I was using:

object Picklers {
  import upickle.Aliases._
  import upickle.Js

  implicit def TreeR: R[Tree] = R[Tree](
    NodeR.read.orElse(LeafR.read)
  )

  implicit def TreeW: W[Tree] = W[Tree] {
    case x @ Tree.Node(_, _, _) => NodeW.write(x)
    case x @ Tree.Leaf(_, _, _) => LeafW.write(x)
  }

  implicit def NodeR: R[Tree.Node] = R[Tree.Node] {
    case Js.Arr(Js.Num(0), a, b, c) => Tree.Node(
      upickle.readJs[Int](a),
      upickle.readJs[Prop](b),
      upickle.readJs[Seq[Tree]](c)
    )
  }

  implicit def LeafR: R[Tree.Leaf] = R[Tree.Leaf] {
    case Js.Arr(Js.Num(1), a, b, c) => Tree.Leaf(
      upickle.readJs[Int](a),
      upickle.readJs[Prop](b),
      upickle.readJs[String](c)
    )
  }

  implicit def NodeW: W[Tree.Node] = W[Tree.Node] { x: Tree.Node =>
    Js.Arr(
      upickle.writeJs(0),
      upickle.writeJs(x.id),
      upickle.writeJs(x.prop),
      upickle.writeJs(x.children)
    )
  }

  implicit def LeafW: W[Tree.Leaf] = W[Tree.Leaf] { x: Tree.Leaf =>
    Js.Arr(
      upickle.writeJs(1),
      upickle.writeJs(x.id),
      upickle.writeJs(x.prop),
      upickle.writeJs(x.text)
    )
  }
}

Edit: After discussing the matter again with Kyle, we found that the macros from the previous posting may even fail when the manual reader/writer is at the declaration site.

If this is the case, the only possible solution will be to mimic the behaviour of the macro so that knownDirectSubclasses isn't queried in the first place:

sealed trait Base
object Base {
  case class Child1() extends Base
  case class Child2() extends Base
  case class Child3() extends Base

  import upickle._

  implicit val BaseR: Reader[Base] = Reader[Base](
    implicitly[Reader[Child1]].read
      .orElse(implicitly[Reader[Child2]].read)
      .orElse(implicitly[Reader[Child3]].read)
  )

  implicit val BaseW: Writer[Base] = Writer[Base] {
    case x: Child1 => writeJs(x)
    case x: Child2 => writeJs(x)
    case x: Child3 => writeJs(x)
  }
}

@lihaoyi
Copy link
Member

lihaoyi commented Feb 14, 2015

Sorry I've been unresponsive, been moving across continents. Thanks @tindzk for providing help!

tindzk added a commit to tindzk/upickle that referenced this issue Feb 15, 2015
This currently prevents us from easily defining our own composite
readers, which is often necessary; see com-lihaoyi#31 and com-lihaoyi#68. Without the
exceptions, implicitly[Reader[_]].read calls can be chained using
orElse(), as done internally in uPickle.

```scala
sealed trait Base
case class Child1() extends Base
case class Child2() extends Base

import upickle._
implicit val BaseR: Reader[Base] = Reader[Base](
  implicitly[Reader[Child1]].read
    .orElse(implicitly[Reader[Child2]].read)
)
```

The exceptions are only useful for debugging purposes.
@BardurArantsson
Copy link

Is there any chance that this or a similar issue could be causing runtime failures? (I ask because I'm seeing weird issues where upickle fails to deserialize its own messages. It works fine when doing a serizalize->deserialize round-trip in the JVM project, but fails with the rather inexplicable message "... msg: Object" when doing serialize(JVM)->deserialize(JS).

@tindzk
Copy link
Contributor

tindzk commented Feb 19, 2015

Indeed, the implicit will throw an exception if the first read fails. I only noticed this after posting the above workaround. We could probably use Try(), but a cleaner way is not to throw the exception in the first place. I made a pull request that changes the default behaviour. @lihaoyi What do you think?

@Voltir
Copy link

Voltir commented May 8, 2015

I think I am running into a similar issue

[error] java.lang.AssertionError: assertion failed
[error]         at scala.Predef$.assert(Predef.scala:151)
[error]         at upickle.Macros$.macroWImpl(Macros.scala:49)
[error]         at sun.reflect.GeneratedMethodAccessor4.invoke(Unknown Source)
[error]         at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
[error]         at java.lang.reflect.Method.invoke(Method.java:483)
[error]         at scala.reflect.macros.runtime.JavaReflectionRuntimes$JavaReflectionResolvers$$anonfun$resolveJavaReflectionRuntime$2.apply(JavaReflectionRuntimes.scala:34)
[error]         at scala.reflect.macros.runtime.JavaReflectionRuntimes$JavaReflectionResolvers$$anonfun$resolveJavaReflectionRuntime$2.apply(JavaReflectionRuntimes.scala:22)
[error]         at scala.tools.nsc.typechecker.Macros$class.macroExpandWithRuntime(Macros.scala:755)
[error]   implicit def makework: upickle.Writer[UserProfile] = Writer.macroW[UserProfile]

The error occurs when I add an Int field to an otherwise unremarkable case class - but it does have Lists and Options as other elements.

@hrj
Copy link

hrj commented Jul 3, 2015

I did an update to scala 2.11.7 and scalajs 2.6.4, issued a clean, and now, bam. I am having this error too.

IMO, this is such a serious issue that it deserves to be put in the README.md, under a known issues header.

@lihaoyi lihaoyi closed this as completed Jul 5, 2015
@lihaoyi lihaoyi reopened this Jul 5, 2015
@lihaoyi
Copy link
Member

lihaoyi commented Jul 5, 2015

I'm gonna close this in preference for #31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants