Merge pull request #2513 from drocco007/test-parentheses

test: handle additional parentheses edge cases

src/uu/test/src/parser.rs

@@ -10,6 +10,21 @@
 use std::ffi::OsString;
 use std::iter::Peekable;
 
+/// Represents one of the binary comparison operators for strings, integers, or files
+#[derive(Debug, PartialEq)]
+pub enum Operator {
+    String(OsString),
+    Int(OsString),
+    File(OsString),
+}
+
+/// Represents one of the unary test operators for strings or files
+#[derive(Debug, PartialEq)]
+pub enum UnaryOperator {
+    StrlenOp(OsString),
+    FiletestOp(OsString),
+}
+
 /// Represents a parsed token from a test expression
 #[derive(Debug, PartialEq)]
 pub enum Symbol {
@@ -17,11 +32,8 @@ pub enum Symbol {
     Bang,
     BoolOp(OsString),
     Literal(OsString),
-    StringOp(OsString),
+    Op(Operator),
-    IntOp(OsString),
+    UnaryOp(UnaryOperator),
-    FileOp(OsString),
-    StrlenOp(OsString),
-    FiletestOp(OsString),
     None,
 }
 
@@ -35,12 +47,14 @@ impl Symbol {
                 "(" => Symbol::LParen,
                 "!" => Symbol::Bang,
                 "-a" | "-o" => Symbol::BoolOp(s),
-                "=" | "==" | "!=" => Symbol::StringOp(s),
+                "=" | "==" | "!=" => Symbol::Op(Operator::String(s)),
-                "-eq" | "-ge" | "-gt" | "-le" | "-lt" | "-ne" => Symbol::IntOp(s),
+                "-eq" | "-ge" | "-gt" | "-le" | "-lt" | "-ne" => Symbol::Op(Operator::Int(s)),
-                "-ef" | "-nt" | "-ot" => Symbol::FileOp(s),
+                "-ef" | "-nt" | "-ot" => Symbol::Op(Operator::File(s)),
-                "-n" | "-z" => Symbol::StrlenOp(s),
+                "-n" | "-z" => Symbol::UnaryOp(UnaryOperator::StrlenOp(s)),
                 "-b" | "-c" | "-d" | "-e" | "-f" | "-g" | "-G" | "-h" | "-k" | "-L" | "-O"
-                | "-p" | "-r" | "-s" | "-S" | "-t" | "-u" | "-w" | "-x" => Symbol::FiletestOp(s),
+                | "-p" | "-r" | "-s" | "-S" | "-t" | "-u" | "-w" | "-x" => {
+                    Symbol::UnaryOp(UnaryOperator::FiletestOp(s))
+                }
                 _ => Symbol::Literal(s),
             },
             None => Symbol::None,
@@ -60,11 +74,11 @@ impl Symbol {
             Symbol::Bang => OsString::from("!"),
             Symbol::BoolOp(s)
             | Symbol::Literal(s)
-            | Symbol::StringOp(s)
+            | Symbol::Op(Operator::String(s))
-            | Symbol::IntOp(s)
+            | Symbol::Op(Operator::Int(s))
-            | Symbol::FileOp(s)
+            | Symbol::Op(Operator::File(s))
-            | Symbol::StrlenOp(s)
+            | Symbol::UnaryOp(UnaryOperator::StrlenOp(s))
-            | Symbol::FiletestOp(s) => s,
+            | Symbol::UnaryOp(UnaryOperator::FiletestOp(s)) => s,
             Symbol::None => panic!(),
         })
     }
@@ -78,7 +92,6 @@ impl Symbol {
 ///
 ///   EXPR → TERM | EXPR BOOLOP EXPR
 ///   TERM → ( EXPR )
-///   TERM → ( )
 ///   TERM → ! EXPR
 ///   TERM → UOP str
 ///   UOP → STRLEN | FILETEST
@@ -113,6 +126,20 @@ impl Parser {
         Symbol::new(self.tokens.next())
     }
 
+    /// 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) {
+        match self.next_token() {
+            Symbol::Literal(s) if s == value => (),
+            _ => panic!("expected ‘{}’", value),
+        }
+    }
+
     /// 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()`.
