The Float::int_pow_fast_path default method takes two arguments: an exponent, and a radix. The exponent has a safety invariant (that depends on the radix) which is documented. However, radixalso has a safety invariant: when the compact feature flag is not enabled (which it is not by default), Float::int_pow_fast_path results in UB if radix is not 5 or 10. This invariant is not documented:
In the current codebase, this method is indeed only called with a radix of 5 or 10. However, this method is part of the public API, so this invariant should be documented.
Alternatively (and my preference), we could remove the invariant entirely by changing radix into an enum, to enforce its allowed set of values. On its own this would require the enum to be crate-public (though not nameable), but I also note that Float::int_pow_fast_path doesn't actually use anything about Self at all, so we could just move it to a crate-private utility function and remove Float::int_pow_fast_path from the public API entirely (at which point using an enum for radix would be easy).
Prerequisites
minimal-lexical version: 0.1 and 0.2
It would be great if any change made to address this issue were backported to minimal-lexical 0.1.
Description
The
Float::int_pow_fast_pathdefault method takes two arguments: an exponent, and a radix. The exponent has a safety invariant (that depends on the radix) which is documented. However,radixalso has a safety invariant: when thecompactfeature flag is not enabled (which it is not by default),Float::int_pow_fast_pathresults in UB ifradixis not 5 or 10. This invariant is not documented:https://github.com/Alexhuszagh/minimal-lexical/blob/e997c46656ebe83e696b866bd954da1fa3f64eef/src/num.rs#L156-L167
In the current codebase, this method is indeed only called with a radix of 5 or 10. However, this method is part of the public API, so this invariant should be documented.
Alternatively (and my preference), we could remove the invariant entirely by changing
radixinto an enum, to enforce its allowed set of values. On its own this would require the enum to be crate-public (though not nameable), but I also note thatFloat::int_pow_fast_pathdoesn't actually use anything aboutSelfat all, so we could just move it to a crate-private utility function and removeFloat::int_pow_fast_pathfrom the public API entirely (at which point using an enum forradixwould be easy).Prerequisites
It would be great if any change made to address this issue were backported to
minimal-lexical 0.1.Additional Context
I'm in the process of migrating Fuchsia to use
nom 7, which depends onminimal-lexical 0.1, and am opening issues in response to review comments (visible here: https://fuchsia-review.googlesource.com/c/fuchsia/+/589324).