test: refactor error handling with ParseError enum

src/uu/test/src/error.rs

@@ -0,0 +1,44 @@
+/// 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<T> = Result<T, ParseError>;
+
+/// 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 {
+        match self {
+            Self::Expected(_) => 2,
+            Self::MissingArgument(_) => 2,
+            Self::ExtraArgument(_) => 2,
+            Self::UnknownOperator(_) => 2,
+            Self::ExpectedValue => 2,
+            Self::InvalidInteger(_) => 2,
+        }
+    }
+}
+
+/// Implement standard Error trait for UError
+impl std::error::Error for ParseError {}

src/uu/test/src/parser.rs

@@ -10,7 +10,10 @@
 use std::ffi::OsString;
 use std::iter::Peekable;
 
+use super::error::{ParseError, ParseResult};
+
 use uucore::display::Quotable;
+use uucore::error::UResult;
 
 /// Represents one of the binary comparison operators for strings, integers, or files
 #[derive(Debug, PartialEq, Eq)]
@@ -39,25 +42,6 @@ pub enum Symbol {
     None,
 }
 
-/// Represents an error encountered while parsing a test expression
-pub enum ParseError {
-    MissingArgument(OsString),
-    Expected(OsString),
-}
-
-/// A Result type for parsing test expressions
-type ParseResult<T> = Result<T, ParseError>;
-
-/// Display an error message for a ParseError
-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.to_string_lossy()),
-            Self::MissingArgument(s) => write!(f, "missing argument for {}", s.to_string_lossy()),
-        }
-    }
-}
-
 impl Symbol {
     /// Create a new Symbol from an OsString.
     ///
@@ -108,6 +92,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 => OsString::from("("),
+            Self::Bang => OsString::from("!"),
+            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)) => s.clone(),
+            Self::None => OsString::from("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.
@@ -224,7 +229,7 @@ impl Parser {
             }
 
             // case 2: error if end of stream is `( <any_token>`
-            [symbol] => Err(ParseError::MissingArgument(format!("{symbol:#?}").into())),
+            [symbol] => Err(ParseError::MissingArgument(format!("{symbol}"))),
 
             // case 3: `( uop <any_token> )` → parenthesized unary operation;
             //         this case ensures we don’t get confused by `( -f ) )`
@@ -407,7 +412,7 @@ impl Parser {
 
             match self.next_token() {
                 Symbol::None => {
-                    return Err(ParseError::MissingArgument(format!("{op:#?}").into()));
+                    return Err(ParseError::MissingArgument(format!("{op}")));
                 }
                 token => self.stack.push(token.into_literal()),
             }
@@ -419,11 +424,11 @@ impl Parser {
 
     /// 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().map_err(|e| e.to_string())?;
+    fn parse(&mut self) -> UResult<()> {
+        self.expr()?;
 
         match self.tokens.next() {
-            Some(token) => Err(format!("extra argument {}", token.quote())),
+            Some(token) => Err(ParseError::ExtraArgument(token.quote().to_string()).into()),
             None => Ok(()),
         }
     }
@@ -431,7 +436,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<OsString>) -> Result<Vec<Symbol>, String> {
+pub fn parse(args: Vec<OsString>) -> UResult<Vec<Symbol>> {
     let mut p = Parser::new(args);
     p.parse()?;
     Ok(p.stack)

src/uu/test/src/test.rs

@@ -8,6 +8,7 @@
 
 // spell-checker:ignore (vars) egid euid FiletestOp StrlenOp
 
+pub(crate) mod error;
 mod parser;
 
 use clap::{crate_version, Command};
@@ -19,6 +20,7 @@ use std::os::unix::fs::MetadataExt;
 use uucore::display::Quotable;
 use uucore::error::{UResult, USimpleError};
 use uucore::format_usage;
+use error::{ParseError, ParseResult};
 
 const USAGE: &str = "\
     {} EXPRESSION
@@ -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)),
-    }
-}
 
 /// Evaluate a stack of Symbols, returning the result of the evaluation or
 /// an error message if evaluation failed.
-fn eval(stack: &mut Vec<Symbol>) -> Result<bool, String> {
+fn eval(stack: &mut Vec<Symbol>) -> ParseResult<bool> {
     macro_rules! pop_literal {
         () => {
             match stack.pop() {
@@ -186,7 +183,7 @@ fn eval(stack: &mut Vec<Symbol>) -> Result<bool, String> {
                     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<Symbol>) -> Result<bool, String> {
 
             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<Symbol>) -> Result<bool, String> {
 /// `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<bool, String> {
-    let format_err = |value: &OsStr| format!("invalid integer {}", value.quote());
-
+fn integers(a: &OsStr, b: &OsStr, op: &OsStr) -> ParseResult<bool> {
     // 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<bool, String> {
         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<bool, String> {
 /// `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<bool, String> {
+fn files(a: &OsStr, b: &OsStr, op: &OsStr) -> ParseResult<bool> {
     // 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<bool, String> {
         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<bool, String> {
+fn isatty(fd: &OsStr) -> ParseResult<bool> {
     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 {

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'"
+    );
+}