From e2b996ae17a8a6942018822ba81d6b0602c5a57e Mon Sep 17 00:00:00 2001
From: reuben olinsky <reubeno@users.noreply.github.com>
Date: Fri, 17 May 2024 14:11:32 -0700
Subject: [PATCH 1/3] Better impl of trap builtin

---
 cli/src/main.rs            |   3 +
 shell/src/builtins/trap.rs | 137 ++++++++++++++++++++++++++++---------
 shell/src/error.rs         |   3 +
 shell/src/interp.rs        |   1 +
 shell/src/lib.rs           |   1 +
 shell/src/shell.rs         |   6 +-
 shell/src/traps.rs         |  20 ++++++
 src/lib.rs                 |   1 -
 8 files changed, 136 insertions(+), 36 deletions(-)
 create mode 100644 shell/src/traps.rs
 delete mode 100644 src/lib.rs

diff --git a/cli/src/main.rs b/cli/src/main.rs
index 78845454..4c1803c1 100644
--- a/cli/src/main.rs
+++ b/cli/src/main.rs
@@ -76,6 +76,8 @@ struct CommandLineArgs {
 enum TraceEvent {
     #[clap(name = "arithmetic")]
     Arithmetic,
+    #[clap(name = "commands")]
+    Commands,
     #[clap(name = "complete")]
     Complete,
     #[clap(name = "expand")]
@@ -124,6 +126,7 @@ fn main() {
     for event in enabled_trace_events {
         let targets = match event {
             TraceEvent::Arithmetic => vec!["parser::arithmetic"],
+            TraceEvent::Commands => vec!["commands"],
             TraceEvent::Complete => vec!["shell::completion", "shell::builtins::complete"],
             TraceEvent::Expand => vec![],
             TraceEvent::Parse => vec!["parse"],
diff --git a/shell/src/builtins/trap.rs b/shell/src/builtins/trap.rs
index ce3ab8f9..094e4f3f 100644
--- a/shell/src/builtins/trap.rs
+++ b/shell/src/builtins/trap.rs
@@ -1,7 +1,10 @@
 use clap::Parser;
-use std::io::Write;
+use std::{io::Write, str::FromStr};
 
-use crate::builtin::{BuiltinCommand, BuiltinExitCode};
+use crate::{
+    builtin::{BuiltinCommand, BuiltinExitCode},
+    error,
+};
 
 #[derive(Parser)]
 pub(crate) struct TrapCommand {
@@ -11,52 +14,120 @@ pub(crate) struct TrapCommand {
     #[arg(short = 'p')]
     print_trap_commands: bool,
 
-    command: Option<String>,
-    signals: Vec<String>,
+    args: Vec<String>,
 }
 
 #[async_trait::async_trait]
 impl BuiltinCommand for TrapCommand {
     async fn execute(
         &self,
-        context: crate::context::CommandExecutionContext<'_>,
+        mut context: crate::context::CommandExecutionContext<'_>,
     ) -> Result<crate::builtin::BuiltinExitCode, crate::error::Error> {
         if self.list_signals {
-            writeln!(context.stderr(), "UNIMPLEMENTED: trap -l")?;
-            return Ok(BuiltinExitCode::Unimplemented);
+            Self::display_signals(&context)?;
+            Ok(BuiltinExitCode::Success)
+        } else if self.print_trap_commands || self.args.is_empty() {
+            if !self.args.is_empty() {
+                for signal_type in &self.args {
+                    let signal_type = parse_signal(signal_type)?;
+                    Self::display_handlers_for(&context, signal_type)?;
+                }
+            } else {
+                Self::display_all_handlers(&context)?;
+            }
+            Ok(BuiltinExitCode::Success)
+        } else if self.args.len() == 1 {
+            let signal = self.args[0].as_str();
+            let signal_type = parse_signal(signal)?;
+            Self::remove_all_handlers(&mut context, signal_type)?;
+            Ok(BuiltinExitCode::Success)
+        } else {
+            let handler = &self.args[0];
+
+            let mut signal_types = vec![];
+            for signal in &self.args[1..] {
+                signal_types.push(parse_signal(signal)?);
+            }
+
+            Self::register_handler(&mut context, signal_types, handler.as_str())?;
+            Ok(BuiltinExitCode::Success)
         }
+    }
+}
 
-        if self.print_trap_commands {
-            writeln!(context.stderr(), "UNIMPLEMENTED: trap -p")?;
-            return Ok(BuiltinExitCode::Unimplemented);
+impl TrapCommand {
+    fn display_signals(
+        context: &crate::context::CommandExecutionContext<'_>,
+    ) -> Result<(), error::Error> {
+        for signal in nix::sys::signal::Signal::iterator() {
+            writeln!(context.stdout(), "{}: {signal}", signal as i32)?;
         }
 
-        // TODO: handle case where trap_command is a signal itself
-        if let Some(trap_command) = &self.command {
-            if self.signals.is_empty() {
-                writeln!(
-                    context.stderr(),
-                    "UNIMPLEMENTED: trap builtin called with command but no signals"
-                )?;
-                return Ok(BuiltinExitCode::Unimplemented);
-            }
+        Ok(())
+    }
 
-            for signal in &self.signals {
-                match signal.as_str() {
-                    "DEBUG" => (),
-                    _ => {
-                        writeln!(context.stderr(), "UNIMPLEMENTED: trap builtin called for signal {signal} (command: '{trap_command}')")?;
-                    }
-                }
+    fn display_all_handlers(
+        context: &crate::context::CommandExecutionContext<'_>,
+    ) -> Result<(), error::Error> {
+        for signal in context.shell.traps.handlers.keys() {
+            Self::display_handlers_for(context, *signal)?;
+        }
+        Ok(())
+    }
+
+    fn display_handlers_for(
+        context: &crate::context::CommandExecutionContext<'_>,
+        signal_type: nix::sys::signal::Signal,
+    ) -> Result<(), error::Error> {
+        if let Some(handlers) = context.shell.traps.handlers.get(&signal_type) {
+            for handler in handlers {
+                writeln!(context.stdout(), "trap -- '{handler}' {signal_type}")?;
             }
-        } else {
-            writeln!(
-                context.stderr(),
-                "UNIMPLEMENTED: trap builtin called without command"
-            )?;
-            return Ok(BuiltinExitCode::Unimplemented);
+        }
+        Ok(())
+    }
+
+    #[allow(clippy::unnecessary_wraps)]
+    fn remove_all_handlers(
+        context: &mut crate::context::CommandExecutionContext<'_>,
+        signal: nix::sys::signal::Signal,
+    ) -> Result<(), error::Error> {
+        context.shell.traps.remove_handlers(signal);
+        Ok(())
+    }
+
+    #[allow(clippy::unnecessary_wraps)]
+    fn register_handler(
+        context: &mut crate::context::CommandExecutionContext<'_>,
+        signals: Vec<nix::sys::signal::Signal>,
+        handler: &str,
+    ) -> Result<(), error::Error> {
+        for signal in signals {
+            context
+                .shell
+                .traps
+                .register_handler(signal, handler.to_owned());
+        }
+
+        Ok(())
+    }
+}
+
+fn parse_signal(signal: &str) -> Result<nix::sys::signal::Signal, error::Error> {
+    if signal.chars().all(|c| c.is_ascii_digit()) {
+        let digits = signal
+            .parse::<i32>()
+            .map_err(|_| error::Error::InvalidSignal)?;
+
+        nix::sys::signal::Signal::try_from(digits).map_err(|_| error::Error::InvalidSignal)
+    } else {
+        let mut signal_to_parse = signal.to_ascii_uppercase();
+
+        if !signal_to_parse.starts_with("SIG") {
+            signal_to_parse.insert_str(0, "SIG");
         }
 
-        Ok(BuiltinExitCode::Success)
+        nix::sys::signal::Signal::from_str(signal_to_parse.as_str())
+            .map_err(|_| error::Error::InvalidSignal)
     }
 }
diff --git a/shell/src/error.rs b/shell/src/error.rs
index 45b2713a..c381ddbd 100644
--- a/shell/src/error.rs
+++ b/shell/src/error.rs
@@ -100,6 +100,9 @@ pub enum Error {
 
     #[error("threading error")]
     ThreadingError(#[from] tokio::task::JoinError),
+
+    #[error("invalid signal")]
+    InvalidSignal,
 }
 
 pub(crate) fn unimp<T>(msg: &'static str) -> Result<T, Error> {
diff --git a/shell/src/interp.rs b/shell/src/interp.rs
index f05ef95c..bf4f9466 100644
--- a/shell/src/interp.rs
+++ b/shell/src/interp.rs
@@ -1240,6 +1240,7 @@ async fn execute_external_command(
     }
 
     tracing::debug!(
+        target: "commands",
         "Spawning: {} {}",
         cmd.as_std().get_program().to_string_lossy().to_string(),
         cmd.as_std()
diff --git a/shell/src/lib.rs b/shell/src/lib.rs
index c3a61e8a..c014d57d 100644
--- a/shell/src/lib.rs
+++ b/shell/src/lib.rs
@@ -20,6 +20,7 @@ mod prompt;
 mod regex;
 mod shell;
 mod tests;
+mod traps;
 mod users;
 mod variables;
 
diff --git a/shell/src/shell.rs b/shell/src/shell.rs
index 2e17d8e0..29124d93 100644
--- a/shell/src/shell.rs
+++ b/shell/src/shell.rs
@@ -11,14 +11,14 @@ use crate::options::RuntimeOptions;
 use crate::variables::{self, ShellValue, ShellVariable};
 use crate::{
     builtin, builtins, commands, completion, context, env, error, expansion, jobs, keywords,
-    openfiles, patterns, prompt, users,
+    openfiles, patterns, prompt, traps, users,
 };
 
 pub struct Shell {
     //
     // Core state required by specification
     //
-    // TODO: trap state
+    pub traps: traps::TrapHandlerConfig,
     pub open_files: openfiles::OpenFiles,
     pub working_dir: PathBuf,
     pub umask: u32,
@@ -65,6 +65,7 @@ pub struct Shell {
 impl Clone for Shell {
     fn clone(&self) -> Self {
         Self {
+            traps: self.traps.clone(),
             open_files: self.open_files.clone(),
             working_dir: self.working_dir.clone(),
             umask: self.umask,
@@ -113,6 +114,7 @@ impl Shell {
     pub async fn new(options: &CreateOptions) -> Result<Shell, error::Error> {
         // Instantiate the shell with some defaults.
         let mut shell = Shell {
+            traps: traps::TrapHandlerConfig::default(),
             open_files: openfiles::OpenFiles::default(),
             working_dir: std::env::current_dir()?,
             umask: Default::default(),           // TODO: populate umask
diff --git a/shell/src/traps.rs b/shell/src/traps.rs
new file mode 100644
index 00000000..46462fd5
--- /dev/null
+++ b/shell/src/traps.rs
@@ -0,0 +1,20 @@
+use std::collections::HashMap;
+
+#[derive(Clone, Default)]
+pub struct TrapHandlerConfig {
+    pub handlers: HashMap<nix::sys::signal::Signal, Vec<String>>,
+}
+
+impl TrapHandlerConfig {
+    pub fn register_handler(&mut self, signal_type: nix::sys::signal::Signal, command: String) {
+        if let Some(handlers) = self.handlers.get_mut(&signal_type) {
+            handlers.push(command);
+        } else {
+            self.handlers.insert(signal_type, vec![command]);
+        }
+    }
+
+    pub fn remove_handlers(&mut self, signal_type: nix::sys::signal::Signal) {
+        self.handlers.remove(&signal_type);
+    }
+}
diff --git a/src/lib.rs b/src/lib.rs
deleted file mode 100644
index 8b137891..00000000
--- a/src/lib.rs
+++ /dev/null
@@ -1 +0,0 @@
-

From 6eb6a753e48219cff95a2598d9dd7afdc40be5a6 Mon Sep 17 00:00:00 2001
From: reuben olinsky <reubeno@users.noreply.github.com>
Date: Fri, 17 May 2024 16:20:30 -0700
Subject: [PATCH 2/3] Basic DEBUG trap impl

---
 cli/tests/cases/builtins/help.yaml |  6 +++++
 cli/tests/cases/builtins/trap.yaml | 29 ++++++++++++++++++++++
 shell/src/builtins/trap.rs         | 31 +++++++++++++++---------
 shell/src/interp.rs                | 39 +++++++++++++++++++++++++++++-
 shell/src/traps.rs                 | 34 +++++++++++++++++++-------
 5 files changed, 117 insertions(+), 22 deletions(-)
 create mode 100644 cli/tests/cases/builtins/trap.yaml

diff --git a/cli/tests/cases/builtins/help.yaml b/cli/tests/cases/builtins/help.yaml
index f4afe644..46565e0d 100644
--- a/cli/tests/cases/builtins/help.yaml
+++ b/cli/tests/cases/builtins/help.yaml
@@ -5,3 +5,9 @@ cases:
     ignore_stderr: true
     stdin: |
       help
+
+  - name: "Topic-specific help"
+    ignore_stdout: true
+    ignore_stderr: true
+    stdin: |
+      help echo
diff --git a/cli/tests/cases/builtins/trap.yaml b/cli/tests/cases/builtins/trap.yaml
new file mode 100644
index 00000000..08c0eea4
--- /dev/null
+++ b/cli/tests/cases/builtins/trap.yaml
@@ -0,0 +1,29 @@
+name: "Builtins: trap"
+cases:
+  - name: "trap registration"
+    stdin: |
+      trap "echo 1" SIGINT
+      trap "echo 2" SIGINT
+      trap
+
+      trap "echo 3" int
+      trap
+
+      trap "echo 4" 2
+      trap
+
+  - name: "trap EXIT"
+    known_failure: true
+    stdin: |
+      trap "echo [exit]" EXIT
+      trap
+
+  - name: "trap DEBUG"
+    stdin: |
+      trap 'echo [command: ${BASH_COMMAND}]' DEBUG
+      trap
+
+  - name: "trap ERR"
+    stdin: |
+      trap "echo [err]" ERR
+      trap
diff --git a/shell/src/builtins/trap.rs b/shell/src/builtins/trap.rs
index 094e4f3f..cc300b25 100644
--- a/shell/src/builtins/trap.rs
+++ b/shell/src/builtins/trap.rs
@@ -3,7 +3,7 @@ use std::{io::Write, str::FromStr};
 
 use crate::{
     builtin::{BuiltinCommand, BuiltinExitCode},
-    error,
+    error, traps,
 };
 
 #[derive(Parser)]
@@ -77,12 +77,10 @@ impl TrapCommand {
 
     fn display_handlers_for(
         context: &crate::context::CommandExecutionContext<'_>,
-        signal_type: nix::sys::signal::Signal,
+        signal_type: traps::TrapSignal,
     ) -> Result<(), error::Error> {
-        if let Some(handlers) = context.shell.traps.handlers.get(&signal_type) {
-            for handler in handlers {
-                writeln!(context.stdout(), "trap -- '{handler}' {signal_type}")?;
-            }
+        if let Some(handler) = context.shell.traps.handlers.get(&signal_type) {
+            writeln!(context.stdout(), "trap -- '{handler}' {signal_type}")?;
         }
         Ok(())
     }
@@ -90,7 +88,7 @@ impl TrapCommand {
     #[allow(clippy::unnecessary_wraps)]
     fn remove_all_handlers(
         context: &mut crate::context::CommandExecutionContext<'_>,
-        signal: nix::sys::signal::Signal,
+        signal: traps::TrapSignal,
     ) -> Result<(), error::Error> {
         context.shell.traps.remove_handlers(signal);
         Ok(())
@@ -99,7 +97,7 @@ impl TrapCommand {
     #[allow(clippy::unnecessary_wraps)]
     fn register_handler(
         context: &mut crate::context::CommandExecutionContext<'_>,
-        signals: Vec<nix::sys::signal::Signal>,
+        signals: Vec<traps::TrapSignal>,
         handler: &str,
     ) -> Result<(), error::Error> {
         for signal in signals {
@@ -113,13 +111,15 @@ impl TrapCommand {
     }
 }
 
-fn parse_signal(signal: &str) -> Result<nix::sys::signal::Signal, error::Error> {
+fn parse_signal(signal: &str) -> Result<traps::TrapSignal, error::Error> {
     if signal.chars().all(|c| c.is_ascii_digit()) {
         let digits = signal
             .parse::<i32>()
             .map_err(|_| error::Error::InvalidSignal)?;
 
-        nix::sys::signal::Signal::try_from(digits).map_err(|_| error::Error::InvalidSignal)
+        Ok(traps::TrapSignal::Signal(
+            nix::sys::signal::Signal::try_from(digits).map_err(|_| error::Error::InvalidSignal)?,
+        ))
     } else {
         let mut signal_to_parse = signal.to_ascii_uppercase();
 
@@ -127,7 +127,14 @@ fn parse_signal(signal: &str) -> Result<nix::sys::signal::Signal, error::Error>
             signal_to_parse.insert_str(0, "SIG");
         }
 
-        nix::sys::signal::Signal::from_str(signal_to_parse.as_str())
-            .map_err(|_| error::Error::InvalidSignal)
+        match signal_to_parse {
+            s if s == "SIGDEBUG" => Ok(traps::TrapSignal::Debug),
+            s if s == "SIGERR" => Ok(traps::TrapSignal::Err),
+            s if s == "SIGEXIT" => Ok(traps::TrapSignal::Exit),
+            _ => Ok(traps::TrapSignal::Signal(
+                nix::sys::signal::Signal::from_str(signal_to_parse.as_str())
+                    .map_err(|_| error::Error::InvalidSignal)?,
+            )),
+        }
     }
 }
diff --git a/shell/src/interp.rs b/shell/src/interp.rs
index bf4f9466..d3ccc52b 100644
--- a/shell/src/interp.rs
+++ b/shell/src/interp.rs
@@ -19,7 +19,7 @@ use crate::shell::Shell;
 use crate::variables::{
     ArrayLiteral, ShellValue, ShellValueLiteral, ShellValueUnsetType, ShellVariable,
 };
-use crate::{builtin, context, error, expansion, extendedtests, jobs, openfiles};
+use crate::{builtin, context, error, expansion, extendedtests, jobs, openfiles, traps};
 
 #[derive(Debug, Default)]
 pub struct ExecutionResult {
@@ -895,6 +895,43 @@ impl ExecuteInPipeline for ast::SimpleCommand {
                     .trace_command(args.iter().map(|arg| arg.to_string()).join(" "))?;
             }
 
+            // 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 {
+                    let params = ExecutionParameters {
+                        open_files: open_files.clone(),
+                    };
+
+                    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.as_str(), &params)
+                        .await?;
+
+                    context.shell.traps.handler_depth -= 1;
+                }
+            }
+
             let cmd_context = context::CommandExecutionContext {
                 shell: context.shell,
                 command_name: cmd_name,
diff --git a/shell/src/traps.rs b/shell/src/traps.rs
index 46462fd5..a3190d5a 100644
--- a/shell/src/traps.rs
+++ b/shell/src/traps.rs
@@ -1,20 +1,36 @@
-use std::collections::HashMap;
+use std::{collections::HashMap, fmt::Display};
+
+#[derive(Clone, Copy, Eq, Hash, PartialEq)]
+pub enum TrapSignal {
+    Signal(nix::sys::signal::Signal),
+    Debug,
+    Err,
+    Exit,
+}
+
+impl Display for TrapSignal {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            TrapSignal::Signal(s) => s.fmt(f),
+            TrapSignal::Debug => write!(f, "DEBUG"),
+            TrapSignal::Err => write!(f, "ERR"),
+            TrapSignal::Exit => write!(f, "EXIT"),
+        }
+    }
+}
 
 #[derive(Clone, Default)]
 pub struct TrapHandlerConfig {
-    pub handlers: HashMap<nix::sys::signal::Signal, Vec<String>>,
+    pub handlers: HashMap<TrapSignal, String>,
+    pub handler_depth: i32,
 }
 
 impl TrapHandlerConfig {
-    pub fn register_handler(&mut self, signal_type: nix::sys::signal::Signal, command: String) {
-        if let Some(handlers) = self.handlers.get_mut(&signal_type) {
-            handlers.push(command);
-        } else {
-            self.handlers.insert(signal_type, vec![command]);
-        }
+    pub fn register_handler(&mut self, signal_type: TrapSignal, command: String) {
+        let _ = self.handlers.insert(signal_type, command);
     }
 
-    pub fn remove_handlers(&mut self, signal_type: nix::sys::signal::Signal) {
+    pub fn remove_handlers(&mut self, signal_type: TrapSignal) {
         self.handlers.remove(&signal_type);
     }
 }

From 3dae2af950f027dd6218f07b13cab6427ef73822 Mon Sep 17 00:00:00 2001
From: reuben olinsky <reubeno@users.noreply.github.com>
Date: Fri, 17 May 2024 23:48:06 -0700
Subject: [PATCH 3/3] Updates

---
 shell/src/builtin.rs                          | 113 +++++++++++++++++-
 shell/src/builtins/alias.rs                   |   1 +
 shell/src/builtins/bg.rs                      |   2 +
 shell/src/builtins/{brea.rs => break_.rs}     |   1 +
 shell/src/builtins/builtin_.rs                |   3 +
 shell/src/builtins/colon.rs                   |   1 +
 shell/src/builtins/complete.rs                |   3 +
 .../src/builtins/{continu.rs => continue_.rs} |   1 +
 shell/src/builtins/declare.rs                 |   5 +-
 shell/src/builtins/dirs.rs                    |   1 +
 shell/src/builtins/dot.rs                     |   1 +
 shell/src/builtins/echo.rs                    |   1 +
 shell/src/builtins/enable.rs                  |   1 +
 shell/src/builtins/eval.rs                    |   1 +
 shell/src/builtins/exec.rs                    |   1 +
 shell/src/builtins/exit.rs                    |   1 +
 shell/src/builtins/export.rs                  |   1 +
 shell/src/builtins/false_.rs                  |   1 +
 shell/src/builtins/fg.rs                      |   1 +
 shell/src/builtins/getopts.rs                 |   1 +
 shell/src/builtins/help.rs                    |  46 +++++--
 shell/src/builtins/jobs.rs                    |   1 +
 shell/src/builtins/kill.rs                    |   1 +
 shell/src/builtins/let_.rs                    |   1 +
 shell/src/builtins/mod.rs                     |  27 +++--
 shell/src/builtins/popd.rs                    |   1 +
 shell/src/builtins/printf.rs                  |   1 +
 shell/src/builtins/pushd.rs                   |   1 +
 shell/src/builtins/pwd.rs                     |   1 +
 shell/src/builtins/read.rs                    |   1 +
 shell/src/builtins/{retur.rs => return_.rs}   |   1 +
 shell/src/builtins/set.rs                     |   1 +
 shell/src/builtins/shift.rs                   |   1 +
 shell/src/builtins/shopt.rs                   |   1 +
 shell/src/builtins/test.rs                    |   1 +
 shell/src/builtins/trap.rs                    |   1 +
 shell/src/builtins/true_.rs                   |   1 +
 shell/src/builtins/type_.rs                   |   1 +
 shell/src/builtins/umask.rs                   |   1 +
 shell/src/builtins/unalias.rs                 |   1 +
 shell/src/builtins/unimp.rs                   |   1 +
 shell/src/builtins/unset.rs                   |   1 +
 shell/src/builtins/wait.rs                    |   1 +
 shell/src/completion.rs                       |   1 -
 shell/src/variables.rs                        |   1 -
 45 files changed, 204 insertions(+), 33 deletions(-)
 rename shell/src/builtins/{brea.rs => break_.rs} (94%)
 rename shell/src/builtins/{continu.rs => continue_.rs} (92%)
 rename shell/src/builtins/{retur.rs => return_.rs} (95%)

diff --git a/shell/src/builtin.rs b/shell/src/builtin.rs
index fa6961be..f911d0c4 100644
--- a/shell/src/builtin.rs
+++ b/shell/src/builtin.rs
@@ -116,9 +116,14 @@ pub trait BuiltinCommand: Parser {
         context: context::CommandExecutionContext<'_>,
     ) -> Result<BuiltinExitCode, error::Error>;
 
-    #[allow(dead_code)]
-    fn get_long_help() -> String {
-        Self::command().render_long_help().to_string()
+    fn get_content(name: &str, content_type: BuiltinContentType) -> String {
+        match content_type {
+            BuiltinContentType::DetailedHelp => Self::command().render_help().to_string(),
+            BuiltinContentType::ShortUsage => get_builtin_short_usage(name, &Self::command()),
+            BuiltinContentType::ShortDescription => {
+                get_builtin_short_description(name, &Self::command())
+            }
+        }
     }
 }
 
@@ -128,14 +133,21 @@ pub trait BuiltinDeclarationCommand: BuiltinCommand {
     fn set_declarations(&mut self, declarations: Vec<CommandArg>);
 }
 
+#[allow(clippy::module_name_repetitions)]
+pub enum BuiltinContentType {
+    DetailedHelp,
+    ShortUsage,
+    ShortDescription,
+}
+
 #[allow(clippy::module_name_repetitions)]
 #[derive(Clone)]
 pub struct BuiltinRegistration {
     /// Function to execute the builtin.
     pub execute_func: BuiltinCommandExecuteFunc,
 
-    /// Function to retrieve the builtin's detailed help text.
-    pub help_func: fn() -> String,
+    /// Function to retrieve the builtin's content/help text.
+    pub content_func: fn(&str, BuiltinContentType) -> String,
 
     /// Has this registration been disabled?
     pub disabled: bool,
@@ -146,3 +158,94 @@ pub struct BuiltinRegistration {
     /// Is this builtin one that takes specially handled declarations?
     pub declaration_builtin: bool,
 }
+
+fn get_builtin_short_description(name: &str, command: &clap::Command) -> String {
+    let about = command
+        .get_about()
+        .map_or_else(String::new, |s| s.to_string());
+
+    std::format!("{name} - {about}\n")
+}
+
+fn get_builtin_short_usage(name: &str, command: &clap::Command) -> String {
+    let mut usage = String::new();
+
+    let mut needs_space = false;
+
+    let mut optional_short_opts = vec![];
+    let mut required_short_opts = vec![];
+    for opt in command.get_opts() {
+        if opt.is_hide_set() {
+            continue;
+        }
+
+        if let Some(c) = opt.get_short() {
+            if !opt.is_required_set() {
+                optional_short_opts.push(c);
+            } else {
+                required_short_opts.push(c);
+            }
+        }
+    }
+
+    if !optional_short_opts.is_empty() {
+        if needs_space {
+            usage.push(' ');
+        }
+
+        usage.push('[');
+        usage.push('-');
+        for c in optional_short_opts {
+            usage.push(c);
+        }
+
+        usage.push(']');
+        needs_space = true;
+    }
+
+    if !required_short_opts.is_empty() {
+        if needs_space {
+            usage.push(' ');
+        }
+
+        usage.push('-');
+        for c in required_short_opts {
+            usage.push(c);
+        }
+
+        needs_space = true;
+    }
+
+    for pos in command.get_positionals() {
+        if pos.is_hide_set() {
+            continue;
+        }
+
+        if !pos.is_required_set() {
+            if needs_space {
+                usage.push(' ');
+            }
+
+            usage.push('[');
+            needs_space = false;
+        }
+
+        if let Some(names) = pos.get_value_names() {
+            for name in names {
+                if needs_space {
+                    usage.push(' ');
+                }
+
+                usage.push_str(name);
+                needs_space = true;
+            }
+        }
+
+        if !pos.is_required_set() {
+            usage.push(']');
+            needs_space = true;
+        }
+    }
+
+    std::format!("{name}: {name} {usage}\n")
+}
diff --git a/shell/src/builtins/alias.rs b/shell/src/builtins/alias.rs
index 535ac23d..121420a3 100644
--- a/shell/src/builtins/alias.rs
+++ b/shell/src/builtins/alias.rs
@@ -10,6 +10,7 @@ pub(crate) struct AliasCommand {
     #[arg(short = 'p')]
     print: bool,
 
+    /// List of aliases to display or update.
     #[arg(name = "name[=value]")]
     aliases: Vec<String>,
 }
diff --git a/shell/src/builtins/bg.rs b/shell/src/builtins/bg.rs
index e07f9022..382eda96 100644
--- a/shell/src/builtins/bg.rs
+++ b/shell/src/builtins/bg.rs
@@ -3,8 +3,10 @@ use std::io::Write;
 
 use crate::builtin::{BuiltinCommand, BuiltinExitCode};
 
+/// Moves a job to run in the background.
 #[derive(Parser)]
 pub(crate) struct BgCommand {
+    /// List of job specs to move to background.
     job_specs: Vec<String>,
 }
 
diff --git a/shell/src/builtins/brea.rs b/shell/src/builtins/break_.rs
similarity index 94%
rename from shell/src/builtins/brea.rs
rename to shell/src/builtins/break_.rs
index ceef30ee..4c137049 100644
--- a/shell/src/builtins/brea.rs
+++ b/shell/src/builtins/break_.rs
@@ -2,6 +2,7 @@ use clap::Parser;
 
 use crate::builtin::{BuiltinCommand, BuiltinExitCode};
 
+/// Breaks out of a control-flow loop.
 #[derive(Parser)]
 pub(crate) struct BreakCommand {
     #[clap(default_value = "1")]
diff --git a/shell/src/builtins/builtin_.rs b/shell/src/builtins/builtin_.rs
index 1ac40bc0..fbe52afa 100644
--- a/shell/src/builtins/builtin_.rs
+++ b/shell/src/builtins/builtin_.rs
@@ -6,10 +6,13 @@ use crate::{
     commands,
 };
 
+/// Directly invokes a built-in, without going through typical search order.
 #[derive(Parser)]
 pub(crate) struct BuiltiCommand {
+    /// Name of built-in to invoke.
     builtin_name: Option<String>,
 
+    /// Arguments for the built-in.
     #[arg(trailing_var_arg = true, allow_hyphen_values = true)]
     args: Vec<String>,
 }
diff --git a/shell/src/builtins/colon.rs b/shell/src/builtins/colon.rs
index 8307637d..15a9ce65 100644
--- a/shell/src/builtins/colon.rs
+++ b/shell/src/builtins/colon.rs
@@ -2,6 +2,7 @@ use clap::Parser;
 
 use crate::builtin::{BuiltinCommand, BuiltinExitCode};
 
+/// No-op command.
 #[derive(Parser)]
 #[clap(disable_help_flag = true, disable_version_flag = true)]
 pub(crate) struct ColonCommand {
diff --git a/shell/src/builtins/complete.rs b/shell/src/builtins/complete.rs
index b6d6c3a9..032b3ebd 100644
--- a/shell/src/builtins/complete.rs
+++ b/shell/src/builtins/complete.rs
@@ -168,6 +168,7 @@ impl CommonCompleteCommandArgs {
     }
 }
 
+/// Configure programmable command completion.
 #[derive(Parser)]
 pub(crate) struct CompleteCommand {
     #[arg(short = 'p')]
@@ -404,6 +405,7 @@ impl CompleteCommand {
     }
 }
 
+/// Generate command completions.
 #[derive(Parser)]
 pub(crate) struct CompGenCommand {
     #[clap(flatten)]
@@ -454,6 +456,7 @@ impl BuiltinCommand for CompGenCommand {
     }
 }
 
+/// Set programmable command completion options.
 #[derive(Parser)]
 pub(crate) struct CompOptCommand {
     #[arg(short = 'D')]
diff --git a/shell/src/builtins/continu.rs b/shell/src/builtins/continue_.rs
similarity index 92%
rename from shell/src/builtins/continu.rs
rename to shell/src/builtins/continue_.rs
index 7aee1059..a80eaa7e 100644
--- a/shell/src/builtins/continu.rs
+++ b/shell/src/builtins/continue_.rs
@@ -2,6 +2,7 @@ use clap::Parser;
 
 use crate::builtin::{BuiltinCommand, BuiltinExitCode};
 
+/// Continue to the next iteration of a control-flow loop.
 #[derive(Parser)]
 pub(crate) struct ContinueCommand {
     #[clap(default_value = "1")]
diff --git a/shell/src/builtins/declare.rs b/shell/src/builtins/declare.rs
index cb1c7d47..b1a2df1f 100644
--- a/shell/src/builtins/declare.rs
+++ b/shell/src/builtins/declare.rs
@@ -23,6 +23,7 @@ builtin::minus_or_plus_flag_arg!(MakeTracedFlag, 't', "");
 builtin::minus_or_plus_flag_arg!(UppercaseValueOnAssignmentFlag, 'u', "");
 builtin::minus_or_plus_flag_arg!(MakeExportedFlag, 'x', "");
 
+/// Display or update variables and their attributes.
 #[derive(Parser)]
 pub(crate) struct DeclareCommand {
     /// Constrain to function names or definitions.
@@ -220,7 +221,7 @@ impl DeclareCommand {
 
         // Extract the variable name and the initial value being assigned (if any).
         let (name, assigned_index, initial_value, name_is_array) =
-            self.declaration_to_name_and_value(declaration)?;
+            Self::declaration_to_name_and_value(declaration)?;
 
         // Figure out where we should look.
         let lookup = if create_var_local {
@@ -283,9 +284,7 @@ impl DeclareCommand {
         Ok(true)
     }
 
-    #[allow(clippy::unused_self)]
     fn declaration_to_name_and_value(
-        &self,
         declaration: &CommandArg,
     ) -> Result<(String, Option<String>, Option<ShellValueLiteral>, bool), error::Error> {
         let name;
diff --git a/shell/src/builtins/dirs.rs b/shell/src/builtins/dirs.rs
index 6d8be9e2..7ae67a69 100644
--- a/shell/src/builtins/dirs.rs
+++ b/shell/src/builtins/dirs.rs
@@ -3,6 +3,7 @@ use std::io::Write;
 
 use crate::builtin::{BuiltinCommand, BuiltinExitCode};
 
+/// Manage the current directory stack.
 #[derive(Parser, Debug, Default)]
 pub(crate) struct DirsCommand {
     #[arg(short = 'c')]
diff --git a/shell/src/builtins/dot.rs b/shell/src/builtins/dot.rs
index cb7bb6e7..fecacb65 100644
--- a/shell/src/builtins/dot.rs
+++ b/shell/src/builtins/dot.rs
@@ -7,6 +7,7 @@ use crate::{
     interp::ExecutionParameters,
 };
 
+/// Evalute the provided script in the current shell environment.
 #[derive(Debug, Parser)]
 pub(crate) struct DotCommand {
     pub script_path: String,
diff --git a/shell/src/builtins/echo.rs b/shell/src/builtins/echo.rs
index affdee8b..15c49b68 100644
--- a/shell/src/builtins/echo.rs
+++ b/shell/src/builtins/echo.rs
@@ -4,6 +4,7 @@ use std::io::Write;
 use crate::builtin::{BuiltinCommand, BuiltinExitCode};
 use crate::escape;
 
+/// Echo text to standard output.
 #[derive(Parser)]
 #[clap(disable_help_flag = true, disable_version_flag = true)]
 pub(crate) struct EchoCommand {
diff --git a/shell/src/builtins/enable.rs b/shell/src/builtins/enable.rs
index 1a73179c..e2f535fe 100644
--- a/shell/src/builtins/enable.rs
+++ b/shell/src/builtins/enable.rs
@@ -5,6 +5,7 @@ use std::io::Write;
 use crate::builtin::{BuiltinCommand, BuiltinExitCode};
 use crate::error;
 
+/// Enable, disable, or display built-in commands.
 #[derive(Parser)]
 pub(crate) struct EnableCommand {
     #[arg(short = 'a')]
diff --git a/shell/src/builtins/eval.rs b/shell/src/builtins/eval.rs
index 68beb44d..34fb0bee 100644
--- a/shell/src/builtins/eval.rs
+++ b/shell/src/builtins/eval.rs
@@ -4,6 +4,7 @@ use crate::{
 };
 use clap::Parser;
 
+/// Evalute the given string as script.
 #[derive(Parser)]
 pub(crate) struct EvalCommand {
     #[clap(allow_hyphen_values = true)]
diff --git a/shell/src/builtins/exec.rs b/shell/src/builtins/exec.rs
index f20b4c52..366f9e8f 100644
--- a/shell/src/builtins/exec.rs
+++ b/shell/src/builtins/exec.rs
@@ -3,6 +3,7 @@ use std::io::Write;
 
 use crate::builtin::{BuiltinCommand, BuiltinExitCode};
 
+/// Exec the provided command.
 #[derive(Parser)]
 pub(crate) struct ExecCommand {
     /// Pass given name as zeroth argument to command.
diff --git a/shell/src/builtins/exit.rs b/shell/src/builtins/exit.rs
index 692c2e81..4247ca2c 100644
--- a/shell/src/builtins/exit.rs
+++ b/shell/src/builtins/exit.rs
@@ -2,6 +2,7 @@ use clap::Parser;
 
 use crate::builtin::{BuiltinCommand, BuiltinExitCode};
 
+/// Exit the shell.
 #[derive(Parser)]
 pub(crate) struct ExitCommand {
     code: Option<i32>,
diff --git a/shell/src/builtins/export.rs b/shell/src/builtins/export.rs
index e57a5621..0aef6164 100644
--- a/shell/src/builtins/export.rs
+++ b/shell/src/builtins/export.rs
@@ -9,6 +9,7 @@ use crate::{
     variables,
 };
 
+/// Add or update exported shell variables.
 #[derive(Parser)]
 pub(crate) struct ExportCommand {
     #[arg(short = 'f')]
diff --git a/shell/src/builtins/false_.rs b/shell/src/builtins/false_.rs
index 72b657d6..f5e0ae45 100644
--- a/shell/src/builtins/false_.rs
+++ b/shell/src/builtins/false_.rs
@@ -2,6 +2,7 @@ use clap::Parser;
 
 use crate::builtin::{BuiltinCommand, BuiltinExitCode};
 
+/// Return a non-zero exit code.
 #[derive(Parser)]
 pub(crate) struct FalseCommand {}
 
diff --git a/shell/src/builtins/fg.rs b/shell/src/builtins/fg.rs
index 6fb1d262..5a6d5d09 100644
--- a/shell/src/builtins/fg.rs
+++ b/shell/src/builtins/fg.rs
@@ -3,6 +3,7 @@ use std::io::Write;
 
 use crate::builtin::{BuiltinCommand, BuiltinExitCode};
 
+/// Move a specified job to the foreground.
 #[derive(Parser)]
 pub(crate) struct FgCommand {
     job_spec: Option<String>,
diff --git a/shell/src/builtins/getopts.rs b/shell/src/builtins/getopts.rs
index 1e348189..038e253b 100644
--- a/shell/src/builtins/getopts.rs
+++ b/shell/src/builtins/getopts.rs
@@ -7,6 +7,7 @@ use crate::{
     variables,
 };
 
+/// Parse command options.
 #[derive(Parser)]
 pub(crate) struct GetOptsCommand {
     /// Specification for options
diff --git a/shell/src/builtins/help.rs b/shell/src/builtins/help.rs
index 36ffbe27..9dd0b8d8 100644
--- a/shell/src/builtins/help.rs
+++ b/shell/src/builtins/help.rs
@@ -1,8 +1,12 @@
-use crate::builtin::{BuiltinCommand, BuiltinExitCode, BuiltinRegistration};
+use crate::{
+    builtin::{self, BuiltinCommand, BuiltinExitCode, BuiltinRegistration},
+    context,
+};
 use clap::Parser;
 use itertools::Itertools;
 use std::io::Write;
 
+/// Display command help.
 #[derive(Parser)]
 pub(crate) struct HelpCommand {
     #[arg(short = 'd')]
@@ -50,12 +54,7 @@ impl HelpCommand {
             "The following commands are implemented as shell built-ins:"
         )?;
 
-        let builtins: Vec<(&String, &BuiltinRegistration)> = context
-            .shell
-            .builtins
-            .iter()
-            .sorted_by_key(|(name, _)| *name)
-            .collect();
+        let builtins = get_builtins_sorted_by_name(context);
         let items_per_column = (builtins.len() + COLUMN_COUNT - 1) / COLUMN_COUNT;
 
         for i in 0..items_per_column {
@@ -79,9 +78,13 @@ impl HelpCommand {
         let pattern = crate::patterns::Pattern::from(topic_pattern);
 
         let mut found_count = 0;
-        for (builtin_name, builtin_registration) in &context.shell.builtins {
+        for (builtin_name, builtin_registration) in get_builtins_sorted_by_name(context) {
             if pattern.exactly_matches(builtin_name.as_str(), false)? {
-                self.display_help_for_builtin(context, builtin_registration)?;
+                self.display_help_for_builtin(
+                    context,
+                    builtin_name.as_str(),
+                    builtin_registration,
+                )?;
                 found_count += 1;
             }
         }
@@ -93,13 +96,34 @@ impl HelpCommand {
         Ok(())
     }
 
-    #[allow(clippy::unused_self)]
     fn display_help_for_builtin(
         &self,
         context: &crate::context::CommandExecutionContext<'_>,
+        name: &str,
         registration: &BuiltinRegistration,
     ) -> Result<(), crate::error::Error> {
-        writeln!(context.stdout(), "{}", (registration.help_func)())?;
+        let content_type = if self.short_description {
+            builtin::BuiltinContentType::ShortDescription
+        } else if self.short_usage {
+            builtin::BuiltinContentType::ShortUsage
+        } else {
+            builtin::BuiltinContentType::DetailedHelp
+        };
+
+        let content = (registration.content_func)(name, content_type);
+
+        write!(context.stdout(), "{content}")?;
         Ok(())
     }
 }
+
+fn get_builtins_sorted_by_name<'a>(
+    context: &'a context::CommandExecutionContext<'_>,
+) -> Vec<(&'a String, &'a BuiltinRegistration)> {
+    context
+        .shell
+        .builtins
+        .iter()
+        .sorted_by_key(|(name, _)| *name)
+        .collect()
+}
diff --git a/shell/src/builtins/jobs.rs b/shell/src/builtins/jobs.rs
index a39d606d..f9f918e6 100644
--- a/shell/src/builtins/jobs.rs
+++ b/shell/src/builtins/jobs.rs
@@ -4,6 +4,7 @@ use std::io::Write;
 use crate::builtin::{BuiltinCommand, BuiltinExitCode};
 use crate::error;
 
+/// Manage jobs.
 #[derive(Parser)]
 pub(crate) struct JobsCommand {
     #[arg(short = 'l')]
diff --git a/shell/src/builtins/kill.rs b/shell/src/builtins/kill.rs
index ba83d47e..3c04c686 100644
--- a/shell/src/builtins/kill.rs
+++ b/shell/src/builtins/kill.rs
@@ -6,6 +6,7 @@ use crate::{
     error,
 };
 
+/// Signal a job or process.
 #[derive(Parser)]
 pub(crate) struct KillCommand {
     #[arg(short = 's')]
diff --git a/shell/src/builtins/let_.rs b/shell/src/builtins/let_.rs
index 70fb55ee..d354a26e 100644
--- a/shell/src/builtins/let_.rs
+++ b/shell/src/builtins/let_.rs
@@ -6,6 +6,7 @@ use crate::{
     builtin::{BuiltinCommand, BuiltinExitCode},
 };
 
+/// Evalute arithmetic expressions.
 #[derive(Parser)]
 pub(crate) struct LetCommand {
     #[arg(trailing_var_arg = true, allow_hyphen_values = true)]
diff --git a/shell/src/builtins/mod.rs b/shell/src/builtins/mod.rs
index c86ad9b1..335dc9d3 100644
--- a/shell/src/builtins/mod.rs
+++ b/shell/src/builtins/mod.rs
@@ -11,12 +11,12 @@ use crate::error;
 
 mod alias;
 mod bg;
-mod brea;
+mod break_;
 mod builtin_;
 mod cd;
 mod colon;
 mod complete;
-mod continu;
+mod continue_;
 mod declare;
 mod dirs;
 mod dot;
@@ -40,7 +40,7 @@ mod printf;
 mod pushd;
 mod pwd;
 mod read;
-mod retur;
+mod return_;
 mod set;
 mod shift;
 mod shopt;
@@ -57,7 +57,7 @@ mod wait;
 fn builtin<B: BuiltinCommand + Send>() -> BuiltinRegistration {
     BuiltinRegistration {
         execute_func: exec_builtin::<B>,
-        help_func: get_builtin_help::<B>,
+        content_func: get_builtin_content::<B>,
         disabled: false,
         special_builtin: false,
         declaration_builtin: false,
@@ -67,7 +67,7 @@ fn builtin<B: BuiltinCommand + Send>() -> BuiltinRegistration {
 fn special_builtin<B: BuiltinCommand + Send>() -> BuiltinRegistration {
     BuiltinRegistration {
         execute_func: exec_builtin::<B>,
-        help_func: get_builtin_help::<B>,
+        content_func: get_builtin_content::<B>,
         disabled: false,
         special_builtin: true,
         declaration_builtin: false,
@@ -77,7 +77,7 @@ fn special_builtin<B: BuiltinCommand + Send>() -> BuiltinRegistration {
 fn decl_builtin<B: BuiltinDeclarationCommand + Send>() -> BuiltinRegistration {
     BuiltinRegistration {
         execute_func: exec_declaration_builtin::<B>,
-        help_func: get_builtin_help::<B>,
+        content_func: get_builtin_content::<B>,
         disabled: false,
         special_builtin: false,
         declaration_builtin: true,
@@ -88,15 +88,18 @@ fn decl_builtin<B: BuiltinDeclarationCommand + Send>() -> BuiltinRegistration {
 fn special_decl_builtin<B: BuiltinDeclarationCommand + Send>() -> BuiltinRegistration {
     BuiltinRegistration {
         execute_func: exec_declaration_builtin::<B>,
-        help_func: get_builtin_help::<B>,
+        content_func: get_builtin_content::<B>,
         disabled: false,
         special_builtin: true,
         declaration_builtin: true,
     }
 }
 
-fn get_builtin_help<T: BuiltinCommand + Send>() -> String {
-    T::get_long_help()
+fn get_builtin_content<T: BuiltinCommand + Send>(
+    name: &str,
+    content_type: builtin::BuiltinContentType,
+) -> String {
+    T::get_content(name, content_type)
 }
 
 fn exec_builtin<T: BuiltinCommand + Send>(
@@ -185,11 +188,11 @@ pub(crate) fn get_default_builtins(
     // should be a special built-in.
     //
 
-    m.insert("break".into(), special_builtin::<brea::BreakCommand>());
+    m.insert("break".into(), special_builtin::<break_::BreakCommand>());
     m.insert(":".into(), special_builtin::<colon::ColonCommand>());
     m.insert(
         "continue".into(),
-        special_builtin::<continu::ContinueCommand>(),
+        special_builtin::<continue_::ContinueCommand>(),
     );
     m.insert(".".into(), special_builtin::<dot::DotCommand>());
     m.insert("eval".into(), special_builtin::<eval::EvalCommand>());
@@ -200,7 +203,7 @@ pub(crate) fn get_default_builtins(
         "export".into(),
         special_decl_builtin::<export::ExportCommand>(),
     );
-    m.insert("return".into(), special_builtin::<retur::ReturnCommand>());
+    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>());
diff --git a/shell/src/builtins/popd.rs b/shell/src/builtins/popd.rs
index e5bfc987..6859a7cb 100644
--- a/shell/src/builtins/popd.rs
+++ b/shell/src/builtins/popd.rs
@@ -3,6 +3,7 @@ use std::io::Write;
 
 use crate::builtin::{BuiltinCommand, BuiltinExitCode};
 
+/// Pop a path from the current directory stack.
 #[derive(Parser)]
 pub(crate) struct PopdCommand {
     #[clap(short = 'n')]
diff --git a/shell/src/builtins/printf.rs b/shell/src/builtins/printf.rs
index d497a583..28c1d0ea 100644
--- a/shell/src/builtins/printf.rs
+++ b/shell/src/builtins/printf.rs
@@ -6,6 +6,7 @@ use crate::{
     expansion,
 };
 
+/// Format a string.
 #[derive(Parser)]
 #[clap(disable_help_flag = true, disable_version_flag = true)]
 pub(crate) struct PrintfCommand {
diff --git a/shell/src/builtins/pushd.rs b/shell/src/builtins/pushd.rs
index 30ffcd1f..52dec183 100644
--- a/shell/src/builtins/pushd.rs
+++ b/shell/src/builtins/pushd.rs
@@ -3,6 +3,7 @@ use std::io::Write;
 
 use crate::builtin::{BuiltinCommand, BuiltinExitCode};
 
+/// Push a path onto the current directory stack.
 #[derive(Parser)]
 pub(crate) struct PushdCommand {
     #[clap(short = 'n')]
diff --git a/shell/src/builtins/pwd.rs b/shell/src/builtins/pwd.rs
index 3896276a..fbd3702e 100644
--- a/shell/src/builtins/pwd.rs
+++ b/shell/src/builtins/pwd.rs
@@ -2,6 +2,7 @@ use crate::builtin::{BuiltinCommand, BuiltinExitCode};
 use clap::Parser;
 use std::io::Write;
 
+/// Display the current working directory.
 #[derive(Parser)]
 pub(crate) struct PwdCommand {
     #[arg(
diff --git a/shell/src/builtins/read.rs b/shell/src/builtins/read.rs
index d99cec9d..44d24257 100644
--- a/shell/src/builtins/read.rs
+++ b/shell/src/builtins/read.rs
@@ -3,6 +3,7 @@ use std::{collections::VecDeque, io::Read};
 
 use crate::{builtin::BuiltinCommand, env, error, openfiles, variables};
 
+/// Parse standard input.
 #[derive(Parser)]
 pub(crate) struct ReadCommand {
     #[clap(short = 'a')]
diff --git a/shell/src/builtins/retur.rs b/shell/src/builtins/return_.rs
similarity index 95%
rename from shell/src/builtins/retur.rs
rename to shell/src/builtins/return_.rs
index 8d07d50a..cc0cb521 100644
--- a/shell/src/builtins/retur.rs
+++ b/shell/src/builtins/return_.rs
@@ -2,6 +2,7 @@ use clap::Parser;
 
 use crate::builtin::{BuiltinCommand, BuiltinExitCode};
 
+/// Return from the current function.
 #[derive(Parser)]
 pub(crate) struct ReturnCommand {
     code: Option<i32>,
diff --git a/shell/src/builtins/set.rs b/shell/src/builtins/set.rs
index 985e22e7..5526cbb8 100644
--- a/shell/src/builtins/set.rs
+++ b/shell/src/builtins/set.rs
@@ -77,6 +77,7 @@ pub(crate) struct SetOption {
     disable: Vec<String>,
 }
 
+/// Manage set-based shell options.
 #[derive(Parser)]
 #[clap(disable_help_flag = true)]
 pub(crate) struct SetCommand {
diff --git a/shell/src/builtins/shift.rs b/shell/src/builtins/shift.rs
index 44677415..8dab6fae 100644
--- a/shell/src/builtins/shift.rs
+++ b/shell/src/builtins/shift.rs
@@ -2,6 +2,7 @@ use clap::Parser;
 
 use crate::builtin::{BuiltinCommand, BuiltinExitCode};
 
+/// Shift positional arguments.
 #[derive(Parser)]
 pub(crate) struct ShiftCommand {
     n: Option<i32>,
diff --git a/shell/src/builtins/shopt.rs b/shell/src/builtins/shopt.rs
index ce04a491..7f911b79 100644
--- a/shell/src/builtins/shopt.rs
+++ b/shell/src/builtins/shopt.rs
@@ -4,6 +4,7 @@ use std::io::Write;
 
 use crate::builtin::{BuiltinCommand, BuiltinExitCode};
 
+/// Manage shopt-style options.
 #[derive(Parser)]
 pub(crate) struct ShoptCommand {
     #[arg(short = 'o')]
diff --git a/shell/src/builtins/test.rs b/shell/src/builtins/test.rs
index 0f26a3d4..05a65368 100644
--- a/shell/src/builtins/test.rs
+++ b/shell/src/builtins/test.rs
@@ -6,6 +6,7 @@ use crate::{
     error, tests, Shell,
 };
 
+/// Evaluate test expression.
 #[derive(Parser)]
 #[clap(disable_help_flag = true, disable_version_flag = true)]
 pub(crate) struct TestCommand {
diff --git a/shell/src/builtins/trap.rs b/shell/src/builtins/trap.rs
index cc300b25..fb97d435 100644
--- a/shell/src/builtins/trap.rs
+++ b/shell/src/builtins/trap.rs
@@ -6,6 +6,7 @@ use crate::{
     error, traps,
 };
 
+/// Manage signal traps.
 #[derive(Parser)]
 pub(crate) struct TrapCommand {
     #[arg(short = 'l')]
diff --git a/shell/src/builtins/true_.rs b/shell/src/builtins/true_.rs
index 140e444b..fddd21bc 100644
--- a/shell/src/builtins/true_.rs
+++ b/shell/src/builtins/true_.rs
@@ -2,6 +2,7 @@ use clap::Parser;
 
 use crate::builtin::{BuiltinCommand, BuiltinExitCode};
 
+/// Return 0.
 #[derive(Parser)]
 pub(crate) struct TrueCommand {}
 
diff --git a/shell/src/builtins/type_.rs b/shell/src/builtins/type_.rs
index 17f5e3f9..fc96c546 100644
--- a/shell/src/builtins/type_.rs
+++ b/shell/src/builtins/type_.rs
@@ -10,6 +10,7 @@ use crate::{
     Shell,
 };
 
+/// Inspect the type of a named shell item.
 #[derive(Parser)]
 pub(crate) struct TypeCommand {
     #[arg(short = 'a')]
diff --git a/shell/src/builtins/umask.rs b/shell/src/builtins/umask.rs
index f6763a02..406c92e5 100644
--- a/shell/src/builtins/umask.rs
+++ b/shell/src/builtins/umask.rs
@@ -2,6 +2,7 @@ use crate::builtin::{BuiltinCommand, BuiltinExitCode};
 use clap::Parser;
 use std::io::Write;
 
+/// Manage the process umask.
 #[derive(Parser)]
 pub(crate) struct UmaskCommand {
     #[arg(
diff --git a/shell/src/builtins/unalias.rs b/shell/src/builtins/unalias.rs
index 38287c01..c50fd4e0 100644
--- a/shell/src/builtins/unalias.rs
+++ b/shell/src/builtins/unalias.rs
@@ -3,6 +3,7 @@ use std::io::Write;
 
 use crate::builtin::{BuiltinCommand, BuiltinExitCode};
 
+/// Unset a shell alias.
 #[derive(Parser)]
 pub(crate) struct UnaliasCommand {
     #[arg(short = 'a')]
diff --git a/shell/src/builtins/unimp.rs b/shell/src/builtins/unimp.rs
index 735b522a..e75c78a4 100644
--- a/shell/src/builtins/unimp.rs
+++ b/shell/src/builtins/unimp.rs
@@ -6,6 +6,7 @@ use std::io::Write;
 
 use clap::Parser;
 
+/// (UNIMPLEMENTED COMMAND)
 #[derive(Parser)]
 pub(crate) struct UnimplementedCommand {
     #[clap(allow_hyphen_values = true)]
diff --git a/shell/src/builtins/unset.rs b/shell/src/builtins/unset.rs
index f94b0ea9..5cb59971 100644
--- a/shell/src/builtins/unset.rs
+++ b/shell/src/builtins/unset.rs
@@ -2,6 +2,7 @@ use clap::Parser;
 
 use crate::builtin::{BuiltinCommand, BuiltinExitCode};
 
+/// Unset a variable.
 #[derive(Parser)]
 pub(crate) struct UnsetCommand {
     #[clap(flatten)]
diff --git a/shell/src/builtins/wait.rs b/shell/src/builtins/wait.rs
index e248bdf1..bbb8da63 100644
--- a/shell/src/builtins/wait.rs
+++ b/shell/src/builtins/wait.rs
@@ -3,6 +3,7 @@ use clap::Parser;
 use crate::builtin::{BuiltinCommand, BuiltinExitCode};
 use crate::error;
 
+/// Wait for jobs to terminate.
 #[derive(Parser)]
 pub(crate) struct WaitCommand {
     #[arg(short = 'f')]
diff --git a/shell/src/completion.rs b/shell/src/completion.rs
index fbac6900..b5cbaca4 100644
--- a/shell/src/completion.rs
+++ b/shell/src/completion.rs
@@ -522,7 +522,6 @@ impl CompletionConfig {
         }
     }
 
-    #[allow(clippy::unused_self)]
     async fn get_completions_for_token<'a>(
         &self,
         shell: &mut Shell,
diff --git a/shell/src/variables.rs b/shell/src/variables.rs
index 211284e8..cbf38026 100644
--- a/shell/src/variables.rs
+++ b/shell/src/variables.rs
@@ -281,7 +281,6 @@ impl ShellVariable {
         }
     }
 
-    #[allow(clippy::unused_self)]
     #[allow(clippy::needless_pass_by_value)]
     pub fn assign_at_index(
         &mut self,