diff --git a/fish-rust/src/builtins/random.rs b/fish-rust/src/builtins/random.rs index b6e8533c7..232088d3f 100644 --- a/fish-rust/src/builtins/random.rs +++ b/fish-rust/src/builtins/random.rs @@ -7,11 +7,14 @@ use crate::builtins::shared::{ use crate::ffi::parser_t; use crate::wchar::{wstr, L}; use crate::wgetopt::{wgetopter_t, wopt, woption, woption_argument_t}; -use crate::wutil::{self, fish_wcstoi_radix_all, format::printf::sprintf, wgettext_fmt}; +use crate::wutil::{ + self, fish_wcstoi_opts, format::printf::sprintf, wgettext_fmt, Options as WcstoiOptions, +}; use num_traits::PrimInt; use once_cell::sync::Lazy; use rand::rngs::SmallRng; use rand::{Rng, SeedableRng}; +use std::default::Default; use std::sync::Mutex; static RNG: Lazy> = Lazy::new(|| Mutex::new(SmallRng::from_entropy())); @@ -73,7 +76,13 @@ pub fn random( cmd: &wstr, num: &wstr, ) -> Result { - let res = fish_wcstoi_radix_all(num, None, true); + let res = fish_wcstoi_opts( + num, + WcstoiOptions { + consume_all: true, + ..Default::default() + }, + ); if res.is_err() { streams .err diff --git a/fish-rust/src/wutil/wcstoi.rs b/fish-rust/src/wutil/wcstoi.rs index b7c3c8de9..f96c9ddfb 100644 --- a/fish-rust/src/wutil/wcstoi.rs +++ b/fish-rust/src/wutil/wcstoi.rs @@ -1,6 +1,7 @@ pub use super::errors::Error; use crate::wchar::IntoCharIter; use num_traits::{NumCast, PrimInt}; +use std::default::Default; use std::iter::{Fuse, Peekable}; use std::result::Result; @@ -11,6 +12,20 @@ struct ParseResult { consumed: usize, } +#[derive(Copy, Clone, Debug, Default)] +pub struct Options { + /// If set, and the requested type is unsigned, then negative values are wrapped + /// to positive, as strtoul does. + /// For example, strtoul("-2") returns ULONG_MAX - 1. + pub wrap_negatives: bool, + + /// If set, it is an error to have unconsumed characters. + pub consume_all: bool, + + /// The radix, or None to infer it. + pub mradix: Option, +} + struct CharsIterator> { chars: Peekable>, consumed: usize, @@ -43,9 +58,10 @@ impl> CharsIterator { /// - Leading 0 means 8. /// - Otherwise 10. /// The parse result contains the number as a u64, and whether it was negative. -fn fish_parse_radix>( +fn parse_radix>( iter: Iter, mradix: Option, + error_if_negative: bool, ) -> Result { if let Some(r) = mradix { assert!((2..=36).contains(&r), "fish_parse_radix: invalid radix {r}"); @@ -76,6 +92,10 @@ fn fish_parse_radix>( _ => negative = false, } + if negative && error_if_negative { + return Err(Error::InvalidChar); + } + // Determine the radix. let radix = if let Some(radix) = mradix { radix @@ -134,8 +154,7 @@ fn fish_parse_radix>( /// Parse some iterator over Chars into some Integer type, optionally with a radix. fn fish_wcstoi_impl( src: Chars, - mradix: Option, - consume_all: bool, + options: Options, out_consumed: &mut usize, ) -> Result where @@ -146,23 +165,41 @@ where assert!(bits <= 64, "fish_wcstoi: Int must be <= 64 bits"); let signed = Int::min_value() < Int::zero(); + let Options { + wrap_negatives, + consume_all, + mradix, + } = options; + let ParseResult { result, negative, consumed_all, consumed, - } = fish_parse_radix(src, mradix)?; + } = parse_radix(src, mradix, !signed && !wrap_negatives)?; *out_consumed = consumed; - if !signed && negative { - Err(Error::InvalidChar) - } else if consume_all && !consumed_all { + assert!(!negative || result > 0, "Should never get negative zero"); + + if consume_all && !consumed_all { Err(Error::CharsLeft) - } else if !signed || !negative { + } else if !negative { match Int::from(result) { Some(r) => Ok(r), None => Err(Error::Overflow), } + } else if !signed { + // strtoul documents "if there was a leading minus sign, the negation of the result of the conversion". + // However in practice it's modulo the base. For example strtoul("-1") returns ULONG_MAX. + // We wish to check if `val + base` is in the range [0, base), where val is negative. + // Rewrite this as `base - -val`; this will be in range iff val is at least 1 and less than base. + assert!(result > 0, "Should never get negative zero"); + let max_val = Int::max_value().to_u64().unwrap(); + if result > max_val { + Err(Error::Overflow) + } else { + Ok(Int::from(max_val - result + 1).expect("value should be in range")) + } } else { assert!(signed && negative); // Signed type, so convert to s64. @@ -187,29 +224,17 @@ where Chars: IntoCharIter, Int: PrimInt, { - fish_wcstoi_impl(src.chars(), None, false, &mut 0) + fish_wcstoi_impl(src.chars(), Default::default(), &mut 0) } /// Convert the given wide string to an integer using the given radix. /// Leading whitespace is skipped. -pub fn fish_wcstoi_radix(src: Chars, radix: u32) -> Result +pub fn fish_wcstoi_opts(src: Chars, options: Options) -> Result where Chars: IntoCharIter, Int: PrimInt, { - fish_wcstoi_impl(src.chars(), Some(radix), false, &mut 0) -} - -pub fn fish_wcstoi_radix_all( - src: Chars, - radix: Option, - consume_all: bool, -) -> Result -where - Chars: IntoCharIter, - Int: PrimInt, -{ - fish_wcstoi_impl(src.chars(), radix, consume_all, &mut 0) + fish_wcstoi_impl(src.chars(), options, &mut 0) } /// Convert the given wide string to an integer. @@ -218,12 +243,16 @@ where /// - 0 means octal, 0x means hex /// - Leading + is supported. /// The number of consumed characters is returned in out_consumed. -pub fn fish_wcstoi_partial(src: Chars, out_consumed: &mut usize) -> Result +pub fn fish_wcstoi_partial( + src: Chars, + options: Options, + out_consumed: &mut usize, +) -> Result where Chars: IntoCharIter, Int: PrimInt, { - fish_wcstoi_impl(src.chars(), None, false, out_consumed) + fish_wcstoi_impl(src.chars(), options, out_consumed) } #[cfg(test)] @@ -236,10 +265,17 @@ mod tests { } #[test] - fn tests() { + fn test_signed() { let run1 = |s: &str| -> Result { fish_wcstoi(s.chars()) }; - let run1_rad = - |s: &str, radix: u32| -> Result { fish_wcstoi_radix(s.chars(), radix) }; + let run1_rad = |s: &str, radix: u32| -> Result { + fish_wcstoi_opts( + s.chars(), + Options { + mradix: Some(radix), + ..Default::default() + }, + ) + }; assert_eq!(run1(""), Err(Error::Empty)); assert_eq!(run1(" \n "), Err(Error::Empty)); assert_eq!(run1("0"), Ok(0)); @@ -283,12 +319,90 @@ mod tests { test_min_max(std::u64::MIN, std::u64::MAX); } + #[test] + fn test_unsigned() { + fn negu(x: u64) -> u64 { + std::u64::MAX - x + 1 + } + + let run1 = |s: &str| -> Result { + fish_wcstoi_opts( + s.chars(), + Options { + wrap_negatives: true, + ..Default::default() + }, + ) + }; + let run1_rad = |s: &str, radix: u32| -> Result { + fish_wcstoi_opts( + s.chars(), + Options { + wrap_negatives: true, + mradix: Some(radix), + ..Default::default() + }, + ) + }; + assert_eq!(run1("-5"), Ok(negu(5))); + assert_eq!(run1(""), Err(Error::Empty)); + assert_eq!(run1(" \n "), Err(Error::Empty)); + assert_eq!(run1("0"), Ok(0)); + assert_eq!(run1("-0"), Ok(0)); + assert_eq!(run1("+0"), Ok(0)); + assert_eq!(run1("+00"), Ok(0)); + assert_eq!(run1("-00"), Ok(0)); + assert_eq!(run1("+0x00"), Ok(0)); + assert_eq!(run1("-0x00"), Ok(0)); + assert_eq!(run1("+-0"), Err(Error::InvalidChar)); + assert_eq!(run1("-+0"), Err(Error::InvalidChar)); + assert_eq!(run1("5"), Ok(5)); + assert_eq!(run1("-5"), Ok(negu(5))); + assert_eq!(run1("123"), Ok(123)); + assert_eq!(run1("+123"), Ok(123)); + assert_eq!(run1("-123"), Ok(negu(123))); + assert_eq!(run1("123"), Ok(123)); + assert_eq!(run1("+0x123"), Ok(291)); + assert_eq!(run1("-0x123"), Ok(negu(291))); + assert_eq!(run1("+0X123"), Ok(291)); + assert_eq!(run1("-0X123"), Ok(negu(291))); + assert_eq!(run1("+0123"), Ok(83)); + assert_eq!(run1("-0123"), Ok(negu(83))); + assert_eq!(run1(" 345 "), Ok(345)); + assert_eq!(run1(" -345 "), Ok(negu(345))); + assert_eq!(run1(" x345"), Err(Error::InvalidChar)); + assert_eq!(run1("456x"), Ok(456)); + assert_eq!(run1("456 x"), Ok(456)); + assert_eq!(run1("99999999999999999999999"), Err(Error::Overflow)); + assert_eq!(run1("-99999999999999999999999"), Err(Error::Overflow)); + // This is subtle. "567" in base 8 is "375" in base 10. The final "8" is not converted. + assert_eq!(run1_rad("5678", 8), Ok(375)); + } + + #[test] + fn test_wrap_neg() { + fn negu(x: u64) -> u64 { + std::u64::MAX - x + 1 + } + + let run1 = |s: &str, opts: Options| -> Result { fish_wcstoi_opts(s, opts) }; + let mut opts = Options::default(); + assert_eq!(run1("-123", opts), Err(Error::InvalidChar)); + assert_eq!(run1("-0x123", opts), Err(Error::InvalidChar)); + assert_eq!(run1("-0", opts), Err(Error::InvalidChar)); + + opts.wrap_negatives = true; + assert_eq!(run1("-123", opts), Ok(negu(123))); + assert_eq!(run1("-0x123", opts), Ok(negu(0x123))); + assert_eq!(run1("-0", opts), Ok(0)); + } + #[test] fn test_partial() { let run1 = |s: &str| -> (i32, usize) { let mut consumed = 0; - let res = - fish_wcstoi_partial(s.chars(), &mut consumed).expect("Should have parsed an int"); + let res = fish_wcstoi_partial(s, Default::default(), &mut consumed) + .expect("Should have parsed an int"); (res, consumed) };