Repository: reubeno/brush
Pull request #324 (2025-01-13 16:24:28)

chore: remove unneeded string cloning for arithmetic eval
When we have a string that needs to be interpreted as an unexpanded arithmetic expression, there's no reason to wrap it in a struct that insists on an owned copy; we can directly pass through the `&str` to the core logic that's performing expansion and evaluation.

github-actions[bot]: # Performance Benchmark Report
| Benchmark name                         | Baseline (μs) |  Test/PR (μs) |    Delta (μs) | Delta %         |
| -------------------------------------- | ------------- | ------------- | ------------- | --------------- |
| `clone_shell_object                  ` | `   16.46 μs` | `   16.45 μs` | `   -0.01 μs` | `⚪  Unchanged` |
| `eval_arithmetic                     ` | `    0.19 μs` | `    0.18 μs` | `   -0.01 μs` | `⚪  Unchanged` |
| `expand_one_string                   ` | `    1.60 μs` | `    1.64 μs` | `    0.04 μs` | `⚪  Unchanged` |
| `for_loop                            ` | `   34.10 μs` | `   32.56 μs` | `   -1.54 μs` | `🟢 -4.52%    ` |
| `function_call                       ` | `    3.50 μs` | `    3.28 μs` | `   -0.21 μs` | `⚪  Unchanged` |
| `instantiate_shell                   ` | `   47.98 μs` | `   47.50 μs` | `   -0.48 μs` | `⚪  Unchanged` |
| `instantiate_shell_with_init_scripts ` | `22678.80 μs` | `21941.94 μs` | ` -736.86 μs` | `🟢 -3.25%    ` |
| `parse_bash_completion               ` | ` 1663.53 μs` | ` 1666.17 μs` | `    2.65 μs` | `⚪  Unchanged` |
| `parse_sample_script                 ` | `    1.74 μs` | `    1.80 μs` | `    0.06 μs` | `🟠 +3.50%    ` |
| `run_echo_builtin_command            ` | `   16.57 μs` | `   15.88 μs` | `   -0.70 μs` | `⚪  Unchanged` |
| `run_one_external_command            ` | ` 2077.24 μs` | ` 2092.53 μs` | `   15.28 μs` | `⚪  Unchanged` |
| `tokenize_sample_script              ` | `    2.89 μs` | `    2.77 μs` | `   -0.13 μs` | `🟢 -4.36%    ` |

# Code Coverage Report: Only Changed Files listed

| Package | Base Coverage | New Coverage | Difference |
| ------- | ------------- | ------------ | ---------- |
| brush-core/src/arithmetic.rs | 🟢 98.4% | 🟢 98.46% | 🟢 0.06% |
| brush-core/src/extendedtests.rs | 🟠 70.91% | 🟠 67.35% | 🔴 -3.56% |
| brush-core/src/interp.rs | 🟢 90.76% | 🟢 90.75% | 🔴 -0.01% |
| brush-core/src/shell.rs | 🟢 82.4% | 🟢 82.27% | 🔴 -0.13% |
| **Overall Coverage** | **🟢 76.96%** | **🟢 76.91%** | **🔴 -0.05%** |

_Minimum allowed coverage is_ `70%`_, this run produced_ `76.91%`
<!-- Sticky Pull Request Comment -->
github-actions[bot]: ## Test Results
    2 files      9 suites   1m 24s ⏱️
  587 tests   587 ✅ 0 💤 0 ❌
1 160 runs  1 160 ✅ 0 💤 0 ❌

Results for commit 32128bbb.

[test-results]:data:application/gzip;base64,H4sIAAJBhWcC/13MTQ7CIBhF0a00jB3wAQJ1MwYoJMS2GH5GjXuXEtHW4bkveRtyfrYJ3QZyGVAqPjeMFVOJKvuwVkpWXZe8b1cpuu6pGPOfHv5ZE/4Gp/x8CjbGED8llnX/BOBd/fOUfp/Nh8vm46MJy+JzBaIEiNRaK06J0RYbwSynZgLNGSdyZBScwCDR6w3uGPi3BgEAAA==


