Introduce collect_seq and collect_map #736

nox · 2017-01-31T22:04:30Z

These are two convenience functions to serialize iterators.

nox · 2017-01-31T22:12:05Z

@sfackler on IRC taught me that there is a wrapper behind the feature unstable to serialise iterators through a wrapper putting it in a RefCell. I posit that we should not do that, because that doesn't help for struct fields and friends, as it is an opaque thing that will consume the iterator on serialisation.

nox · 2017-01-31T22:15:51Z

Silly me, it uses IntoIterator too so it doesn't consume it, but given there is no function bound to this behaviour, it can't be used in #[serde(serialize_with)].

If we think the wrapper is useful, we could keep it but simplify it using mitochondria::MoveCell. :P

sfackler · 2017-01-31T22:23:04Z

If we were going to do this it'd probably be worth also having one that serializes an iterator of tuples as a map.

nox · 2017-01-31T22:25:09Z

@sfackler Should I rename this one to serialize_seq, and make another one serialize_map? We could also make serialize_fixed_size_seq relying on TrustedLen once it's stable too.

dtolnay · 2017-02-01T02:27:20Z

Can you share some example code of how you imagine people using this?

As I see it, there are two primary use cases for these functions:

Helpers when implementing Serialize for a type. I don't think the tradeoff of including these functions for this purpose makes sense. Just use the loop way and it is 5 straightforward lines and those 5 lines match a pattern that is common to all other compound type serialization in Serde (tuples, tuple structs, structs, struct variants, tuple variants). In my opinion the typing you save by using these helpers does not outweigh the people stumbling over this when browsing our rustdoc, the confusion of mixing up ser::serialize_seq and Serializer::serialize_seq and needing to think about which one to use where.
As a serialize_with function in cases where the type is not from your crate and the original author does not want to add a Serialize implementation. In this case I would prefer to include these in Helper library of useful de/serialize_with functions #553 which is a library dedicated to this purpose, rather than in the main serde crate.

nox · 2017-02-01T09:07:31Z

Can you share some example code of how you imagine people using this?

Same way as the unstable iterator thing, except that there is no "this must be serialised only once" as on the Iterator struct. Tuples, tuple structs and structs are similar, but there is no legit reason to support them from an iterator, after all tuples, tuple structs and structs cannot be iterated.

As for the fact that it is just a 5 lines loop, the Pair trait with its two implementations is certainly less trivial to repeat all the time.

IMO this is core enough that it should be in serde.

dtolnay · 2017-02-03T05:02:04Z

Sorry for the silence on my end - I have been busy with #739. I will consider this again tomorrow, including your suggestion from IRC of possibly making these Serializer trait methods with default implementations.

sfackler · 2017-02-03T05:23:24Z

I like the default method approach personally. It's a nice convenience similar to https://doc.rust-lang.org/std/fmt/struct.DebugList.html#method.entries

dtolnay · 2017-02-03T16:00:56Z

It's a little different in that case because DebugList is a struct and not a trait. But sure, let's do it. Any thoughts on naming? Serializer::serialize_iter_as_seq and serialize_iter_as_map aren't perfect. It would be nice to reference Extend somehow because that is one of the most common methods taking an IntoIterator, but that would fit better as SerializeSeq::extend or something.

nox · 2017-02-03T16:04:48Z

Rather than Extend, if you want to reuse iterator terminology, I would opt for collect_seq and collect_map methods on Serializer.

dtolnay · 2017-02-03T16:07:51Z

Good idea, let's go with that. Thanks!

dtolnay · 2017-02-03T17:31:42Z

Also I would prefer not to make the Pair changes. It makes sense for MapDeserializer because people pass around variables of type MapDeserializer<I, E> so additional redundant type parameters are a nuisance, but here let's just use K, V type parameters on collect_map.

dtolnay · 2017-02-03T22:59:52Z

Do you expect any serializers to want to override these default implementations? If so, please add a comment that explains why someone would want to override. If not, please add a comment that serializers should not need to override. I don't want serializer implementors to wonder whether the default implementation is doing the wrong thing for their format.

nox · 2017-02-03T23:01:50Z

~~I would expect serde_json to override it to not even have to allocate an intermediate String, instead writing directly on its output the string escaped appropriately.~~ Wrong issue hah.

nox · 2017-02-03T23:06:24Z

@dtolnay I added the comments, I don't expect implementors to have to override this.

dtolnay

LGTM. @oli-obk what do you think?

oli-obk · 2017-02-04T08:03:17Z

serde/src/ser/mod.rs

+    /// The default implementation serializes each item yielded by the iterator
+    /// using `Self::SerializeSeq`. Implementors should not need to override
+    /// this method.
+    fn collect_seq<I>(self, iter: I) -> Result<Self::Ok, Self::Error>


we can immediately use this in https://github.com/nox/serde/blob/a0a4d782957d351b608d08aa892e7e86317b5182/serde/src/ser/impls.rs#L224

Can we use the same size-trick so we don't pass None?

Is there a way to use specialization to improve this impl for ExactSizeIterator? If so, can this function be written in a way so that is possible (by adding a feature=unstable specialization, which gets checked on nightly travis)?

Why would you even trust that size trick? I don't understand what tells you that this is actually correct to do.

size_hint() is primarily intended to be used for optimizations such as reserving space for the elements of the iterator, but must not be trusted to e.g. omit bounds checks in unsafe code. An incorrect implementation of size_hint() should not lead to memory safety violations.

Quoting the standard, since we don't guarantee that the Option in serialize_seq is correct, we can depend on the size_hint to be just as correct.

All correct Serialize impls will benefit from this, by being compatible with formats like bincode.

Ok, then I don't understand the second question, if we can just check that the two hints are equal and use that, why specialise for ExactSizeIterator at all?

