amnaredo / test

0 stars 0 forks source link

Omit field keys in serialised ADTs #105

Open amnaredo opened 3 years ago

amnaredo commented 3 years ago

As mentioned in #55, I'd like to use uPickle to parse an aeson-produced file. Another interoperability issue I encountered is that uPickle doesn't encode field keys. Take the following example:

case class Format(wrapped: String)
case class RawBlock(format: Format, str: String)

RawBlock(Format("html"), "<center>")

Here is a comparison of the differences:

aeson

{"t":"RawBlock","c":["html","<center>"]}

uPickle

["RawBlock",{"format":{"wrapped":"html"},"str":"<center>"}]

As long as a class constructor doesn't use any optional parameters, keys should not matter. I assume that this behaviour could even be beneficial as a default because it significantly reduces the size.

Another minor issue I noticed is that uPickle uses the full class name, i.e., it includes the package names. As a workaround I could just rewrite the aeson JSON tree that I'm parsing (as I do with t and c right now), but setting a relative path could further shorten the JSON.

It may make sense to create a Configuration trait that contains these kind of settings.

ID: 59 Original Author: tindzk

amnaredo commented 3 years ago

Yeah it used to be like that. Then I put the keys in because everyone else was complaining that it didn't interoperate well with all their existing JSON infrastructure =P

You should be able to shadow the macroR and macroW pretty easily to make this happen. In fact, it probably doesn't even need to be a macro anymore: just write the 10 or 22 overrides for differently sized ProductN traits and you're done.

Original Author: lihaoyi

amnaredo commented 3 years ago

Ok, that might work as a temporary solution. Yet, I'd like to make uPickle more configurable, so that I can easily use different set-ups within the same project. This is solved nicely in aeson with a configuration object. As described in #55, I did try something similar for uPickle, but ran into problems with implicits. Perhaps you can think of a better way to implement it — without implicits?

Original Author: tindzk

amnaredo commented 3 years ago

Your choice is between implicits or cakes. I've had great success using cakes for Scalatags, where the entire library is essentially giant cake that I instantiate 2-3 times for Text/DOM/React versions. Perhaps you could try doing the same with uPickle? The advantage of cakes is, of course, they don't use implicit resolution at all and thus are much less fragile to e.g. bad inference or imports.

In your case, you'd turn uPickle into one giant cake (done before, i.e. https://github.com/lihaoyi/upickle/tree/cake) and then parametrize the cake on either a function or an abstract-def which does what you want. Then the default upickle.default namespace would contain the default version, but you could easily do

object MySpecialSnowflake extends upickle.Cake{ 
    override def macroR = ...
}

And use that.

p.s. This is a massive refactor, but not something you can't do in a few hours beating your head against the compiler. I've already done it twice (upickle as an experiment, and Scalatags in production) and it's not that bad.

Original Author: lihaoyi

amnaredo commented 3 years ago

I experimented with the Cake approach. The major change I made is that there is now a trait Picklers (previously Implicits) which by default only contains the picklers for the Scala standard library as well as the auto-generated ones (for tuples). It can be used as follows:

object MyPicklers extends Picklers
def test() {
    import MyPicklers._
    println(write(42))
}

Additionally, the macro picklers (AutoPicklers) could be mixed in. Setting a configuration object is compulsory, though:

object MyPicklers extends AutoPicklers with Picklers {
    val config = Configuration.Default
}

The reasoning behind having the user manually define a pickler object is three-fold:

Therefore, I believe that this approach is superior to passing a configuration object via an implicit to the macros. I published my changes here. Please tell me what you think.

Unfortunately, the macro test cases are not working yet. This is because mixing in AutoPicklers is problematic; apparently, the stdlib picklers always get overridden, so that the macros are called even for native Scala types. My assumption is that the traits in object MyPicklers extends AutoPicklers with Picklers don't preserve their order. Is there a workaround that I could use?

Original Author: tindzk

amnaredo commented 3 years ago

I think this issue can be solved more easily with implicits. In fact, I intended to write up something on this after I'm done with annotators and filters.

Suppose there is a trait

trait Materializers {
  def reader[A]: Reader[A]
  def writer[A]: Writer[A]
}

One of its implementations could look like

val macroMaterializers = new Materializers {
  override def reader[A] = macro Macros.macroRImpl[A]
  override def writer[A] = macro Macros.macroWImpl[A]
}

Then the default implicits defined in Reader/Writer companions could look like this:

object Reader {
  def defaultR[A](implicit m: Materializers) = m.reader[A]
}

object Writer {
  def defaultW[A](implicit m: Materializers) = m.writer[A]
}

Now an implicit Materializers instance can be added to the configuration object (which is introduced in my annotators PR):

trait DefaultConfig extends Config {
  override implicit val materializers = Materializers.macroMaterializers
}

After this the user code could look like

import upickle._
import upickle.config.default._

write(SomeClass(...))

The write call is resolved to something like

write(SomeClass(...))(Writer.defaultW[SomeClass](materializers /* from config object /*))

Now you can easily substitute your own Materializers (probably even based on reflection; in that case its methods would need TypeTag bound) via extending the DefaultConfig trait:

object MyConfig extends DefaultConfig {
  override implicit val materializers = new MyCustomMaterializers
}

import upickle._
import mypackage.MyConfig._

write(MyClass(...))

Original Author: netvl

amnaredo commented 3 years ago

It's a matter of taste, but I'm generally inclined towards solutions without the use of implicits. The Cake pattern which @lihaoyi suggested seems to be a good fit for our problem. I just need to find a workaround for the bug I mentioned above.

I'm pretty sure your approach works fine and it is very similar to what I've done in my branch, but perhaps we can find a more readable way that doesn't use any implicits and doesn't require to import the whole contents of a package or object (like import upickle._)?

My initial idea was something along the lines:

val upickle = Upickle(Configuration.Default)
upickle.write(...)

But this probably wouldn't work because we need the implicits in our scope.

Original Author: tindzk

amnaredo commented 3 years ago

Unfortunately, you can't do without implicits completely - you still need to obtain implicit converters in write()/read() methods. Scala has two categories of implicit definitions which are searched for arguments for implicit parameters - first, all identifiers in the current scope (explicit definitions, explicit imports, wildcard imports, inherited items for the current class, etc.), second, implicit scope for the affected types: companion objects, package objects, scopes for argument types, scopes for type parameters, package objects for affected types, probably somewhere else. Implicits from the first category take precedence over those from the second category.

This article gives suggestions on where to put implicits to avoid explicit imports. I believe that something along these lines would work:

trait Upickle {
  trait Writer[T] { def write: T => Js.Value }
  object Writer {
    implicit def macroW[T]: Writer[T] = ...
  }
}

Now if Upickle.apply() returns an instance of Upickle, your example should work:

val upickle = Upickle(Configuration.Default)
upickle.write(SomeClass(...))

That's because write() call would accept a path-dependent type upickle.Writer[SomeClass], and according to the implicit resolution rules I think it should automatically resolve upickle.Writer.macroW[SomeClass] as the parameter.

Original Author: netvl

amnaredo commented 3 years ago

Yeah you can't do without implicits completely, but a mixed approach has worked well for e.g. Scalatags which uses cake-pattern to create multiple "modules" and using implicits for the nitty-gritty stuff.

I'm reluctant to put default implicits, because you miss them in one place, and suddenly everything compiles and does the wrong thing. With caked modules, you still have to import upickle.default._ or import myproject.upickleconfig._ and won't accidentally leave it out.

Original Author: lihaoyi

amnaredo commented 3 years ago

Closing for https://github.com/lihaoyi/upickle/issues/55

Original Author: lihaoyi