@@ -144,8 +171,7 @@ impl Parser {
         match symbol {
             Symbol::LParen => self.lparen(),
             Symbol::Bang => self.bang(),
-            Symbol::StrlenOp(_) => self.uop(symbol),
+            Symbol::UnaryOp(_) => self.uop(symbol),
-            Symbol::FiletestOp(_) => self.uop(symbol),
             Symbol::None => self.stack.push(symbol),
             literal => self.literal(literal),
         }
@@ -154,21 +180,75 @@ impl Parser {
     /// Parse a (possibly) parenthesized expression.
     ///
     /// test has no reserved keywords, so "(" will be interpreted as a literal
-    /// if it is followed by nothing or a comparison operator OP.
+    /// 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) {
-        match self.peek() {
+        // Look ahead up to 3 tokens to determine if the lparen is being used
-            // lparen is a literal when followed by nothing or comparison
+        // as a grouping operator or should be treated as a literal string
-            Symbol::None | Symbol::StringOp(_) | Symbol::IntOp(_) | Symbol::FileOp(_) => {
+        let peek3: Vec<Symbol> = self
+            .tokens
+            .clone()
+            .take(3)
+            .map(|token| Symbol::new(Some(token)))
+            .collect();
+
+        match peek3.as_slice() {
+            // case 1: lparen is a literal when followed by nothing
+            [] => self.literal(Symbol::LParen.into_literal()),
+
+            // case 2: error if end of stream is `( <any_token>`
+            [symbol] => {
+                eprintln!("test: missing argument after ‘{:?}’", symbol);
+                std::process::exit(2);
+            }
+
+            // case 3: `( uop <any_token> )` → 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(")");
+            }
+
+            // 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());
             }
-            // empty parenthetical
+
-            Symbol::Literal(s) if s == ")" => {}
+            // 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(")");
+            }
+
+            // 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(")");
+            }
+
+            // case 7: if earlier cases didn’t match, `( op <any_token>…`
+            //         indicates binary comparison of literal lparen with
+            //         anything _except_ ")" (case 4)
+            [Symbol::Op(_), _] | [Symbol::Op(_), _, _] => {
+                self.literal(Symbol::LParen.into_literal());
+            }
+
+            // Otherwise, lparen indicates the start of a parenthesized
+            // expression
             _ => {
                 self.expr();
-                match self.next_token() {
+                self.expect(")");
-                    Symbol::Literal(s) if s == ")" => (),
-                    _ => panic!("expected ')'"),
-                }
             }
         }
     }
