Repository: uutils/coreutils (language: rust)

Issue:
`expr`: evaluation should happen "during" parsing
With https://github.com/uutils/coreutils/pull/5586 merged, we can there are a few issues leftover. Here's an interesting one:
```
> expr '(' 1 / 0
expr: division by zero
```
In uutils `expr`, this currently gives a `syntax error` instead. This is because uutils first parses the entire syntax tree and evaluates it afterwards. GNU does not seem to do that. Instead, they immediately evaluate every subtree they parse. This might actually simplify the code a bit, but will be a pretty big refactor.


Discussion / hints:

Fix patch:
diff --git a/Cargo.lock b/Cargo.lock
index 9f4a5796327..120647be1d2 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -2781,6 +2781,7 @@ dependencies = [
  "num-bigint",
  "num-traits",
  "onig",
+ "thiserror 2.0.11",
  "uucore",
 ]
 
diff --git a/src/uu/expr/Cargo.toml b/src/uu/expr/Cargo.toml
index 1abf853d760..a16c37a6b86 100644
--- a/src/uu/expr/Cargo.toml
+++ b/src/uu/expr/Cargo.toml
@@ -22,6 +22,7 @@ num-bigint = { workspace = true }
 num-traits = { workspace = true }
 onig = { workspace = true }
 uucore = { workspace = true }
+thiserror = { workspace = true }
 
 [[bin]]
 name = "expr"
diff --git a/src/uu/expr/src/expr.rs b/src/uu/expr/src/expr.rs
index 47fdb2a4ea7..4b9f6b9d64c 100644
--- a/src/uu/expr/src/expr.rs
+++ b/src/uu/expr/src/expr.rs
@@ -3,18 +3,15 @@
 // For the full copyright and license information, please view the LICENSE
 // file that was distributed with this source code.
 
-use std::fmt::Display;
-
 use clap::{crate_version, Arg, ArgAction, Command};
-use syntax_tree::AstNode;
+use syntax_tree::{is_truthy, AstNode};
+use thiserror::Error;
 use uucore::{
     display::Quotable,
     error::{UError, UResult},
     format_usage, help_about, help_section, help_usage,
 };
 
