Serialize to binary if the serde format is not human readable

[Marwes]

This implements the KISS suggested in #790.
It is possible that one of the other approaches may be better but this
seemed like the simplest one to reignite som discussion.

Personally I find the original suggestion of adding two traits perhaps slightly
cleaner in theory but I think it ends up more complicated in the end
since the added traits also need to be duplicated to to the Seed
traits.

Example usage in https://github.com/rust-lang-nursery/uuid: Marwes/uuid@f908fd3

Closes #790

[Marwes]

FYI: The is not fully fleshed out in method names and documentation, will fix that before merging.

[dtolnay]

I am still on board with this approach. Thanks for getting things moving!

Could you brainstorm a few advantages and disadvantages of using &self vs no &self?

[Marwes]

With &self it becomes possible for [de]serializers to choose at runtime whether to be human readable or not. This is probably not common occurrence though so not a big deal either way.

The erased-serde crate will need to take &self so taking &self here would be consistent.

Downside is that one needs an instance to check if the format is human readable. Seems to me that would always be the case? Maybe in the future with const functions it would be useful to query is_human_readable at compile time? If so, maybe it should simply be an associated constant?

&self is in theory some extra overhead but I can't imagine LLVM struggling with that!

[dtolnay]

Makes sense to me. The erased-serde case is a great point, I had not thought of how that would work.

What would you recommend for rolling this out? If I release this now, it would be a breaking change to use this in any of our currently built-in Serialize impls later because you might have serialized one in human-readable form to a non-self-describing format and then be unable to deserialize it in non-human-readable form. That seems to mean we need to update all built-in impls like IpAddr in the same release as this change, right?

Changing a format from human-readable to non-human-readable must be done in a breaking change right?

[clarfonthey]

Wouldn't it make the most sense for formats to simply allow deserialisation from all formats (readable and not) and only serialise to one or the other depending on whether it's marked as readable or not?

That makes the most sense to me.

[Marwes]

Wouldn't it make the most sense for formats to simply allow deserialisation from all formats (readable and not) and only serialise to one or the other depending on whether it's marked as readable or not?

That is not possible for formats such as bincode which require that the deserialized value tell the deserializer what the upcoming data is as the serialized data is just bytes without knowledge what values it contains.

[Marwes]

Changing a format from human-readable to non-human-readable must be done in a breaking change right?

Yeah :/ . In theory we could document that making a format aware of the human readable distinction require a breaking change. The formats could then avoid the breaking change by making is_human_readable() == false opt-in or by themselves making a breaking change. Does that make sense?

[dtolnay]

Makes sense. We still need to update all the relevant Serialize and Deserialize impls in this PR.

[Marwes]

I believe only ip addresses and socket address needs a non-human readble serialization? Those are the only types which use str::parse or did I miss some other types?.

Just need to document a bit better and this should be done.

EDIT: Also need to fix serialization of net::IpAddr as they I broke roundtripping in ad3335e as well as tests for that.

[Marwes]

This is ready for review now!

[Marwes]

@dtolnay Any problems with the current implementation?

[dtolnay]

Sorry I didn't get a chance to review this weekend, but it is high on my list and I will try to get to it in the next couple days.

Can you think of any other backward-compatible ways to expose this in serde_test without adding so many new functions?

[Marwes]

Can you think of any other backward-compatible ways to expose this in serde_test without adding so many new functions?

I could change it to use the builder pattern, but that won't make it smaller now but would only serve to reduce the API surface if more configuration parameters were added in the future.

[Marwes]

I suppose I could move the assert_tokens functions to be methods on Serializer and Deserializer (forwarding the current implementations to those). That way the current functions are just for convenience/backwards compatibility while the more general way to assert requires one to use Serializer and Deserializer as "builders".

[dtolnay]

How about either of these?

• Having the &[Token] arguments accept a type parameter instead, where the type parameter bound is implemented for &[Token] as well as Readable(&[Token]) or something like that. I think this may require Unsize to implement backward compatibly but you would have to experiment.
• Provide a wrapper around a T: Serialize that always serializes it as readable or always as unreadable, regardless of what the T wants to do. Then the caller invokes assert_tokens(&Unreadable(T), ...).

