diff --git a/src/uu/sleep/Cargo.toml b/src/uu/sleep/Cargo.toml index 5215606b8..141447cd3 100644 --- a/src/uu/sleep/Cargo.toml +++ b/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" diff --git a/src/uu/sleep/src/sleep.rs b/src/uu/sleep/src/sleep.rs index 0f71c8e55..e377b375f 100644 --- a/src/uu/sleep/src/sleep.rs +++ b/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::::saturating_into(n)) - }); + .fold(Duration::ZERO, |acc, n| acc.saturating_add(n)); if arg_error { return Err(UUsageError::new(1, "")); diff --git a/src/uucore/src/lib/features/parser/num_parser.rs b/src/uucore/src/lib/features/parser/num_parser.rs index f21aa0114..8e0968509 100644 --- a/src/uucore/src/lib/features/parser/num_parser.rs +++ b/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> { - parse(input, false) + parse(input, ParseTarget::Decimal, &[]) } } -fn parse_special_value( - input: &str, +fn parse_special_value<'a>( + input: &'a str, negative: bool, -) -> Result> { + allowed_suffixes: &'a [(char, u32)], +) -> Result> { 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> { +pub(crate) fn parse<'a>( + input: &'a str, + target: ParseTarget, + allowed_suffixes: &'a [(char, u32)], +) -> Result> { // 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 diff --git a/src/uucore/src/lib/features/parser/parse_time.rs b/src/uucore/src/lib/features/parser/parse_time.rs index 2fd7854d5..ef8407add 100644 --- a/src/uucore/src/lib/features/parser/parse_time.rs +++ b/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 { 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 { _ => 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(); diff --git a/tests/by-util/test_sleep.rs b/tests/by-util/test_sleep.rs index 25672d91a..3fece35fc 100644 --- a/tests/by-util/test_sleep.rs +++ b/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,9 +229,7 @@ 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)) @@ -238,6 +237,17 @@ fn test_sleep_when_input_has_whitespace_then_no_error(#[case] input: &str) { .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)] diff --git a/tests/by-util/test_timeout.rs b/tests/by-util/test_timeout.rs index 12da849db..30c94dbb8 100644 --- a/tests/by-util/test_timeout.rs +++ b/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]