Merge pull request #7675 from Qelxiros/7669-sleep-hex-parsing

fix(sleep): use uucore's from_str to support parsing hex

Author: RenjiSann

src/uu/sleep/Cargo.toml

@@ -20,7 +20,7 @@ path = "src/sleep.rs"
 [dependencies]
 clap = { workspace = true }
 fundu = { workspace = true }
-uucore = { workspace = true }
+uucore = { workspace = true, features = ["parser"] }
 
 [[bin]]
 name = "sleep"

src/uu/sleep/src/sleep.rs

@@ -8,11 +8,12 @@ use std::time::Duration;
 
 use uucore::{
     error::{UResult, USimpleError, UUsageError},
-    format_usage, help_about, help_section, help_usage, show_error,
+    format_usage, help_about, help_section, help_usage,
+    parser::parse_time,
+    show_error,
 };
 
 use clap::{Arg, ArgAction, Command};
-use fundu::{DurationParser, ParseError, SaturatingInto};
 
 static ABOUT: &str = help_about!("sleep.md");
 const USAGE: &str = help_usage!("sleep.md");
@@ -61,37 +62,17 @@ pub fn uu_app() -> Command {
 fn sleep(args: &[&str]) -> UResult<()> {
     let mut arg_error = false;
 
-    use fundu::TimeUnit::{Day, Hour, Minute, Second};
-    let parser = DurationParser::with_time_units(&[Second, Minute, Hour, Day]);
-
     let sleep_dur = args
         .iter()
-        .filter_map(|input| match parser.parse(input.trim()) {
+        .filter_map(|input| match parse_time::from_str(input) {
             Ok(duration) => Some(duration),
             Err(error) => {
                 arg_error = true;
-
-                let reason = match error {
-                    ParseError::Empty if input.is_empty() => "Input was empty".to_string(),
-                    ParseError::Empty => "Found only whitespace in input".to_string(),
-                    ParseError::Syntax(pos, description)
-                    | ParseError::TimeUnit(pos, description) => {
-                        format!("{description} at position {}", pos.saturating_add(1))
-                    }
-                    ParseError::NegativeExponentOverflow | ParseError::PositiveExponentOverflow => {
-                        "Exponent was out of bounds".to_string()
-                    }
-                    ParseError::NegativeNumber => "Number was negative".to_string(),
-                    error => error.to_string(),
-                };
-                show_error!("invalid time interval '{input}': {reason}");
-
+                show_error!("{error}");
                 None
             }
         })
-        .fold(Duration::ZERO, |acc, n| {
-            acc.saturating_add(SaturatingInto::<Duration>::saturating_into(n))
-        });
+        .fold(Duration::ZERO, |acc, n| acc.saturating_add(n));
 
     if arg_error {
         return Err(UUsageError::new(1, ""));

src/uucore/src/lib/features/parser/num_parser.rs

@@ -150,7 +150,7 @@ impl ExtendedParser for i64 {
             }
         }
 
-        match parse(input, true) {
+        match parse(input, ParseTarget::Integral, &[]) {
             Ok(v) => into_i64(v),
             Err(e) => Err(e.map(into_i64)),
         }
@@ -187,7 +187,7 @@ impl ExtendedParser for u64 {
             }
         }
 
-        match parse(input, true) {
+        match parse(input, ParseTarget::Integral, &[]) {
             Ok(v) => into_u64(v),
             Err(e) => Err(e.map(into_u64)),
         }
@@ -219,7 +219,7 @@ impl ExtendedParser for f64 {
             Ok(v)
         }
 
-        match parse(input, false) {
+        match parse(input, ParseTarget::Decimal, &[]) {
             Ok(v) => into_f64(v),
             Err(e) => Err(e.map(into_f64)),
         }
@@ -231,14 +231,15 @@ impl ExtendedParser for ExtendedBigDecimal {
     fn extended_parse(
         input: &str,
     ) -> Result<ExtendedBigDecimal, ExtendedParserError<'_, ExtendedBigDecimal>> {
-        parse(input, false)
+        parse(input, ParseTarget::Decimal, &[])
     }
 }
 
-fn parse_special_value(
-    input: &str,
+fn parse_special_value<'a>(
+    input: &'a str,
     negative: bool,
-) -> Result<ExtendedBigDecimal, ExtendedParserError<'_, ExtendedBigDecimal>> {
+    allowed_suffixes: &'a [(char, u32)],
+) -> Result<ExtendedBigDecimal, ExtendedParserError<'a, ExtendedBigDecimal>> {
     let input_lc = input.to_ascii_lowercase();
 
     // Array of ("String to match", return value when sign positive, when sign negative)
@@ -254,7 +255,14 @@ fn parse_special_value(
             if negative {
                 special = -special;
             }
-            let match_len = str.len();
+            let mut match_len = str.len();
+            if let Some(ch) = input.chars().nth(str.chars().count()) {
+                if allowed_suffixes.iter().any(|(c, _)| ch == *c) {
+                    // multiplying is unnecessary for these special values, but we have to note that
+                    // we processed the character to avoid a partial match error
+                    match_len += 1;
+                }
+            }
             return if input.len() == match_len {
                 Ok(special)
             } else {
@@ -381,24 +389,34 @@ fn construct_extended_big_decimal<'a>(
     Ok(ExtendedBigDecimal::BigDecimal(bd))
 }
 
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub(crate) enum ParseTarget {
+    Decimal,
+    Integral,
+    Duration,
+}
+
 // TODO: As highlighted by clippy, this function _is_ high cognitive complexity, jumps
 // around between integer and float parsing, and should be split in multiple parts.
 #[allow(clippy::cognitive_complexity)]
-fn parse(
-    input: &str,
-    integral_only: bool,
-) -> Result<ExtendedBigDecimal, ExtendedParserError<'_, ExtendedBigDecimal>> {
+pub(crate) fn parse<'a>(
+    input: &'a str,
+    target: ParseTarget,
+    allowed_suffixes: &'a [(char, u32)],
+) -> Result<ExtendedBigDecimal, ExtendedParserError<'a, ExtendedBigDecimal>> {
     // Parse the " and ' prefixes separately
-    if let Some(rest) = input.strip_prefix(['\'', '"']) {
-        let mut chars = rest.char_indices().fuse();
-        let v = chars
-            .next()
-            .map(|(_, c)| ExtendedBigDecimal::BigDecimal(u32::from(c).into()));
-        return match (v, chars.next()) {
-            (Some(v), None) => Ok(v),
-            (Some(v), Some((i, _))) => Err(ExtendedParserError::PartialMatch(v, &rest[i..])),
-            (None, _) => Err(ExtendedParserError::NotNumeric),
-        };
+    if target != ParseTarget::Duration {
+        if let Some(rest) = input.strip_prefix(['\'', '"']) {
+            let mut chars = rest.char_indices().fuse();
+            let v = chars
+                .next()
+                .map(|(_, c)| ExtendedBigDecimal::BigDecimal(u32::from(c).into()));
+            return match (v, chars.next()) {
+                (Some(v), None) => Ok(v),
+                (Some(v), Some((i, _))) => Err(ExtendedParserError::PartialMatch(v, &rest[i..])),
+                (None, _) => Err(ExtendedParserError::NotNumeric),
+            };
+        }
     }
 
     let trimmed_input = input.trim_ascii_start();
@@ -419,7 +437,7 @@ fn parse(
     let (base, rest) = if let Some(rest) = unsigned.strip_prefix('0') {
         if let Some(rest) = rest.strip_prefix(['x', 'X']) {
             (Base::Hexadecimal, rest)
-        } else if integral_only {
+        } else if target == ParseTarget::Integral {
             // Binary/Octal only allowed for integer parsing.
             if let Some(rest) = rest.strip_prefix(['b', 'B']) {
                 (Base::Binary, rest)
@@ -447,7 +465,7 @@ fn parse(
     }
 
     // Parse fractional/exponent part of the number for supported bases.
-    if matches!(base, Base::Decimal | Base::Hexadecimal) && !integral_only {
+    if matches!(base, Base::Decimal | Base::Hexadecimal) && target != ParseTarget::Integral {
         // Parse the fractional part of the number if there can be one and the input contains
         // a '.' decimal separator.
         if matches!(chars.peek(), Some(&(_, '.'))) {
@@ -493,13 +511,24 @@ fn parse(
 
     // If nothing has been parsed, check if this is a special value, or declare the parsing unsuccessful
     if let Some((0, _)) = chars.peek() {
-        return if integral_only {
+        return if target == ParseTarget::Integral {
             Err(ExtendedParserError::NotNumeric)
         } else {
-            parse_special_value(unsigned, negative)
+            parse_special_value(unsigned, negative, allowed_suffixes)
         };
     }
 
+    if let Some((_, ch)) = chars.peek() {
+        if let Some(times) = allowed_suffixes
+            .iter()
+            .find(|(c, _)| ch == c)
+            .map(|&(_, t)| t)
+        {
+            chars.next();
+            digits *= times;
+        }
+    }
+
     let ebd_result = construct_extended_big_decimal(digits, negative, base, scale, exponent);
 
     // Return what has been parsed so far. If there are extra characters, mark the

src/uucore/src/lib/features/parser/parse_time.rs

@@ -11,9 +11,8 @@
 use crate::{
     display::Quotable,
     extendedbigdecimal::ExtendedBigDecimal,
-    parser::num_parser::{ExtendedParser, ExtendedParserError},
+    parser::num_parser::{self, ExtendedParserError, ParseTarget},
 };
-use bigdecimal::BigDecimal;
 use num_traits::Signed;
 use num_traits::ToPrimitive;
 use num_traits::Zero;
@@ -59,26 +58,18 @@ pub fn from_str(string: &str) -> Result<Duration, String> {
 
     let len = string.len();
     if len == 0 {
-        return Err("empty string".to_owned());
-    }
-    let Some(slice) = string.get(..len - 1) else {
         return Err(format!("invalid time interval {}", string.quote()));
-    };
-    let (numstr, times) = match string.chars().next_back().unwrap() {
-        's' => (slice, 1),
-        'm' => (slice, 60),
-        'h' => (slice, 60 * 60),
-        'd' => (slice, 60 * 60 * 24),
-        val if !val.is_alphabetic() => (string, 1),
-        _ => match string.to_ascii_lowercase().as_str() {
-            "inf" | "infinity" => ("inf", 1),
-            _ => return Err(format!("invalid time interval {}", string.quote())),
-        },
-    };
-    let num = match ExtendedBigDecimal::extended_parse(numstr) {
+    }
+    let num = match num_parser::parse(
+        string,
+        ParseTarget::Duration,
+        &[('s', 1), ('m', 60), ('h', 60 * 60), ('d', 60 * 60 * 24)],
+    ) {
         Ok(ebd) | Err(ExtendedParserError::Overflow(ebd)) => ebd,
         Err(ExtendedParserError::Underflow(_)) => return Ok(NANOSECOND_DURATION),
-        _ => return Err(format!("invalid time interval {}", string.quote())),
+        _ => {
+            return Err(format!("invalid time interval {}", string.quote()));
+        }
     };
 
     // Allow non-negative durations (-0 is fine), and infinity.
@@ -89,9 +80,6 @@ pub fn from_str(string: &str) -> Result<Duration, String> {
         _ => return Err(format!("invalid time interval {}", string.quote())),
     };
 
-    // Pre-multiply times to avoid precision loss
-    let num: BigDecimal = num * times;
-
     // Transform to nanoseconds (9 digits after decimal point)
     let (nanos_bi, _) = num.with_scale(9).into_bigint_and_scale();
 

tests/by-util/test_sleep.rs

@@ -4,7 +4,8 @@
 // file that was distributed with this source code.
 use rstest::rstest;
 
-// spell-checker:ignore dont SIGBUS SIGSEGV sigsegv sigbus
+use uucore::display::Quotable;
+// spell-checker:ignore dont SIGBUS SIGSEGV sigsegv sigbus infd
 use uutests::new_ucmd;
 use uutests::util::TestScenario;
 use uutests::util_name;
@@ -19,11 +20,11 @@ fn test_invalid_time_interval() {
     new_ucmd!()
         .arg("xyz")
         .fails()
-        .usage_error("invalid time interval 'xyz': Invalid input: xyz");
+        .usage_error("invalid time interval 'xyz'");
     new_ucmd!()
         .args(&["--", "-1"])
         .fails()
-        .usage_error("invalid time interval '-1': Number was negative");
+        .usage_error("invalid time interval '-1'");
 }
 
 #[test]
@@ -228,16 +229,25 @@ fn test_sleep_when_multiple_inputs_exceed_max_duration_then_no_error() {
 #[rstest]
 #[case::whitespace_prefix(" 0.1s")]
 #[case::multiple_whitespace_prefix("   0.1s")]
-#[case::whitespace_suffix("0.1s ")]
-#[case::mixed_newlines_spaces_tabs("\n\t0.1s \n ")]
-fn test_sleep_when_input_has_whitespace_then_no_error(#[case] input: &str) {
+fn test_sleep_when_input_has_leading_whitespace_then_no_error(#[case] input: &str) {
     new_ucmd!()
         .arg(input)
         .timeout(Duration::from_secs(10))
         .succeeds()
         .no_output();
 }
 
+#[rstest]
+#[case::whitespace_suffix("0.1s ")]
+#[case::mixed_newlines_spaces_tabs("\n\t0.1s \n ")]
+fn test_sleep_when_input_has_trailing_whitespace_then_error(#[case] input: &str) {
+    new_ucmd!()
+        .arg(input)
+        .timeout(Duration::from_secs(10))
+        .fails()
+        .usage_error(format!("invalid time interval {}", input.quote()));
+}
+
 #[rstest]
 #[case::only_space(" ")]
 #[case::only_tab("\t")]
@@ -247,16 +257,14 @@ fn test_sleep_when_input_has_only_whitespace_then_error(#[case] input: &str) {
         .arg(input)
         .timeout(Duration::from_secs(10))
         .fails()
-        .usage_error(format!(
-            "invalid time interval '{input}': Found only whitespace in input"
-        ));
+        .usage_error(format!("invalid time interval {}", input.quote()));
 }
 
 #[test]
 fn test_sleep_when_multiple_input_some_with_error_then_shows_all_errors() {
-    let expected = "invalid time interval 'abc': Invalid input: abc\n\
-                    sleep: invalid time interval '1years': Invalid time unit: 'years' at position 2\n\
-                    sleep: invalid time interval ' ': Found only whitespace in input";
+    let expected = "invalid time interval 'abc'\n\
+                    sleep: invalid time interval '1years'\n\
+                    sleep: invalid time interval ' '";
 
     // Even if one of the arguments is valid, but the rest isn't, we should still fail and exit early.
     // So, the timeout of 10 seconds ensures we haven't executed `thread::sleep` with the only valid
@@ -273,7 +281,35 @@ fn test_negative_interval() {
     new_ucmd!()
         .args(&["--", "-1"])
         .fails()
-        .usage_error("invalid time interval '-1': Number was negative");
+        .usage_error("invalid time interval '-1'");
+}
+
+#[rstest]
+#[case::int("0x0")]
+#[case::negative_zero("-0x0")]
+#[case::int_suffix("0x0s")]
+#[case::int_suffix("0x0h")]
+#[case::frac("0x0.1")]
+#[case::frac_suffix("0x0.1s")]
+#[case::frac_suffix("0x0.001h")]
+#[case::scientific("0x1.0p-3")]
+#[case::scientific_suffix("0x1.0p-4s")]
+fn test_valid_hex_duration(#[case] input: &str) {
+    new_ucmd!().args(&["--", input]).succeeds().no_output();
+}
+
+#[rstest]
+#[case::negative("-0x1")]
+#[case::negative_suffix("-0x1s")]
+#[case::negative_frac_suffix("-0x0.1s")]
+#[case::wrong_capitalization("infD")]
+#[case::wrong_capitalization("INFD")]
+#[case::wrong_capitalization("iNfD")]
+fn test_invalid_hex_duration(#[case] input: &str) {
+    new_ucmd!()
+        .args(&["--", input])
+        .fails()
+        .usage_error(format!("invalid time interval {}", input.quote()));
 }
 
 #[cfg(unix)]

tests/by-util/test_timeout.rs

@@ -77,7 +77,7 @@ fn test_command_empty_args() {
     new_ucmd!()
         .args(&["", ""])
         .fails()
-        .stderr_contains("timeout: empty string");
+        .stderr_contains("timeout: invalid time interval ''");
 }
 
 #[test]