Re: [PATCH v13 1/6] rust: str: add radix prefixed integer parsing functions

"Benno Lossin" [off-list ref] writes:

On Thu Jun 19, 2025 at 1:12 PM CEST, Andreas Hindborg wrote:

quoted

I'm having a difficult time parsing. Are you suggesting that we guard
against implementations of `TryInto<u64>` that misbehave?

Let me try a different explanation:

The safety requirement for implementing the `FromStrRadix`:

    /// The member functions of this trait must be implemented according to
    /// their documentation.

Together with the functions of the trait:

    /// Parse `src` to [`Self`] using radix `radix`.
    fn from_str_radix(src: &BStr, radix: u32) -> Result<Self, crate::error::Error>;

    /// Return the absolute value of [`Self::MIN`].
    fn abs_min() -> u64;

    /// Perform bitwise 2's complement on `self`.
    ///
    /// Note: This function does not make sense for unsigned integers.
    fn complement(self) -> Self;

Doesn't make sense. What does it mean to return the "absolute value of
[`Self::MIN`]"? We don't have "absolute value" defined for an arbitrary
type. Similarly for `complement` and `from_str_radix`, what does "Parse
`src` to [`Self`] using radex `radix`" mean? It's not well-defined.

You use this safety requirement in the parsing branch for negative
numbers (the `unsafe` call at the bottom):

    [b'-', rest @ ..] => {
        let (radix, digits) = strip_radix(rest.as_ref());
        // 2's complement values range from -2^(b-1) to 2^(b-1)-1.
        // So if we want to parse negative numbers as positive and
        // later multiply by -1, we have to parse into a larger
        // integer. We choose `u64` as sufficiently large.
        //
        // NOTE: 128 bit integers are not available on all
        // platforms, hence the choice of 64 bits.
        let val =
            u64::from_str_radix(core::str::from_utf8(digits).map_err(|_| EINVAL)?, radix)
                .map_err(|_| EINVAL)?;

        if val > Self::abs_min() {
            return Err(EINVAL);
        }

        if val == Self::abs_min() {
            return Ok(Self::MIN);
        }

        // SAFETY: We checked that `val` will fit in `Self` above.
        let val: Self = unsafe { val.try_into().unwrap_unchecked() };

        Ok(val.complement())
    }

But you don't mention that the check is valid due to the safety
requirements of implementing `FromStrRadix`. But even if you did, that
wouldn't mean anything as I explained above.

So let's instead move all of this negation & u64 conversion logic into
the `FromStrRadix` trait. Then it can be safe & the `ParseInt::from_str`
function doesn't use `unsafe` (there still will be `unsafe` in the
macro, but that is fine, as it's more local and knows the concrete
types).

Alright. I guess my safety comments are slightly hand-wavy. Thanks for
the suggestion, I'll apply that for next spin.

Best regards,
Andreas Hindborg