From 5c298ed48f453aee5a0bbe17aaae42758424cf56 Mon Sep 17 00:00:00 2001 From: "Tom D." <15268361+anastygnome@users.noreply.github.com> Date: Tue, 29 Sep 2026 23:00:20 +0200 Subject: [PATCH 1/3] test: rework expression parsing for GNU compatibility `test` has special rules for expressions with one through four arguments. Keeping a postfix `Symbol` stack makes those cases harder to follow and clones `OsString`s while looking ahead. Evaluate directly from the argument slice instead. Keep the short forms explicit, then use the usual `!`, `-a`, `-o` precedence for longer expressions. This also fills in GNU-compatible `-l` integer operands, locale ordering for `<` and `>`, and the `-t` overflow case. This matches GNU logic, and makes the test pass. --- src/uu/test/Cargo.toml | 2 +- src/uu/test/locales/en-US.ftl | 6 +- src/uu/test/locales/fr-FR.ftl | 4 +- src/uu/test/src/diagnostics.rs | 7 +- src/uu/test/src/error.rs | 8 +- src/uu/test/src/parser.rs | 856 ++++++++++++++++++--------------- src/uu/test/src/test.rs | 472 +++++++++++------- tests/by-util/test_test.rs | 53 +- 8 files changed, 836 insertions(+), 572 deletions(-) diff --git a/src/uu/test/Cargo.toml b/src/uu/test/Cargo.toml index 79189ae24e5..eec13996903 100644 --- a/src/uu/test/Cargo.toml +++ b/src/uu/test/Cargo.toml @@ -19,7 +19,7 @@ doctest = false clap = { workspace = true } fluent = { workspace = true } thiserror = { workspace = true } -uucore = { workspace = true, features = ["fs", "wide"] } +uucore = { workspace = true, features = ["fs", "i18n-collator", "wide"] } [target.'cfg(not(any(windows, target_os = "wasi")))'.dependencies] rustix = { workspace = true, features = ["process"] } diff --git a/src/uu/test/locales/en-US.ftl b/src/uu/test/locales/en-US.ftl index 795140e3c70..df33b65ebf6 100644 --- a/src/uu/test/locales/en-US.ftl +++ b/src/uu/test/locales/en-US.ftl @@ -155,13 +155,15 @@ test-after-help = Exit with the status determined by EXPRESSION. # Error messages test-error-missing-closing-bracket = missing '{"]"}' -test-error-expected = expected { $value } -test-error-expected-value = expected value +test-error-expected = { $value } expected test-error-missing-argument = missing argument after { $argument } test-error-extra-argument = extra argument { $argument } test-error-unknown-operator = unknown operator { $operator } test-error-invalid-integer = invalid integer { $value } test-error-unary-operator-expected = { $operator }: unary operator expected +test-error-binary-operator-expected = { $operator }: binary operator expected +test-error-expected-found = { $expected } expected, found { $found } +test-error-does-not-accept-length = { $operator } does not accept -l # Diagnostic labels, used when errors are rendered with a source snippet test-diag-label-unary-operator-expected = this needs an expression on both sides diff --git a/src/uu/test/locales/fr-FR.ftl b/src/uu/test/locales/fr-FR.ftl index dd6c2ce595a..5bdd2c9e859 100644 --- a/src/uu/test/locales/fr-FR.ftl +++ b/src/uu/test/locales/fr-FR.ftl @@ -156,12 +156,14 @@ test-after-help = Quitter avec le statut déterminé par EXPRESSION. # Messages d'erreur test-error-missing-closing-bracket = '{"]"}' manquant test-error-expected = { $value } attendu -test-error-expected-value = valeur attendue test-error-missing-argument = argument manquant après { $argument } test-error-extra-argument = argument supplémentaire { $argument } test-error-unknown-operator = opérateur inconnu { $operator } test-error-invalid-integer = entier invalide { $value } test-error-unary-operator-expected = { $operator } : opérateur unaire attendu +test-error-binary-operator-expected = { $operator } : opérateur binaire attendu +test-error-expected-found = { $expected } attendu, trouvé { $found } +test-error-does-not-accept-length = { $operator } n'accepte pas -l # Étiquettes de diagnostic, utilisées quand les erreurs sont rendues avec un extrait test-diag-label-unary-operator-expected = nécessite une expression de chaque côté diff --git a/src/uu/test/src/diagnostics.rs b/src/uu/test/src/diagnostics.rs index 9b554d2eb2f..0736c255c73 100644 --- a/src/uu/test/src/diagnostics.rs +++ b/src/uu/test/src/diagnostics.rs @@ -33,7 +33,10 @@ pub fn render(args: &[OsString], err: &ParseError) -> bool { // Labelled only where a label would add to the message, per the convention // in `uucore::diagnostics`. let (label, help) = match &err.kind { - ParseErrorKind::Expected(_) => (None, None), + ParseErrorKind::Expected(_) + | ParseErrorKind::ExpectedFound(_, _) + | ParseErrorKind::BinaryOperatorExpected(_) + | ParseErrorKind::DoesNotAcceptLength(_) => (None, None), ParseErrorKind::ExtraArgument(_) => ( Some(translate!("diagnostics-label-expression-complete")), Some(translate!("test-diag-help-extra-argument")), @@ -63,8 +66,6 @@ pub fn render(args: &[OsString], err: &ParseError) -> bool { "name" => uucore::util_name() )), ), - // Never carries a position, so it is filtered out above. - ParseErrorKind::ExpectedValue => return false, }; snapshot.render( diff --git a/src/uu/test/src/error.rs b/src/uu/test/src/error.rs index e70112b2159..e274251cc9b 100644 --- a/src/uu/test/src/error.rs +++ b/src/uu/test/src/error.rs @@ -10,8 +10,6 @@ use uucore::translate; /// Represents an error encountered while parsing a test expression #[derive(Error, Debug)] pub enum ParseErrorKind { - #[error("{}", translate!("test-error-expected-value"))] - ExpectedValue, #[error("{}", translate!("test-error-expected", "value" => .0))] Expected(String), #[error("{}", translate!("test-error-extra-argument", "argument" => .0))] @@ -27,6 +25,12 @@ pub enum ParseErrorKind { InvalidFileDescriptor(String), #[error("{}", translate!("test-error-unary-operator-expected", "operator" => .0))] UnaryOperatorExpected(String), + #[error("{}", translate!("test-error-binary-operator-expected", "operator" => .0))] + BinaryOperatorExpected(String), + #[error("{}", translate!("test-error-expected-found", "expected" => .0, "found" => .1))] + ExpectedFound(String, String), + #[error("{}", translate!("test-error-does-not-accept-length", "operator" => .0))] + DoesNotAcceptLength(String), } /// Where in the original argument list an error occurred. diff --git a/src/uu/test/src/parser.rs b/src/uu/test/src/parser.rs index 85b9c5c5f0c..225dfe5bd61 100644 --- a/src/uu/test/src/parser.rs +++ b/src/uu/test/src/parser.rs @@ -3,482 +3,540 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. -// spell-checker:ignore (grammar) BOOLOP STRLEN FILETEST FILEOP INTOP STRINGOP ; (vars) LParen StrlenOp +// spell-checker:ignore (grammar) BOOL_OP INT_OP STRING_OP FILE_OP UNARY_OP lparen rparen nargs use std::ffi::{OsStr, OsString}; -use std::iter::Peekable; use super::error::{ParseError, ParseErrorKind, ParseResult}; - use uucore::display::Quotable; -/// Represents one of the binary comparison operators for strings, integers, or files -#[derive(Debug, PartialEq, Eq)] -pub enum Operator { - String(OsString), - Int(OsString), - File(OsString), +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) enum BinaryOp { + StrEq, + StrNe, + StrLt, + StrGt, + IntEq, + IntNe, + IntLt, + IntLe, + IntGt, + IntGe, + FileEf, + FileNt, + FileOt, } -/// Represents one of the unary test operators for strings or files -#[derive(Debug, PartialEq, Eq)] -pub enum UnaryOperator { - StrlenOp(OsString), - FiletestOp(OsString), +impl BinaryOp { + pub(crate) const fn as_str(self) -> &'static str { + match self { + Self::StrEq => "=", + Self::StrNe => "!=", + Self::StrLt => "<", + Self::StrGt => ">", + Self::IntEq => "-eq", + Self::IntNe => "-ne", + Self::IntLt => "-lt", + Self::IntLe => "-le", + Self::IntGt => "-gt", + Self::IntGe => "-ge", + Self::FileEf => "-ef", + Self::FileNt => "-nt", + Self::FileOt => "-ot", + } + } } -/// Represents a parsed token from a test expression -#[derive(Debug, PartialEq, Eq)] -pub enum Symbol { - LParen, - Bang, - BoolOp(OsString), - Literal(OsString), - Op(Operator), - UnaryOp(UnaryOperator), - None, +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) enum UnaryOp { + BlockSpecial, + CharacterSpecial, + Directory, + Exists, + Regular, + GroupIdFlag, + GroupOwns, + SymLink, + Sticky, + ModifiedSinceRead, + UserOwns, + Fifo, + Readable, + NonEmpty, + Socket, + Tty, + UserIdFlag, + Writable, + Executable, + StrNonEmpty, + StrEmpty, } -impl Symbol { - /// Create a new Symbol from an [`OsString`]. - /// - /// Returns `Symbol::None` in place of None - fn new(token: Option) -> Self { - match token { - Some(s) => match s.to_str() { - Some(t) => match t { - "(" => Self::LParen, - "!" => Self::Bang, - "-a" | "-o" => Self::BoolOp(s), - "=" | "==" | "!=" | "<" | ">" => Self::Op(Operator::String(s)), - "-eq" | "-ge" | "-gt" | "-le" | "-lt" | "-ne" => Self::Op(Operator::Int(s)), - "-ef" | "-nt" | "-ot" => Self::Op(Operator::File(s)), - "-n" | "-z" => Self::UnaryOp(UnaryOperator::StrlenOp(s)), - "-b" | "-c" | "-d" | "-e" | "-f" | "-g" | "-G" | "-h" | "-k" | "-L" | "-N" - | "-O" | "-p" | "-r" | "-s" | "-S" | "-t" | "-u" | "-w" | "-x" => { - Self::UnaryOp(UnaryOperator::FiletestOp(s)) - } - _ => Self::Literal(s), - }, - None => Self::Literal(s), - }, - None => Self::None, - } - } +#[derive(Clone, Copy, Debug)] +pub(crate) enum Operand<'a> { + Value(&'a OsStr), + Length(&'a OsStr), +} - /// Convert this Symbol into a [`Symbol::Literal`], useful for cases where - /// test treats an operator as a string operand (test has no reserved - /// words). - /// - /// # Panics - /// - /// Panics if `self` is [`Symbol::None`] - fn into_literal(self) -> Self { - Self::Literal(match self { - Self::LParen => OsString::from("("), - Self::Bang => OsString::from("!"), - Self::BoolOp(s) - | Self::Literal(s) - | Self::Op(Operator::String(s) | Operator::Int(s) | Operator::File(s)) - | Self::UnaryOp(UnaryOperator::StrlenOp(s) | UnaryOperator::FiletestOp(s)) => s, - Self::None => panic!(), - }) - } +pub(crate) trait Evaluator { + fn unary(&mut self, op: UnaryOp, arg: &OsStr) -> ParseResult; + fn binary(&mut self, op: BinaryOp, lhs: Operand<'_>, rhs: Operand<'_>) -> ParseResult; } -/// 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) | Operator::Int(s) | Operator::File(s)) - | Self::UnaryOp(UnaryOperator::StrlenOp(s) | UnaryOperator::FiletestOp(s)) => { - OsStr::new(s) - } - Self::None => OsStr::new("None"), - }; - write!(f, "{}", s.quote()) - } +#[inline] +fn bytes(s: &OsStr) -> &[u8] { + s.as_encoded_bytes() +} + +#[inline] +fn token_is(s: &OsStr, token: &[u8]) -> bool { + bytes(s) == token +} + +#[inline] +fn is_bang(s: &OsStr) -> bool { + token_is(s, b"!") +} + +#[inline] +fn is_lparen(s: &OsStr) -> bool { + token_is(s, b"(") +} + +#[inline] +fn is_rparen(s: &OsStr) -> bool { + token_is(s, b")") +} + +#[inline] +fn is_and(s: &OsStr) -> bool { + token_is(s, b"-a") +} + +#[inline] +fn is_or(s: &OsStr) -> bool { + token_is(s, b"-o") +} + +#[inline] +fn is_length(s: &OsStr) -> bool { + token_is(s, b"-l") } -/// Recursive descent parser for test, which converts a list of [`OsString`]s -/// (typically command line arguments) into a stack of Symbols in postfix -/// order. +#[inline] +fn is_two_byte_switch(s: &OsStr) -> bool { + let b = bytes(s); + b.len() == 2 && b[0] == b'-' && b[1] != 0 +} + +#[inline] +fn binary_op(s: &OsStr) -> Option { + Some(match bytes(s) { + b"=" | b"==" => BinaryOp::StrEq, + b"!=" => BinaryOp::StrNe, + b"<" => BinaryOp::StrLt, + b">" => BinaryOp::StrGt, + b"-eq" => BinaryOp::IntEq, + b"-ne" => BinaryOp::IntNe, + b"-lt" => BinaryOp::IntLt, + b"-le" => BinaryOp::IntLe, + b"-gt" => BinaryOp::IntGt, + b"-ge" => BinaryOp::IntGe, + b"-ef" => BinaryOp::FileEf, + b"-nt" => BinaryOp::FileNt, + b"-ot" => BinaryOp::FileOt, + _ => return None, + }) +} + +#[inline] +fn unary_op(s: &OsStr) -> Option { + Some(match bytes(s) { + b"-b" => UnaryOp::BlockSpecial, + b"-c" => UnaryOp::CharacterSpecial, + b"-d" => UnaryOp::Directory, + b"-e" => UnaryOp::Exists, + b"-f" => UnaryOp::Regular, + b"-g" => UnaryOp::GroupIdFlag, + b"-G" => UnaryOp::GroupOwns, + b"-h" | b"-L" => UnaryOp::SymLink, + b"-k" => UnaryOp::Sticky, + b"-N" => UnaryOp::ModifiedSinceRead, + b"-O" => UnaryOp::UserOwns, + b"-p" => UnaryOp::Fifo, + b"-r" => UnaryOp::Readable, + b"-s" => UnaryOp::NonEmpty, + b"-S" => UnaryOp::Socket, + b"-t" => UnaryOp::Tty, + b"-u" => UnaryOp::UserIdFlag, + b"-w" => UnaryOp::Writable, + b"-x" => UnaryOp::Executable, + b"-n" => UnaryOp::StrNonEmpty, + b"-z" => UnaryOp::StrEmpty, + _ => return None, + }) +} + +/// Parser for `test` expressions. /// -/// Grammar: +/// `test` has special, historical meanings for expressions with one to four +/// arguments. Longer expressions use the following grammar, with the usual +/// precedence from strongest to weakest: `!`, `-a`, then `-o`. /// -/// EXPR → TERM | EXPR BOOLOP EXPR -/// TERM → ( EXPR ) -/// TERM → ! EXPR -/// TERM → UOP str -/// UOP → STRLEN | FILETEST -/// TERM → str OP str -/// TERM → str | 𝜖 -/// OP → STRINGOP | INTOP | FILEOP -/// STRINGOP → = | == | != -/// INTOP → -eq | -ge | -gt | -le | -lt | -ne -/// FILEOP → -ef | -nt | -ot -/// STRLEN → -n | -z -/// FILETEST → -b | -c | -d | -e | -f | -g | -G | -h | -k | -L | -N | -O | -p | -/// -r | -s | -S | -t | -u | -w | -x -/// BOOLOP → -a | -o +/// ```text +/// EXPR := OR_EXPR +/// OR_EXPR := AND_EXPR ( "-o" AND_EXPR )* +/// AND_EXPR := TERM ( "-a" TERM )* +/// TERM := "!"* ATOM +/// ATOM := "(" EXPR ")" +/// | VALUE OP VALUE +/// | "-l" VALUE INT_OP VALUE +/// | VALUE INT_OP "-l" VALUE +/// | UNARY_OP VALUE +/// | VALUE +/// OP := STRING_OP | INT_OP | FILE_OP +/// STRING_OP := "=" | "==" | "!=" | "<" | ">" +/// INT_OP := "-eq" | "-ne" | "-lt" | "-le" | "-gt" | "-ge" +/// FILE_OP := "-ef" | "-nt" | "-ot" +/// UNARY_OP := "-n" | "-z" | FILE_TEST +/// FILE_TEST := "-b" | "-c" | "-d" | "-e" | "-f" | "-g" | "-G" | "-h" +/// | "-k" | "-L" | "-N" | "-O" | "-p" | "-r" | "-s" | "-S" +/// | "-t" | "-u" | "-w" | "-x" +///``` /// -#[derive(Debug)] -struct Parser { - /// Only `next_raw` may advance this iterator, so that `pos` stays in sync. - /// Lookahead must go through `peek` or a `clone` of the iterator. - tokens: Peekable>, - /// Index of the next token to be consumed. +/// The grammar is only a description of the long-expression path. Operators +/// are recognized according to their position, so tokens such as `!`, `-a`, +/// `-o`, and `(` can still be literal operands in the short forms. Parsing and +/// evaluation happen together, directly from the argument list; this preserves +/// `test`'s requirement to evaluate both sides of `-a` and `-o`. +struct Parser<'a, E> { + args: &'a [OsString], pos: usize, - pub stack: Vec, + eval: E, } -impl Parser { - /// Construct a new Parser from a `Vec` of tokens. - fn new(tokens: Vec) -> Self { - Self { - tokens: tokens.into_iter().peekable(), - pos: 0, - stack: vec![], - } +impl<'a, E: Evaluator> Parser<'a, E> { + #[inline] + fn arg(&self, index: usize) -> &'a OsStr { + self.args[index].as_os_str() } - /// Consume the next token from the input stream, tracking its position. - fn next_raw(&mut self) -> Option { - let token = self.tokens.next(); - if token.is_some() { - self.pos += 1; - } - token + #[cold] + fn missing_after_last(&self) -> ParseError { + let index = self.args.len().saturating_sub(1); + let value = self + .args + .get(index) + .map_or_else(|| "''".to_owned(), |s| s.quote().to_string()); + ParseError::at_token(ParseErrorKind::MissingArgument(value), index) } - /// Fetch the next token from the input stream as a Symbol. - fn next_token(&mut self) -> Symbol { - Symbol::new(self.next_raw()) + #[inline] + fn advance_required(&mut self) -> ParseResult<()> { + self.pos += 1; + if self.pos >= self.args.len() { + Err(self.missing_after_last()) + } else { + Ok(()) + } } - /// Index of the token most recently consumed. - fn last_pos(&self) -> usize { - self.pos.saturating_sub(1) + #[inline] + fn eval_one(&mut self) -> bool { + let result = !self.arg(self.pos).is_empty(); + self.pos += 1; + result } - /// Consume the next token & verify that it matches the provided value. - fn expect(&mut self, value: &str) -> ParseResult<()> { - match self.next_token() { - Symbol::Literal(s) if s == value => Ok(()), - _ => Err(ParseError::at_token( - ParseErrorKind::Expected(value.quote().to_string()), - self.last_pos(), - )), + fn eval_two(&mut self) -> ParseResult { + if is_bang(self.arg(self.pos)) { + self.pos += 1; + return Ok(!self.eval_one()); } - } - /// Peek at the next token from the input stream, returning it as a Symbol. - /// The stream is unchanged and will return the same Symbol on subsequent - /// calls to `next()` or `peek()`. - fn peek(&mut self) -> Symbol { - Symbol::new(self.tokens.peek().cloned()) - } + // In this form every two-byte -X is treated as a unary operator first. + // That is why an unknown switch is an error here, not a string literal. + if is_two_byte_switch(self.arg(self.pos)) { + return self.eval_unary(); + } - /// Test if the next token in the stream is a BOOLOP (-a or -o), without - /// removing the token from the stream. - fn peek_is_boolop(&mut self) -> bool { - matches!(self.peek(), Symbol::BoolOp(_)) + Err(self.missing_after_last()) } - /// Parse an expression. - /// - /// EXPR → TERM | EXPR BOOLOP EXPR - fn expr(&mut self) -> ParseResult<()> { - let has_term = !self.peek_is_boolop(); - if has_term { - self.term()?; + fn eval_three(&mut self) -> ParseResult { + if let Some(op) = binary_op(self.arg(self.pos + 1)) { + return self.eval_binary(false, op); } - self.maybe_boolop(has_term)?; - Ok(()) - } - /// Parse a term token and possible subsequent symbols: "(", "!", UOP, - /// literal, or None. - fn term(&mut self) -> ParseResult<()> { - let symbol = self.next_token(); - - match symbol { - Symbol::LParen => self.lparen()?, - Symbol::Bang => self.bang()?, - Symbol::UnaryOp(_) => { - // Three-argument string comparison: `-f = a` means "-f" = "a", not file test - let is_string_cmp = matches!(self.peek(), Symbol::Op(Operator::String(_))) - && !matches!(Symbol::new(self.tokens.clone().nth(1)), Symbol::None); - if is_string_cmp { - self.literal(symbol.into_literal())?; - } else { - self.uop(symbol); - } - } - Symbol::None => self.stack.push(symbol), - literal => self.literal(literal)?, + if is_bang(self.arg(self.pos)) { + self.advance_required()?; + return Ok(!self.eval_two()?); } - Ok(()) - } - /// Parse a (possibly) parenthesized expression. - /// - /// test has no reserved keywords, so "(" will be interpreted as a literal - /// in certain cases: - /// - /// * when found at the end of the token stream - /// * when followed by a binary operator that is not _itself_ interpreted - /// as a literal - /// - 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 - .tokens - .clone() - .take(3) - .map(|token| Symbol::new(Some(token))) - .collect(); - - match peek3.as_slice() { - // case 2: error if end of stream is `( ` - // `symbol` was only peeked at, so it sits at the current position - [symbol] => Err(ParseError::at_token( - ParseErrorKind::MissingArgument(format!("{symbol}")), - self.pos, - )), - - // case 3: `( uop )` → parenthesized unary operation; - // this case ensures we don’t get confused by `( -f ) )` - // or `( -f ( )`, for example - [Symbol::UnaryOp(_), _, Symbol::Literal(s)] if s == ")" => { - let symbol = self.next_token(); - self.uop(symbol); - self.expect(")")?; - Ok(()) - } + if is_lparen(self.arg(self.pos)) && is_rparen(self.arg(self.pos + 2)) { + self.pos += 1; + let result = self.eval_one(); + self.pos += 1; + return Ok(result); + } - // 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())?; - Ok(()) - } + if is_and(self.arg(self.pos + 1)) + || is_or(self.arg(self.pos + 1)) + || token_is(self.arg(self.pos + 1), b">") + || token_is(self.arg(self.pos + 1), b"<") + { + return self.eval_expr(); + } - // 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(")")?; - Ok(()) - } + let index = self.pos + 1; + Err(ParseError::at_token( + ParseErrorKind::BinaryOperatorExpected(self.arg(index).quote().to_string()), + index, + )) + } - // 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(")")?; - Ok(()) - } + /// Apply the historical one- to four-argument rules before using the + /// precedence-based parser for longer expressions. + fn eval_by_arity(&mut self, nargs: usize) -> ParseResult { + match nargs { + 1 => Ok(self.eval_one()), + 2 => self.eval_two(), + 3 => self.eval_three(), + 4 => { + if is_bang(self.arg(self.pos)) { + self.advance_required()?; + return Ok(!self.eval_three()?); + } - // case 1: lparen is a literal when followed by nothing - // 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())?; - Ok(()) - } + if is_lparen(self.arg(self.pos)) && is_rparen(self.arg(self.pos + 3)) { + self.pos += 1; + let result = self.eval_two()?; + self.pos += 1; + return Ok(result); + } - // Otherwise, lparen indicates the start of a parenthesized - // expression - _ => { - self.expr()?; - self.expect(")")?; - Ok(()) + self.eval_expr() } + _ => self.eval_expr(), } } - /// Parse a (possibly) negated expression. - /// - /// Example cases: - /// - /// * `! =`: negate the result of the implicit string length test of `=` - /// * `! = foo`: compare the literal strings `!` and `foo` - /// * `! = = str`: negate comparison of literal `=` and `str` - /// * `!`: bang followed by nothing is literal - /// * `! EXPR`: negate the result of the expression - /// - /// Combined Boolean & negation: - /// - /// * `! ( EXPR ) [BOOLOP EXPR]`: negate the parenthesized expression only - /// * `! UOP str BOOLOP EXPR`: negate the unary subexpression - /// * `! 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) -> ParseResult<()> { - match self.peek() { - Symbol::Op(_) | Symbol::BoolOp(_) => { - // we need to peek ahead one more token to disambiguate the first - // three cases listed above - let peek2 = Symbol::new(self.tokens.clone().nth(1)); - - match peek2 { - // case 1: `! ` - // case 3: `! = OP str` - Symbol::Op(_) | Symbol::None => { - // op is literal - let op = self.next_token().into_literal(); - 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(true)?; - } - } - } + /// Parse a long expression using `-a` precedence over `-o`. + fn eval_expr(&mut self) -> ParseResult { + if self.pos >= self.args.len() { + return Err(self.missing_after_last()); + } + self.eval_or() + } - // bang followed by nothing is literal - Symbol::None => self.stack.push(Symbol::Bang.into_literal()), - - _ => { - // peek ahead up to 4 tokens to determine if we need to negate - // the entire expression or just the first term - let peek4: Vec = self - .tokens - .clone() - .take(4) - .map(|token| Symbol::new(Some(token))) - .collect(); - - if let [Symbol::Literal(_), Symbol::BoolOp(_), Symbol::Literal(_)] = - peek4.as_slice() - { - // we peeked ahead 4 but there were only 3 tokens left - self.expr()?; - self.stack.push(Symbol::Bang); - } else { - self.term()?; - self.stack.push(Symbol::Bang); - } + fn eval_or(&mut self) -> ParseResult { + let mut result = false; + loop { + // `test` evaluates both sides of -o, even when the result is known. + result |= self.eval_and()?; + if self.pos >= self.args.len() || !is_or(self.arg(self.pos)) { + return Ok(result); } + self.pos += 1; } - Ok(()) } - /// Peek at the next token and parse it as a BOOLOP or string literal, - /// as appropriate. - /// - /// `has_left_operand` tells whether an expression was already parsed for - /// the BOOLOP to apply to, which decides what a BOOLOP at the end of the - /// stream means. - fn maybe_boolop(&mut self, has_left_operand: bool) -> ParseResult<()> { - if self.peek_is_boolop() { - let symbol = self.next_token(); - - if let Symbol::None = self.peek() { - if has_left_operand { - // The BOOLOP joins the expression so far to nothing. - return Err(ParseError::at_token( - ParseErrorKind::MissingArgument(format!("{symbol}")), - self.last_pos(), - )); - } - // With no operand on either side it is an ordinary string: - // `test -a` is the length test of the string "-a". - self.literal(symbol.into_literal())?; - } else { - self.boolop(symbol)?; - self.maybe_boolop(true)?; + fn eval_and(&mut self) -> ParseResult { + let mut result = true; + loop { + // `test` evaluates both sides of -a, even when the result is known. + result &= self.eval_term()?; + if self.pos >= self.args.len() || !is_and(self.arg(self.pos)) { + return Ok(result); } + self.pos += 1; } - Ok(()) } - /// Parse a Boolean expression. - /// - /// 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) -> ParseResult<()> { - if op == Symbol::BoolOp(OsString::from("-a")) { - self.term()?; + /// Parse a term, consuming leading negations and then an atom. + fn eval_term(&mut self) -> ParseResult { + let mut negate = false; + while self.pos < self.args.len() && is_bang(self.arg(self.pos)) { + self.advance_required()?; + negate = !negate; + } + + if self.pos >= self.args.len() { + return Err(self.missing_after_last()); + } + + let remaining = self.args.len() - self.pos; + let value = if is_lparen(self.arg(self.pos)) { + self.eval_parenthesized()? + } else if remaining >= 4 + && is_length(self.arg(self.pos)) + && binary_op(self.arg(self.pos + 2)).is_some() + { + let op = binary_op(self.arg(self.pos + 2)).unwrap(); + self.eval_binary(true, op)? + } else if remaining >= 3 { + if let Some(op) = binary_op(self.arg(self.pos + 1)) { + self.eval_binary(false, op)? + } else if self.is_general_unary_start() { + self.eval_unary()? + } else { + self.eval_literal() + } + } else if self.is_general_unary_start() { + self.eval_unary()? } else { - self.expr()?; + self.eval_literal() + }; + + Ok(negate ^ value) + } + + #[inline] + fn is_general_unary_start(&self) -> bool { + if !is_two_byte_switch(self.arg(self.pos)) { + return false; } - self.stack.push(op); - Ok(()) + let b = bytes(self.arg(self.pos)); + b[1] != b'a' && b[1] != b'o' + } + + #[inline] + fn eval_literal(&mut self) -> bool { + let value = !self.arg(self.pos).is_empty(); + self.pos += 1; + value } - /// Parse a (possible) unary argument test (string length or file - /// attribute check). - /// - /// If a UOP is followed by nothing it is interpreted as a literal string. - fn uop(&mut self, op: Symbol) { - match self.next_token() { - Symbol::None => self.stack.push(op.into_literal()), - symbol => { - self.stack.push(symbol.into_literal()); - self.stack.push(op); + /// Parse a parenthesized expression while preserving short-form arity + /// rules inside the parentheses. + fn eval_parenthesized(&mut self) -> ParseResult { + self.advance_required()?; + + // For short parenthesized expressions the arity rules still matter. + // Once there are more than four arguments, the normal parser takes over. + let mut nargs = 1usize; + // A closing parenthesis can be the right-hand string operand in + // `( ( != ) )`, so do not mistake it for the group's delimiter. + let right_paren_is_operand = self.pos + 3 < self.args.len() + && is_lparen(self.arg(self.pos)) + && binary_op(self.arg(self.pos + 1)).is_some() + && is_rparen(self.arg(self.pos + 2)) + && is_rparen(self.arg(self.pos + 3)); + while self.pos + nargs < self.args.len() + && (!is_rparen(self.arg(self.pos + nargs)) || (right_paren_is_operand && nargs == 2)) + { + if nargs == 4 { + nargs = self.args.len() - self.pos; + break; } + nargs += 1; } + + let result = self.eval_by_arity(nargs)?; + if self.pos >= self.args.len() { + return Err(ParseError::at_token( + ParseErrorKind::Expected(OsStr::new(")").quote().to_string()), + self.args.len().saturating_sub(1), + )); + } + if !is_rparen(self.arg(self.pos)) { + return Err(ParseError::at_token( + ParseErrorKind::ExpectedFound( + OsStr::new(")").quote().to_string(), + self.arg(self.pos).quote().to_string(), + ), + self.pos, + )); + } + self.pos += 1; + Ok(result) } - /// Parse a string literal, optionally followed by a comparison operator - /// and a second string literal. - fn literal(&mut self, token: Symbol) -> ParseResult<()> { - self.stack.push(token.into_literal()); + fn eval_unary(&mut self) -> ParseResult { + let op_index = self.pos; + let op_token = self.arg(op_index); + let Some(op) = unary_op(op_token) else { + return Err(ParseError::at_token( + ParseErrorKind::UnaryOperatorExpected(op_token.quote().to_string()), + op_index, + )); + }; - // EXPR → str OP str - if let Symbol::Op(_) = self.peek() { - let op = self.next_token(); + self.advance_required()?; + let arg = self.arg(self.pos); + self.pos += 1; + self.eval.unary(op, arg) + } - match self.next_token() { - Symbol::None => { + fn eval_binary(&mut self, lhs_is_length: bool, op: BinaryOp) -> ParseResult { + let start = self.pos; + let op_index = start + if lhs_is_length { 2 } else { 1 }; + let rhs_index = op_index + 1; + let rhs_is_length = op_index + 2 < self.args.len() && is_length(self.arg(rhs_index)); + + // A right-hand -l consumes its string before the operator kind is + // considered. Keep that oddity because it is visible in expressions. + self.pos = op_index + if rhs_is_length { 3 } else { 2 }; + + match op { + BinaryOp::IntEq + | BinaryOp::IntNe + | BinaryOp::IntLt + | BinaryOp::IntLe + | BinaryOp::IntGt + | BinaryOp::IntGe => { + let lhs = if lhs_is_length { + Operand::Length(self.arg(start + 1)) + } else { + Operand::Value(self.arg(start)) + }; + let rhs = if rhs_is_length { + Operand::Length(self.arg(op_index + 2)) + } else { + Operand::Value(self.arg(rhs_index)) + }; + self.eval.binary(op, lhs, rhs) + } + BinaryOp::FileEf | BinaryOp::FileNt | BinaryOp::FileOt => { + if lhs_is_length || rhs_is_length { return Err(ParseError::at_token( - ParseErrorKind::MissingArgument(format!("{op}")), - self.last_pos(), + ParseErrorKind::DoesNotAcceptLength(op.as_str().to_owned()), + op_index, )); } - token => self.stack.push(token.into_literal()), + self.eval.binary( + op, + Operand::Value(self.arg(start)), + Operand::Value(self.arg(rhs_index)), + ) + } + BinaryOp::StrEq | BinaryOp::StrNe | BinaryOp::StrLt | BinaryOp::StrGt => { + let lhs_index = if lhs_is_length { start + 1 } else { start }; + self.eval.binary( + op, + Operand::Value(self.arg(lhs_index)), + Operand::Value(self.arg(rhs_index)), + ) } - - 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) -> ParseResult<()> { - self.expr()?; - - match self.next_raw() { - Some(token) => Err(ParseError::at_token( - ParseErrorKind::ExtraArgument(token.quote().to_string()), - self.last_pos(), - )), - None => Ok(()), + fn finish(mut self) -> ParseResult { + if self.args.is_empty() { + return Ok(false); + } + + let result = self.eval_by_arity(self.args.len())?; + if self.pos != self.args.len() { + return Err(ParseError::at_token( + ParseErrorKind::ExtraArgument(self.arg(self.pos).quote().to_string()), + self.pos, + )); } + Ok(result) } } -/// Parse the token stream `args`, returning a `Symbol` stack representing the -/// operations to perform in postfix order. -pub fn parse(args: Vec) -> ParseResult> { - let mut p = Parser::new(args); - p.parse()?; - Ok(p.stack) +pub(crate) fn evaluate(args: &[OsString], eval: E) -> ParseResult { + Parser { args, pos: 0, eval }.finish() } diff --git a/src/uu/test/src/test.rs b/src/uu/test/src/test.rs index 01f477ed62c..8bdaef6871d 100644 --- a/src/uu/test/src/test.rs +++ b/src/uu/test/src/test.rs @@ -3,7 +3,7 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. -// spell-checker:ignore (vars) egid euid FiletestOp StrlenOp +// spell-checker:ignore (vars) egid euid mod diagnostics; pub(crate) mod error; @@ -13,7 +13,7 @@ mod platform; use clap::Command; use error::{ParseError, ParseErrorKind, ParseResult}; -use parser::{Operator, Symbol, UnaryOperator, parse}; +use parser::{BinaryOp, Evaluator, Operand, UnaryOp, evaluate}; #[cfg(windows)] use platform::fd_is_terminal; #[cfg(target_os = "wasi")] @@ -23,11 +23,13 @@ use rustix::process::{getegid, geteuid}; use std::cmp::Ordering; use std::ffi::{OsStr, OsString}; use std::fs; +use std::mem::size_of; #[cfg(unix)] use std::os::unix::fs::MetadataExt; use uucore::display::Quotable; use uucore::error::{UResult, USimpleError}; use uucore::format_usage; +use uucore::i18n::collator::{init_locale_collation, locale_cmp}; use uucore::translate; // The help_usage method replaces util name (the first word) with {}. @@ -76,11 +78,9 @@ pub fn uumain(mut args: impl uucore::Args) -> UResult<()> { // Show actual name with error let _ = uu_app().name("test"); } - // `parse` consumes the arguments, so keep a copy for the diagnostic — but - // only when one could actually be rendered. let expression = uucore::diagnostics::capture(&args); - match parse(args).and_then(|mut stack| eval(&mut stack)) { + match evaluate(&args, TestEvaluator) { Ok(true) => Ok(()), Ok(false) => Err(1.into()), Err(e) => Err(uucore::diagnostics::error_after_report( @@ -91,106 +91,75 @@ pub fn uumain(mut args: impl uucore::Args) -> UResult<()> { } } -/// Evaluate a stack of Symbols, returning the result of the evaluation or -/// an error message if evaluation failed. -fn eval(stack: &mut Vec) -> ParseResult { - macro_rules! pop_literal { - () => { - match stack.pop() { - Some(Symbol::Literal(s)) => s, - _ => panic!(), - } - }; - } +struct TestEvaluator; - let s = stack.pop(); +#[inline] +fn operand_value(operand: Operand<'_>) -> &OsStr { + match operand { + Operand::Value(value) => value, + Operand::Length(_) => unreachable!("length operand passed to non-integer operator"), + } +} - match s { - Some(Symbol::Bang) => { - let result = eval(stack)?; +impl Evaluator for TestEvaluator { + fn unary(&mut self, op: UnaryOp, arg: &OsStr) -> ParseResult { + Ok(match op { + UnaryOp::BlockSpecial => path(arg, &PathCondition::BlockSpecial), + UnaryOp::CharacterSpecial => path(arg, &PathCondition::CharacterSpecial), + UnaryOp::Directory => path(arg, &PathCondition::Directory), + UnaryOp::Exists => path(arg, &PathCondition::Exists), + UnaryOp::Regular => path(arg, &PathCondition::Regular), + UnaryOp::GroupIdFlag => path(arg, &PathCondition::GroupIdFlag), + UnaryOp::GroupOwns => path(arg, &PathCondition::GroupOwns), + UnaryOp::SymLink => path(arg, &PathCondition::SymLink), + UnaryOp::Sticky => path(arg, &PathCondition::Sticky), + UnaryOp::ModifiedSinceRead => path(arg, &PathCondition::ExistsModifiedLastRead), + UnaryOp::UserOwns => path(arg, &PathCondition::UserOwns), + UnaryOp::Fifo => path(arg, &PathCondition::Fifo), + UnaryOp::Readable => path(arg, &PathCondition::Readable), + UnaryOp::NonEmpty => path(arg, &PathCondition::NonEmpty), + UnaryOp::Socket => path(arg, &PathCondition::Socket), + UnaryOp::Tty => isatty(arg)?, + UnaryOp::UserIdFlag => path(arg, &PathCondition::UserIdFlag), + UnaryOp::Writable => path(arg, &PathCondition::Writable), + UnaryOp::Executable => path(arg, &PathCondition::Executable), + UnaryOp::StrNonEmpty => !arg.is_empty(), + UnaryOp::StrEmpty => arg.is_empty(), + }) + } - Ok(!result) - } - Some(Symbol::Op(Operator::String(op))) => { - let b = pop_literal!(); - let a = pop_literal!(); - match op.as_encoded_bytes() { - b"!=" => Ok(a != b), - b"<" => Ok(a < b), - b">" => Ok(a > b), - _ => Ok(a == b), + fn binary(&mut self, op: BinaryOp, lhs: Operand<'_>, rhs: Operand<'_>) -> ParseResult { + match op { + BinaryOp::StrEq => Ok(operand_value(lhs) == operand_value(rhs)), + BinaryOp::StrNe => Ok(operand_value(lhs) != operand_value(rhs)), + BinaryOp::StrLt => { + let _ = init_locale_collation(); + Ok(locale_cmp( + operand_value(lhs).as_encoded_bytes(), + operand_value(rhs).as_encoded_bytes(), + ) + .is_lt()) } - } - Some(Symbol::Op(Operator::Int(op))) => { - let b = pop_literal!(); - let a = pop_literal!(); - - Ok(integers(&a, &b, &op)?) - } - Some(Symbol::Op(Operator::File(op))) => { - let b = pop_literal!(); - let a = pop_literal!(); - Ok(files(&a, &b, &op)?) - } - Some(Symbol::UnaryOp(UnaryOperator::StrlenOp(op))) => { - let s = match stack.pop() { - Some(Symbol::Literal(s)) => s, - Some(Symbol::None) => OsString::from(""), - None => return Ok(true), - _ => { - return Err(ParseError::at_value( - ParseErrorKind::MissingArgument(op.quote().to_string()), - &op, - )); - } - }; - - Ok((op == "-z") == s.is_empty()) - } - Some(Symbol::UnaryOp(UnaryOperator::FiletestOp(op))) => { - let op = op.to_str().unwrap(); - - let f = pop_literal!(); - - Ok(match op { - "-b" => path(&f, &PathCondition::BlockSpecial), - "-c" => path(&f, &PathCondition::CharacterSpecial), - "-d" => path(&f, &PathCondition::Directory), - "-e" => path(&f, &PathCondition::Exists), - "-f" => path(&f, &PathCondition::Regular), - "-g" => path(&f, &PathCondition::GroupIdFlag), - "-G" => path(&f, &PathCondition::GroupOwns), - "-h" | "-L" => path(&f, &PathCondition::SymLink), - "-k" => path(&f, &PathCondition::Sticky), - "-N" => path(&f, &PathCondition::ExistsModifiedLastRead), - "-O" => path(&f, &PathCondition::UserOwns), - "-p" => path(&f, &PathCondition::Fifo), - "-r" => path(&f, &PathCondition::Readable), - "-S" => path(&f, &PathCondition::Socket), - "-s" => path(&f, &PathCondition::NonEmpty), - "-t" => isatty(&f)?, - "-u" => path(&f, &PathCondition::UserIdFlag), - "-w" => path(&f, &PathCondition::Writable), - "-x" => path(&f, &PathCondition::Executable), - _ => panic!(), - }) - } - Some(Symbol::Literal(s)) => Ok(!s.is_empty()), - Some(Symbol::None) | None => Ok(false), - Some(Symbol::BoolOp(op)) => { - if (op == "-a" || op == "-o") && stack.len() < 2 { - return Err(ParseError::at_value( - ParseErrorKind::UnaryOperatorExpected(op.quote().to_string()), - &op, - )); + BinaryOp::StrGt => { + let _ = init_locale_collation(); + Ok(locale_cmp( + operand_value(lhs).as_encoded_bytes(), + operand_value(rhs).as_encoded_bytes(), + ) + .is_gt()) } - - let b = eval(stack)?; - let a = eval(stack)?; - - Ok(if op == "-a" { a && b } else { a || b }) + BinaryOp::IntEq + | BinaryOp::IntNe + | BinaryOp::IntLt + | BinaryOp::IntLe + | BinaryOp::IntGt + | BinaryOp::IntGe => compare_integer_operands(lhs, rhs, op), + BinaryOp::FileEf | BinaryOp::FileNt | BinaryOp::FileOt => files( + operand_value(lhs), + operand_value(rhs), + OsStr::new(op.as_str()), + ), } - _ => Err(ParseErrorKind::ExpectedValue.into()), } } @@ -209,8 +178,12 @@ struct Integer<'a> { impl<'a> Integer<'a> { /// Parse an operand of the form `[+-]?[0-9]+`, surrounded by optional /// whitespace, returning [`None`] when it has any other shape. + /// The [POSIX locale convention](https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_05) + /// includes U+000B VERTICAL TAB as whitespace. fn parse(value: &'a OsStr) -> Option { - let value = value.to_str()?.trim(); + let value = value + .to_str()? + .trim_matches(|c: char| c.is_ascii_whitespace() || c == '\u{000b}'); // Only ASCII `+`/`-` are sliced off, so this always cuts on a char boundary. let (negative, digits) = match value.as_bytes().first()? { @@ -263,35 +236,75 @@ impl PartialOrd for Integer<'_> { } } -/// Operations to compare integers -/// `a` is the left hand side -/// `b` is the right hand side -/// `op` the operation (ex: -eq, -lt, etc) -fn integers(a: &OsStr, b: &OsStr, op: &OsStr) -> ParseResult { - // Parse the two inputs - let left = Integer::parse(a).ok_or_else(|| { - ParseError::at_value(ParseErrorKind::InvalidInteger(a.quote().to_string()), a) - })?; - let right = Integer::parse(b).ok_or_else(|| { - ParseError::at_value(ParseErrorKind::InvalidInteger(b.quote().to_string()), b) - })?; +#[derive(Debug)] +enum IntegerOperand<'a> { + Parsed(Integer<'a>), + Length(usize), +} - // Do the maths - let order = left.cmp(&right); - - Ok(match op.to_str() { - Some("-eq") => order.is_eq(), - Some("-ne") => order.is_ne(), - Some("-gt") => order.is_gt(), - Some("-ge") => order.is_ge(), - Some("-lt") => order.is_lt(), - Some("-le") => order.is_le(), - _ => { - return Err(ParseError::at_value( - ParseErrorKind::UnknownOperator(op.quote().to_string()), - op, - )); +fn integer_operand(operand: Operand<'_>) -> ParseResult> { + match operand { + Operand::Length(value) => Ok(IntegerOperand::Length(value.as_encoded_bytes().len())), + Operand::Value(value) => Integer::parse(value) + .map(IntegerOperand::Parsed) + .ok_or_else(|| { + ParseError::at_value( + ParseErrorKind::InvalidInteger(value.quote().to_string()), + value, + ) + }), + } +} + +fn integer_cmp_usize(value: &Integer<'_>, other: usize) -> Ordering { + if value.negative { + return Ordering::Less; + } + + let mut buf = [0_u8; 3 * size_of::()]; + let mut n = other; + let mut start = buf.len(); + loop { + start -= 1; + buf[start] = b'0' + (n % 10) as u8; + n /= 10; + if n == 0 { + break; } + } + + let digits = if value.digits.is_empty() { + b"0".as_slice() + } else { + value.digits.as_bytes() + }; + let other = &buf[start..]; + digits + .len() + .cmp(&other.len()) + .then_with(|| digits.cmp(other)) +} + +fn compare_integer_operands(lhs: Operand<'_>, rhs: Operand<'_>, op: BinaryOp) -> ParseResult { + let lhs = integer_operand(lhs)?; + let rhs = integer_operand(rhs)?; + let order = match (&lhs, &rhs) { + (IntegerOperand::Parsed(lhs), IntegerOperand::Parsed(rhs)) => lhs.cmp(rhs), + (IntegerOperand::Parsed(lhs), IntegerOperand::Length(rhs)) => integer_cmp_usize(lhs, *rhs), + (IntegerOperand::Length(lhs), IntegerOperand::Parsed(rhs)) => { + integer_cmp_usize(rhs, *lhs).reverse() + } + (IntegerOperand::Length(lhs), IntegerOperand::Length(rhs)) => lhs.cmp(rhs), + }; + + Ok(match op { + BinaryOp::IntEq => order.is_eq(), + BinaryOp::IntNe => order.is_ne(), + BinaryOp::IntLt => order.is_lt(), + BinaryOp::IntLe => order.is_le(), + BinaryOp::IntGt => order.is_gt(), + BinaryOp::IntGe => order.is_ge(), + _ => unreachable!("non-integer operator passed to integer comparison"), }) } @@ -325,16 +338,28 @@ fn files(a: &OsStr, b: &OsStr, op: &OsStr) -> ParseResult { } fn isatty(fd: &OsStr) -> ParseResult { - fd.to_str() - .map(str::trim) - .and_then(|s| s.parse::().ok()) - .ok_or_else(|| { - ParseError::at_value( - ParseErrorKind::InvalidFileDescriptor(fd.quote().to_string()), - fd, - ) - }) - .map(fd_is_terminal) + let value = Integer::parse(fd).ok_or_else(|| { + ParseError::at_value( + ParseErrorKind::InvalidFileDescriptor(fd.quote().to_string()), + fd, + ) + })?; + + if value.negative { + return Ok(false); + } + let descriptor = if value.digits.is_empty() { + 0 + } else { + let Ok(descriptor) = value.digits.parse::() else { + return Ok(false); + }; + if descriptor > i32::MAX as u32 { + return Ok(false); + } + descriptor as i32 + }; + Ok(fd_is_terminal(descriptor)) } #[cfg(not(windows))] @@ -553,19 +578,34 @@ mod tests { fn test_integer_op() { let a = OsStr::new("18446744073709551616"); let b = OsStr::new("0"); - assert!(!integers(a, b, OsStr::new("-lt")).unwrap()); + assert!( + !compare_integer_operands(Operand::Value(a), Operand::Value(b), BinaryOp::IntLt) + .unwrap() + ); let a = OsStr::new("18446744073709551616"); let b = OsStr::new("0"); - assert!(integers(a, b, OsStr::new("-gt")).unwrap()); + assert!( + compare_integer_operands(Operand::Value(a), Operand::Value(b), BinaryOp::IntGt) + .unwrap() + ); let a = OsStr::new("-1"); let b = OsStr::new("0"); - assert!(integers(a, b, OsStr::new("-lt")).unwrap()); + assert!( + compare_integer_operands(Operand::Value(a), Operand::Value(b), BinaryOp::IntLt) + .unwrap() + ); let a = OsStr::new("42"); let b = OsStr::new("42"); - assert!(integers(a, b, OsStr::new("-eq")).unwrap()); + assert!( + compare_integer_operands(Operand::Value(a), Operand::Value(b), BinaryOp::IntEq) + .unwrap() + ); let a = OsStr::new("42"); let b = OsStr::new("42"); - assert!(!integers(a, b, OsStr::new("-ne")).unwrap()); + assert!( + !compare_integer_operands(Operand::Value(a), Operand::Value(b), BinaryOp::IntNe) + .unwrap() + ); } /// The 71-digit operand reported in the GNU compatibility issue, which is @@ -585,23 +625,79 @@ mod tests { let smaller = OsStr::new(SMALLER); let one = OsStr::new("1"); - assert!(integers(big, big, OsStr::new("-eq")).unwrap()); - assert!(!integers(big, big, OsStr::new("-ne")).unwrap()); - assert!(integers(big, big, OsStr::new("-ge")).unwrap()); - assert!(integers(big, big, OsStr::new("-le")).unwrap()); + assert!( + compare_integer_operands(Operand::Value(big), Operand::Value(big), BinaryOp::IntEq) + .unwrap() + ); + assert!( + !compare_integer_operands(Operand::Value(big), Operand::Value(big), BinaryOp::IntNe) + .unwrap() + ); + assert!( + compare_integer_operands(Operand::Value(big), Operand::Value(big), BinaryOp::IntGe) + .unwrap() + ); + assert!( + compare_integer_operands(Operand::Value(big), Operand::Value(big), BinaryOp::IntLe) + .unwrap() + ); - assert!(integers(one, big, OsStr::new("-ne")).unwrap()); - assert!(integers(one, big, OsStr::new("-lt")).unwrap()); - assert!(integers(big, one, OsStr::new("-gt")).unwrap()); + assert!( + compare_integer_operands(Operand::Value(one), Operand::Value(big), BinaryOp::IntNe) + .unwrap() + ); + assert!( + compare_integer_operands(Operand::Value(one), Operand::Value(big), BinaryOp::IntLt) + .unwrap() + ); + assert!( + compare_integer_operands(Operand::Value(big), Operand::Value(one), BinaryOp::IntGt) + .unwrap() + ); // Same width, differing only in the least significant digit. - assert!(integers(big_plus_one, big, OsStr::new("-gt")).unwrap()); - assert!(integers(big, big_plus_one, OsStr::new("-lt")).unwrap()); - assert!(!integers(big, big_plus_one, OsStr::new("-eq")).unwrap()); + assert!( + compare_integer_operands( + Operand::Value(big_plus_one), + Operand::Value(big), + BinaryOp::IntGt + ) + .unwrap() + ); + assert!( + compare_integer_operands( + Operand::Value(big), + Operand::Value(big_plus_one), + BinaryOp::IntLt + ) + .unwrap() + ); + assert!( + !compare_integer_operands( + Operand::Value(big), + Operand::Value(big_plus_one), + BinaryOp::IntEq + ) + .unwrap() + ); // Differing widths. - assert!(integers(big, smaller, OsStr::new("-gt")).unwrap()); - assert!(integers(smaller, big, OsStr::new("-lt")).unwrap()); + assert!( + compare_integer_operands( + Operand::Value(big), + Operand::Value(smaller), + BinaryOp::IntGt + ) + .unwrap() + ); + assert!( + compare_integer_operands( + Operand::Value(smaller), + Operand::Value(big), + BinaryOp::IntLt + ) + .unwrap() + ); } #[test] @@ -612,14 +708,56 @@ mod tests { let neg_smaller = OsStr::new("-1626727727812627722772878217278288262727828288217276267762367276278378"); - assert!(integers(neg_big, neg_big, OsStr::new("-eq")).unwrap()); - assert!(integers(neg_big, OsStr::new("0"), OsStr::new("-lt")).unwrap()); - assert!(integers(neg_big, big, OsStr::new("-lt")).unwrap()); - assert!(integers(big, neg_big, OsStr::new("-gt")).unwrap()); + assert!( + compare_integer_operands( + Operand::Value(neg_big), + Operand::Value(neg_big), + BinaryOp::IntEq + ) + .unwrap() + ); + assert!( + compare_integer_operands( + Operand::Value(neg_big), + Operand::Value(OsStr::new("0")), + BinaryOp::IntLt + ) + .unwrap() + ); + assert!( + compare_integer_operands( + Operand::Value(neg_big), + Operand::Value(big), + BinaryOp::IntLt + ) + .unwrap() + ); + assert!( + compare_integer_operands( + Operand::Value(big), + Operand::Value(neg_big), + BinaryOp::IntGt + ) + .unwrap() + ); // A wider negative number is the smaller of the two. - assert!(integers(neg_big, neg_smaller, OsStr::new("-lt")).unwrap()); - assert!(integers(neg_smaller, neg_big, OsStr::new("-gt")).unwrap()); + assert!( + compare_integer_operands( + Operand::Value(neg_big), + Operand::Value(neg_smaller), + BinaryOp::IntLt + ) + .unwrap() + ); + assert!( + compare_integer_operands( + Operand::Value(neg_smaller), + Operand::Value(neg_big), + BinaryOp::IntGt + ) + .unwrap() + ); } #[test] @@ -674,11 +812,21 @@ mod tests { ] { let operand = OsStr::new(operand); assert!( - integers(operand, OsStr::new("0"), OsStr::new("-eq")).is_err(), + compare_integer_operands( + Operand::Value(operand), + Operand::Value(OsStr::new("0")), + BinaryOp::IntEq, + ) + .is_err(), "{operand:?} should not parse as an integer" ); assert!( - integers(OsStr::new("0"), operand, OsStr::new("-eq")).is_err(), + compare_integer_operands( + Operand::Value(OsStr::new("0")), + Operand::Value(operand), + BinaryOp::IntEq, + ) + .is_err(), "{operand:?} should not parse as an integer" ); } diff --git a/tests/by-util/test_test.rs b/tests/by-util/test_test.rs index 63f66ccdcbc..c0326a7465f 100644 --- a/tests/by-util/test_test.rs +++ b/tests/by-util/test_test.rs @@ -77,8 +77,11 @@ fn test_not_and_is_false() { } #[test] -fn test_not_and_not_succeeds() { - new_ucmd!().args(&["!", "-a", "!"]).succeeds(); +fn test_not_and_not_is_a_syntax_error() { + new_ucmd!() + .args(&["!", "-a", "!"]) + .fails_with_code(2) + .stderr_contains("'-a': unary operator expected"); } #[test] @@ -98,6 +101,18 @@ fn test_errors_miss_and_or() { .stderr_contains("'-a': unary operator expected"); } +#[test] +fn test_unknown_two_byte_operator_errors() { + new_ucmd!() + .args(&["-Q", "x"]) + .fails_with_code(2) + .stderr_contains("'-Q': unary operator expected"); + new_ucmd!() + .args(&["x", "-Q", "y"]) + .fails_with_code(2) + .stderr_contains("'-Q': binary operator expected"); +} + #[test] fn test_negated_or() { new_ucmd!() @@ -254,6 +269,40 @@ fn test_some_int_compares() { } } +#[test] +fn test_integer_length_operands() { + new_ucmd!().args(&["-l", "abc", "-eq", "3"]).succeeds(); + new_ucmd!().args(&["3", "-eq", "-l", "abc"]).succeeds(); + new_ucmd!().args(&["-l", "abc", "-ne", "4"]).succeeds(); +} + +#[test] +fn test_file_operator_rejects_length_operand() { + new_ucmd!() + .args(&["-l", "a", "-nt", "b"]) + .fails_with_code(2) + .stderr_contains("-nt does not accept -l"); +} + +#[test] +fn test_tty_out_of_range_is_false() { + new_ucmd!() + .args(&["-t", "999999999999999999999999999999999999"]) + .fails_with_code(1); +} + +#[test] +fn test_and_or_do_not_short_circuit() { + new_ucmd!() + .args(&["", "-a", "1", "-eq", "bad"]) + .fails_with_code(2) + .stderr_contains("invalid integer 'bad'"); + new_ucmd!() + .args(&["x", "-o", "1", "-eq", "bad"]) + .fails_with_code(2) + .stderr_contains("invalid integer 'bad'"); +} + #[test] fn test_values_greater_than_i64_allowed() { new_ucmd!() From 4eb783defd822295a2735b478352264d234676de Mon Sep 17 00:00:00 2001 From: "Tom D." <15268361+anastygnome@users.noreply.github.com> Date: Wed, 30 Sep 2026 06:40:57 +0200 Subject: [PATCH 2/3] test: re-enable tests --- tests/by-util/test_test.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/by-util/test_test.rs b/tests/by-util/test_test.rs index c0326a7465f..d12eaadd40c 100644 --- a/tests/by-util/test_test.rs +++ b/tests/by-util/test_test.rs @@ -190,12 +190,11 @@ fn test_string_comparison() { } #[test] -#[ignore = "fixme: error reporting"] fn test_dangling_string_comparison_is_error() { new_ucmd!() .args(&["missing_something", "="]) .fails_with_code(2) - .stderr_is("test: missing argument after '='"); + .stderr_is("test: missing argument after '='\n"); } #[test] @@ -1164,7 +1163,6 @@ fn test_inverted_parenthetical_bool_op_precedence() { } #[test] -#[ignore = "fixme: error reporting"] fn test_dangling_parenthesis() { new_ucmd!() .args(&["(", "(", "a", "!=", "b", ")", "-o", "-n", "c"]) @@ -1200,7 +1198,6 @@ fn test_or_as_filename() { } #[test] -#[ignore = "TODO: Busybox has this working"] fn test_filename_or_with_equal() { new_ucmd!().args(&["-f", "=", "a", "-o", "b"]).succeeds(); } From ea352eba2e925ff312c49177e2aa2759b5525d74 Mon Sep 17 00:00:00 2001 From: "Tom D." <15268361+anastygnome@users.noreply.github.com> Date: Thu, 1 Oct 2026 10:33:33 +0200 Subject: [PATCH 3/3] test: fix effective access check --- src/uu/test/Cargo.toml | 2 +- src/uu/test/src/faccessat.rs | 86 ++++++++++++++++++++++++++ src/uu/test/src/test.rs | 115 ++++++++++++++++++++--------------- 3 files changed, 154 insertions(+), 49 deletions(-) create mode 100644 src/uu/test/src/faccessat.rs diff --git a/src/uu/test/Cargo.toml b/src/uu/test/Cargo.toml index eec13996903..c901e47d892 100644 --- a/src/uu/test/Cargo.toml +++ b/src/uu/test/Cargo.toml @@ -22,7 +22,7 @@ thiserror = { workspace = true } uucore = { workspace = true, features = ["fs", "i18n-collator", "wide"] } [target.'cfg(not(any(windows, target_os = "wasi")))'.dependencies] -rustix = { workspace = true, features = ["process"] } +rustix = { workspace = true, features = ["fs", "process"] } [target.'cfg(not(windows))'.dependencies] libc = { workspace = true } diff --git a/src/uu/test/src/faccessat.rs b/src/uu/test/src/faccessat.rs new file mode 100644 index 00000000000..3f634681abf --- /dev/null +++ b/src/uu/test/src/faccessat.rs @@ -0,0 +1,86 @@ +// This file is part of the uutils coreutils package. +// +// For the full copyright and license information, please view the LICENSE +// file that was distributed with this source code. + +// spell-checker:ignore (vars) egid euid accessat EACCESS OPNOTSUPP NOSYS XOTH XGRP XUSR +use rustix::fs::Access; +use rustix::io; +#[cfg(not(any(windows, target_os = "wasi")))] +use rustix::process::{getegid, geteuid}; +use std::ffi::OsStr; +use std::fs::OpenOptions; + +/// Check `-r`, `-w` and `-x` with the process's effective credentials. +/// +/// Let the OS account for supplementary groups, ACLs and privileged users. +#[cfg(not(any(windows, target_os = "wasi")))] +pub(crate) fn effective_access(path: &OsStr, access: Access) -> bool { + use rustix::fs::{AtFlags, CWD, accessat}; + + #[cfg(target_os = "android")] + let flags = AtFlags::empty(); + + #[cfg(not(target_os = "android"))] + let flags = AtFlags::EACCESS; + + match accessat(CWD, path, access, flags) { + Ok(()) => true, + Err(io::Errno::NOSYS) | Err(io::Errno::OPNOTSUPP) => { + effective_access_fallback(path, access) + } + Err(_) => false, + } +} +fn effective_access_fallback(path: &OsStr, access: Access) -> bool { + use rustix::fs::Mode; + use rustix::fs::stat; + use rustix::process::getgroups; + if access.contains(Access::EXEC_OK) { + let Ok(st) = stat(path) else { + return false; + }; + + let mode = Mode::from_raw_mode(st.st_mode); + let euid = geteuid().as_raw(); + let egid = getegid().as_raw(); + + let executable = if euid == 0 { + mode.intersects(Mode::XUSR | Mode::XGRP | Mode::XOTH) + } else if st.st_uid == euid { + mode.contains(Mode::XUSR) + } else { + let in_group = st.st_gid == egid + || getgroups() + .is_ok_and(|groups| groups.iter().any(|gid| gid.as_raw() == st.st_gid)); + + if in_group { + mode.contains(Mode::XGRP) + } else { + mode.contains(Mode::XOTH) + } + }; + + if !executable { + return false; + } + } + + if access.intersects(Access::READ_OK | Access::WRITE_OK) { + let mut options = OpenOptions::new(); + + options + .read(access.contains(Access::READ_OK)) + .write(access.contains(Access::WRITE_OK)); + + if options.open(path).is_err() { + return false; + } + } + + if access == Access::EXISTS { + return stat(path).is_ok(); + } + + true +} diff --git a/src/uu/test/src/test.rs b/src/uu/test/src/test.rs index 8bdaef6871d..d6537b2c50a 100644 --- a/src/uu/test/src/test.rs +++ b/src/uu/test/src/test.rs @@ -3,14 +3,19 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. -// spell-checker:ignore (vars) egid euid +// spell-checker:ignore (vars) egid euid faccessat mod diagnostics; pub(crate) mod error; +#[cfg(not(any(windows, target_os = "wasi")))] +mod faccessat; mod parser; + #[cfg(any(windows, target_os = "wasi"))] mod platform; +#[cfg(not(any(windows, target_os = "wasi")))] +use crate::faccessat::effective_access; use clap::Command; use error::{ParseError, ParseErrorKind, ParseResult}; use parser::{BinaryOp, Evaluator, Operand, UnaryOp, evaluate}; @@ -19,6 +24,8 @@ use platform::fd_is_terminal; #[cfg(target_os = "wasi")] use platform::path; #[cfg(not(any(windows, target_os = "wasi")))] +use rustix::fs::Access; +#[cfg(not(any(windows, target_os = "wasi")))] use rustix::process::{getegid, geteuid}; use std::cmp::Ordering; use std::ffi::{OsStr, OsString}; @@ -29,6 +36,8 @@ use std::os::unix::fs::MetadataExt; use uucore::display::Quotable; use uucore::error::{UResult, USimpleError}; use uucore::format_usage; +#[cfg(not(any(windows, target_os = "wasi")))] +use uucore::fs::mode::{S_ISGID, S_ISUID, S_ISVTX}; use uucore::i18n::collator::{init_locale_collation, locale_cmp}; use uucore::translate; @@ -311,7 +320,7 @@ fn compare_integer_operands(lhs: Operand<'_>, rhs: Operand<'_>, op: BinaryOp) -> /// Operations to compare files metadata /// `a` is the left hand side /// `b` is the right hand side -/// `op` the operation (ex: -ef, -nt, etc) +/// `op` the operation (ex: -ef, -nt, etc.) fn files(a: &OsStr, b: &OsStr, op: &OsStr) -> ParseResult { let f_a = fs::metadata(a); let f_b = fs::metadata(b); @@ -402,63 +411,52 @@ pub(crate) fn modified_since_read(metadata: &fs::Metadata) -> bool { #[cfg(not(any(windows, target_os = "wasi")))] fn path(path: &OsStr, condition: &PathCondition) -> bool { - use std::fs::Metadata; use std::os::unix::fs::FileTypeExt; - const S_ISUID: u32 = 0o4000; - const S_ISGID: u32 = 0o2000; - const S_ISVTX: u32 = 0o1000; - - enum Permission { - Read = 0o4, - Write = 0o2, - Execute = 0o1, - } - - let perm = |metadata: Metadata, p: Permission| { - if geteuid().as_raw() == metadata.uid() { - metadata.mode() & ((p as u32) << 6) != 0 - } else if getegid().as_raw() == metadata.gid() { - metadata.mode() & ((p as u32) << 3) != 0 + let metadata = || { + if matches!(condition, PathCondition::SymLink) { + fs::symlink_metadata(path) } else { - metadata.mode() & (p as u32) != 0 + fs::metadata(path) } }; - let metadata = if condition == &PathCondition::SymLink { - fs::symlink_metadata(path) - } else { - fs::metadata(path) - }; + match condition { + PathCondition::Readable => effective_access(path, Access::READ_OK), + PathCondition::Writable => effective_access(path, Access::WRITE_OK), + PathCondition::Executable => effective_access(path, Access::EXEC_OK), - let Ok(metadata) = metadata else { - return false; - }; + PathCondition::BlockSpecial => metadata().is_ok_and(|m| m.file_type().is_block_device()), - let file_type = metadata.file_type(); + PathCondition::CharacterSpecial => metadata().is_ok_and(|m| m.file_type().is_char_device()), - match condition { - PathCondition::BlockSpecial => file_type.is_block_device(), - PathCondition::CharacterSpecial => file_type.is_char_device(), - PathCondition::Directory => file_type.is_dir(), - PathCondition::Exists => true, - PathCondition::ExistsModifiedLastRead => modified_since_read(&metadata), - PathCondition::Regular => file_type.is_file(), - PathCondition::GroupIdFlag => metadata.mode() & S_ISGID != 0, - PathCondition::GroupOwns => metadata.gid() == getegid().as_raw(), - PathCondition::SymLink => metadata.file_type().is_symlink(), - PathCondition::Sticky => metadata.mode() & S_ISVTX != 0, - PathCondition::UserOwns => metadata.uid() == geteuid().as_raw(), - PathCondition::Fifo => file_type.is_fifo(), - PathCondition::Readable => perm(metadata, Permission::Read), - PathCondition::Socket => file_type.is_socket(), - PathCondition::NonEmpty => metadata.size() > 0, - PathCondition::UserIdFlag => metadata.mode() & S_ISUID != 0, - PathCondition::Writable => perm(metadata, Permission::Write), - PathCondition::Executable => perm(metadata, Permission::Execute), + PathCondition::Directory => metadata().is_ok_and(|m| m.file_type().is_dir()), + + PathCondition::Exists => metadata().is_ok(), + + PathCondition::ExistsModifiedLastRead => metadata().is_ok_and(|m| modified_since_read(&m)), + + PathCondition::Regular => metadata().is_ok_and(|m| m.file_type().is_file()), + + PathCondition::GroupIdFlag => metadata().is_ok_and(|m| m.mode() & S_ISGID != 0), + + PathCondition::GroupOwns => metadata().is_ok_and(|m| m.gid() == getegid().as_raw()), + + PathCondition::SymLink => metadata().is_ok_and(|m| m.file_type().is_symlink()), + + PathCondition::Sticky => metadata().is_ok_and(|m| m.mode() & S_ISVTX != 0), + + PathCondition::UserOwns => metadata().is_ok_and(|m| m.uid() == geteuid().as_raw()), + + PathCondition::Fifo => metadata().is_ok_and(|m| m.file_type().is_fifo()), + + PathCondition::Socket => metadata().is_ok_and(|m| m.file_type().is_socket()), + + PathCondition::NonEmpty => metadata().is_ok_and(|m| m.size() > 0), + + PathCondition::UserIdFlag => metadata().is_ok_and(|m| m.mode() & S_ISUID != 0), } } - #[cfg(windows)] fn path(path: &OsStr, condition: &PathCondition) -> bool { use crate::platform::{is_executable, is_readable, is_writable, owned_by_current_token}; @@ -503,6 +501,27 @@ mod tests { use std::{ffi::OsStr, time::UNIX_EPOCH}; use tempfile::NamedTempFile; + #[cfg(target_os = "linux")] + #[test] + fn test_root_access_is_not_owner_mode_bits() { + use std::os::unix::fs::PermissionsExt; + + if geteuid().as_raw() != 0 { + return; + } + + let file = NamedTempFile::new().unwrap(); + fs::set_permissions(file.path(), fs::Permissions::from_mode(0o000)).unwrap(); + let path = file.path().as_os_str(); + + assert!(effective_access(path, Access::READ_OK)); + assert!(effective_access(path, Access::WRITE_OK)); + assert!(!effective_access(path, Access::EXEC_OK)); + + fs::set_permissions(file.path(), fs::Permissions::from_mode(0o001)).unwrap(); + assert!(effective_access(path, Access::EXEC_OK)); + } + #[test] fn test_files_with_unknown_op() { let a = NamedTempFile::new().unwrap();