cli: add support for jj help alias

i consider rewriting the `help` command, but that breaks some other things (e.g. tab completion doesn't work, because `clap_complete` doesn't know about the aliases). instead, i found that i can add each alias to the `clap::Command` dynamically at runtime. this works! the help it shows is useful and easy to modify, and it's only about 25 lines of code.

the problem is this breaks `resolve_aliases` pretty badly. the replacement loop uses *clap's* API to determine the arguments that come after the alias, and clap does not expose a way to get the raw arguments, only the parsed arguments. it happened to work before because the commands were all `External`, in which case clap doesn't do any parsing.

i tried to just stop using clap's API, which simplifies the code a lot and works with the new dynamically added alias subcommands. but that breaks global flags that precede the alias, things like `jj --no-pager alias`.

instead, i waited to add the aliases to `Command` until after replacing the string arguments. that breaks because `expand_args()` gets called twice, once before and once after processing `--repository`. so even if i do this afterwards in the `resolve_aliases` function, the *next* call will still break because it parses differently.

to work around *that*, i clone the app before passing it to `expand_args`, so that the modifications don't affect the --repository parsing. it's kind of hacky, but i haven't found a simpler solution.

---

## future work

* `jj alias -h` shows the help for the *resolved* alias, not the "Alias for ..." short help. that happens because the alias is replaced with its definition before passing it to get_matches. it would be nice to make it consistent with `help`.
* `jj help recursive-alias` will only "peel" one level of aliasing in the help; it would be nice to show the fully expanded command instead.

Author: jyn514

CHANGELOG.md

@@ -17,6 +17,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 * `jj split` will now refuse to split an empty commit.
 
+* TOML type errors in alias definitions are now reported eagerly. Previously, they would not error unless you used the alias in a jj command.
+
 ### Deprecations
 
 - Attempting to alias a built-in command now gives a warning, rather than being silently ignored.
@@ -33,6 +35,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 * `jj branch track` now show conflicts if there are some.
 
+* `jj help` and `jj util completion` now support aliases.
+
+
 ### Fixed bugs
 
 * When the working copy commit becomes immutable, a new one is automatically created on top of it 

Cargo.lock

@@ -74,6 +74,7 @@ regex = { workspace = true }
 rpassword = { workspace = true }
 scm-record = { workspace = true }
 serde = { workspace = true }
+shlex = "1.3.0"
 slab = { workspace = true }
 strsim = { workspace = true }
 tempfile = { workspace = true }

cli/Cargo.toml

@@ -14,7 +14,7 @@
 
 use core::fmt;
 use std::borrow::Cow;
-use std::collections::{BTreeMap, HashSet};
+use std::collections::{BTreeMap, HashMap, HashSet};
 use std::env::{self, ArgsOs, VarError};
 use std::ffi::OsString;
 use std::fmt::Debug;
@@ -2599,12 +2599,16 @@ fn resolve_default_command(
     Ok(string_args)
 }
 
