From a76790545044127d1f6443591cc58e2c3ae55f11 Mon Sep 17 00:00:00 2001
From: reuben olinsky <reubeno@users.noreply.github.com>
Date: Thu, 9 Jan 2025 12:25:57 -0800
Subject: [PATCH] perf: reimplement colon command as a "simple builtin" (#315)

---
 brush-core/src/builtins.rs         | 11 ++++++
 brush-core/src/builtins/colon.rs   | 37 +++++++++++--------
 brush-core/src/builtins/factory.rs | 58 ++++++++++++------------------
 brush-core/src/sys/unix/fs.rs      | 14 ++++----
 brush-shell/src/main.rs            |  2 +-
 5 files changed, 64 insertions(+), 58 deletions(-)

diff --git a/brush-core/src/builtins.rs b/brush-core/src/builtins.rs
index ad45ab8a..a5c44337 100644
--- a/brush-core/src/builtins.rs
+++ b/brush-core/src/builtins.rs
@@ -282,6 +282,17 @@ pub struct Registration {
     pub declaration_builtin: bool,
 }
 
+impl Registration {
+    /// Updates the given registration to mark it for a special builtin.
+    #[must_use]
+    pub fn special(self) -> Self {
+        Self {
+            special_builtin: true,
+            ..self
+        }
+    }
+}
+
 fn get_builtin_man_page(_name: &str, _command: &clap::Command) -> Result<String, error::Error> {
     error::unimp("man page rendering is not yet implemented")
 }
diff --git a/brush-core/src/builtins/colon.rs b/brush-core/src/builtins/colon.rs
index 371d1ed0..d754bcd2 100644
--- a/brush-core/src/builtins/colon.rs
+++ b/brush-core/src/builtins/colon.rs
@@ -1,20 +1,29 @@
-use clap::Parser;
-
-use crate::{builtins, commands};
+use crate::{builtins, commands, error};
 
 /// No-op command.
-#[derive(Parser)]
-#[clap(disable_help_flag = true, disable_version_flag = true)]
-pub(crate) struct ColonCommand {
-    #[clap(allow_hyphen_values = true)]
-    args: Vec<String>,
-}
+pub(crate) struct ColonCommand {}
+
+impl builtins::SimpleCommand for ColonCommand {
+    fn get_content(
+        _name: &str,
+        content_type: builtins::ContentType,
+    ) -> Result<String, crate::error::Error> {
+        match content_type {
+            builtins::ContentType::DetailedHelp => {
+                Ok("Null command; always returns success.".into())
+            }
+            builtins::ContentType::ShortUsage => Ok(":: :".into()),
+            builtins::ContentType::ShortDescription => Ok(": - Null command".into()),
+            builtins::ContentType::ManPage => error::unimp("man page not yet implemented"),
+        }
+    }
 
-impl builtins::Command for ColonCommand {
-    async fn execute(
-        &self,
+    fn execute(
         _context: commands::ExecutionContext<'_>,
-    ) -> Result<crate::builtins::ExitCode, crate::error::Error> {
-        Ok(builtins::ExitCode::Success)
+        _args: &[&str],
+    ) -> Result<builtins::BuiltinResult, crate::error::Error> {
+        Ok(builtins::BuiltinResult {
+            exit_code: builtins::ExitCode::Success,
+        })
     }
 }
diff --git a/brush-core/src/builtins/factory.rs b/brush-core/src/builtins/factory.rs
index 3f95ae24..aa04382d 100644
--- a/brush-core/src/builtins/factory.rs
+++ b/brush-core/src/builtins/factory.rs
@@ -46,16 +46,6 @@ pub fn builtin<B: builtins::Command + Send + Sync>() -> builtins::Registration {
     }
 }
 
-fn special_builtin<B: builtins::Command + Send + Sync>() -> builtins::Registration {
-    builtins::Registration {
-        execute_func: exec_builtin::<B>,
-        content_func: get_builtin_content::<B>,
-        disabled: false,
-        special_builtin: true,
-        declaration_builtin: false,
-    }
-}
-
 fn decl_builtin<B: builtins::DeclarationCommand + Send + Sync>() -> builtins::Registration {
     builtins::Registration {
         execute_func: exec_declaration_builtin::<B>,
@@ -66,16 +56,6 @@ fn decl_builtin<B: builtins::DeclarationCommand + Send + Sync>() -> builtins::Re
     }
 }
 
-fn special_decl_builtin<B: builtins::DeclarationCommand + Send + Sync>() -> builtins::Registration {
-    builtins::Registration {
-        execute_func: exec_declaration_builtin::<B>,
-        content_func: get_builtin_content::<B>,
-        disabled: false,
-        special_builtin: true,
-        declaration_builtin: true,
-    }
-}
-
 fn get_builtin_content<T: builtins::Command + Send + Sync>(
     name: &str,
     content_type: builtins::ContentType,
@@ -194,32 +174,38 @@ pub(crate) fn get_default_builtins(
     // should be a special built-in.
     //
 
-    m.insert("break".into(), special_builtin::<break_::BreakCommand>());
-    m.insert(":".into(), special_builtin::<colon::ColonCommand>());
+    m.insert("break".into(), builtin::<break_::BreakCommand>().special());
+    m.insert(
+        ":".into(),
+        simple_builtin::<colon::ColonCommand>().special(),
+    );
     m.insert(
         "continue".into(),
-        special_builtin::<continue_::ContinueCommand>(),
+        builtin::<continue_::ContinueCommand>().special(),
     );
-    m.insert(".".into(), special_builtin::<dot::DotCommand>());
-    m.insert("eval".into(), special_builtin::<eval::EvalCommand>());
+    m.insert(".".into(), builtin::<dot::DotCommand>().special());
+    m.insert("eval".into(), builtin::<eval::EvalCommand>().special());
     #[cfg(unix)]
-    m.insert("exec".into(), special_builtin::<exec::ExecCommand>());
-    m.insert("exit".into(), special_builtin::<exit::ExitCommand>());
+    m.insert("exec".into(), builtin::<exec::ExecCommand>().special());
+    m.insert("exit".into(), builtin::<exit::ExitCommand>().special());
     m.insert(
         "export".into(),
-        special_decl_builtin::<export::ExportCommand>(),
+        decl_builtin::<export::ExportCommand>().special(),
+    );
+    m.insert(
+        "return".into(),
+        builtin::<return_::ReturnCommand>().special(),
     );
-    m.insert("return".into(), special_builtin::<return_::ReturnCommand>());
-    m.insert("set".into(), special_builtin::<set::SetCommand>());
-    m.insert("shift".into(), special_builtin::<shift::ShiftCommand>());
-    m.insert("trap".into(), special_builtin::<trap::TrapCommand>());
-    m.insert("unset".into(), special_builtin::<unset::UnsetCommand>());
+    m.insert("set".into(), builtin::<set::SetCommand>().special());
+    m.insert("shift".into(), builtin::<shift::ShiftCommand>().special());
+    m.insert("trap".into(), builtin::<trap::TrapCommand>().special());
+    m.insert("unset".into(), builtin::<unset::UnsetCommand>().special());
 
     m.insert(
         "readonly".into(),
-        special_decl_builtin::<declare::DeclareCommand>(),
+        decl_builtin::<declare::DeclareCommand>().special(),
     );
-    m.insert("times".into(), special_builtin::<times::TimesCommand>());
+    m.insert("times".into(), builtin::<times::TimesCommand>().special());
 
     //
     // Non-special builtins
@@ -260,7 +246,7 @@ pub(crate) fn get_default_builtins(
         m.insert("mapfile".into(), builtin::<mapfile::MapFileCommand>());
         m.insert("printf".into(), builtin::<printf::PrintfCommand>());
         m.insert("shopt".into(), builtin::<shopt::ShoptCommand>());
-        m.insert("source".into(), special_builtin::<dot::DotCommand>());
+        m.insert("source".into(), builtin::<dot::DotCommand>().special());
         #[cfg(unix)]
         m.insert("suspend".into(), builtin::<suspend::SuspendCommand>());
         m.insert("test".into(), builtin::<test::TestCommand>());
diff --git a/brush-core/src/sys/unix/fs.rs b/brush-core/src/sys/unix/fs.rs
index b1a5952b..c3710c4a 100644
--- a/brush-core/src/sys/unix/fs.rs
+++ b/brush-core/src/sys/unix/fs.rs
@@ -15,37 +15,37 @@ impl crate::sys::fs::PathExt for Path {
     }
 
     fn exists_and_is_block_device(&self) -> bool {
-        try_get_file_type(self).map_or(false, |ft| ft.is_block_device())
+        try_get_file_type(self).is_some_and(|ft| ft.is_block_device())
     }
 
     fn exists_and_is_char_device(&self) -> bool {
-        try_get_file_type(self).map_or(false, |ft| ft.is_char_device())
+        try_get_file_type(self).is_some_and(|ft| ft.is_char_device())
     }
 
     fn exists_and_is_fifo(&self) -> bool {
-        try_get_file_type(self).map_or(false, |ft: std::fs::FileType| ft.is_fifo())
+        try_get_file_type(self).is_some_and(|ft: std::fs::FileType| ft.is_fifo())
     }
 
     fn exists_and_is_socket(&self) -> bool {
-        try_get_file_type(self).map_or(false, |ft| ft.is_socket())
+        try_get_file_type(self).is_some_and(|ft| ft.is_socket())
     }
 
     fn exists_and_is_setgid(&self) -> bool {
         const S_ISGID: u32 = 0o2000;
         let file_mode = try_get_file_mode(self);
-        file_mode.map_or(false, |mode| mode & S_ISGID != 0)
+        file_mode.is_some_and(|mode| mode & S_ISGID != 0)
     }
 
     fn exists_and_is_setuid(&self) -> bool {
         const S_ISUID: u32 = 0o4000;
         let file_mode = try_get_file_mode(self);
-        file_mode.map_or(false, |mode| mode & S_ISUID != 0)
+        file_mode.is_some_and(|mode| mode & S_ISUID != 0)
     }
 
     fn exists_and_is_sticky_bit(&self) -> bool {
         const S_ISVTX: u32 = 0o1000;
         let file_mode = try_get_file_mode(self);
-        file_mode.map_or(false, |mode| mode & S_ISVTX != 0)
+        file_mode.is_some_and(|mode| mode & S_ISVTX != 0)
     }
 }
 
diff --git a/brush-shell/src/main.rs b/brush-shell/src/main.rs
index 312095cf..fa7e3696 100644
--- a/brush-shell/src/main.rs
+++ b/brush-shell/src/main.rs
@@ -203,7 +203,7 @@ async fn instantiate_shell(
                 .disallow_overwriting_regular_files_via_output_redirection,
             enabled_shopt_options: args.enabled_shopt_options.clone(),
             do_not_execute_commands: args.do_not_execute_commands,
-            login: args.login || argv0.as_ref().map_or(false, |a0| a0.starts_with('-')),
+            login: args.login || argv0.as_ref().is_some_and(|a0| a0.starts_with('-')),
             interactive,
             no_editing: args.no_editing,
             no_profile: args.no_profile,