Add optional serde support

[meh]

Two open questions:

1. Right now for the sake of feature clarity, the feature is called serde, but that means people have to explicitly enable std as well if they disable all features, it ends up working out if they just use the default features tho.
2. Should the deserializer be smarter and check the value is within bounds and fail deserialization?

[kjetilkjeka]

Thanks a lot for the PR @meh !

Serde seems great, but I'm not really familiar with it. Hope you can help me shed some light on things.

Right now for the sake of feature clarity, the feature is called serde, but that means people have to explicitly enable std as well if they disable all features, it ends up working out if they just use the default features tho.

If i remember correctly calling the feature serde is the consensus from other crates, so this should be fine. If people disable all features then std goes away, that is the expected behavior and is fine as well.

Should the deserializer be smarter and check the value is within bounds and fail deserialization?

Yes! We must not allow ways to create illegal values inside the wrappers, that fully defeats the purpose of using them. This crate aims to be as consistent with the native types as possible and serializing 128 into u7 should give the same result as serializing 256 into u8.

If you would have asked me what the benefits of being able to (de)serialize these types directly with serde instead of converting from native types i would assume the built in bounds checking was the main reason. Would you mind mentioning other reasons (if you have any) it would be nice to have serde support?

Perhaps this is clear when familiar with serde, but is it trivial to accept non byte aligned values for deserialization with serde from a binary format? I assume it's expected that the following struct should require 2 bytes of payload for deserialization (not 3 where a lot of the bits were padding which i suspect is the case when using derive). I guess being able to tightly pack bit fields would be another reason to want serde support for this library?

struct Foo {
    a: u9,
    b: u7,
}

Another concern that needs to be addressed is stability. Even though serde is very stable, i guess its possible that a version 2 would be released some time in the future. Since this crate is mimicking the std library and aiming to be completely stable once a good way to handle construction is found we should have a strategy of avoiding to bump the version number even if serde needs to. Do you know how other crates plan for this?

[meh]

If you would have asked me what the benefits of being able to (de)serialize these types directly with serde instead of converting from native types i would assume the built in bounds checking was the main reason. Would you mind mentioning other reasons (if you have any) it would be nice to have serde support?

It's just because I have structs that contain uX types, and I want to be able to use serde with them, checking the bounds on deserialization seems to be a must to maintain the invariants anyway.

Perhaps this is clear when familiar with serde, but is it trivial to accept non byte aligned values for deserialization with serde from a binary format? I assume it's expected that the following struct should require 2 bytes of payload for deserialization (not 3 where a lot of the bits were padding which i suspect is the case when using derive). I guess being able to tightly pack bit fields would be another reason to want serde support for this library?

Personally I'm just using these types to maintain invariants about the actual size of the fields, I'm probably going to end up using MessagePack, bincode and probably JSON to serialize/deserialize these structs, so I don't think it would be possible to tightly pack them right now.

I'm also not entirely sure if serde even supports this kind of thing as an optional hint for serializers/deserializers.

Another concern that needs to be addressed is stability. Even though serde is very stable, i guess its possible that a version 2 would be released some time in the future. Since this crate is mimicking the std library and aiming to be completely stable once a good way to handle construction is found we should have a strategy of avoiding to bump the version number even if serde needs to. Do you know how other crates plan for this?

Having a serde2 would probably be catastrophic for the ecosystem, so I don't think it's going to happen any time soon, if ever.

I'm going to add a custom deserializer that checks bounds.

[meh]

Okay, implemented Serialize as a forwarding serializer to the primitive type, and Deserialize as a forwarder with an additional bound check.

[kjetilkjeka]

I'm not really sure what we can expect when deriving serde traits on a struct containing these fields. I think it would be beneficial of having a way to derive packed serialization code. I would like to create an issue to discuss this a bit more before committing to it. I will merge this PR early if you:

• Create a nightly cfg attribute using build.rs and https://crates.io/crates/rustc_version
• Throw a compile error if serde is enabled outside nightly.
• Implement serde in a seperate file. Then just use the serde feature on the mod declaration instead of spread out.
• Document the serde feature in the README.md and be specific about it being an unstable nightly only feature. Linking to the issue where
• Implement relevant test cases (in the serde file) and make travis run them in the nightly configuration.
• Create entry in changelog

[kjetilkjeka]

Should this be an invalid_value with some Expected explaining that it was OOB?

