Repository: reubeno/brush
Pull request #396 (2025-03-07 20:39:32)

Expand contents of PS4 when tracing
This PR calls `expand_prompt_var()` on the contents of PS4 within `trace_command()`.  The addition necessitates `trace_command()` become `async` and calls to it use `await`.  A new test case checks expansion against an oracle.

github-actions[bot]: # Performance Benchmark Report
| Benchmark name                         | Baseline (μs) |  Test/PR (μs) |    Delta (μs) | Delta %         |
| -------------------------------------- | ------------- | ------------- | ------------- | --------------- |
| `clone_shell_object                  ` | `   18.16 μs` | `   18.05 μs` | `   -0.11 μs` | `⚪  Unchanged` |
| `eval_arithmetic                     ` | `    0.17 μs` | `    0.17 μs` | `    0.00 μs` | `⚪  Unchanged` |
| `expand_one_string                   ` | `    1.83 μs` | `    1.81 μs` | `   -0.02 μs` | `⚪  Unchanged` |
| `for_loop                            ` | `   21.80 μs` | `   22.18 μs` | `    0.37 μs` | `🟠 +1.71%    ` |
| `function_call                       ` | `    2.30 μs` | `    2.26 μs` | `   -0.04 μs` | `⚪  Unchanged` |
| `instantiate_shell                   ` | `   57.96 μs` | `   57.11 μs` | `   -0.85 μs` | `🟢 -1.47%    ` |
| `instantiate_shell_with_init_scripts ` | `23755.33 μs` | `23692.26 μs` | `  -63.07 μs` | `⚪  Unchanged` |
| `parse_bash_completion               ` | ` 1637.16 μs` | ` 1642.73 μs` | `    5.57 μs` | `⚪  Unchanged` |
| `parse_sample_script                 ` | `    1.73 μs` | `    1.72 μs` | `   -0.01 μs` | `⚪  Unchanged` |
| `run_echo_builtin_command            ` | `   15.26 μs` | `   15.39 μs` | `    0.13 μs` | `⚪  Unchanged` |
| `run_one_external_command            ` | ` 2143.41 μs` | ` 4536.47 μs` | ` 2393.06 μs` | `🟠 +111.65%  ` |
| `tokenize_sample_script              ` | `    2.78 μs` | `    2.81 μs` | `    0.03 μs` | `🟠 +1.19%    ` |

# Code Coverage Report: Only Changed Files listed

| Package | Base Coverage | New Coverage | Difference |
| ------- | ------------- | ------------ | ---------- |
| brush-core/src/arithmetic.rs | 🟢 98.52% | 🟢 98.53% | 🟢 0.01% |
| brush-core/src/extendedtests.rs | 🟠 66.46% | 🟠 61.97% | 🔴 -4.49% |
| brush-core/src/interp.rs | 🟢 91.68% | 🟢 91.57% | 🔴 -0.11% |
| brush-core/src/jobs.rs | 🔴 37.07% | 🔴 43.1% | 🟢 6.03% |
| brush-core/src/shell.rs | 🟢 85.83% | 🟢 85.87% | 🟢 0.04% |
| **Overall Coverage** | **🟢 78.28%** | **🟢 78.25%** | **🔴 -0.03%** |

_Minimum allowed coverage is_ `70%`_, this run produced_ `78.25%`

# Test Summary: bash-completion test suite
| Outcome            | Count                   | Percentage |
| ------------------ | ----------------------: | ---------: |
| ✅ Pass            | 1327            | <span style='color:green'>63.19</span> |
| ❗️ Error           | 240           | <span style='color:red'>11.43</span> |
| ❌ Fail            | 158            | <span style='color:red'>7.52</span> |
| ⏩ Skip            | 360            | 17.14 |
| ❎ Expected Fail   | 14   | 0.67 |
| ✔️ Unexpected Pass | 1 | <span style='color:red'>0.05</span> |
| 📊 Total           | 2100           | 100.00 |

<!-- Sticky Pull Request Comment -->