-fn resolve_aliases(
+type AliasMap = HashMap<String, Vec<String>>;
+
+// NOTE: logging doesn't work in this function because we don't enable it until
+// after expanding arguments.
+fn expand_aliases(
     ui: &Ui,
     config: &config::Config,
     app: &Command,
     mut string_args: Vec<String>,
-) -> Result<Vec<String>, CommandError> {
+) -> Result<(Vec<String>, AliasMap), CommandError> {
     let mut aliases_map = config.get_table("aliases")?;
     if let Ok(alias_map) = config.get_table("alias") {
         for (alias, definition) in alias_map {
@@ -2618,21 +2622,31 @@ fn resolve_aliases(
         }
     }
 
-    let mut resolved_aliases = HashSet::new();
+    let mut expanded_aliases = HashSet::new();
     let mut real_commands = HashSet::new();
     for command in app.get_subcommands() {
         real_commands.insert(command.get_name().to_string());
         for alias in command.get_all_aliases() {
             real_commands.insert(alias.to_string());
         }
     }
-    for alias in aliases_map.keys() {
-        if real_commands.contains(alias) {
+
+    let mut resolved_alias_map = HashMap::new();
+    for (alias_name, value) in aliases_map {
+        if real_commands.contains(&alias_name) {
             writeln!(
                 ui.warning_default(),
-                "Cannot define an alias that overrides the built-in command '{alias}'"
+                "Cannot define an alias that overrides the built-in command '{alias_name}'"
             )?;
+            continue;
         }
+
+        let Ok(alias_definition) = value.try_deserialize::<Vec<String>>() else {
+            return Err(user_error(format!(
+                r#"Alias definition for "{alias_name}" must be a string list"#
+            )));
+        };
+        resolved_alias_map.insert(alias_name, alias_definition);
     }
 
     loop {
@@ -2646,32 +2660,106 @@ fn resolve_aliases(
                     .unwrap_or_default()
                     .map(|arg| arg.to_str().unwrap().to_string())
                     .collect_vec();
-                if resolved_aliases.contains(&alias_name) {
+                if expanded_aliases.contains(&alias_name) {
                     return Err(user_error(format!(
                         r#"Recursive alias definition involving "{alias_name}""#
                     )));
                 }
-                if let Some(value) = aliases_map.remove(&alias_name) {
-                    if let Ok(alias_definition) = value.try_deserialize::<Vec<String>>() {
-                        assert!(string_args.ends_with(&alias_args));
-                        string_args.truncate(string_args.len() - 1 - alias_args.len());
-                        string_args.extend(alias_definition);
-                        string_args.extend_from_slice(&alias_args);
-                        resolved_aliases.insert(alias_name.clone());
-                        continue;
-                    } else {
-                        return Err(user_error(format!(
-                            r#"Alias definition for "{alias_name}" must be a string list"#
-                        )));
-                    }
+                if let Some(alias_definition) = resolved_alias_map.get(&alias_name) {
+                    assert!(string_args.ends_with(&alias_args));
+                    string_args.truncate(string_args.len() - 1 - alias_args.len());
+                    string_args.extend(alias_definition.clone());
+                    string_args.extend_from_slice(&alias_args);
+                    expanded_aliases.insert(alias_name.clone());
+                    continue;
                 } else {
                     // Not a real command and not an alias, so return what we've resolved so far
-                    return Ok(string_args);
+                    break;
                 }
             }
         }
         // No more alias commands, or hit unknown option
-        return Ok(string_args);
+        break;
+    }
+
+    Ok((string_args, resolved_alias_map))
+}
+
+fn add_alias_help(app: &mut Command, mut aliases: AliasMap) {
+    let render_definition = |def: &[String]| match shlex::try_join(def.iter().map(|s| &**s)) {
+        Ok(s) => format!("Alias for \"{s}\""),
+        Err(_) => format!("Alias for {def:?}"),
+    };
+
+    // Add each alias to `Command` so it shows up in help
+    // Aliases may be defined in terms of other aliases, and it's allowed for them
+    // to be defined in any order in the TOML config. Repeat this algorithm
+    // until we no longer make progress.
+    let mut progress;
+    let mut try_again_later = HashMap::new();
+    loop {
+        progress = false;
+        for (alias_name, alias_definition) in aliases.drain() {
+            // Find the innermost subcommand so we can use its help.
+            let mut subcmd = &mut *app;
+            let mut depth = 0;
+            for arg in &alias_definition {
+                if subcmd.find_subcommand_mut(arg).is_some() {
+                    // hack: without -Zpolonius, the borrow checker thinks the command in the if
+                    // condition isn't assignable
+                    subcmd = subcmd.find_subcommand_mut(arg).unwrap();
+                    depth += 1;
+                } else {
+                    break;
+                }
+            }
+
+            if depth == 0 {
+                // We never found a valid subcommand. Skip this iteration; maybe it's an alias
+                // to another alias.
+                try_again_later.insert(alias_name, alias_definition);
+                continue;
+            };
+            progress = true;
+
+            if depth == 1 && alias_definition.len() == 1 {
+                // if this definition has no arguments, we can add it as a `visible_alias`,
+                // which renders nicer. note that this only works for the root
+                // command; for some reason, `visible_alias` does not seem to propagate from
+                // subcommands to the root :(
+                *subcmd = subcmd.clone().visible_alias(alias_name);
+            } else {
+                // have to add a standalone subcmd
+                let help = render_definition(&alias_definition);
+                let subcmd_alias = subcmd
+                    .clone()
+                    .name(alias_name)
+                    // if we leave the built-in aliases, clap doesn't know whether to call the user
+                    // alias or the built-in command
+                    .alias(None)
+                    // TODO: for recursive aliases, it would be nice to show the full args this
+                    // expands to rather than just the immediately next alias
+                    .before_help(&help)
+                    .before_long_help(help);
+                *app = std::mem::take(app).subcommand(subcmd_alias);
+            }
+        }
+        if !progress {
+            break;
+        }
+        aliases.extend(try_again_later.drain());
+    }
+    // We may still have some unhandled aliases. That means there's an alias with
+    // valid toml syntax, but points to a subcommand that doesn't exist. Add
+    // help for that too.
+    for (name, definition) in try_again_later {
+        let help = render_definition(&definition);
+        // TODO: we can't distinguish empty/option-only aliases from aliases without a
+        // subcommand :( This leaves the default "global options" help in case
+        // it does happen to be valid. Perhaps we should do
+        // `app.try_parse_matches` here so we can distinguish the two?
+        let subcmd = Command::new(name).about(help);
+        *app = std::mem::take(app).subcommand(subcmd);
     }
 }
 
@@ -2713,7 +2801,7 @@ pub fn expand_args(
     app: &Command,
     args_os: ArgsOs,
     config: &config::Config,
-) -> Result<Vec<String>, CommandError> {
+) -> Result<(Vec<String>, AliasMap), CommandError> {
     let mut string_args: Vec<String> = vec![];
     for arg_os in args_os {
         if let Some(string_arg) = arg_os.to_str() {
@@ -2724,7 +2812,7 @@ pub fn expand_args(
     }
 
     let string_args = resolve_default_command(ui, config, app, string_args)?;
-    resolve_aliases(ui, config, app, string_args)
+    expand_aliases(ui, config, app, string_args)
 }
 
 pub fn parse_args(
@@ -2903,7 +2991,7 @@ impl CliRunner {
 
     #[instrument(skip_all)]
     fn run_internal(
-        self,
+        mut self,
         ui: &mut Ui,
         mut layered_configs: LayeredConfigs,
     ) -> Result<(), CommandError> {
@@ -2929,7 +3017,11 @@ impl CliRunner {
         let config = layered_configs.merge();
         ui.reset(&config)?;
 
-        let string_args = expand_args(ui, &self.app, env::args_os(), &config)?;
+        // save this - since `expand_args` modifies app, it won't parse the same when we
+        // process -R
+        let original_app = self.app.clone();
+        let (string_args, aliases) = expand_args(ui, &self.app, env::args_os(), &config)?;
+        add_alias_help(&mut self.app, aliases);
         let (matches, args) = parse_args(
             ui,
             &self.app,
@@ -2959,7 +3051,9 @@ impl CliRunner {
         // If -R is specified, check if the expanded arguments differ. Aliases
         // can also be injected by --config-toml, but that's obviously wrong.
         if args.global_args.repository.is_some() {
-            let new_string_args = expand_args(ui, &self.app, env::args_os(), &config).ok();
+            let new_string_args = expand_args(ui, &original_app, env::args_os(), &config)
+                .ok()
+                .map(|(args, _)| args);
             if new_string_args.as_ref() != Some(&string_args) {
                 writeln!(
                     ui.warning_default(),