@@ -192,7 +272,7 @@ impl Parser {
     ///
     fn bang(&mut self) {
         match self.peek() {
-            Symbol::StringOp(_) | Symbol::IntOp(_) | Symbol::FileOp(_) | Symbol::BoolOp(_) => {
+            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));
@@ -200,7 +280,7 @@ impl Parser {
                 match peek2 {
                     // case 1: `! <OP as literal>`
                     // case 3: `! = OP str`
-                    Symbol::StringOp(_) | Symbol::None => {
+                    Symbol::Op(_) | Symbol::None => {
                         // op is literal
                         let op = self.next_token().into_literal();
                         self.literal(op);
@@ -293,8 +373,7 @@ impl Parser {
         self.stack.push(token.into_literal());
 
         // EXPR → str OP str
-        match self.peek() {
+        if let Symbol::Op(_) = self.peek() {
-            Symbol::StringOp(_) | Symbol::IntOp(_) | Symbol::FileOp(_) => {
             let op = self.next_token();
 
             match self.next_token() {
@@ -304,8 +383,6 @@ impl Parser {
 
             self.stack.push(op);
         }
-            _ => {}
-        }
     }
 
     /// Parser entry point: parse the token stream `self.tokens`, storing the

src/uu/test/src/test.rs

@@ -11,7 +11,7 @@
 mod parser;
 
 use clap::{crate_version, App, AppSettings};
-use parser::{parse, Symbol};
+use parser::{parse, Operator, Symbol, UnaryOperator};
 use std::ffi::{OsStr, OsString};
 use std::path::Path;
 
@@ -159,19 +159,19 @@ fn eval(stack: &mut Vec<Symbol>) -> Result<bool, String> {
 
             Ok(!result)
         }
-        Some(Symbol::StringOp(op)) => {
+        Some(Symbol::Op(Operator::String(op))) => {
             let b = stack.pop();
             let a = stack.pop();
             Ok(if op == "!=" { a != b } else { a == b })
         }
-        Some(Symbol::IntOp(op)) => {
+        Some(Symbol::Op(Operator::Int(op))) => {
             let b = pop_literal!();
             let a = pop_literal!();
 
             Ok(integers(&a, &b, &op)?)
         }
-        Some(Symbol::FileOp(_op)) => unimplemented!(),
+        Some(Symbol::Op(Operator::File(_op))) => unimplemented!(),
-        Some(Symbol::StrlenOp(op)) => {
+        Some(Symbol::UnaryOp(UnaryOperator::StrlenOp(op))) => {
             let s = match stack.pop() {
                 Some(Symbol::Literal(s)) => s,
                 Some(Symbol::None) => OsString::from(""),
@@ -189,7 +189,7 @@ fn eval(stack: &mut Vec<Symbol>) -> Result<bool, String> {
                 !s.is_empty()
             })
         }
-        Some(Symbol::FiletestOp(op)) => {
+        Some(Symbol::UnaryOp(UnaryOperator::FiletestOp(op))) => {
             let op = op.to_string_lossy();
 
             let f = pop_literal!();

tests/by-util/test_test.rs

@@ -35,6 +35,35 @@ fn test_solo_and_or_or_is_a_literal() {
     new_ucmd!().arg("-o").succeeds();
 }
 
+#[test]
+fn test_some_literals() {
+    let scenario = TestScenario::new(util_name!());
+    let tests = [
+        "a string",
+        "(",
+        ")",
+        "-",
+        "--",
+        "-0",
+        "-f",
+        "--help",
+        "--version",
+        "-eq",
+        "-lt",
+        "-ef",
+        "[",
+    ];
+
+    for test in &tests {
+        scenario.ucmd().arg(test).succeeds();
+    }
+
+    // run the inverse of all these tests
+    for test in &tests {
+        scenario.ucmd().arg("!").arg(test).run().status_code(1);
+    }
+}
+
 #[test]
 fn test_double_not_is_false() {
     new_ucmd!().args(&["!", "!"]).run().status_code(1);
@@ -99,21 +128,6 @@ fn test_zero_len_of_empty() {
     new_ucmd!().args(&["-z", ""]).succeeds();
 }
 
-#[test]
-fn test_solo_parenthesis_is_literal() {
-    let scenario = TestScenario::new(util_name!());
-    let tests = [["("], [")"]];
-
-    for test in &tests {
-        scenario.ucmd().args(&test[..]).succeeds();
-    }
-}
-
-#[test]
-fn test_solo_empty_parenthetical_is_error() {
-    new_ucmd!().args(&["(", ")"]).run().status_code(2);
-}
-
 #[test]
 fn test_zero_len_equals_zero_len() {
     new_ucmd!().args(&["", "=", ""]).succeeds();
@@ -139,6 +153,7 @@ fn test_string_comparison() {
         ["contained\nnewline", "=", "contained\nnewline"],
         ["(", "=", "("],
         ["(", "!=", ")"],
+        ["(", "!=", "="],
         ["!", "=", "!"],
         ["=", "=", "="],
     ];
@@ -199,11 +214,13 @@ fn test_a_bunch_of_not() {
 
 #[test]
 fn test_pseudofloat_equal() {
+    // string comparison; test(1) doesn't support comparison of actual floats
     new_ucmd!().args(&["123.45", "=", "123.45"]).succeeds();
 }
 
 #[test]
 fn test_pseudofloat_not_equal() {
+    // string comparison; test(1) doesn't support comparison of actual floats
     new_ucmd!().args(&["123.45", "!=", "123.450"]).succeeds();
 }
 
@@ -230,6 +247,16 @@ fn test_some_int_compares() {
     for test in &tests {
         scenario.ucmd().args(&test[..]).succeeds();
     }
+
+    // run the inverse of all these tests
+    for test in &tests {
+        scenario
+            .ucmd()
+            .arg("!")
+            .args(&test[..])
+            .run()
+            .status_code(1);
+    }
 }
 
 #[test]
@@ -257,6 +284,16 @@ fn test_negative_int_compare() {
     for test in &tests {
         scenario.ucmd().args(&test[..]).succeeds();
     }
+
+    // run the inverse of all these tests
+    for test in &tests {
+        scenario
+            .ucmd()
+            .arg("!")
+            .args(&test[..])
+            .run()
+            .status_code(1);
+    }
 }
 
 #[test]
@@ -499,6 +536,93 @@ fn test_file_is_not_sticky() {
         .status_code(1);
 }
 
+#[test]
+fn test_solo_empty_parenthetical_is_error() {
+    new_ucmd!().args(&["(", ")"]).run().status_code(2);
+}
+
+#[test]
+fn test_parenthesized_literal() {
+    let scenario = TestScenario::new(util_name!());
+    let tests = [
+        "a string",
+        "(",
+        ")",
+        "-",
+        "--",
+        "-0",
+        "-f",
+        "--help",
+        "--version",
+        "-e",
+        "-t",
+        "!",
+        "-n",
+        "-z",
+        "[",
+        "-a",
+        "-o",
+    ];
+
+    for test in &tests {
+        scenario.ucmd().arg("(").arg(test).arg(")").succeeds();
+    }
+
+    // run the inverse of all these tests
+    for test in &tests {
+        scenario
+            .ucmd()
+            .arg("!")
+            .arg("(")
+            .arg(test)
+            .arg(")")
+            .run()
+            .status_code(1);
+    }
+}
+
+#[test]
+fn test_parenthesized_op_compares_literal_parenthesis() {
+    // ensure we aren’t treating this case as “string length of literal equal
+    // sign”
+    new_ucmd!().args(&["(", "=", ")"]).run().status_code(1);
+}
+
+#[test]
+fn test_parenthesized_string_comparison() {
+    let scenario = TestScenario::new(util_name!());
+    let tests = [
+        ["(", "foo", "!=", "bar", ")"],
+        ["(", "contained\nnewline", "=", "contained\nnewline", ")"],
+        ["(", "(", "=", "(", ")"],
+        ["(", "(", "!=", ")", ")"],
+        ["(", "!", "=", "!", ")"],
+        ["(", "=", "=", "=", ")"],
+    ];
+
+    for test in &tests {
+        scenario.ucmd().args(&test[..]).succeeds();
+    }
+
+    // run the inverse of all these tests
+    for test in &tests {
+        scenario
+            .ucmd()
+            .arg("!")
+            .args(&test[..])
+            .run()
+            .status_code(1);
+    }
+}
+
+#[test]
+fn test_parenthesized_right_parenthesis_as_literal() {
+    new_ucmd!()
+        .args(&["(", "-f", ")", ")"])
+        .run()
+        .status_code(1);
+}
+
 #[test]
 #[cfg(not(windows))]
 fn test_file_owned_by_euid() {