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

Unable to serialize sequence without knowing all elements. #386

Closed
dpc opened this issue Jun 15, 2016 · 16 comments · Fixed by #437
Closed

Unable to serialize sequence without knowing all elements. #386

dpc opened this issue Jun 15, 2016 · 16 comments · Fixed by #437

Comments

@dpc
Copy link
Contributor

dpc commented Jun 15, 2016

In the logging library I'm working on, I would like to serialize map elements, provided by user API calls. However at any given time, I have only one key: value pair.

So I can't efficiently serialize a map:

    fn serialize_map<V>(&mut self, visitor: V) -> Result<(), Self::Error> where V: MapVisitor;
    fn serialize_map_elt<K, V>(&mut self, key: K, value: V) -> Result<(), Self::Error> where K: Serialize, V: Serialize;

As I have to build a Visitor

If the API would be returning some kind of Visitor that one can call to serialize field after field, then both my and currently-supported use cases would work. Would it be worse in some aspect that I'm missing?

@oli-obk
Copy link
Member

oli-obk commented Jun 15, 2016

Have you seen https://serde-rs.github.io/serde/serde/ser/impls/struct.MapIteratorVisitor.html ? Either that fits your use case or I don't understand your situation (please elaborate with some code in that case).

@dpc
Copy link
Contributor Author

dpc commented Jun 15, 2016

@oli-obk: This visitor is taking an iterator. I can't provide an iterator since I don't have a collection of map elements. I only have one pair (key, value) at the time.

Example module:

struct Logger {
    serializer : serde::Serializer,
}

impl Logger {
    pub fn start() {}

    pub fn write_one(key : &str, val :&str) {
         self.serializer.emit_map_elt(key, val);
   }

   pub fn end() {}
}

The user of this library will cal write_one couple of times with some borrowed elements that are valid for the lifetime of write_one, but not necessarily for the lifetime of whole Logger. As in the above implementation, I can emit them one at the time, but I can't emit the whole map they belong to, as I can't build the whole map/Visitor at once.

@oli-obk
Copy link
Member

oli-obk commented Jun 16, 2016

That's a tough one. I don't think serde can work with this setup. Even if you'd use threads, it would get messy due to the lifetimes.

If you are willing to turn the &str into String, then you could send them over a channel and implement a MapVisitor that iterates over the receiving end of the channel. In another thread you'd create the serializer and call serialize_map on your custom MapVisitor.

@dpc
Copy link
Contributor Author

dpc commented Jun 17, 2016

That kind of heavy workaround. I have eventually rewritten everything using macros etc. to solve all the issues I've reported, so I'm OK with this not being immediately solvable.

@dpc
Copy link
Contributor Author

dpc commented Jul 10, 2016

I am keep hitting this problem. The more I look at it, the more I'm convinced that that the Visitor pattern is backwards (inversion of control). The Visitor should be returned by serialize_map, and not taken. The whole Serializer API is "push"-y - meaning you call methods from the outside for each element you have, and suddenly, when you want to serialize map, it becomes "pull"-y - serde is calling into custom struct (Visitor) to ask about all elements. It forces you to create weird struct MyVistior and implement error-prone logic, play with lifetimes etc., where all you wanted to do is to go through all elements of your collection and serialize them one by one.

@dpc
Copy link
Contributor Author

dpc commented Jul 10, 2016

I guess the problem is that lack of abstract return types... :(

@oli-obk
Copy link
Member

oli-obk commented Jul 10, 2016

I have investigated this before by playing with closures. I'll have a look whether this can be refactored somewhat. We might be able to add a new associated type to the serializer

@dpc
Copy link
Contributor Author

dpc commented Jul 10, 2016

I actually tried with associated types

pub trait MapSerializer : Sized {
    type Error : Error;
    /// Serializes an element of a map (key-value pair).
    fn serialize_elt<K, V>(&mut self, key: K, value: V) -> Result<(), Self::Error>
        where K: Serialize,
              V: Serialize
}

pub trait SeqSerializer : Sized {
    type Error : Error;
    /// Serializes an element of a sequence.
    fn serialize_elt<T>(&mut self, value: T) -> Result<(), Self::Error>
        where T: Serialize,
}

 pub trait Serializer {
     type SeqSerializer : SeqSerializer+Sized+'static;
     type MapSerializer : SeqSerializer+Sized+'static;

(...)
    /// Serialize a map.
    fn serialize_map<V>(&mut self) -> Result<MapSerializer, Self::Error>;

(...)
}

As far as I understand, it's not possible to return Result<MapSerializer, Self::Error>; as it's called "abstract return types". But it is kind of getting merged in already: rust-lang/rfcs#1522 so maybe it would be possible already.

@oli-obk
Copy link
Member

oli-obk commented Jul 10, 2016

Should work. Abstract return types are sth different

Note: you might have name clashes. Use Self::MapSerializer

@dpc
Copy link
Contributor Author

dpc commented Jul 10, 2016

Oh, you might be right about the clashes. That's how I confused myself.

@dpc
Copy link
Contributor Author

dpc commented Jul 10, 2016

OK, so I got it to compile. dpc@59e9992

I probably broke some stuff as I don't really understand all the details, and some things I did not know how to approach (tuple macro, fixed size arrays).

Also, the actual Serializer might need to return MapSerializer<'a> where <'a> is a liftime of main Serializer, as I they are always going to borrow Serializer.

Nice part is that a lot of Visitor handling boilerplate is gone.

Does it seem convincing? I'm not sure if I'll be able to complete such change myself (as I have little free time, and my understanding of serde is not that great).

@oli-obk
Copy link
Member

oli-obk commented Jul 10, 2016

Awesome. I'll try to integrate json with your design tomorrow to see how it holds up against a real format impl

@oli-obk
Copy link
Member

oli-obk commented Jul 11, 2016

Also, the actual Serializer might need to return MapSerializer<'a> where <'a> is a liftime of main Serializer, as I they are always going to borrow Serializer.

this is a problem... While we do have HKL, associated types can't be generic like typedefs can be (I think? I might be wrong and just using the wrong syntax)

@dpc
Copy link
Contributor Author

dpc commented Jul 11, 2016

One pattern that could work is to make MapSerializer take ownership of the parent Serializer and then add into_inner(self) -> Serializer to it. This way no lifetimes are needed.

@oli-obk
Copy link
Member

oli-obk commented Jul 11, 2016

I got something working... Fixing tests now

@oli-obk
Copy link
Member

oli-obk commented Jul 12, 2016

works, but the design as originally intended won't work... There are many ways to go, I'll strip it down to the smallest useful one for now.

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

Successfully merging a pull request may close this issue.

3 participants