-use crate::syntax_tree::is_truthy;
-
 mod syntax_tree;
 
 mod options {
@@ -25,63 +22,36 @@ mod options {
 
 pub type ExprResult<T> = Result<T, ExprError>;
 
-#[derive(Debug, PartialEq, Eq)]
+#[derive(Error, Clone, Debug, PartialEq, Eq)]
 pub enum ExprError {
+    #[error("syntax error: unexpected argument {}", .0.quote())]
     UnexpectedArgument(String),
+    #[error("syntax error: missing argument after {}", .0.quote())]
     MissingArgument(String),
+    #[error("non-integer argument")]
     NonIntegerArgument,
+    #[error("missing operand")]
     MissingOperand,
+    #[error("division by zero")]
     DivisionByZero,
+    #[error("Invalid regex expression")]
     InvalidRegexExpression,
+    #[error("syntax error: expecting ')' after {}", .0.quote())]
     ExpectedClosingBraceAfter(String),
+    #[error("syntax error: expecting ')' instead of {}", .0.quote())]
     ExpectedClosingBraceInsteadOf(String),
+    #[error("Unmatched ( or \\(")]
     UnmatchedOpeningParenthesis,
+    #[error("Unmatched ) or \\)")]
     UnmatchedClosingParenthesis,
+    #[error("Unmatched \\{{")]
     UnmatchedOpeningBrace,
+    #[error("Unmatched ) or \\}}")]
     UnmatchedClosingBrace,
+    #[error("Invalid content of {0}")]
     InvalidContent(String),
 }
 
-impl Display for ExprError {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        match self {
-            Self::UnexpectedArgument(s) => {
-                write!(f, "syntax error: unexpected argument {}", s.quote())
-            }
-            Self::MissingArgument(s) => {
-                write!(f, "syntax error: missing argument after {}", s.quote())
-            }
-            Self::NonIntegerArgument => write!(f, "non-integer argument"),
-            Self::MissingOperand => write!(f, "missing operand"),
-            Self::DivisionByZero => write!(f, "division by zero"),
-            Self::InvalidRegexExpression => write!(f, "Invalid regex expression"),
-            Self::ExpectedClosingBraceAfter(s) => {
-                write!(f, "syntax error: expecting ')' after {}", s.quote())
-            }
-            Self::ExpectedClosingBraceInsteadOf(s) => {
-                write!(f, "syntax error: expecting ')' instead of {}", s.quote())
-            }
-            Self::UnmatchedOpeningParenthesis => {
-                write!(f, "Unmatched ( or \\(")
-            }
-            Self::UnmatchedClosingParenthesis => {
-                write!(f, "Unmatched ) or \\)")
-            }
-            Self::UnmatchedOpeningBrace => {
-                write!(f, "Unmatched \\{{")
-            }
-            Self::UnmatchedClosingBrace => {
-                write!(f, "Unmatched ) or \\}}")
-            }
-            Self::InvalidContent(s) => {
-                write!(f, "Invalid content of {}", s)
-            }
-        }
-    }
-}
-
-impl std::error::Error for ExprError {}
-
 impl UError for ExprError {
     fn code(&self) -> i32 {
         2
diff --git a/src/uu/expr/src/syntax_tree.rs b/src/uu/expr/src/syntax_tree.rs
index 0288a67361b..008cd530745 100644
--- a/src/uu/expr/src/syntax_tree.rs
+++ b/src/uu/expr/src/syntax_tree.rs
@@ -292,7 +292,7 @@ const PRECEDENCE: &[&[(&str, BinOp)]] = &[
     &[(":", BinOp::String(StringOp::Match))],
 ];
 
-#[derive(Debug)]
+#[derive(Debug, Clone, PartialEq, Eq)]
 pub enum NumOrStr {
     Num(BigInt),
     Str(String),
@@ -343,6 +343,9 @@ impl NumOrStr {
 
 #[derive(Debug, PartialEq, Eq)]
 pub enum AstNode {
+    Evaluated {
+        value: NumOrStr,
+    },
     Leaf {
         value: String,
     },
@@ -366,8 +369,15 @@ impl AstNode {
         Parser::new(input).parse()
     }
 
+    pub fn evaluated(self) -> ExprResult<Self> {
+        Ok(Self::Evaluated {
+            value: self.eval()?,
+        })
+    }
+
     pub fn eval(&self) -> ExprResult<NumOrStr> {
         match self {
+            Self::Evaluated { value } => Ok(value.clone()),
             Self::Leaf { value } => Ok(value.to_string().into()),
             Self::BinOp {
                 op_type,
@@ -536,7 +546,10 @@ impl<'a> Parser<'a> {
                 value: self.next()?.into(),
             },
             "(" => {
-                let s = self.parse_expression()?;
+                // Evaluate the node just after parsing to we detect arithmetic
+                // errors before checking for the closing parenthesis.
+                let s = self.parse_expression()?.evaluated()?;
+
                 match self.next() {
                     Ok(")") => {}
                     // Since we have parsed at least a '(', there will be a token
@@ -680,7 +693,9 @@ mod test {
             AstNode::parse(&["(", "1", "+", "2", ")", "*", "3"]),
             Ok(op(
                 BinOp::Numeric(NumericOp::Mul),
-                op(BinOp::Numeric(NumericOp::Add), "1", "2"),
+                op(BinOp::Numeric(NumericOp::Add), "1", "2")
+                    .evaluated()
+                    .unwrap(),
                 "3"
             ))
         );


Test patch:
diff --git a/tests/by-util/test_expr.rs b/tests/by-util/test_expr.rs
index 5cbf91e0c6a..3e35887c468 100644
--- a/tests/by-util/test_expr.rs
+++ b/tests/by-util/test_expr.rs
@@ -370,3 +370,11 @@ fn test_num_str_comparison() {
         .succeeds()
         .stdout_is("1\n");
 }
+
+#[test]
+fn test_eager_evaluation() {
+    new_ucmd!()
+        .args(&["(", "1", "/", "0"])
+        .fails()
+        .stderr_contains("division by zero");
+}