Repository: reubeno/brush
Pull request #466 (2025-05-06 04:02:19)

fix(builtin): fix issues with 'builtin' invoking declaration builtins
Testing `zoxide` uncovered that the `builtin` builtin wasn't correctly wrapping inner builtins that take declarations (e.g., `declare`). This adds some tests to expose that--and a few other tests--and fixes them by adding a new helper that wraps executing `builtin`. Along the way, some of the command execution code benefited from some minimal simplification.

The logic that specially handles declaration arguments is getting increasingly... special. The next time we find ourselves updating it, we should take a hard look at whether there's a cleaner approach.

github-actions[bot]: # Performance Benchmark Report
| Benchmark name                         | Baseline (μs) |  Test/PR (μs) |    Delta (μs) | Delta %         |
| -------------------------------------- | ------------- | ------------- | ------------- | --------------- |
| `clone_shell_object                  ` | `   17.86 μs` | `   17.98 μs` | `    0.12 μs` | `⚪  Unchanged` |
| `eval_arithmetic                     ` | `    0.17 μs` | `    0.17 μs` | `   -0.00 μs` | `⚪  Unchanged` |
| `expand_one_string                   ` | `    1.86 μs` | `    1.81 μs` | `   -0.05 μs` | `⚪  Unchanged` |
| `for_loop                            ` | `   22.32 μs` | `   23.43 μs` | `    1.12 μs` | `🟠 +5.01%    ` |
| `function_call                       ` | `    2.29 μs` | `    2.39 μs` | `    0.10 μs` | `⚪  Unchanged` |
| `instantiate_shell                   ` | `   57.36 μs` | `   57.74 μs` | `    0.38 μs` | `⚪  Unchanged` |
| `instantiate_shell_with_init_scripts ` | `24003.53 μs` | `23921.40 μs` | `  -82.14 μs` | `⚪  Unchanged` |
| `parse_bash_completion               ` | ` 1662.85 μs` | ` 1665.28 μs` | `    2.43 μs` | `⚪  Unchanged` |
| `parse_sample_script                 ` | `    1.73 μs` | `    1.79 μs` | `    0.06 μs` | `🟠 +3.64%    ` |
| `run_echo_builtin_command            ` | `   15.45 μs` | `   15.81 μs` | `    0.36 μs` | `⚪  Unchanged` |
| `run_one_external_command            ` | ` 6978.65 μs` | ` 4255.09 μs` | `-2723.55 μs` | `🟢 -39.03%   ` |
| `tokenize_sample_script              ` | `    2.86 μs` | `    2.87 μs` | `    0.01 μs` | `⚪  Unchanged` |

# Code Coverage Report: Only Changed Files listed

| Package | Base Coverage | New Coverage | Difference |
| ------- | ------------- | ------------ | ---------- |
| brush-core/src/builtins/builtin_.rs | 🟢 100% | 🟢 96.15% | 🔴 -3.85% |
| brush-core/src/builtins/factory.rs | 🟢 94.12% | 🟢 95.2% | 🟢 1.08% |
| brush-core/src/interp.rs | 🟢 91.06% | 🟢 91.13% | 🟢 0.07% |
| brush-core/src/options.rs | 🟢 93.48% | 🟢 92.39% | 🔴 -1.09% |
| **Overall Coverage** | **🟢 76.53%** | **🟢 76.58%** | **🟢 0.05%** |

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

# Test Summary: bash-completion test suite
| Outcome            | Count                   | Percentage |
| ------------------ | ----------------------: | ---------: |
| ✅ Pass            | 1310            | <span style='color:green'>62.11</span> |
| ❗️ Error           | 231           | <span style='color:red'>10.95</span> |
| ❌ Fail            | 186            | <span style='color:red'>8.82</span> |
| ⏩ Skip            | 367            | 17.40 |
| ❎ Expected Fail   | 14   | 0.66 |
| ✔️ Unexpected Pass | 1 | <span style='color:red'>0.05</span> |
| 📊 Total           | 2109           | 100.00 |

