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

[Andreas Hindborg <a.hindborg@kernel.org>]

"Benno Lossin" <lossin@kernel.org> writes:

> On Thu Jun 19, 2025 at 1:12 PM CEST, Andreas Hindborg wrote:
>> 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