Patch:
diff --git a/brush-core/src/arithmetic.rs b/brush-core/src/arithmetic.rs
index 42c315a..ae92d7c 100644
--- a/brush-core/src/arithmetic.rs
+++ b/brush-core/src/arithmetic.rs
@@ -52,25 +52,40 @@ pub trait ExpandAndEvaluate {
 
 impl ExpandAndEvaluate for ast::UnexpandedArithmeticExpr {
     async fn eval(&self, shell: &mut Shell, trace_if_needed: bool) -> Result<i64, EvalError> {
-        // Per documentation, first shell-expand it.
-        let expanded_self = expansion::basic_expand_str_without_tilde(shell, self.value.as_str())
-            .await
-            .map_err(|_e| EvalError::FailedToExpandExpression)?;
-
-        // Now parse.
-        let expr = brush_parser::arithmetic::parse(&expanded_self)
-            .map_err(|_e| EvalError::ParseError(expanded_self))?;
-
-        // Trace if applicable.
-        if trace_if_needed && shell.options.print_commands_and_arguments {
-            shell
-                .trace_command(std::format!("(( {expr} ))"))
-                .map_err(|_err| EvalError::TraceError)?;
-        }
+        expand_and_eval(shell, self.value.as_str(), trace_if_needed).await
+    }
+}
 
-        // Now evaluate.
-        expr.eval(shell)
+/// Evaluate the given arithmetic expression, returning the resulting numeric value.
+///
+/// # Arguments
+///
+/// * `shell` - The shell to use for evaluation.
+/// * `expr` - The unexpanded arithmetic expression to evaluate.
+/// * `trace_if_needed` - Whether to trace the evaluation.
+pub(crate) async fn expand_and_eval(
+    shell: &mut Shell,
+    expr: &str,
+    trace_if_needed: bool,
+) -> Result<i64, EvalError> {
+    // Per documentation, first shell-expand it.
+    let expanded_self = expansion::basic_expand_str_without_tilde(shell, expr)
+        .await
+        .map_err(|_e| EvalError::FailedToExpandExpression)?;
+
+    // Now parse.
+    let expr = brush_parser::arithmetic::parse(&expanded_self)
+        .map_err(|_e| EvalError::ParseError(expanded_self))?;
+
+    // Trace if applicable.
+    if trace_if_needed && shell.options.print_commands_and_arguments {
+        shell
+            .trace_command(std::format!("(( {expr} ))"))
+            .map_err(|_err| EvalError::TraceError)?;
     }
+
+    // Now evaluate.
+    expr.eval(shell)
 }
 
 /// Trait implemented by evaluatable arithmetic expressions.
diff --git a/brush-core/src/expansion.rs b/brush-core/src/expansion.rs
index 6be5910..a2ef88f 100644
--- a/brush-core/src/expansion.rs
+++ b/brush-core/src/expansion.rs
@@ -6,6 +6,7 @@ use brush_parser::word::ParameterTransformOp;
 use brush_parser::word::SubstringMatchKind;
 use itertools::Itertools;
 
+use crate::arithmetic;
 use crate::arithmetic::ExpandAndEvaluate;
 use crate::commands;
 use crate::env;
@@ -1334,10 +1335,9 @@ impl<'a> WordExpander<'a> {
         let index_to_use = if for_set_associative_array {
             self.basic_expand_to_str(index).await?
         } else {
-            let index_expr = ast::UnexpandedArithmeticExpr {
-                value: index.to_owned(),
-            };
-            self.expand_arithmetic_expr(index_expr).await?
+            arithmetic::expand_and_eval(self.shell, index, false)
+                .await?
+                .to_string()
         };
 
         Ok(index_to_use)
diff --git a/brush-core/src/interp.rs b/brush-core/src/interp.rs
index 5cb1dd5..8fc3d64 100644
--- a/brush-core/src/interp.rs
+++ b/brush-core/src/interp.rs
@@ -9,7 +9,7 @@ use std::os::unix::process::ExitStatusExt;
 use std::path::{Path, PathBuf};
 use std::sync::Arc;
 
-use crate::arithmetic::ExpandAndEvaluate;
+use crate::arithmetic::{self, ExpandAndEvaluate};
 use crate::commands::{self, CommandArg, CommandSpawnResult};
 use crate::env::{EnvironmentLookup, EnvironmentScope};
 use crate::openfiles::{OpenFile, OpenFiles};
@@ -1219,8 +1219,7 @@ async fn apply_assignment(
 
         if will_be_indexed_array {
             array_index = Some(
-                ast::UnexpandedArithmeticExpr { value: idx.clone() }
-                    .eval(shell, false)
+                arithmetic::expand_and_eval(shell, idx.as_str(), false)
                     .await?
                     .to_string(),
             );


Test patch:
diff --git a/brush-core/src/extendedtests.rs b/brush-core/src/extendedtests.rs
index 824e9dd..6cc4140 100644
--- a/brush-core/src/extendedtests.rs
+++ b/brush-core/src/extendedtests.rs
@@ -2,8 +2,7 @@ use brush_parser::ast;
 use std::path::Path;
 
 use crate::{
-    arithmetic::ExpandAndEvaluate,
-    env, error, escape, expansion, namedoptions, patterns,
+    arithmetic, env, error, escape, expansion, namedoptions, patterns,
     sys::{
         fs::{MetadataExt, PathExt},
         users,
@@ -269,14 +268,8 @@ async fn apply_binary_predicate(
             Ok(left > right)
         }
         ast::BinaryPredicate::ArithmeticEqualTo => {
-            let unexpanded_left = ast::UnexpandedArithmeticExpr {
-                value: left.value.clone(),
-            };
-            let unexpanded_right = ast::UnexpandedArithmeticExpr {
-                value: right.value.clone(),
-            };
-            let left = unexpanded_left.eval(shell, false).await?;
-            let right = unexpanded_right.eval(shell, false).await?;
+            let left = arithmetic::expand_and_eval(shell, left.value.as_str(), false).await?;
+            let right = arithmetic::expand_and_eval(shell, right.value.as_str(), false).await?;
 
             if shell.options.print_commands_and_arguments {
                 shell.trace_command(std::format!("[[ {left} {op} {right} ]]"))?;
@@ -285,14 +278,8 @@ async fn apply_binary_predicate(
             Ok(left == right)
         }
         ast::BinaryPredicate::ArithmeticNotEqualTo => {
-            let unexpanded_left = ast::UnexpandedArithmeticExpr {
-                value: left.value.clone(),
-            };
-            let unexpanded_right = ast::UnexpandedArithmeticExpr {
-                value: right.value.clone(),
-            };
-            let left = unexpanded_left.eval(shell, false).await?;
-            let right = unexpanded_right.eval(shell, false).await?;
+            let left = arithmetic::expand_and_eval(shell, left.value.as_str(), false).await?;
+            let right = arithmetic::expand_and_eval(shell, right.value.as_str(), false).await?;
 
             if shell.options.print_commands_and_arguments {
                 shell.trace_command(std::format!("[[ {left} {op} {right} ]]"))?;
@@ -301,14 +288,8 @@ async fn apply_binary_predicate(
             Ok(left != right)
         }
         ast::BinaryPredicate::ArithmeticLessThan => {
-            let unexpanded_left = ast::UnexpandedArithmeticExpr {
-                value: left.value.clone(),
-            };
-            let unexpanded_right = ast::UnexpandedArithmeticExpr {
-                value: right.value.clone(),
-            };
-            let left = unexpanded_left.eval(shell, false).await?;
-            let right = unexpanded_right.eval(shell, false).await?;
+            let left = arithmetic::expand_and_eval(shell, left.value.as_str(), false).await?;
+            let right = arithmetic::expand_and_eval(shell, right.value.as_str(), false).await?;
 
             if shell.options.print_commands_and_arguments {
                 shell.trace_command(std::format!("[[ {left} {op} {right} ]]"))?;
@@ -317,14 +298,8 @@ async fn apply_binary_predicate(
             Ok(left < right)
         }
         ast::BinaryPredicate::ArithmeticLessThanOrEqualTo => {
-            let unexpanded_left = ast::UnexpandedArithmeticExpr {
-                value: left.value.clone(),
-            };
-            let unexpanded_right = ast::UnexpandedArithmeticExpr {
-                value: right.value.clone(),
-            };
-            let left = unexpanded_left.eval(shell, false).await?;
-            let right = unexpanded_right.eval(shell, false).await?;
+            let left = arithmetic::expand_and_eval(shell, left.value.as_str(), false).await?;
+            let right = arithmetic::expand_and_eval(shell, right.value.as_str(), false).await?;
 
             if shell.options.print_commands_and_arguments {
                 shell.trace_command(std::format!("[[ {left} {op} {right} ]]"))?;
@@ -333,14 +308,8 @@ async fn apply_binary_predicate(
             Ok(left <= right)
         }
         ast::BinaryPredicate::ArithmeticGreaterThan => {
-            let unexpanded_left = ast::UnexpandedArithmeticExpr {
-                value: left.value.clone(),
-            };
-            let unexpanded_right = ast::UnexpandedArithmeticExpr {
-                value: right.value.clone(),
-            };
-            let left = unexpanded_left.eval(shell, false).await?;
-            let right = unexpanded_right.eval(shell, false).await?;
+            let left = arithmetic::expand_and_eval(shell, left.value.as_str(), false).await?;
+            let right = arithmetic::expand_and_eval(shell, right.value.as_str(), false).await?;
 
             if shell.options.print_commands_and_arguments {
                 shell.trace_command(std::format!("[[ {left} {op} {right} ]]"))?;
@@ -349,14 +318,8 @@ async fn apply_binary_predicate(
             Ok(left > right)
         }
         ast::BinaryPredicate::ArithmeticGreaterThanOrEqualTo => {
-            let unexpanded_left = ast::UnexpandedArithmeticExpr {
-                value: left.value.clone(),
-            };
-            let unexpanded_right = ast::UnexpandedArithmeticExpr {
-                value: right.value.clone(),
-            };
-            let left = unexpanded_left.eval(shell, false).await?;
-            let right = unexpanded_right.eval(shell, false).await?;
+            let left = arithmetic::expand_and_eval(shell, left.value.as_str(), false).await?;
+            let right = arithmetic::expand_and_eval(shell, right.value.as_str(), false).await?;
 
             if shell.options.print_commands_and_arguments {
                 shell.trace_command(std::format!("[[ {left} {op} {right} ]]"))?;