feat: implement num_trait gaps Bounded, NumCast, ConstZero, ConstOne…#800
feat: implement num_trait gaps Bounded, NumCast, ConstZero, ConstOne…#800gechelberger wants to merge 2 commits into
Conversation
…, and saturating binary op traits
|
|
||
| impl num_traits::NumCast for Decimal { | ||
| fn from<T: ToPrimitive>(n: T) -> Option<Self> { | ||
| n.to_f64().and_then(<Decimal as FromPrimitive>::from_f64) |
There was a problem hiding this comment.
The other contracts look good, but the implementation of this one I feel is problematic. For example:
use num_traits::FromPrimitive;
let n: u64 = 9_007_199_254_740_993;
assert_eq!(
<Decimal as n>::from(n),
Some(Decimal::from_u64(n).expect("Parsed successfully"))
);Will fail with:
assertion `left == right` failed
left: Some(9007199254740992)
right: Some(9007199254740993)
This feels tricky to justify as a rounding error, given it's an integer.
Another one:
use num_traits::FromPrimitive;
let n: i128 = 79_228_162_514_264_337_593_543_950_000; // close to Decimal::MAX
assert_eq!(
<Decimal as NumCast>::from(n),
Some(Decimal::from_i128(n).expect("Parsed successfully"))
);Will fail with:
assertion `left == right` failed
left: None
right: Some(79228162514264337593543950000)
My concern here is that you get subtle differences depending on how you parse the number, which I would consider as unexpected - given it's target is a Decimal. I think the latter example is validated somewhat in the docs for NumCast:
If the source value cannot be represented by the target type, then None is returned.
I actually think this interface is a really tough one to work with and not really made for numbers outside of the ToPrimitive set. The only solution I can think of that kind of meets in the middle is to use f64 but if there is no fract then fallback to wide integer arithmetic. It wouldn't be the "performant" path, but may reduce some confusion over conversion mistmatches.
e.g. (untested, but something like this)
impl num_traits::NumCast for Decimal {
fn from<T: ToPrimitive>(n: T) -> Option<Self> {
let f = n.to_f64()?;
if f.is_finite() && f.fract() == 0.0 {
if let Some(i) = n.to_i128() { return Decimal::from_i128(i); }
if let Some(u) = n.to_u128() { return Decimal::from_u128(u); }
}
Decimal::from_f64(f)
}
}I think the other thing we need here is more test coverage just to make sure we're covering alternate angles.
There was a problem hiding this comment.
That's a good point.
I think we can drop the second u128 fallback. It should always return None since the extrema of i128 are already outside the bounds of what Decimal can represent.
Either way, I think that this resolves your round-trip concern. I'll update it and include a test suite to prove to myself that it actually does what I expect.
|
I took your proposed solution and it seems like it does what we want. The one exception is that f32 doesn't round trip exactly but instead takes on an f64 flavor. I don't think we can do better. Either we accept this or choose not to implement NumCast for Decimal (which seems like a perfectly defensible position to me). Since proptest was already an optional feature on the cate, I added proptest as a dev-dependency to get proptest style coverage for the desired contracts |
Closes #573.
Fills a handful of
num_traitsgaps whereDecimalalready had theunderlying constants/methods but no trait impl, which forces downstream
generic code to special-case
Decimal.Trait impls added
BoundedDecimal::MIN/Decimal::MAXsrc/decimal.rsConstZeroDecimal::ZERO(const)src/decimal.rsConstOneDecimal::ONE(const)src/decimal.rsNumCastToPrimitive::to_f64→Decimal::from_f64src/decimal.rsSaturatingAddDecimal::saturating_addsrc/arithmetic_impls.rsSaturatingSubDecimal::saturating_subSaturatingMulDecimal::saturating_mulAlso exposes
Boundedthroughrust_decimal::preludenext to the existingOne/Zero/Signedre-exports. (NumCast/Const*/Saturating*aretypically used through generic bounds, not prelude imports — left out
intentionally.)
This PR bumps the num_traits dependency from 0.2.0 to 0.2.18 in order to include ConstZero and ConstOne. My original plan was to leave it at 0.2.0 but after I had included the saturating operation traits I realized that they would force a bump to 0.2.12. I figured we could just include them all to see what the full changeset would look like. If you decide that you don't want to change the dependency resolution, it's trivial to strip those traits and trim the scope down to NumCast and Bounded.