From 1e12c46c246d58115a33b5f1279251ea866df9a8 Mon Sep 17 00:00:00 2001 From: ndd7xv Date: Thu, 10 Feb 2022 19:24:16 -0500 Subject: [PATCH 1/2] uucore(memo): replace err_conv with result --- src/uu/printf/src/printf.rs | 2 +- src/uu/seq/src/seq.rs | 12 ++- src/uucore/src/lib/features/memo.rs | 15 +-- src/uucore/src/lib/features/tokenize/sub.rs | 94 +++++++++++++------ src/uucore/src/lib/features/tokenize/token.rs | 4 +- .../lib/features/tokenize/unescaped_text.rs | 6 +- 6 files changed, 90 insertions(+), 43 deletions(-) diff --git a/src/uu/printf/src/printf.rs b/src/uu/printf/src/printf.rs index a30d18c53..5732d4ced 100644 --- a/src/uu/printf/src/printf.rs +++ b/src/uu/printf/src/printf.rs @@ -284,7 +284,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { None => vec![], }; - memo::Memo::run_all(format_string, &values[..]); + memo::Memo::run_all(format_string, &values[..])?; Ok(()) } diff --git a/src/uu/seq/src/seq.rs b/src/uu/seq/src/seq.rs index 3646effc1..28524c55c 100644 --- a/src/uu/seq/src/seq.rs +++ b/src/uu/seq/src/seq.rs @@ -5,6 +5,7 @@ // TODO: Support -f flag // spell-checker:ignore (ToDO) istr chiter argptr ilen extendedbigdecimal extendedbigint numberparse use std::io::{stdout, ErrorKind, Write}; +use std::process::exit; use clap::{crate_version, App, AppSettings, Arg}; use num_traits::Zero; @@ -12,6 +13,7 @@ use num_traits::Zero; use uucore::error::FromIo; use uucore::error::UResult; use uucore::memo::Memo; +use uucore::show; mod error; mod extendedbigdecimal; @@ -287,7 +289,10 @@ fn print_seq( match format { Some(f) => { let s = format!("{}", value); - Memo::run_all(f, &[s]); + if let Err(x) = Memo::run_all(f, &[s]) { + show!(x); + exit(1); + } } None => write_value_float( &mut stdout, @@ -349,7 +354,10 @@ fn print_seq_integers( match format { Some(f) => { let s = format!("{}", value); - Memo::run_all(f, &[s]); + if let Err(x) = Memo::run_all(f, &[s]) { + show!(x); + exit(1); + } } None => write_value_int(&mut stdout, &value, padding, pad, is_first_iteration)?, } diff --git a/src/uucore/src/lib/features/memo.rs b/src/uucore/src/lib/features/memo.rs index fd57c33b5..0e3a6376c 100644 --- a/src/uucore/src/lib/features/memo.rs +++ b/src/uucore/src/lib/features/memo.rs @@ -6,6 +6,7 @@ //! that prints tokens. use crate::display::Quotable; +use crate::error::UResult; use crate::features::tokenize::sub::Sub; use crate::features::tokenize::token::{Token, Tokenizer}; use crate::features::tokenize::unescaped_text::UnescapedText; @@ -26,17 +27,17 @@ fn warn_excess_args(first_arg: &str) { } impl Memo { - pub fn new(pf_string: &str, pf_args_it: &mut Peekable>) -> Self { + pub fn new(pf_string: &str, pf_args_it: &mut Peekable>) -> UResult { let mut pm = Self { tokens: Vec::new() }; let mut tmp_token: Option>; let mut it = put_back_n(pf_string.chars()); let mut has_sub = false; loop { - tmp_token = UnescapedText::from_it(&mut it, pf_args_it); + tmp_token = UnescapedText::from_it(&mut it, pf_args_it)?; if let Some(x) = tmp_token { pm.tokens.push(x); } - tmp_token = Sub::from_it(&mut it, pf_args_it); + tmp_token = Sub::from_it(&mut it, pf_args_it)?; if let Some(x) = tmp_token { if !has_sub { has_sub = true; @@ -64,19 +65,19 @@ impl Memo { } } } - pm + Ok(pm) } pub fn apply(&self, pf_args_it: &mut Peekable>) { for tkn in &self.tokens { tkn.print(pf_args_it); } } - pub fn run_all(pf_string: &str, pf_args: &[String]) { + pub fn run_all(pf_string: &str, pf_args: &[String]) -> UResult<()> { let mut arg_it = pf_args.iter().peekable(); - let pm = Self::new(pf_string, &mut arg_it); + let pm = Self::new(pf_string, &mut arg_it)?; loop { if arg_it.peek().is_none() { - break; + return Ok(()); } pm.apply(&mut arg_it); } diff --git a/src/uucore/src/lib/features/tokenize/sub.rs b/src/uucore/src/lib/features/tokenize/sub.rs index ac471ae3e..a1c2f3807 100644 --- a/src/uucore/src/lib/features/tokenize/sub.rs +++ b/src/uucore/src/lib/features/tokenize/sub.rs @@ -5,7 +5,7 @@ //! it is created by Sub's implementation of the Tokenizer trait //! Subs which have numeric field chars make use of the num_format //! submodule -use crate::show_error; +use crate::error::{UResult, UUsageError}; use itertools::{put_back_n, PutBackN}; use std::iter::Peekable; use std::process::exit; @@ -20,11 +20,6 @@ use super::unescaped_text::UnescapedText; const EXIT_ERR: i32 = 1; -fn err_conv(sofar: &str) { - show_error!("%{}: invalid conversion specification", sofar); - exit(EXIT_ERR); -} - fn convert_asterisk_arg_int(asterisk_arg: &str) -> isize { // this is a costly way to parse the // args used for asterisk values into integers @@ -116,14 +111,14 @@ impl SubParser { fn from_it( it: &mut PutBackN, args: &mut Peekable>, - ) -> Option> { + ) -> UResult>> { let mut parser = Self::new(); - if parser.sub_vals_retrieved(it) { + if parser.sub_vals_retrieved(it)? { let t: Box = Self::build_token(parser); t.print(args); - Some(t) + Ok(Some(t)) } else { - None + Ok(None) } } fn build_token(parser: Self) -> Box { @@ -151,9 +146,9 @@ impl SubParser { )); t } - fn sub_vals_retrieved(&mut self, it: &mut PutBackN) -> bool { - if !Self::successfully_eat_prefix(it, &mut self.text_so_far) { - return false; + fn sub_vals_retrieved(&mut self, it: &mut PutBackN) -> UResult { + if !Self::successfully_eat_prefix(it, &mut self.text_so_far)? { + return Ok(false); } // this fn in particular is much longer than it needs to be // .could get a lot @@ -177,7 +172,10 @@ impl SubParser { '-' | '*' | '0'..='9' => { if !self.past_decimal { if self.min_width_is_asterisk || self.specifiers_found { - err_conv(&self.text_so_far); + return Err(UUsageError::new( + 1, + format!("%{}: invalid conversion specification", &self.text_so_far), + )); } if self.min_width_tmp.is_none() { self.min_width_tmp = Some(String::new()); @@ -185,7 +183,13 @@ impl SubParser { match self.min_width_tmp.as_mut() { Some(x) => { if (ch == '-' || ch == '*') && !x.is_empty() { - err_conv(&self.text_so_far); + return Err(UUsageError::new( + 1, + format!( + "%{}: invalid conversion specification", + &self.text_so_far + ), + )); } if ch == '*' { self.min_width_is_asterisk = true; @@ -200,7 +204,10 @@ impl SubParser { // second field should never have a // negative value if self.second_field_is_asterisk || ch == '-' || self.specifiers_found { - err_conv(&self.text_so_far); + return Err(UUsageError::new( + 1, + format!("%{}: invalid conversion specification", &self.text_so_far), + )); } if self.second_field_tmp.is_none() { self.second_field_tmp = Some(String::new()); @@ -208,7 +215,13 @@ impl SubParser { match self.second_field_tmp.as_mut() { Some(x) => { if ch == '*' && !x.is_empty() { - err_conv(&self.text_so_far); + return Err(UUsageError::new( + 1, + format!( + "%{}: invalid conversion specification", + &self.text_so_far + ), + )); } if ch == '*' { self.second_field_is_asterisk = true; @@ -225,7 +238,10 @@ impl SubParser { if !self.past_decimal { self.past_decimal = true; } else { - err_conv(&self.text_so_far); + return Err(UUsageError::new( + 1, + format!("%{}: invalid conversion specification", &self.text_so_far), + )); } } x if legal_fields.binary_search(&x).is_ok() => { @@ -242,18 +258,24 @@ impl SubParser { } } _ => { - err_conv(&self.text_so_far); + return Err(UUsageError::new( + 1, + format!("%{}: invalid conversion specification", &self.text_so_far), + )); } } } if self.field_char.is_none() { - err_conv(&self.text_so_far); + return Err(UUsageError::new( + 1, + format!("%{}: invalid conversion specification", &self.text_so_far), + )); } let field_char_retrieved = self.field_char.unwrap(); if self.past_decimal && self.second_field_tmp.is_none() { self.second_field_tmp = Some(String::from("0")); } - self.validate_field_params(field_char_retrieved); + self.validate_field_params(field_char_retrieved)?; // if the dot is provided without a second field // printf interprets it as 0. if let Some(x) = self.second_field_tmp.as_mut() { @@ -262,9 +284,12 @@ impl SubParser { } } - true + Ok(true) } - fn successfully_eat_prefix(it: &mut PutBackN, text_so_far: &mut String) -> bool { + fn successfully_eat_prefix( + it: &mut PutBackN, + text_so_far: &mut String, + ) -> UResult { // get next two chars, // if they're '%%' we're not tokenizing it // else put chars back @@ -274,12 +299,14 @@ impl SubParser { match n_ch { Some(x) => { it.put_back(x); - true + Ok(true) } None => { text_so_far.push('%'); - err_conv(text_so_far); - false + return Err(UUsageError::new( + 1, + format!("%{}: invalid conversion specification", &text_so_far[..]), + )); } } } else { @@ -289,10 +316,10 @@ impl SubParser { if let Some(x) = preface { it.put_back(x); }; - false + Ok(false) } } - fn validate_field_params(&self, field_char: char) { + fn validate_field_params(&self, field_char: char) -> UResult<()> { // check for illegal combinations here when possible vs // on each application so we check less per application // to do: move these checks to Sub::new @@ -304,8 +331,15 @@ impl SubParser { || self.past_decimal || self.second_field_tmp.is_some())) { - err_conv(&self.text_so_far); + // invalid string substitution + // to do: include information about an invalid + // string substitution + return Err(UUsageError::new( + 1, + format!("%{}: invalid conversion specification", &self.text_so_far), + )); } + Ok(()) } } @@ -313,7 +347,7 @@ impl token::Tokenizer for Sub { fn from_it( it: &mut PutBackN, args: &mut Peekable>, - ) -> Option> { + ) -> UResult>> { SubParser::from_it(it, args) } } diff --git a/src/uucore/src/lib/features/tokenize/token.rs b/src/uucore/src/lib/features/tokenize/token.rs index 1d8ee5ead..6a25b620f 100644 --- a/src/uucore/src/lib/features/tokenize/token.rs +++ b/src/uucore/src/lib/features/tokenize/token.rs @@ -4,6 +4,8 @@ use std::iter::Peekable; use std::slice::Iter; use std::str::Chars; +use crate::error::UResult; + // A token object is an object that can print the expected output // of a contiguous segment of the format string, and // requires at most 1 argument @@ -27,5 +29,5 @@ pub trait Tokenizer { fn from_it( it: &mut PutBackN, args: &mut Peekable>, - ) -> Option>; + ) -> UResult>>; } diff --git a/src/uucore/src/lib/features/tokenize/unescaped_text.rs b/src/uucore/src/lib/features/tokenize/unescaped_text.rs index 0ec721b48..d186dbc26 100644 --- a/src/uucore/src/lib/features/tokenize/unescaped_text.rs +++ b/src/uucore/src/lib/features/tokenize/unescaped_text.rs @@ -13,6 +13,8 @@ use std::process::exit; use std::slice::Iter; use std::str::Chars; +use crate::error::UResult; + use super::token; const EXIT_OK: i32 = 0; @@ -266,8 +268,8 @@ impl token::Tokenizer for UnescapedText { fn from_it( it: &mut PutBackN, _: &mut Peekable>, - ) -> Option> { - Self::from_it_core(it, false) + ) -> UResult>> { + Ok(Self::from_it_core(it, false)) } } impl token::Token for UnescapedText { From f04f22b0123505137091134c962bb02915b4ead1 Mon Sep 17 00:00:00 2001 From: ndd7xv Date: Sun, 13 Feb 2022 17:28:33 -0500 Subject: [PATCH 2/2] uucore(memo): refactor error propogation with new SubError enum --- src/uucore/src/lib/features/tokenize/sub.rs | 76 +++++++++------------ 1 file changed, 33 insertions(+), 43 deletions(-) diff --git a/src/uucore/src/lib/features/tokenize/sub.rs b/src/uucore/src/lib/features/tokenize/sub.rs index a1c2f3807..f8b5b5caf 100644 --- a/src/uucore/src/lib/features/tokenize/sub.rs +++ b/src/uucore/src/lib/features/tokenize/sub.rs @@ -5,8 +5,10 @@ //! it is created by Sub's implementation of the Tokenizer trait //! Subs which have numeric field chars make use of the num_format //! submodule -use crate::error::{UResult, UUsageError}; +use crate::error::{UError, UResult}; use itertools::{put_back_n, PutBackN}; +use std::error::Error; +use std::fmt::Display; use std::iter::Peekable; use std::process::exit; use std::slice::Iter; @@ -20,6 +22,23 @@ use super::unescaped_text::UnescapedText; const EXIT_ERR: i32 = 1; +#[derive(Debug)] +pub enum SubError { + InvalidSpec(String), +} + +impl Display for SubError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> { + match self { + Self::InvalidSpec(s) => write!(f, "%{}: invalid conversion specification", s), + } + } +} + +impl Error for SubError {} + +impl UError for SubError {} + fn convert_asterisk_arg_int(asterisk_arg: &str) -> isize { // this is a costly way to parse the // args used for asterisk values into integers @@ -172,10 +191,7 @@ impl SubParser { '-' | '*' | '0'..='9' => { if !self.past_decimal { if self.min_width_is_asterisk || self.specifiers_found { - return Err(UUsageError::new( - 1, - format!("%{}: invalid conversion specification", &self.text_so_far), - )); + return Err(SubError::InvalidSpec(self.text_so_far.clone()).into()); } if self.min_width_tmp.is_none() { self.min_width_tmp = Some(String::new()); @@ -183,13 +199,9 @@ impl SubParser { match self.min_width_tmp.as_mut() { Some(x) => { if (ch == '-' || ch == '*') && !x.is_empty() { - return Err(UUsageError::new( - 1, - format!( - "%{}: invalid conversion specification", - &self.text_so_far - ), - )); + return Err( + SubError::InvalidSpec(self.text_so_far.clone()).into() + ); } if ch == '*' { self.min_width_is_asterisk = true; @@ -204,10 +216,7 @@ impl SubParser { // second field should never have a // negative value if self.second_field_is_asterisk || ch == '-' || self.specifiers_found { - return Err(UUsageError::new( - 1, - format!("%{}: invalid conversion specification", &self.text_so_far), - )); + return Err(SubError::InvalidSpec(self.text_so_far.clone()).into()); } if self.second_field_tmp.is_none() { self.second_field_tmp = Some(String::new()); @@ -215,13 +224,9 @@ impl SubParser { match self.second_field_tmp.as_mut() { Some(x) => { if ch == '*' && !x.is_empty() { - return Err(UUsageError::new( - 1, - format!( - "%{}: invalid conversion specification", - &self.text_so_far - ), - )); + return Err( + SubError::InvalidSpec(self.text_so_far.clone()).into() + ); } if ch == '*' { self.second_field_is_asterisk = true; @@ -238,10 +243,7 @@ impl SubParser { if !self.past_decimal { self.past_decimal = true; } else { - return Err(UUsageError::new( - 1, - format!("%{}: invalid conversion specification", &self.text_so_far), - )); + return Err(SubError::InvalidSpec(self.text_so_far.clone()).into()); } } x if legal_fields.binary_search(&x).is_ok() => { @@ -258,18 +260,12 @@ impl SubParser { } } _ => { - return Err(UUsageError::new( - 1, - format!("%{}: invalid conversion specification", &self.text_so_far), - )); + return Err(SubError::InvalidSpec(self.text_so_far.clone()).into()); } } } if self.field_char.is_none() { - return Err(UUsageError::new( - 1, - format!("%{}: invalid conversion specification", &self.text_so_far), - )); + return Err(SubError::InvalidSpec(self.text_so_far.clone()).into()); } let field_char_retrieved = self.field_char.unwrap(); if self.past_decimal && self.second_field_tmp.is_none() { @@ -303,10 +299,7 @@ impl SubParser { } None => { text_so_far.push('%'); - return Err(UUsageError::new( - 1, - format!("%{}: invalid conversion specification", &text_so_far[..]), - )); + Err(SubError::InvalidSpec(text_so_far.clone()).into()) } } } else { @@ -334,10 +327,7 @@ impl SubParser { // invalid string substitution // to do: include information about an invalid // string substitution - return Err(UUsageError::new( - 1, - format!("%{}: invalid conversion specification", &self.text_so_far), - )); + return Err(SubError::InvalidSpec(self.text_so_far.clone()).into()); } Ok(()) }