<!-- Sticky Pull Request Comment -->

Patch:
diff --git a/brush-core/src/builtins/builtin_.rs b/brush-core/src/builtins/builtin_.rs
index a6fbd42..8106906 100644
--- a/brush-core/src/builtins/builtin_.rs
+++ b/brush-core/src/builtins/builtin_.rs
@@ -4,14 +4,16 @@ use std::io::Write;
 use crate::{builtins, commands};
 
 /// Directly invokes a built-in, without going through typical search order.
-#[derive(Parser)]
+#[derive(Default, Parser)]
 pub(crate) struct BuiltinCommand {
-    /// Name of built-in to invoke.
-    builtin_name: Option<String>,
+    #[clap(skip)]
+    args: Vec<commands::CommandArg>,
+}
 
-    /// Arguments for the built-in.
-    #[arg(trailing_var_arg = true, allow_hyphen_values = true)]
-    args: Vec<String>,
+impl builtins::DeclarationCommand for BuiltinCommand {
+    fn set_declarations(&mut self, args: Vec<commands::CommandArg>) {
+        self.args = args;
+    }
 }
 
 impl builtins::Command for BuiltinCommand {
@@ -19,23 +21,26 @@ impl builtins::Command for BuiltinCommand {
         &self,
         mut context: commands::ExecutionContext<'_>,
     ) -> Result<crate::builtins::ExitCode, crate::error::Error> {
-        if let Some(builtin_name) = &self.builtin_name {
-            if let Some(builtin) = context.shell.builtins.get(builtin_name) {
-                context.command_name.clone_from(builtin_name);
-
-                let args: Vec<commands::CommandArg> = std::iter::once(builtin_name.into())
-                    .chain(self.args.iter().map(|arg| arg.into()))
-                    .collect();
-
-                (builtin.execute_func)(context, args)
-                    .await
-                    .map(|res: builtins::BuiltinResult| res.exit_code)
-            } else {
-                writeln!(context.stderr(), "{builtin_name}: command not found")?;
-                Ok(builtins::ExitCode::Custom(1))
-            }
+        if self.args.is_empty() {
+            return Ok(builtins::ExitCode::Success);
+        }
+
+        let args: Vec<_> = self.args.iter().skip(1).cloned().collect();
+        if args.is_empty() {
+            return Ok(builtins::ExitCode::Success);
+        }
+
+        let builtin_name = args[0].to_string();
+
+        if let Some(builtin) = context.shell.builtins.get(&builtin_name) {
+            context.command_name = builtin_name;
+
+            (builtin.execute_func)(context, args)
+                .await
+                .map(|res: builtins::BuiltinResult| res.exit_code)
         } else {
-            Ok(builtins::ExitCode::Success)
+            writeln!(context.stderr(), "{builtin_name}: command not found")?;
+            Ok(builtins::ExitCode::Custom(1))
         }
     }
 }
diff --git a/brush-core/src/builtins/factory.rs b/brush-core/src/builtins/factory.rs
index 6b18e5a..d105a86 100644
--- a/brush-core/src/builtins/factory.rs
+++ b/brush-core/src/builtins/factory.rs
@@ -59,6 +59,23 @@ fn decl_builtin<B: builtins::DeclarationCommand + Send + Sync>() -> builtins::Re
     }
 }
 
+/// Returns a built-in command registration, given an implementation of the
+/// `DeclarationCommand` trait that can be default-constructed. The command
+/// implementation is expected to implement clap's `Parser` trait solely
+/// for help/usage information. Arguments are passed directly to the command
+/// via `set_declarations`. This is primarily only expected to be used with
+/// select builtin commands that wrap other builtins (e.g., "builtin").
+fn raw_arg_builtin<B: builtins::DeclarationCommand + Default + Send + Sync>(
+) -> builtins::Registration {
+    builtins::Registration {
+        execute_func: exec_raw_arg_builtin::<B>,
+        content_func: get_builtin_content::<B>,
+        disabled: false,
+        special_builtin: false,
+        declaration_builtin: true,
+    }
+}
+
 fn get_builtin_content<T: builtins::Command + Send + Sync>(
     name: &str,
     content_type: builtins::ContentType,
@@ -159,6 +176,25 @@ async fn exec_declaration_builtin_impl<T: builtins::DeclarationCommand + Send +
     })
 }
 
