P3KI / bendy

A rust library for encoding and decoding bencode with enforced cannonicalization rules.
BSD 3-Clause "New" or "Revised" License
70 stars 13 forks source link

Add support for all serde data model types #32

Closed casey closed 4 years ago

casey commented 4 years ago

Add support for serializing and deserializing all serde data model types, including bool, f32, f64, (), Option, char, unit structs, enums, maps with byte-string keys.

Also adds support for deserialize_any, which allows deserializing into types which require self-describing formats.

The top-level doc-comment for the module is extended with example representations for values of many of the above types.

I tried to pick reasonable, lossless representations for all serde data model types, and documented what representation users should expect in the module-level doc-comment.

I used le as a sort of nil value, which has at least some precedent with lisps.

I somewhat arbitrarily picked little-endian byte order for floats, since that's the order that most processors use. I'm not sure I made the right choice here, and big endian might feel more "natural", since that's the order usually used in network protocols.

casey commented 4 years ago

I realized that bencode integers are big-endian, so I think it makes sense to be consistent with that, so I changed f32s and f64s to be big-endian.

casey commented 4 years ago

The Travis tests are failing for 1.36. There are a few things I'm using which I can easily remove (like referring to enum variants via the Self alias), but one tricky one is `from_be

It looks like {to,from}_be_bytes was only stabilized in Rust 1.40, and continuous integration runs with 1.36. If it's acceptable, I could bump continuous integration go 1.40.

Another option is to just implement from_be_bytes and to_be_bytes in this crate. If you look at the stdlib implementations, they're just doing a mem::transmute on the u32 bits of the float values, so it wouldn't be much code, although it would involve using unsafe.

Another possibility is to remove float support entirely, which might not be too sorely missed.

thequux commented 4 years ago

I made some comments on the Value PR, as I saw that first on the list :-/ Sorry for being confusing.

I am somewhat mixed on float support; on one hand, a significant part of bencode is that there is exactly one valid encoding for any given format, and near as I can tell, nobody actually uses floats in bencode, but on the other, that's an unfortunate limit to have.

Also, we need to be able to build for ESP32, which currently only supports Rust 1.39, so bumping the CI version is not an option.

However, a quick scan of the documentation for 1.36 shows that we have f32::from_bits and f32::to_bits, which do nearly exactly the transmutation we need. Thus, I think the best option is something like:

fn f32_to_be_bytes(value: f32) -> [u8; 4] {
    value.to_bits().to_be_bytes()
}
fn f32_from_be_bytes(value: [u8;4]) -> f32 {
   f32::from_bits(u32::from_be_bytes(value))
}

How does that sound?

casey commented 4 years ago

I am somewhat mixed on float support; on one hand, a significant part of bencode is that there is exactly one valid encoding for any given format, and near as I can tell, nobody actually uses floats in bencode, but on the other, that's an unfortunate limit to have.

Yeah, I agree that it's unfortunate to introduce types that don't have bencode's canonical representation rule. On the other hand, users would probably just be annoyed if common rust types like f32 and f64 aren't supported. I added a note to the module documentation about floating point values not having canonical representations.

However, a quick scan of the documentation for 1.36 shows that we have f32::from_bits and f32::to_bits, which do nearly exactly the transmutation we need. Thus, I think the best option is something like:

How does that sound?

Sweet, yeah, that sounds great. I modified the float ser/de code to use to_bits and from_bits

I also refactored the code to avoid a few other things I was doing that were incompatible with 1.36, but they were very minor, like using Self::Variant on enums.

thequux commented 4 years ago

I merged this manually with rebase, but unfortunately there's no way for git to indicate that I did that :-/

casey commented 4 years ago

but unfortunately there's no way for git to indicate that I did that

No worries! Thank you very much for reviewing 🙇‍♂️