Not saying these are backward compatibly or better, but a possible direction to think about.

[Marwes]

Having the &[Token] arguments accept a type parameter instead, where the type parameter bound is implemented for &[Token] as well as Readable(&[Token]) or something like that. I think this may require Unsize to implement backward compatibly but you would have to experiment.

That would work and would be backwards compatible modulo deref coercions failing due to being a generic parameter. Seems more difficult to understand though.

Provide a wrapper around a T: Serialize that always serializes it as readable or always as unreadable, regardless of what the T wants to do. Then the caller invokes assert_tokens(&Unreadable(T), ...).

How would that work? A wrapper around a generic T can't affect how Tserializes/deserializes.

[dtolnay]

A wrapper around a generic T can't affect how T serializes/deserializes.

struct Unreadable<T>(T);

impl<T> Serialize for Unreadable<T>
    where T: Serialize
{
    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
        where S: Serializer
    {
        self.0.serialize(UnreadableSerializer(serializer))
    }
}

struct UnreadableSerializer<S>(S);

impl<S> Serializer for UnreadableSerializer<S>
    where S: Serializer
{
    fn is_human_readable(&self) -> bool {
        false
    }

    /* forward others */
}

[Marwes]

Ah, yep that would work. Doing the same thing for Deserialize would work as well I guess.

Personally I feel like it is not very intuitive. If I were to come in with no prior knowledge and look at serde_test I'd find the solution rather strange :/

[dtolnay]

I agree that we may be able to come up with something more intuitive. I will keep brainstorming alternatives.

---

For completeness, here is what that approach would have looked like in tests:

let s = /* ... */;

// In human readable formats, field `timestamp` serializes as ISO 8601.
assert_tokens(&Readable(s), &[
    Token::Struct { name: "S", len: 1 },
    Token::Str("timestamp"),
    Token::String("2017-09-26T21:49:25Z"),
    Token::StructEnd,
]);

// In binary formats, field `timestamp` serializes in compact byte form.
assert_tokens(&Unreadable(s), &[
    Token::Struct { name: "S", len: 1 },
    Token::Str("timestamp"),
    Token::Bytes(&[226, 152, 131]),
    Token::StructEnd,
]);

[Marwes]

Builder pattern on Serializer/Deserializer

Serializer::new(tokens)
    .readable(false)
    .assert_serialize(&serialize_value)
Deserializer::new(tokens)
    .readable(false)
    .assert_deserialize()

[dtolnay]

I am still not sold on the serde_test piece of this but I don't want that to block the feature any longer. Let's get this released in serde and then take a bit more time to design the test api.

To that end:

• Please #[doc(hidden)] the new serde_test code with a mini comment something like // Not public API. That way we can continue to use this in our test suite without committing to it publicly.
• To keep our options open, please have serde_test's serializer and deserializer panic in is_human_readable unless the readableness has been set explicitly through one of the hidden functions. I kind of want to force types that have distinct readable/compact representations to be tested explicitly in one or the other, rather than with a plain assert_tokens which arbitrarily picks one. What do you think?
• File an issue to follow up on exposing this in serde_test, and link to the issue from the panic message.

[Marwes]

@dtolnay Sounds reasonable, should get to it in a few days.

[arthurprs]

This is sort of bikeshedding but is_text_format (or something similar) might make more sense.

Btw, this is a great improvement 😄

[Marwes]

I kind of want to force types that have distinct readable/compact representations to be tested explicitly in one or the other, rather than with a plain assert_tokens which arbitrarily picks one. What do you think?

assert_tokens does not arbitrarily pick one though, it will always beis_human_readable == true since that is the default. So I am not sure how I can make it panic since that would break code using serde_test but which happen to serialize one of the types that now happen to call is_human_readble.

It should be enough to just hide the functions though as is_human_readble == true will still mean that the behavior is preserved.