akubera / bigdecimal-rs

Arbitrary precision decimal crate for Rust
Other
284 stars 71 forks source link

impl Arbitrary for BigDecimal #122

Open Ekleog opened 9 months ago

Ekleog commented 9 months ago

Hey!

I'm trying to fuzz some code that uses BigDecimal, and am hitting the issue that it does not implement Arbitrary. Currently, my solution is the following code to generate one:

BigDecimal::new(u.arbitrary()?, u.arbitrary::<i64>()? % 1000000)

I originally did not have the % 1000000, but it resulted in memory allocation failures.

What's the recommended way to use BigDecimal from a fuzzer? Relatedly, maybe it'd make sense to add an arbitrary feature to BigDecimal, allowing to auto-generate them?

Anyway, thank you for BigDecimal! I'm currently stuck on 0.3 because sqlx didn't update probably due to https://github.com/akubera/bigdecimal-rs/issues/117 ; but it's really coming in handy to not have to care about floating points and integers too much :)

akubera commented 9 months ago

If you check out my property tests I'm using proptest to generate f32 and pairs of (i128, i8) to use as (mantissa, exponent), and using those to build random BigDecimals on which I can test properties like (a - b) == -(b - a).

Doing something more robust would be nice, but it's pretty low on the todo-list.

What's the standard procedure for Arbitrary? Is it add a feature and then just:

#[cfg_attr(feature='arbitrary', derive(Arbitrary)]
struct BigDecimal { ... }

and as long as BigInt implements it too, it works? or would the mod 1000000 be required and it needs something more configurable by the user? Something like ArbitraryBigDecimalBuilder::max(decimal_places)...?

I originally did not have the % 1000000, but it resulted in memory allocation failures.

Yeah I've hit that too :-(.

Ekleog commented 9 months ago

Arbitrary impls usually don't have anything more configurable by the user. Considering the scale issue, I think an impl Arbitrary for BigDecimal { ... } where the contents of the function is basically what I put in my first message should work out fine, and be much more usable for the user than the statu quo :)

That said, if you hit it too, TBH I'm not entirely sure why a too big scale leads to memory allocation issues, so this might actually be some bug inside bigdecimal itself? I thought that for me it was just due to serializing to string, and thus allocating too long a string.

(For full disclosure, I eventually noticed I don't actually need arbitrary decimals, and switched to the finite-length rust_decimal crate since opening this issue, so it went down quite a bit in my priority list too)

akubera commented 9 months ago

I think the bug is in the implementation of Debug/Display, where the decimal is printed in non-exponential form, even when it'll write exabytes of leading/trailing zeros. I think that's what https://github.com/akubera/bigdecimal-rs/pull/121 is addressing but I haven't reviewed it, yet.