+fn exec_raw_arg_builtin<T: builtins::DeclarationCommand + Default + Send + Sync>(
+    context: commands::ExecutionContext<'_>,
+    args: Vec<CommandArg>,
+) -> BoxFuture<'_, Result<builtins::BuiltinResult, error::Error>> {
+    Box::pin(async move { exec_raw_arg_builtin_impl::<T>(context, args).await })
+}
+
+async fn exec_raw_arg_builtin_impl<T: builtins::DeclarationCommand + Default + Send + Sync>(
+    context: commands::ExecutionContext<'_>,
+    args: Vec<CommandArg>,
+) -> Result<builtins::BuiltinResult, error::Error> {
+    let mut command = T::default();
+    command.set_declarations(args);
+
+    Ok(builtins::BuiltinResult {
+        exit_code: command.execute(context).await?,
+    })
+}
+
 #[allow(clippy::too_many_lines)]
 pub(crate) fn get_default_builtins(
     options: &crate::CreateOptions,
@@ -236,7 +272,10 @@ pub(crate) fn get_default_builtins(
     m.insert("ulimit".into(), builtin::<unimp::UnimplementedCommand>());
 
     if !options.sh_mode {
-        m.insert("builtin".into(), builtin::<builtin_::BuiltinCommand>());
+        m.insert(
+            "builtin".into(),
+            raw_arg_builtin::<builtin_::BuiltinCommand>(),
+        );
         m.insert("declare".into(), decl_builtin::<declare::DeclareCommand>());
         m.insert("echo".into(), builtin::<echo::EchoCommand>());
         m.insert("enable".into(), builtin::<enable::EnableCommand>());
diff --git a/brush-core/src/interp.rs b/brush-core/src/interp.rs
index 212e69d..8aa2a39 100644
--- a/brush-core/src/interp.rs
+++ b/brush-core/src/interp.rs
@@ -926,30 +926,21 @@ impl ExecuteInPipeline for ast::SimpleCommand {
         context: &mut PipelineExecutionContext<'_>,
         mut params: ExecutionParameters,
     ) -> Result<CommandSpawnResult, error::Error> {
-        let default_prefix = ast::CommandPrefix::default();
-        let prefix_items = self.prefix.as_ref().unwrap_or(&default_prefix);
+        let prefix_iter = self.prefix.as_ref().map(|s| s.0.iter()).unwrap_or_default();
+        let suffix_iter = self.suffix.as_ref().map(|s| s.0.iter()).unwrap_or_default();
+        let cmd_name_items = self
+            .word_or_name
+            .as_ref()
+            .map(|won| CommandPrefixOrSuffixItem::Word(won.clone()));
 
-        let default_suffix = ast::CommandSuffix::default();
-        let suffix_items = self.suffix.as_ref().unwrap_or(&default_suffix);
-
-        let mut cmd_name_items = vec![];
-        if let Some(cmd_name) = &self.word_or_name {
-            cmd_name_items.push(CommandPrefixOrSuffixItem::Word(cmd_name.clone()));
-        }
+        // Set up pipelining.
+        setup_pipeline_redirection(&mut params.open_files, context)?;
 
         let mut assignments = vec![];
         let mut args: Vec<CommandArg> = vec![];
-        let mut invoking_declaration_builtin = false;
+        let mut command_takes_assignments = false;
 
-        // Set up pipelining.
-        setup_pipeline_redirection(&mut params.open_files, context)?;
-
-        for item in prefix_items
-            .0
-            .iter()
-            .chain(cmd_name_items.iter())
-            .chain(suffix_items.0.iter())
-        {
+        for item in prefix_iter.chain(cmd_name_items.iter()).chain(suffix_iter) {
             match item {
                 CommandPrefixOrSuffixItem::IoRedirect(redirect) => {
                     match setup_redirect(context.shell, &mut params, redirect).await {
@@ -987,7 +978,7 @@ impl ExecuteInPipeline for ast::SimpleCommand {
                         // scoped assignment. Add it to the list we're accumulating.
                         assignments.push(assignment);
                     } else {
-                        if invoking_declaration_builtin {
+                        if command_takes_assignments {
                             // This looks like an assignment, and the command being invoked is a
                             // well-known builtin that takes arguments that need to function like
                             // assignments (but which are processed by the builtin).
@@ -1031,16 +1022,17 @@ impl ExecuteInPipeline for ast::SimpleCommand {
                                 next_args = alias_pieces;
                             }
 
+                            let first_arg = next_args[0].as_str();
+
                             // Check if we're going to be invoking a special declaration builtin.
-                            // That will change how we parse and process
-                            // args.
+                            // That will change how we parse and process args.
                             if context
                                 .shell
                                 .builtins
-                                .get(next_args[0].as_str())
-                                .is_some_and(|r| r.declaration_builtin)
+                                .get(first_arg)
+                                .is_some_and(|r| !r.disabled && r.declaration_builtin)
                             {
-                                invoking_declaration_builtin = true;
+                                command_takes_assignments = true;
                             }
                         }
                     }
@@ -1053,91 +1045,7 @@ impl ExecuteInPipeline for ast::SimpleCommand {
 
         // If we have a command, then execute it.
         if let Some(CommandArg::String(cmd_name)) = args.first().cloned() {
-            // Push a new ephemeral environment scope for the duration of the command. We'll
-            // set command-scoped variable assignments after doing so, and revert them before
-            // returning.
-            context.shell.env.push_scope(EnvironmentScope::Command);
-            for assignment in &assignments {
-                // Ensure it's tagged as exported and created in the command scope.
-                apply_assignment(
-                    assignment,
-                    context.shell,
-                    &params,
-                    true,
-                    Some(EnvironmentScope::Command),
-                    EnvironmentScope::Command,
-                )
-                .await?;
-            }
-
-            if context.shell.options.print_commands_and_arguments {
-                context
-                    .shell
-                    .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
-            // helper.
-            if context.shell.traps.handler_depth == 0 {
-                let debug_trap_handler = context
-                    .shell
-                    .traps
-                    .handlers
-                    .get(&traps::TrapSignal::Debug)
-                    .cloned();
-                if let Some(debug_trap_handler) = debug_trap_handler {
-                    // TODO: Confirm whether trap handlers should be executed in the same process
-                    // group.
-                    let handler_params = ExecutionParameters {
-                        open_files: params.open_files.clone(),
-                        process_group_policy: ProcessGroupPolicy::SameProcessGroup,
-                    };
-
-                    let full_cmd = args.iter().map(|arg| arg.to_string()).join(" ");
-
-                    // TODO: This shouldn't *just* be set in a trap situation.
-                    context.shell.env.update_or_add(
-                        "BASH_COMMAND",
-                        ShellValueLiteral::Scalar(full_cmd),
-                        |_| Ok(()),
-                        EnvironmentLookup::Anywhere,
-                        EnvironmentScope::Global,
-                    )?;
-
-                    context.shell.traps.handler_depth += 1;
-
-                    // TODO: Discard result?
-                    let _ = context
-                        .shell
-                        .run_string(debug_trap_handler, &handler_params)
-                        .await?;
-
-                    context.shell.traps.handler_depth -= 1;
-                }
-            }
-
-            let cmd_context = commands::ExecutionContext {
-                shell: context.shell,
-                command_name: cmd_name,
-                params,
-            };
-
-            // Execute.
-            let execution_result = commands::execute(
-                cmd_context,
-                &mut context.process_group_id,
-                args,
-                true, /* use functions? */
-                None,
-            )
-            .await;
-
-            // Pop off that ephemeral environment scope.
-            // TODO: jobs: do we need to move self back to foreground on error here?
-            context.shell.env.pop_scope(EnvironmentScope::Command)?;
-
-            execution_result
+            execute_command(context, params, cmd_name, assignments, args).await
         } else {
             // Reset last status.
             context.shell.last_exit_status = 0;
@@ -1164,6 +1072,100 @@ impl ExecuteInPipeline for ast::SimpleCommand {
     }
 }
 
+async fn execute_command(
+    context: &mut PipelineExecutionContext<'_>,
+    params: ExecutionParameters,
+    cmd_name: String,
+    assignments: Vec<&ast::Assignment>,
+    args: Vec<CommandArg>,
+) -> Result<CommandSpawnResult, error::Error> {
+    // Push a new ephemeral environment scope for the duration of the command. We'll
+    // set command-scoped variable assignments after doing so, and revert them before
+    // returning.
+    context.shell.env.push_scope(EnvironmentScope::Command);
+    for assignment in &assignments {
+        // Ensure it's tagged as exported and created in the command scope.
+        apply_assignment(
+            assignment,
+            context.shell,
+            &params,
+            true,
+            Some(EnvironmentScope::Command),
+            EnvironmentScope::Command,
+        )
+        .await?;
+    }
+
+    if context.shell.options.print_commands_and_arguments {
+        context
+            .shell
+            .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
+    // helper.
+    if context.shell.traps.handler_depth == 0 {
+        let debug_trap_handler = context
+            .shell
+            .traps
+            .handlers
+            .get(&traps::TrapSignal::Debug)
+            .cloned();
+        if let Some(debug_trap_handler) = debug_trap_handler {
+            // TODO: Confirm whether trap handlers should be executed in the same process
+            // group.
+            let handler_params = ExecutionParameters {
+                open_files: params.open_files.clone(),
+                process_group_policy: ProcessGroupPolicy::SameProcessGroup,
+            };
+
+            let full_cmd = args.iter().map(|arg| arg.to_string()).join(" ");
+
+            // TODO: This shouldn't *just* be set in a trap situation.
+            context.shell.env.update_or_add(
+                "BASH_COMMAND",
+                ShellValueLiteral::Scalar(full_cmd),
+                |_| Ok(()),
+                EnvironmentLookup::Anywhere,
+                EnvironmentScope::Global,
+            )?;
+
+            context.shell.traps.handler_depth += 1;
+
+            // TODO: Discard result?
+            let _ = context
+                .shell
+                .run_string(debug_trap_handler, &handler_params)
+                .await?;
+
+            context.shell.traps.handler_depth -= 1;
+        }
+    }
+
+    let cmd_context = commands::ExecutionContext {
+        shell: context.shell,
+        command_name: cmd_name,
+        params,
+    };
+
+    // Execute.
+    let execution_result = commands::execute(
+        cmd_context,
+        &mut context.process_group_id,
+        args,
+        true, /* use functions? */
+        None,
+    )
+    .await;
+
+    // Pop off that ephemeral environment scope.
+    // TODO: jobs: do we need to move self back to foreground on error here?
+    context.shell.env.pop_scope(EnvironmentScope::Command)?;
+
+    execution_result
+}
+
 async fn expand_assignment(
     shell: &mut Shell,
     params: &ExecutionParameters,


Test patch:
diff --git a/brush-shell/tests/cases/builtins/builtin.yaml b/brush-shell/tests/cases/builtins/builtin.yaml
index c344d4c..6f29bb6 100644
--- a/brush-shell/tests/cases/builtins/builtin.yaml
+++ b/brush-shell/tests/cases/builtins/builtin.yaml
@@ -18,8 +18,11 @@ cases:
       builtin false; echo "builtin false => $?"
       builtin true; echo "builtin true => $?"
 
+  - name: "builtin with non-decl builtin"
+    stdin: |
+      builtin echo variable=value
+
   - name: "builtin with decl builtin"
-    known_failure: true # Needs investigation
     stdin: |
       builtin declare variable=value
       declare -p variable