Repository navigation
Added support for Arbitrary - #163
Conversation
|
For added context, this PR is needed to allow for adding |
|
|
||
| fn arbitrary_take_rest(mut u: Unstructured<'a>) -> ArbitraryResult<Self> { | ||
| // Use arbitrary for scale first (fixed size), then take rest for BigInt | ||
| let scale = i64::arbitrary(&mut u)? % SCALE_LIMIT; |
There was a problem hiding this comment.
Should the arbitrary scale be symmetric about zero, or should it be biased positive which indicates a positive number of decimal points which is the more likely scenario for using these? Or does it not matter for Arbitrary because every valid state should be representable? And do we care that 0 is twice as likely as other numbers (1 out of a million vs 1 out of 2 million)?
I'm not familiar with Arbitrary so I might be overthinking this. I don't know what the fuzzers are looking for.
There was a problem hiding this comment.
Also does order matter? arbitrary makes int first then scale, arbitrary_take_rest does scale then int. I feel like a well behaved impl would do those in the same order.
There was a problem hiding this comment.
Regarding #163 (comment)
I believe it should generate all possible valid states.
If implemented ideally, an Arbitrary impl of a downhill struct should take care of the distribution of the values depending of what values may be expected by the input stream of the downhill code.
Using the derive version should give a "reasonable baseline", and for instance it does not guarantee any self-consistent struct being generated by composition (which in certain harnesses it is the point as it forces the code to deal with any conceivable input it accepts). This would include a range-like struct that has start > end, even when all public methods to construct this struct have a Result that checks that start <= end. In such cases if the dev wants a self-consistent struct they need to write their own impl and avoid the derive.
There was a problem hiding this comment.
Regarding #163 (comment)
Consistency is better, I will adjust that and also fix a possible regression corner case that I noticed was uncovered in the test. It is not a problem at this time, but better safe than sorry.
|
Ok approved. Thanks a lot. Sorry I put this down for so many months. |
No worries, it'll be useful for fuzzing downhill! |
This PR adds support for the
arbitrarycrate by implementing theArbitrarytrait forBigDecimal. This enables usingBigDecimalwith fuzzing tools likecargo-fuzzandlibfuzzer, and more importantly allows to use theArbitraryderive in third party structs which include this struct as a field.A new optional feature
fuzzhas been added toCargo.toml:Note: The feature is named
fuzz(notarbitrary) to avoid a name collision with the dependency, which is required for compatibility with older Cargo versions (pre-1.60). The feature requiresstdbecause thearbitrarycrate itself does not supportno_stdImplementation Details
To prevent memory allocation failures during fuzzing, the implementation limits the generated scale to
±1,000,000. Without this limit, extremely large scale values could cause theBigDecimalto attempt allocating massive amounts of memory when converted to strings or performing certain operations.src/impl_arbitrary.rsimplementingArbitrary<'a>forBigDecimalarbitrary()- Generate a randomBigDecimalfrom unstructured dataarbitrary_take_rest()- Consume all remaining data to generate aBigDecimalsize_hint()- Provide size hints for the fuzzerUsage
Enable the feature in your
Cargo.toml:Example fuzz target:
Closes #122