std::net::IpAddr fails to roundtrip using 0.14.1

[ghost]

We recently updated from 0.14.0 to 0.14.1 (and to serde version 1.0.104) and the following minimal test case

 use std::net::{IpAddr, Ipv4Addr};

use rmp_serde::{from_slice, to_vec};

#[test]
fn roundtrip_ip_addr() {
    let addr = IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1));
    let addr1: IpAddr = from_slice(&to_vec(&addr).unwrap()).unwrap();

    assert_eq!(addr1, addr);
}

started to fail with

Syntax("invalid type: integer `0`, expected `V4` or `V6`")'

[ghost]

Maybe this is related to #178 but that does supposedly not apply to serde version 1.0.81 or later?

[kornelski]

Yeah, that's a regression from #232

[kornelski]

Pinging @Byron

[ghost]

Yeah, that's a regression from #232

Thank you for figuring this out so quickly!

[kabergstrom]

@kornelski Does ExtDeserializer also need to return false for is_human_readable() perhaps?

Here are the serde impl for Ipv4
Deserialize: https://docs.rs/serde/1.0.104/src/serde/de/impls.rs.html#1252
Serialize: https://docs.rs/serde/1.0.104/src/serde/ser/impls.rs.html#679-692

[Byron]

Sorry for being the cause of this with my sloppy PR. I would also think that all [De]Serializer implementations should answer to is_human_readable() consistently. Maybe it would be enough to add that minimal test case to the suite and make it work?

If nobody is on it yet, I could give @kabergstrom suggestion a try.

[Byron]

I could reproduce the issue locally, but it wouldn't resolve itself when adding is_human_readable to all serializers/deserializers.

When looking more closely into this particular implementation, you see it definitely does something different for binary serializers.

Maybe serializing and deserializing newtype variants is actually incorrect right now?

[Byron]

Update: I tried my luck, but couldn't really figure out why it's not working. A decent foundation for looking into this is in my master branch, providing a failing test and the fixes proposed by @kabergstrom, which seem good to have in any case.

[kornelski]

From IpAddr implementation it seems it needs is_human_readable to agree on both serializer and deserializer.

I'm also surprised that it's not sufficient to add fn is_human_readable everywhere. Could it be delegating work to some other (de)serializer?

[kornelski]

            Value::Array(v) => {
                let len = v.len();
                let mut de = SeqDeserializer::new(v.into_iter());
                let seq = visitor.visit_seq(&mut de)?;
                if de.iter.len() == 0 {
                    Ok(seq)
                } else {
                    Err(de::Error::invalid_length(len, &"fewer elements in array"))
                }
            }

I suspect SeqDeserializer is the problem here.

[kabergstrom]

Good catch, looks like all impl... Deserializer<'de> for ... need to return false for is_human_readable()

[kornelski]

Perhaps solution would be to make a newtype Unreadable that overrides is_human_readable, and forwards all other methods to inner (de)serializer.

and then instead of SeqDeserializer::new() do Unreadable(SeqDeserializer::new()) (and same for MapDeserializer and others)

[kornelski]

@Byron would you like to try implementing "unreadable" wrapper for the SeqDeserializer and such?

[Byron]

I think I will, but I can only make myself do it when it starts blocking me. Working with anything 'serde' makes my brain twist itself in an attempt to understand what is calling what and how it all comes together. One moment I think I got it grokked, just to see that go away an instant later.

Anyhow, I am already a happy user of rmp as it powers the serialization needs of criner, a tool to mine crates.io and store that information in fast DB (sled) for later mining. One day, it could power lib.rs and the likes. The only thing I am interested in is to provide a list of 'bloated' crates and to help authors to improve that. That should help with initial compile speeds.

[joshtriplett]

I tried adding fn is_human_readable() { false } to the impls of serde::Serializer and serde::Deserializer. Doing so made the new roundtrip_ip_addr test fail as expected. But interestingly, Ipv4Addr and Ipv6Addr still round-trip just fine; it's the IpAddr wrapper that doesn't.

I also noticed that this test fails with the same error message, whether or not is_human_readable returns false:

#[test]
fn roundtrip_result_num() {
    let val: Result<u32, u32> = Ok(42);
    assert_roundtrips(val);
}

That test gives this error:

thread 'roundtrip_result_num' panicked at 'Does not deserialize: invalid type: integer `0`, expected `Ok` or `Err`
Serialized Ok(42)
Got Map([(Integer(PosInt(0)), Integer(PosInt(42)))])
', rmp-serde/tests/round.rs:443:13

That may be unrelated, despite the similar error message.

[joshtriplett]

In the course of looking into this, I discovered that this appears to be a bug in serde, not in msgpack-rust: serde-rs/serde#1888

If I build msgpack-rust against a version of serde with that patch applied, the test case above passes, along with one of the existing ignored FIXME tests, and several additional tests I added. That fix also makes the tests pass with fn is_human_readable(&self) { false } added to the serializer and deserializer.

As soon as a version of Serde with that patch is released, it should be straightforward to add new Config variants for HumanReadableConfig and BinaryConfig, which would override is_human_readable.

[joshtriplett]

#256 updates to the new serde and adds more tests. That should serve as a good foundation for adding new configuration for binary serialization.

[joshtriplett]

And #257 adds config support for binary vs human-readable.