[kjetilkjeka]

I will be traveling from Aug 11. to Aug 18. I might be able to check in on github a few times but expect communication to be slow this week and the days after my return.

[kjetilkjeka]

I've created #17 to discuss everything related to serde support.

I think we should be able to figure out how to solve the unresolved questions in the mentioned issue pretty quick. I have a few solutions in mind that i think would well. I hope that in the mean time, the above suggestion is acceptable for you.

Even though we're hopefully able to bring the serde feature out of nightly pretty quick, we've discussed unstable features before (#5) and having the infrastructure there would be nice. Hope you don't mind implementing this.

[meh]

I'm not really sure what we can expect when deriving serde traits on a struct containing these fields. I think it would be beneficial of having a way to derive packed serialization code.

There's no support for this in serde at all, and most serialization formats do not have a concept of bitfields either, it might work for bincode but most formats don't support that.

Create a nightly cfg attribute using build.rs and https://crates.io/crates/rustc_version

What is the purpose of making it nightly only? serde works on stable.

[kjetilkjeka]

There's no support for this in serde at all, and most serialization formats do not have a concept of bitfields either, it might work for bincode but most formats don't support that.

Does that mean that result from serializing into bincode is unimportant as long as a serialized value will be deserialized back to the same value?

Does that also mean that new format where binary encoding is important will be impossible to add for serde?

Or is it possible to opt out of supporting formats based on binary representations for the time being? Leaving it to be figured out in the future would be totally acceptable, implementing it in an unfortunate way is not.

What is the purpose of making it nightly only? serde works on stable.

If we're going to publish a new version of this crate with the feature in it, it must adhere to the rust/semver properties, meaning that the serde feature must be compatible to future versions. Well the former is true when compiled on stable, when using nightly rust all bets are off. This allow us to publish a crate with an unstable feature as long as it's only accessible on nightly. When everything about serde is sorted out, the nightly requirement would have been lifted.

I think it's a great idea to have the serde feature, but I want to make sure that we "get it right". Making the nightly only feature was an offer to get this PR merged early since I already wanted to have a nightly only options for this crate. If you prefer to sort these things out before merging that's totally fine as well.

I also want to leave it cooking for some amount of time to allow more people (that have more experience with serde than me) to have a look at it before merging, and giving me some time to experiment with serde as well. I understand that you probably need this feature for your work, and the parts you're going to use should not change after this PR is merged anyway. This means that if you're able to use nightly for the part of your work using this feature you should be able to not do anything after the feature is stabilized (except moving over to stable again).

If you prefer to track your own fork rather than crates.io for the time being, you may discard my review comments relating to the nightly feature.

[GuillaumeGomez]

Seems like a great add!

@kjetilkjeka: Do you intend to merge it? I didn't understand any of your points against it...

[kjetilkjeka]

@kjetilkjeka: Do you intend to merge it? I didn't understand any of your points against it..

Absolutely! I suspect my concerns are related to me not being familiar with serde, and either way I'm sure they're absolutely solvable. But since I'm away from 11. Aug to 18. Aug I didn't really have the time to resolve them before going away. I will try to get to this as soon as possible when I'm back.

If I cannot resolve my concerns related to binary formats quickly when I'm back home I will write a post to explain them further.

I still want the following minor changes before merging:

• I think we should change the custom error to invalid error with an informative expected, am I wrong here?
• Implement serde in a seperate file. Then the serde feature can be used on the mod declaration instead of "everywhere".
• Document the serde feature in the README.md or as doc comments in lib.rs ( If @meh prefers to leave this to me, this is fine)
• Implement test that confirms that deserialization of out of bounds numbers throw the correct error.
• Create entry in changelog ( If @meh prefers to leave this to me, this is fine )

I think it's really great to get contributions to this library, and serde support seems quite important in making it ergonomic to use this library. I'm sorry my unfamiliarity with serde and my trip is delaying this PR.

[GuillaumeGomez]

@kjetilkjeka: Glad to hear it. I was just a bit scared because of your comments. Sounds like things that @meh can solve I assume. :)

[npmccallum]

Can this issue be revisited? ux is being consumed by x86_64_types and we'd like to use it in rust-vmm. Having derived serde traits is super helpful to us.

[trevyn]

Since ux seems to be abandoned, I've published a fork with just this PR applied as ux_serde: https://crates.io/crates/ux_serde