Patch:
diff --git a/brush-core/src/arithmetic.rs b/brush-core/src/arithmetic.rs
index ccd0852..cb89ac5 100644
--- a/brush-core/src/arithmetic.rs
+++ b/brush-core/src/arithmetic.rs
@@ -92,6 +92,7 @@ pub(crate) async fn expand_and_eval(
     if trace_if_needed && shell.options.print_commands_and_arguments {
         shell
             .trace_command(std::format!("(( {expr} ))"))
+            .await
             .map_err(|_err| EvalError::TraceError)?;
     }
 
diff --git a/brush-core/src/interp.rs b/brush-core/src/interp.rs
index afc77b1..f64f33d 100644
--- a/brush-core/src/interp.rs
+++ b/brush-core/src/interp.rs
@@ -609,13 +609,17 @@ impl Execute for ast::ForClauseCommand {
         for value in expanded_values {
             if shell.options.print_commands_and_arguments {
                 if let Some(unexpanded_values) = &self.values {
-                    shell.trace_command(std::format!(
-                        "for {} in {}",
-                        self.variable_name,
-                        unexpanded_values.iter().join(" ")
-                    ))?;
+                    shell
+                        .trace_command(std::format!(
+                            "for {} in {}",
+                            self.variable_name,
+                            unexpanded_values.iter().join(" ")
+                        ))
+                        .await?;
                 } else {
-                    shell.trace_command(std::format!("for {}", self.variable_name,))?;
+                    shell
+                        .trace_command(std::format!("for {}", self.variable_name,))
+                        .await?;
                 }
             }
 
@@ -666,7 +670,9 @@ impl Execute for ast::CaseClauseCommand {
         // N.B. One would think it makes sense to trace the expanded value being switched
         // on, but that's not it.
         if shell.options.print_commands_and_arguments {
-            shell.trace_command(std::format!("case {} in", &self.value))?;
+            shell
+                .trace_command(std::format!("case {} in", &self.value))
+                .await?;
         }
 
         let expanded_value = expansion::basic_expand_word(shell, params, &self.value).await?;
@@ -1054,7 +1060,8 @@ impl ExecuteInPipeline for ast::SimpleCommand {
             if context.shell.options.print_commands_and_arguments {
                 context
                     .shell
-                    .trace_command(args.iter().map(|arg| arg.quote_for_tracing()).join(" "))?;
+                    .trace_command(args.iter().map(|arg| arg.quote_for_tracing()).join(" "))
+                    .await?;
             }
 
             // TODO: This is adding more complexity here; should be factored out into an appropriate
@@ -1273,7 +1280,9 @@ async fn apply_assignment(
 
     if shell.options.print_commands_and_arguments {
         let op = if assignment.append { "+=" } else { "=" };
-        shell.trace_command(std::format!("{}{op}{new_value}", assignment.name))?;
+        shell
+            .trace_command(std::format!("{}{op}{new_value}", assignment.name))
+            .await?;
     }
 
     // See if we need to eval an array index.
diff --git a/brush-core/src/shell.rs b/brush-core/src/shell.rs
index 00624f0..bb6eaca 100644
--- a/brush-core/src/shell.rs
+++ b/brush-core/src/shell.rs
@@ -1506,8 +1506,11 @@ impl Shell {
     /// # Arguments
     ///
     /// * `command` - The command to trace.
-    pub(crate) fn trace_command<S: AsRef<str>>(&self, command: S) -> Result<(), std::io::Error> {
-        let ps4 = self.get_env_str("PS4").unwrap_or_else(|| "".into());
+    pub(crate) async fn trace_command<S: AsRef<str>>(
+        &mut self,
+        command: S,
+    ) -> Result<(), error::Error> {
+        let ps4 = self.as_mut().expand_prompt_var("PS4", "").await?;
 
         let mut prefix = ps4.to_string();
 
@@ -1518,7 +1521,8 @@ impl Shell {
             }
         }
 
-        writeln!(self.stderr(), "{prefix}{}", command.as_ref())
+        writeln!(self.stderr(), "{prefix}{}", command.as_ref())?;
+        Ok(())
     }
 
     /// Returns the keywords that are reserved by the shell.


Test patch:
diff --git a/brush-core/src/extendedtests.rs b/brush-core/src/extendedtests.rs
index 224f197..ba10429 100644
--- a/brush-core/src/extendedtests.rs
+++ b/brush-core/src/extendedtests.rs
@@ -54,10 +54,12 @@ async fn apply_unary_predicate(
     let expanded_operand = expansion::basic_expand_word(shell, params, operand).await?;
 
     if shell.options.print_commands_and_arguments {
-        shell.trace_command(std::format!(
-            "[[ {op} {} ]]",
-            escape::quote_if_needed(&expanded_operand, escape::QuoteMode::SingleQuote)
-        ))?;
+        shell
+            .trace_command(std::format!(
+                "[[ {op} {} ]]",
+                escape::quote_if_needed(&expanded_operand, escape::QuoteMode::SingleQuote)
+            ))
+            .await?;
     }
 
     apply_unary_predicate_to_str(op, expanded_operand.as_str(), shell, params)
@@ -200,7 +202,9 @@ async fn apply_binary_predicate(
             let regex = expansion::basic_expand_regex(shell, params, right).await?;
 
             if shell.options.print_commands_and_arguments {
-                shell.trace_command(std::format!("[[ {s} {op} {right} ]]"))?;
+                shell
+                    .trace_command(std::format!("[[ {s} {op} {right} ]]"))
+                    .await?;
             }
 
             let (matches, captures) = match regex.matches(s.as_str()) {
@@ -237,7 +241,9 @@ async fn apply_binary_predicate(
             let right = expansion::basic_expand_word(shell, params, right).await?;
 
             if shell.options.print_commands_and_arguments {
-                shell.trace_command(std::format!("[[ {left} {op} {right} ]]"))?;
+                shell
+                    .trace_command(std::format!("[[ {left} {op} {right} ]]"))
+                    .await?;
             }
 
             Ok(left == right)
@@ -247,7 +253,9 @@ async fn apply_binary_predicate(
             let right = expansion::basic_expand_word(shell, params, right).await?;
 
             if shell.options.print_commands_and_arguments {
-                shell.trace_command(std::format!("[[ {left} {op} {right} ]]"))?;
+                shell
+                    .trace_command(std::format!("[[ {left} {op} {right} ]]"))
+                    .await?;
             }
 
             Ok(left != right)
@@ -257,7 +265,9 @@ async fn apply_binary_predicate(
             let substring = expansion::basic_expand_word(shell, params, right).await?;
 
             if shell.options.print_commands_and_arguments {
-                shell.trace_command(std::format!("[[ {s} {op} {substring} ]]"))?;
+                shell
+                    .trace_command(std::format!("[[ {s} {op} {substring} ]]"))
+                    .await?;
             }
 
             Ok(s.contains(substring.as_str()))
@@ -276,7 +286,9 @@ async fn apply_binary_predicate(
             let right = expansion::basic_expand_word(shell, params, right).await?;
 
             if shell.options.print_commands_and_arguments {
-                shell.trace_command(std::format!("[[ {left} {op} {right} ]]"))?;
+                shell
+                    .trace_command(std::format!("[[ {left} {op} {right} ]]"))
+                    .await?;
             }
 
             // TODO: According to docs, should be lexicographical order of the current locale.
@@ -287,7 +299,9 @@ async fn apply_binary_predicate(
             let right = expansion::basic_expand_word(shell, params, right).await?;
 
             if shell.options.print_commands_and_arguments {
-                shell.trace_command(std::format!("[[ {left} {op} {right} ]]"))?;
+                shell
+                    .trace_command(std::format!("[[ {left} {op} {right} ]]"))
+                    .await?;
             }
 
             // TODO: According to docs, should be lexicographical order of the current locale.
@@ -300,7 +314,9 @@ async fn apply_binary_predicate(
                 arithmetic::expand_and_eval(shell, params, right.value.as_str(), false).await?;
 
             if shell.options.print_commands_and_arguments {
-                shell.trace_command(std::format!("[[ {left} {op} {right} ]]"))?;
+                shell
+                    .trace_command(std::format!("[[ {left} {op} {right} ]]"))
+                    .await?;
             }
 
             Ok(left == right)
@@ -312,7 +328,9 @@ async fn apply_binary_predicate(
                 arithmetic::expand_and_eval(shell, params, right.value.as_str(), false).await?;
 
             if shell.options.print_commands_and_arguments {
-                shell.trace_command(std::format!("[[ {left} {op} {right} ]]"))?;
+                shell
+                    .trace_command(std::format!("[[ {left} {op} {right} ]]"))
+                    .await?;
             }
 
             Ok(left != right)
@@ -324,7 +342,9 @@ async fn apply_binary_predicate(
                 arithmetic::expand_and_eval(shell, params, right.value.as_str(), false).await?;
 
             if shell.options.print_commands_and_arguments {
-                shell.trace_command(std::format!("[[ {left} {op} {right} ]]"))?;
+                shell
+                    .trace_command(std::format!("[[ {left} {op} {right} ]]"))
+                    .await?;
             }
 
             Ok(left < right)
@@ -336,7 +356,9 @@ async fn apply_binary_predicate(
                 arithmetic::expand_and_eval(shell, params, right.value.as_str(), false).await?;
 
             if shell.options.print_commands_and_arguments {
-                shell.trace_command(std::format!("[[ {left} {op} {right} ]]"))?;
+                shell
+                    .trace_command(std::format!("[[ {left} {op} {right} ]]"))
+                    .await?;
             }
 
             Ok(left <= right)
@@ -348,7 +370,9 @@ async fn apply_binary_predicate(
                 arithmetic::expand_and_eval(shell, params, right.value.as_str(), false).await?;
 
             if shell.options.print_commands_and_arguments {
-                shell.trace_command(std::format!("[[ {left} {op} {right} ]]"))?;
+                shell
+                    .trace_command(std::format!("[[ {left} {op} {right} ]]"))
+                    .await?;
             }
 
             Ok(left > right)
@@ -360,7 +384,9 @@ async fn apply_binary_predicate(
                 arithmetic::expand_and_eval(shell, params, right.value.as_str(), false).await?;
 
             if shell.options.print_commands_and_arguments {
-                shell.trace_command(std::format!("[[ {left} {op} {right} ]]"))?;
+                shell
+                    .trace_command(std::format!("[[ {left} {op} {right} ]]"))
+                    .await?;
             }
 
             Ok(left >= right)
@@ -382,7 +408,9 @@ async fn apply_binary_predicate(
                     expanded_right.as_str(),
                     escape::QuoteMode::BackslashEscape,
                 );
-                shell.trace_command(std::format!("[[ {s} {op} {escaped_right} ]]"))?;
+                shell
+                    .trace_command(std::format!("[[ {s} {op} {escaped_right} ]]"))
+                    .await?;
             }
 
             pattern.exactly_matches(s.as_str())
@@ -400,7 +428,9 @@ async fn apply_binary_predicate(
                     expanded_right.as_str(),
                     escape::QuoteMode::BackslashEscape,
                 );
-                shell.trace_command(std::format!("[[ {s} {op} {escaped_right} ]]"))?;
+                shell
+                    .trace_command(std::format!("[[ {s} {op} {escaped_right} ]]"))
+                    .await?;
             }
 
             let eq = pattern.exactly_matches(s.as_str())?;
diff --git a/brush-shell/tests/cases/builtins/ps4.yaml b/brush-shell/tests/cases/builtins/ps4.yaml
new file mode 100644
index 0000000..19b6bcd
--- /dev/null
+++ b/brush-shell/tests/cases/builtins/ps4.yaml
@@ -0,0 +1,8 @@
+name: "Builtins: PS4"
+cases:
+  - name: "PS4 expansion"
+    stdin: |
+      PS4='+$FUNCNAME '
+      bar() { true; }
+      foo() { set -x; bar; set +x; }
+      foo