diff --git a/src/uu/test/src/error.rs b/src/uu/test/src/error.rs new file mode 100644 index 000000000..e9ab1c082 --- /dev/null +++ b/src/uu/test/src/error.rs @@ -0,0 +1,37 @@ +/// Represents an error encountered while parsing a test expression +#[derive(Debug)] +pub enum ParseError { + ExpectedValue, + Expected(String), + ExtraArgument(String), + MissingArgument(String), + UnknownOperator(String), + InvalidInteger(String), +} + +/// A Result type for parsing test expressions +pub type ParseResult = Result; + +/// Implement Display trait for ParseError to make it easier to print useful errors. +impl std::fmt::Display for ParseError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Expected(s) => write!(f, "expected {}", s), + Self::ExpectedValue => write!(f, "expected value"), + Self::MissingArgument(s) => write!(f, "missing argument after {}", s), + Self::ExtraArgument(s) => write!(f, "extra argument {}", s), + Self::UnknownOperator(s) => write!(f, "unknown operator {}", s), + Self::InvalidInteger(s) => write!(f, "invalid integer {}", s), + } + } +} + +/// Implement UError trait for ParseError to make it easier to return useful error codes from main(). +impl uucore::error::UError for ParseError { + fn code(&self) -> i32 { + 2 + } +} + +/// Implement standard Error trait for UError +impl std::error::Error for ParseError {} diff --git a/src/uu/test/src/parser.rs b/src/uu/test/src/parser.rs index b76333930..db90098a1 100644 --- a/src/uu/test/src/parser.rs +++ b/src/uu/test/src/parser.rs @@ -7,11 +7,12 @@ // spell-checker:ignore (grammar) BOOLOP STRLEN FILETEST FILEOP INTOP STRINGOP ; (vars) LParen StrlenOp -use std::ffi::OsString; +use std::ffi::{OsStr, OsString}; use std::iter::Peekable; +use super::error::{ParseError, ParseResult}; + use uucore::display::Quotable; -use uucore::show_error; /// Represents one of the binary comparison operators for strings, integers, or files #[derive(Debug, PartialEq, Eq)] @@ -90,6 +91,27 @@ impl Symbol { } } +/// Implement Display trait for Symbol to make it easier to print useful errors. +/// We will try to match the format in which the symbol appears in the input. +impl std::fmt::Display for Symbol { + /// Format a Symbol for printing + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let s = match &self { + Self::LParen => OsStr::new("("), + Self::Bang => OsStr::new("!"), + Self::BoolOp(s) + | Self::Literal(s) + | Self::Op(Operator::String(s)) + | Self::Op(Operator::Int(s)) + | Self::Op(Operator::File(s)) + | Self::UnaryOp(UnaryOperator::StrlenOp(s)) + | Self::UnaryOp(UnaryOperator::FiletestOp(s)) => OsStr::new(s), + Self::None => OsStr::new("None"), + }; + write!(f, "{}", s.quote()) + } +} + /// Recursive descent parser for test, which converts a list of OsStrings /// (typically command line arguments) into a stack of Symbols in postfix /// order. @@ -133,16 +155,10 @@ impl Parser { } /// Consume the next token & verify that it matches the provided value. - /// - /// # Panics - /// - /// Panics if the next token does not match the provided value. - /// - /// TODO: remove panics and convert Parser to return error messages. - fn expect(&mut self, value: &str) { + fn expect(&mut self, value: &str) -> ParseResult<()> { match self.next_token() { - Symbol::Literal(s) if s == value => (), - _ => panic!("expected ‘{value}’"), + Symbol::Literal(s) if s == value => Ok(()), + _ => Err(ParseError::Expected(value.quote().to_string())), } } @@ -162,25 +178,27 @@ impl Parser { /// Parse an expression. /// /// EXPR → TERM | EXPR BOOLOP EXPR - fn expr(&mut self) { + fn expr(&mut self) -> ParseResult<()> { if !self.peek_is_boolop() { - self.term(); + self.term()?; } - self.maybe_boolop(); + self.maybe_boolop()?; + Ok(()) } /// Parse a term token and possible subsequent symbols: "(", "!", UOP, /// literal, or None. - fn term(&mut self) { + fn term(&mut self) -> ParseResult<()> { let symbol = self.next_token(); match symbol { - Symbol::LParen => self.lparen(), - Symbol::Bang => self.bang(), + Symbol::LParen => self.lparen()?, + Symbol::Bang => self.bang()?, Symbol::UnaryOp(_) => self.uop(symbol), Symbol::None => self.stack.push(symbol), - literal => self.literal(literal), + literal => self.literal(literal)?, } + Ok(()) } /// Parse a (possibly) parenthesized expression. @@ -192,7 +210,7 @@ impl Parser { /// * when followed by a binary operator that is not _itself_ interpreted /// as a literal /// - fn lparen(&mut self) { + fn lparen(&mut self) -> ParseResult<()> { // Look ahead up to 3 tokens to determine if the lparen is being used // as a grouping operator or should be treated as a literal string let peek3: Vec = self @@ -204,13 +222,13 @@ impl Parser { match peek3.as_slice() { // case 1: lparen is a literal when followed by nothing - [] => self.literal(Symbol::LParen.into_literal()), + [] => { + self.literal(Symbol::LParen.into_literal())?; + Ok(()) + } // case 2: error if end of stream is `( ` - [symbol] => { - show_error!("missing argument after ‘{:?}’", symbol); - std::process::exit(2); - } + [symbol] => Err(ParseError::MissingArgument(format!("{symbol}"))), // case 3: `( uop )` → parenthesized unary operation; // this case ensures we don’t get confused by `( -f ) )` @@ -218,43 +236,49 @@ impl Parser { [Symbol::UnaryOp(_), _, Symbol::Literal(s)] if s == ")" => { let symbol = self.next_token(); self.uop(symbol); - self.expect(")"); + self.expect(")")?; + Ok(()) } // case 4: binary comparison of literal lparen, e.g. `( != )` [Symbol::Op(_), Symbol::Literal(s)] | [Symbol::Op(_), Symbol::Literal(s), _] if s == ")" => { - self.literal(Symbol::LParen.into_literal()); + self.literal(Symbol::LParen.into_literal())?; + Ok(()) } // case 5: after handling the prior cases, any single token inside // parentheses is a literal, e.g. `( -f )` [_, Symbol::Literal(s)] | [_, Symbol::Literal(s), _] if s == ")" => { let symbol = self.next_token(); - self.literal(symbol); - self.expect(")"); + self.literal(symbol)?; + self.expect(")")?; + Ok(()) } // case 6: two binary ops in a row, treat the first op as a literal [Symbol::Op(_), Symbol::Op(_), _] => { let symbol = self.next_token(); - self.literal(symbol); - self.expect(")"); + self.literal(symbol)?; + self.expect(")")?; + Ok(()) } // case 7: if earlier cases didn’t match, `( op …` // indicates binary comparison of literal lparen with // anything _except_ ")" (case 4) [Symbol::Op(_), _] | [Symbol::Op(_), _, _] => { - self.literal(Symbol::LParen.into_literal()); + self.literal(Symbol::LParen.into_literal())?; + Ok(()) } // Otherwise, lparen indicates the start of a parenthesized // expression _ => { - self.expr(); - self.expect(")"); + self.expr()?; + self.expect(")")?; + Ok(()) } } } @@ -276,7 +300,7 @@ impl Parser { /// * `! str BOOLOP str`: negate the entire Boolean expression /// * `! str BOOLOP EXPR BOOLOP EXPR`: negate the value of the first `str` term /// - fn bang(&mut self) { + fn bang(&mut self) -> ParseResult<()> { match self.peek() { Symbol::Op(_) | Symbol::BoolOp(_) => { // we need to peek ahead one more token to disambiguate the first @@ -289,14 +313,14 @@ impl Parser { Symbol::Op(_) | Symbol::None => { // op is literal let op = self.next_token().into_literal(); - self.literal(op); + self.literal(op)?; self.stack.push(Symbol::Bang); } // case 2: ` OP str [BOOLOP EXPR]`. _ => { // bang is literal; parsing continues with op - self.literal(Symbol::Bang.into_literal()); - self.maybe_boolop(); + self.literal(Symbol::Bang.into_literal())?; + self.maybe_boolop()?; } } } @@ -317,32 +341,34 @@ impl Parser { match peek4.as_slice() { // we peeked ahead 4 but there were only 3 tokens left [Symbol::Literal(_), Symbol::BoolOp(_), Symbol::Literal(_)] => { - self.expr(); + self.expr()?; self.stack.push(Symbol::Bang); } _ => { - self.term(); + self.term()?; self.stack.push(Symbol::Bang); } } } } + Ok(()) } /// Peek at the next token and parse it as a BOOLOP or string literal, /// as appropriate. - fn maybe_boolop(&mut self) { + fn maybe_boolop(&mut self) -> ParseResult<()> { if self.peek_is_boolop() { let symbol = self.next_token(); // BoolOp by itself interpreted as Literal if let Symbol::None = self.peek() { - self.literal(symbol.into_literal()); + self.literal(symbol.into_literal())?; } else { - self.boolop(symbol); - self.maybe_boolop(); + self.boolop(symbol)?; + self.maybe_boolop()?; } } + Ok(()) } /// Parse a Boolean expression. @@ -350,13 +376,14 @@ impl Parser { /// Logical and (-a) has higher precedence than or (-o), so in an /// expression like `foo -o '' -a ''`, the and subexpression is evaluated /// first. - fn boolop(&mut self, op: Symbol) { + fn boolop(&mut self, op: Symbol) -> ParseResult<()> { if op == Symbol::BoolOp(OsString::from("-a")) { - self.term(); + self.term()?; } else { - self.expr(); + self.expr()?; } self.stack.push(op); + Ok(()) } /// Parse a (possible) unary argument test (string length or file @@ -375,7 +402,7 @@ impl Parser { /// Parse a string literal, optionally followed by a comparison operator /// and a second string literal. - fn literal(&mut self, token: Symbol) { + fn literal(&mut self, token: Symbol) -> ParseResult<()> { self.stack.push(token.into_literal()); // EXPR → str OP str @@ -383,21 +410,24 @@ impl Parser { let op = self.next_token(); match self.next_token() { - Symbol::None => panic!("missing argument after {op:?}"), + Symbol::None => { + return Err(ParseError::MissingArgument(format!("{op}"))); + } token => self.stack.push(token.into_literal()), } self.stack.push(op); } + Ok(()) } /// Parser entry point: parse the token stream `self.tokens`, storing the /// resulting `Symbol` stack in `self.stack`. - fn parse(&mut self) -> Result<(), String> { - self.expr(); + fn parse(&mut self) -> ParseResult<()> { + self.expr()?; match self.tokens.next() { - Some(token) => Err(format!("extra argument {}", token.quote())), + Some(token) => Err(ParseError::ExtraArgument(token.quote().to_string())), None => Ok(()), } } @@ -405,7 +435,7 @@ impl Parser { /// Parse the token stream `args`, returning a `Symbol` stack representing the /// operations to perform in postfix order. -pub fn parse(args: Vec) -> Result, String> { +pub fn parse(args: Vec) -> ParseResult> { let mut p = Parser::new(args); p.parse()?; Ok(p.stack) diff --git a/src/uu/test/src/test.rs b/src/uu/test/src/test.rs index 8ce4564a3..313ef0413 100644 --- a/src/uu/test/src/test.rs +++ b/src/uu/test/src/test.rs @@ -8,9 +8,11 @@ // spell-checker:ignore (vars) egid euid FiletestOp StrlenOp +pub(crate) mod error; mod parser; use clap::{crate_version, Command}; +use error::{ParseError, ParseResult}; use parser::{parse, Operator, Symbol, UnaryOperator}; use std::ffi::{OsStr, OsString}; use std::fs; @@ -128,23 +130,18 @@ pub fn uumain(mut args: impl uucore::Args) -> UResult<()> { } } - let result = parse(args).and_then(|mut stack| eval(&mut stack)); + let result = parse(args).map(|mut stack| eval(&mut stack))??; - match result { - Ok(result) => { - if result { - Ok(()) - } else { - Err(1.into()) - } - } - Err(e) => Err(USimpleError::new(2, e)), + if result { + Ok(()) + } else { + Err(1.into()) } } /// Evaluate a stack of Symbols, returning the result of the evaluation or /// an error message if evaluation failed. -fn eval(stack: &mut Vec) -> Result { +fn eval(stack: &mut Vec) -> ParseResult { macro_rules! pop_literal { () => { match stack.pop() { @@ -186,7 +183,7 @@ fn eval(stack: &mut Vec) -> Result { return Ok(true); } _ => { - return Err(format!("missing argument after '{op:?}'")); + return Err(ParseError::MissingArgument(op.quote().to_string())); } }; @@ -233,7 +230,7 @@ fn eval(stack: &mut Vec) -> Result { Ok(if op == "-a" { a && b } else { a || b }) } - _ => Err("expected value".to_string()), + _ => Err(ParseError::ExpectedValue), } } @@ -241,19 +238,17 @@ fn eval(stack: &mut Vec) -> Result { /// `a` is the left hand side /// `b` is the left hand side /// `op` the operation (ex: -eq, -lt, etc) -fn integers(a: &OsStr, b: &OsStr, op: &OsStr) -> Result { - let format_err = |value: &OsStr| format!("invalid integer {}", value.quote()); - +fn integers(a: &OsStr, b: &OsStr, op: &OsStr) -> ParseResult { // Parse the two inputs let a: i128 = a .to_str() .and_then(|s| s.parse().ok()) - .ok_or_else(|| format_err(a))?; + .ok_or_else(|| ParseError::InvalidInteger(a.quote().to_string()))?; let b: i128 = b .to_str() .and_then(|s| s.parse().ok()) - .ok_or_else(|| format_err(b))?; + .ok_or_else(|| ParseError::InvalidInteger(b.quote().to_string()))?; // Do the maths Ok(match op.to_str() { @@ -263,7 +258,7 @@ fn integers(a: &OsStr, b: &OsStr, op: &OsStr) -> Result { Some("-ge") => a >= b, Some("-lt") => a < b, Some("-le") => a <= b, - _ => return Err(format!("unknown operator {}", op.quote())), + _ => return Err(ParseError::UnknownOperator(op.quote().to_string())), }) } @@ -271,7 +266,7 @@ fn integers(a: &OsStr, b: &OsStr, op: &OsStr) -> Result { /// `a` is the left hand side /// `b` is the left hand side /// `op` the operation (ex: -ef, -nt, etc) -fn files(a: &OsStr, b: &OsStr, op: &OsStr) -> Result { +fn files(a: &OsStr, b: &OsStr, op: &OsStr) -> ParseResult { // Don't manage the error. GNU doesn't show error when doing // test foo -nt bar let f_a = match fs::metadata(a) { @@ -290,14 +285,14 @@ fn files(a: &OsStr, b: &OsStr, op: &OsStr) -> Result { Some("-ef") => unimplemented!(), Some("-nt") => f_a.modified().unwrap() > f_b.modified().unwrap(), Some("-ot") => f_a.created().unwrap() > f_b.created().unwrap(), - _ => return Err(format!("unknown operator {}", op.quote())), + _ => return Err(ParseError::UnknownOperator(op.quote().to_string())), }) } -fn isatty(fd: &OsStr) -> Result { +fn isatty(fd: &OsStr) -> ParseResult { fd.to_str() .and_then(|s| s.parse().ok()) - .ok_or_else(|| format!("invalid integer {}", fd.quote())) + .ok_or_else(|| ParseError::InvalidInteger(fd.quote().to_string())) .map(|i| { #[cfg(not(target_os = "redox"))] unsafe { diff --git a/tests/by-util/test_test.rs b/tests/by-util/test_test.rs index f3ed83348..bf5c50b7f 100644 --- a/tests/by-util/test_test.rs +++ b/tests/by-util/test_test.rs @@ -928,3 +928,16 @@ fn test_long_integer() { ]) .fails(); } + +#[test] +fn test_missing_argument_after() { + let mut ucmd = new_ucmd!(); + + let result = ucmd.args(&["(", "foo"]).fails(); + result.no_stdout(); + assert_eq!(result.exit_status().code().unwrap(), 2); + assert_eq!( + result.stderr_str().trim(), + "test: missing argument after 'foo'" + ); +}