From 2670885b4fbbbd303d92b5c6f54e9de6072dfe1b Mon Sep 17 00:00:00 2001 From: Dorian Peron Date: Sat, 22 Feb 2025 01:11:06 +0100 Subject: [PATCH 1/3] expr: Use thiserror to be consistent with error definition in other uutils --- Cargo.lock | 1 + src/uu/expr/Cargo.toml | 1 + src/uu/expr/src/expr.rs | 58 +++++++++++------------------------------ 3 files changed, 17 insertions(+), 43 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9f4a57963..120647be1 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 1abf853d7..a16c37a6b 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 47fdb2a4e..dd2b36f74 100644 --- a/src/uu/expr/src/expr.rs +++ b/src/uu/expr/src/expr.rs @@ -3,10 +3,9 @@ // 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 thiserror::Error; use uucore::{ display::Quotable, error::{UError, UResult}, @@ -25,63 +24,36 @@ mod options { pub type ExprResult = Result; -#[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 From 4513b58e590748cfcf0197ef1fbf4de1c9b08e37 Mon Sep 17 00:00:00 2001 From: Dorian Peron Date: Sat, 22 Feb 2025 04:17:36 +0100 Subject: [PATCH 2/3] test(expr): Add test for eager evaluation --- tests/by-util/test_expr.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/by-util/test_expr.rs b/tests/by-util/test_expr.rs index 5cbf91e0c..3e35887c4 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"); +} From 2fc762b9d2676a8dd852c1a86d09253c88b6cddf Mon Sep 17 00:00:00 2001 From: Dorian Peron Date: Sat, 22 Feb 2025 04:52:06 +0100 Subject: [PATCH 3/3] expr: Evaluate parenthesis content before checking for closing parenthesis --- src/uu/expr/src/expr.rs | 4 +--- src/uu/expr/src/syntax_tree.rs | 21 ++++++++++++++++++--- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/src/uu/expr/src/expr.rs b/src/uu/expr/src/expr.rs index dd2b36f74..4b9f6b9d6 100644 --- a/src/uu/expr/src/expr.rs +++ b/src/uu/expr/src/expr.rs @@ -4,7 +4,7 @@ // file that was distributed with this source code. use clap::{crate_version, Arg, ArgAction, Command}; -use syntax_tree::AstNode; +use syntax_tree::{is_truthy, AstNode}; use thiserror::Error; use uucore::{ display::Quotable, @@ -12,8 +12,6 @@ use uucore::{ format_usage, help_about, help_section, help_usage, }; -use crate::syntax_tree::is_truthy; - mod syntax_tree; mod options { diff --git a/src/uu/expr/src/syntax_tree.rs b/src/uu/expr/src/syntax_tree.rs index 0288a6736..008cd5307 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 { + Ok(Self::Evaluated { + value: self.eval()?, + }) + } + pub fn eval(&self) -> ExprResult { 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" )) );