uhm... because I confused myself? ;) ignore it.

@oli-obk I wonder if there is something to simplify around serialize_seq! in ser::impls though.

well... we could replace it with collect_seq, but I wonder if that optimizes the same way. Without benchmarks that's not a change we should do.

I introduced a trait LenHint that is specialised for I: ExactSizeIterator.

nox · 2017-02-04T10:47:42Z

@oli-obk @dtolnay Btw, when do we kill the Iterator wrapper? In my opinion it is misguided.

oli-obk · 2017-02-04T10:48:18Z

Btw, when do we kill the Iterator wrapper? In my opinion it is misguided.

with your changes there's no need for it. Feel free to remove it in this PR. (it was behind an unstable feature flag anyway)

This function serializes the given iterator as a sequence. Its iter parameter has type I: IntoIterator, <I as IntoIterator>::Item: Serialize, which means it will work both for iterators passed by value, therefore consuming them, and as the value for a #[serde(serialize_with)] attribute, where it will be called as Serializer::collect_seq(&self.field, serializer), relying on the common practice of implementing IntoIterator for &C where C is a data type representing some kind of collection.

oli-obk · 2017-02-04T11:40:45Z

@dtolnay: you wanna take another look?

dtolnay · 2017-02-05T03:07:27Z

well... we could replace it with collect_seq, but I wonder if that optimizes the same way. Without benchmarks that's not a change we should do.

There is no difference in performance based on my benchmarks so I removed the old serialize_seq and serialize_map macros in 219abd2.

nox force-pushed the serialize-iter branch from 0b046d4 to a7a75cd Compare January 31, 2017 22:04

nox changed the title ~~Introduce ser::serialize_iter~~ Introduce serialize_seq and serialize_map Jan 31, 2017

nox force-pushed the serialize-iter branch 3 times, most recently from 3c33c5d to 110e799 Compare January 31, 2017 23:29

nox force-pushed the serialize-iter branch 2 times, most recently from 56f3805 to 8f7e8c4 Compare February 3, 2017 20:14

nox force-pushed the serialize-iter branch from 8f7e8c4 to a0a4d78 Compare February 3, 2017 23:06

dtolnay approved these changes Feb 3, 2017

View reviewed changes

dtolnay requested a review from oli-obk February 3, 2017 23:08

nox changed the title ~~Introduce serialize_seq and serialize_map~~ Introduce collect_seq and collect_map Feb 3, 2017

oli-obk requested changes Feb 4, 2017

View reviewed changes

nox added 4 commits February 4, 2017 11:49

Introduce Serializer::collect_map

89bb16d

Remove ser::Iterator

30b8036

Use serialize_seq! to implement Serialize for [T]

763ab9c

Use specialisation to optimise collect_seq and collect_map

17c175a

nox force-pushed the serialize-iter branch from a0a4d78 to 17c175a Compare February 4, 2017 11:24

oli-obk approved these changes Feb 4, 2017

View reviewed changes

dtolnay merged commit 4bd1052 into serde-rs:master Feb 4, 2017

dtolnay mentioned this pull request Apr 5, 2017

impl<T> Serialize for Iterator<T> ? #571

Closed

dtolnay mentioned this pull request Jun 21, 2020

Remove len hint specialization #1841

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce collect_seq and collect_map #736

Introduce collect_seq and collect_map #736

nox commented Jan 31, 2017 •

edited

Loading

nox commented Jan 31, 2017 •

edited

Loading

nox commented Jan 31, 2017

sfackler commented Jan 31, 2017

nox commented Jan 31, 2017

dtolnay commented Feb 1, 2017

nox commented Feb 1, 2017

dtolnay commented Feb 3, 2017

sfackler commented Feb 3, 2017

dtolnay commented Feb 3, 2017

nox commented Feb 3, 2017

dtolnay commented Feb 3, 2017

dtolnay commented Feb 3, 2017

dtolnay commented Feb 3, 2017

nox commented Feb 3, 2017 •

edited

Loading

nox commented Feb 3, 2017

dtolnay left a comment

oli-obk Feb 4, 2017

nox Feb 4, 2017

oli-obk Feb 4, 2017

nox Feb 4, 2017

oli-obk Feb 4, 2017

nox Feb 4, 2017

oli-obk Feb 4, 2017

nox Feb 4, 2017

nox commented Feb 4, 2017

oli-obk commented Feb 4, 2017 •

edited

Loading

oli-obk commented Feb 4, 2017

dtolnay commented Feb 5, 2017

Introduce collect_seq and collect_map #736

Introduce collect_seq and collect_map #736

Conversation

nox commented Jan 31, 2017 • edited Loading

nox commented Jan 31, 2017 • edited Loading

nox commented Jan 31, 2017

sfackler commented Jan 31, 2017

nox commented Jan 31, 2017

dtolnay commented Feb 1, 2017

nox commented Feb 1, 2017

dtolnay commented Feb 3, 2017

sfackler commented Feb 3, 2017

dtolnay commented Feb 3, 2017

nox commented Feb 3, 2017

dtolnay commented Feb 3, 2017

dtolnay commented Feb 3, 2017

dtolnay commented Feb 3, 2017

nox commented Feb 3, 2017 • edited Loading

nox commented Feb 3, 2017

dtolnay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nox commented Feb 4, 2017

oli-obk commented Feb 4, 2017 • edited Loading

oli-obk commented Feb 4, 2017

dtolnay commented Feb 5, 2017

nox commented Jan 31, 2017 •

edited

Loading

nox commented Jan 31, 2017 •

edited

Loading

nox commented Feb 3, 2017 •

edited

Loading

oli-obk commented Feb 4, 2017 •

edited

Loading