paupino / rust-decimal

Decimal number implementation written in pure Rust suitable for financial and fixed-precision calculations.
https://docs.rs/rust_decimal/
MIT License
995 stars 181 forks source link

Expose mutually exclusive rkyv features for size. #637

Open gz opened 9 months ago

gz commented 9 months ago

rkyv by default serializes usize as u32. This isn't ideal on most modern platforms and unfortunately is configured by a set of (mutually exclusive) feature flags.

Currently rust-decimal sets it to 32-bit in it's Cargo.toml file. This means downstream apps can't override it (or just enable 64-bit de-serialization) as it will break compilation because these flags are mutually exclusive.

For libraries it's recommended by the rkyv maintainer to just use default-features=false and then "re-export" features so the user can control the right serialization size instead of setting one. So this fix removes the features that were unnecessarily set by the Cargo.toml and adds some new features to activate different rkyv sizes. The approach is similar to what the ordered-float or chrono crate do:

https://github.com/reem/rust-ordered-float/blob/8111b345372632893af0b8aa12152f3dc7278aba/Cargo.toml#L37 https://github.com/chronotope/chrono/blob/6ec8f97d16ce320a6f948b1cd1494ed3a21b251f/Cargo.toml#L32

gz commented 9 months ago

This breaks library upgraders - to avoid this you'd need to keep the default rkyv feature and point it to rkyv-32. To do this you'll also need to use the dep: format.

Yes. Unfortunately, I'm not sure what's worse in this case. Just to make sure everyone is on the same page: The following code will compile (but produce wrong results on 64-bit -- assert triggers) at runtime and there is currently no way to change it for a client if they want to use rust-decimal.

fn main() {
    use rkyv::Deserialize;
    let value = usize::MAX;

    // serialize and deserialize the value
    use rkyv::ser::{Serializer, serializers::AllocSerializer};
    let mut serializer = AllocSerializer::<0>::default();
    serializer.serialize_value(&value).unwrap();
    let bytes = serializer.into_serializer().into_inner();
    let archived = unsafe { rkyv::archived_root::<usize>(&bytes[..]) };
    let deserialized: usize = archived.deserialize(&mut rkyv::Infallible).unwrap();

    assert_eq!(deserialized, value);
}

So assuming most likely the majority of users of this library are on 64-bit architectures, and if there are actually any users of rkyv they might just have a bug that's much harder to catch than getting a one-time compile-time breakage during upgrade... Those were my thoughts about it but I understand if you don't want to break it.

While this change seems simple, it actually causes a couple of issues:

Yep, unfortunately mutually exclusive features aren't really a great idea. I'm glad to see it got fixed in https://github.com/rkyv/rkyv/pull/461. I'll check with the rkyv maintainer when we can expect a release for it.