From f43a4e6ad3f907ee82b7a3717a98c77207e7b03e Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Sat, 15 Aug 2026 14:26:46 +0000 Subject: [PATCH 1/7] feat(argv): suggest what was probably meant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--fore` → "a similar argument exists: '--force'". Jaro-Winkler above 0.7, which is clap's rule rather than a rule of ours, so the two suggest in the same cases and suggest the same thing. Written out rather than depended on — this crate takes no dependencies, and the algorithm is thirty lines. Three things measured from clap rather than assumed, each of which I had wrong first: Names are scored *without* their dashes. Every flag begins `--`, and Jaro-Winkler rewards a shared prefix, so comparing the dashed forms made `--fore` look similar to `--quiet`. The tests catch that now. Every candidate over the bar is listed, not the best one: `mise config lss` really is close to both `ls` and `list`, and clap says so — "some similar subcommands exist: 'list', 'ls'". The plural is the singular with an `s`. And they come out in *ascending* score, so the closest match is last. That is what clap does; it reads oddly, and I have left it matching rather than fixed on one side, since the point of this module is that an adopter's users see no change. Worth undoing in both places if you agree it is a bug. Suggestions come from what the parser would have accepted at that command — its own flags and any ancestor's globals — so a tip is always something that works. Also recorded, in the gate tests: mise's spec leaves `unknown_flags` at its default, so `mise use --globa` is an error under clap and a *tool named `--globa`* under this parser. That is a decision for the adopting CLI rather than a bug here — declaring `unknown_flags=error` restores both the refusal and these suggestions — but it is the reason no flag typo appears among the parity cases, and it should not be discovered late. Co-Authored-By: Claude Opus 5 --- argv/src/diagnostic.rs | 327 +++++++++++++++++++++++++++++++++-- benches/gate/tests/errors.rs | 69 ++++++++ 2 files changed, 379 insertions(+), 17 deletions(-) diff --git a/argv/src/diagnostic.rs b/argv/src/diagnostic.rs index 6cda8645c..6182956ae 100644 --- a/argv/src/diagnostic.rs +++ b/argv/src/diagnostic.rs @@ -95,6 +95,158 @@ impl Style { } } +/// Every flag a word at this command could have named: its own, then any ancestor's globals. +/// +/// The same set the parser would have accepted, which is what makes a suggestion one that works. +fn flags_in_scope<'a>( + spec: &'a Spec<'a>, + cmd: &Command<'_>, +) -> impl Iterator> { + fn walk<'a>( + meta: &'a CommandMeta<'a>, + cmd: &Command<'_>, + seen: &mut Vec<&'a crate::spec::FlagMeta<'a>>, + inside: bool, + ) { + let here = inside || core::ptr::eq(meta.cmd, cmd); + if here { + seen.extend(meta.flags.iter().filter(|f| !f.hide)); + } else { + // An ancestor contributes only what it declared global, which is the rule the parser + // follows on the way down. + seen.extend(meta.flags.iter().filter(|f| !f.hide && f.flag.global)); + } + if core::ptr::eq(meta.cmd, cmd) { + return; + } + for sub in meta.subcommands { + walk(sub, cmd, seen, false); + } + } + let mut seen = Vec::new(); + walk(spec.root, cmd, &mut seen, false); + seen.into_iter() +} + +/// How alike two words are, from 0 (nothing in common) to 1 (the same word). +/// +/// Jaro-Winkler, which is what clap uses to decide whether to suggest something — so a CLI that +/// moves from clap gets the same suggestions rather than merely similar ones. It favours words +/// that agree at the start, which is right for a mistyped flag: `--fore` is a slip in `--force`, +/// not a different word. +/// +/// Written out rather than depended on, because this crate takes no dependencies and the whole +/// algorithm is thirty lines. +fn jaro_winkler(a: &str, b: &str) -> f64 { + let (a, b): (Vec, Vec) = (a.chars().collect(), b.chars().collect()); + if a.is_empty() && b.is_empty() { + return 1.0; + } + if a.is_empty() || b.is_empty() { + return 0.0; + } + + // Two characters count as matching if they are the same and no further apart than this. + let window = (a.len().max(b.len()) / 2).saturating_sub(1); + let mut a_matched = vec![false; a.len()]; + let mut b_matched = vec![false; b.len()]; + let mut matches = 0usize; + + for (i, ch) in a.iter().enumerate() { + let start = i.saturating_sub(window); + let end = (i + window + 1).min(b.len()); + for j in start..end { + if !b_matched[j] && b[j] == *ch { + a_matched[i] = true; + b_matched[j] = true; + matches += 1; + break; + } + } + } + if matches == 0 { + return 0.0; + } + + // Matching characters that arrive in a different order are half a transposition each. + let mut transpositions = 0usize; + let mut k = 0usize; + for (i, matched) in a_matched.iter().enumerate() { + if !matched { + continue; + } + while !b_matched[k] { + k += 1; + } + if a[i] != b[k] { + transpositions += 1; + } + k += 1; + } + + let matches = matches as f64; + let jaro = (matches / a.len() as f64 + + matches / b.len() as f64 + + (matches - transpositions as f64 / 2.0) / matches) + / 3.0; + + // Winkler's part: a shared prefix of up to four characters pulls the score up. + let prefix = a + .iter() + .zip(b.iter()) + .take(4) + .take_while(|(x, y)| x == y) + .count() as f64; + jaro + prefix * 0.1 * (1.0 - jaro) +} + +/// Everything close enough to `typed` to be worth saying, in the order clap says them. +/// +/// The threshold is clap's — a score above 0.7 — so the two suggest in the same cases. Below it a +/// suggestion is noise: offering `--quiet` for `--zzz` is worse than offering nothing, because a +/// user reads it as the CLI having understood them. +/// +/// All of them, not the best one: clap lists every candidate over the bar, and `mise config lss` +/// really is close to both `ls` and `list`. Sorted *ascending* by score, which is what clap does — +/// so the closest match comes last. That reads oddly, and it is preserved here because the point +/// of this module is that an adopter's users see no change; it is a difference worth undoing on +/// both sides rather than on one. +fn nearest<'a>(typed: &str, candidates: impl Iterator) -> Vec<&'a str> { + let mut scored: Vec<(f64, &str)> = candidates + .map(|candidate| (jaro_winkler(typed, candidate), candidate)) + .filter(|(score, _)| *score > 0.7) + .collect(); + scored.sort_by(|a, b| a.0.total_cmp(&b.0).then_with(|| a.1.cmp(b.1))); + scored.dedup_by(|a, b| a.1 == b.1); + scored.into_iter().map(|(_, candidate)| candidate).collect() +} + +/// A tip naming what was probably meant, or nothing when nothing was close. +/// +/// `noun` is the singular — clap writes "a similar argument exists" for one and "some similar +/// arguments exist" for several, and the plural is the singular with an `s`. +fn tip(style: Style, noun: &str, near: &[&str]) -> String { + match near { + [] => String::new(), + [one] => format!( + "\n {} a similar {noun} exists: '{}'\n", + style.valid("tip:"), + style.valid(one) + ), + many => { + let listed: Vec = many + .iter() + .map(|candidate| format!("'{}'", style.valid(candidate))) + .collect(); + format!( + "\n {} some similar {noun}s exist: {}\n", + style.valid("tip:"), + listed.join(", ") + ) + } + } +} + /// A name as a usage line writes it: ``, `[TOOL]…`, `--jobs`. /// /// The error carries the spec's name for a thing; a user reads the form the help shows. Both come @@ -205,12 +357,30 @@ pub fn render( // The shape of the command line: clap shows a usage block for these. Error::UnknownFlag { token } => { with_usage = true; + let typed = String::from_utf8_lossy(token); let _ = writeln!( out, "{} unexpected argument '{}' found", style.error("error:"), - style.invalid(&String::from_utf8_lossy(token)) + style.invalid(&typed) ); + // Scored without the dashes, and only then written back with them. Every flag + // starts `--`, and the prefix bonus in Jaro-Winkler counts that agreement — so + // `--fore` came out similar to `--quiet`, which it is not. clap compares the bare + // names for the same reason. + let bare = typed.trim_start_matches('-'); + let names: Vec<&str> = flags_in_scope(spec, cmd) + .flat_map(|meta| meta.flag.longs.iter().copied()) + .collect(); + let near: Vec = nearest(bare, names.into_iter()) + .into_iter() + .map(|name| format!("--{name}")) + .collect(); + out.push_str(&tip( + style, + "argument", + &near.iter().map(String::as_str).collect::>(), + )); } Error::UnexpectedArg { token } => { with_usage = true; @@ -232,6 +402,18 @@ pub fn render( style.error("error:"), style.invalid(&word) ); + // Every name a subcommand answers to, hidden ones included: a user who typed a + // near miss of an old alias should be told the name it still works under. + let names: Vec<&str> = cmd + .subcommands + .iter() + .flat_map(|sub| core::iter::once(sub.name).chain(sub.aliases.iter().copied())) + .collect(); + out.push_str(&tip( + style, + "subcommand", + &nearest(&word, names.into_iter()), + )); } } Error::MissingRequired { name } => { @@ -306,6 +488,13 @@ pub fn render( } let listed: Vec = choices.iter().map(|c| style.valid(c)).collect(); let _ = writeln!(out, " [possible values: {}]", listed.join(", ")); + if let Some(typed) = value_bound_to(spec.root.cmd, argv, name, choices) { + out.push_str(&tip( + style, + "value", + &nearest(&typed, choices.iter().copied()), + )); + } } Error::InvalidValue(invalid) => { let _ = writeln!( @@ -417,9 +606,22 @@ mod tests { args: &[&TOOL, &SHELLS], ..Command::EMPTY }; + static QUIET: Flag = Flag { + key: 4, + name: "quiet", + longs: &["quiet"], + global: true, + ..Flag::BOOL + }; + /// A second command close to the same typo, so the plural wording is reachable. + static USER: Command = Command { + name: "user", + ..Command::EMPTY + }; static ROOT: Command = Command { name: "ex", - subcommands: &[&USE], + flags: &[&QUIET], + subcommands: &[&USE, &USER], ..Command::EMPTY }; static USE_META: CommandMeta = CommandMeta { @@ -455,9 +657,19 @@ mod tests { ], ..CommandMeta::EMPTY }; + static USER_META: CommandMeta = CommandMeta { + cmd: &USER, + about: Some("Manage users"), + ..CommandMeta::EMPTY + }; static ROOT_META: CommandMeta = CommandMeta { cmd: &ROOT, - subcommands: &[&USE_META], + flags: &[FlagMeta { + flag: &QUIET, + help: Some("Say less"), + ..FlagMeta::EMPTY + }], + subcommands: &[&USE_META, &USER_META], ..CommandMeta::EMPTY }; static SPEC: Spec = Spec { @@ -473,18 +685,6 @@ mod tests { render(&SPEC, &argv, &error, Style::PLAIN) } - #[test] - fn an_unknown_flag_reads_as_clap_writes_it() { - assert_eq!( - rendered(&["use"], Error::UnknownFlag { token: b"--fore" }), - "error: unexpected argument '--fore' found\n\ - \n\ - Usage: ex use [-f --force] [--jobs ] [SHELLS]…\n\ - \n\ - For more information, try '--help'.\n" - ); - } - #[test] fn the_usage_line_is_the_one_the_help_prints() { // Not clap's, which spells a usage line its own way. An error that disagrees with the @@ -623,10 +823,103 @@ mod tests { }, Style::PLAIN, ); - assert!(message.contains("invalid value 'fsh'"), "{message}"); + // The first line names the value, so assert on that line alone: the tip below it lists + // every choice, `zsh` among them, and a whole-message search cannot tell the two apart. assert!( - !message.contains("'zsh'\n"), + message.starts_with("error: invalid value 'fsh' for '[SHELLS]…'"), "named a value that was fine: {message}" ); } + + #[test] + fn a_near_miss_is_suggested_the_way_clap_suggests_one() { + let message = rendered(&["use"], Error::UnknownFlag { token: b"--fore" }); + assert_eq!( + message, + "error: unexpected argument '--fore' found\n\ + \n\ + \x20 tip: a similar argument exists: '--force'\n\ + \n\ + Usage: ex use [-f --force] [--jobs ] [SHELLS]…\n\ + \n\ + For more information, try '--help'.\n" + ); + } + + #[test] + fn nothing_is_suggested_when_nothing_is_close() { + // Offering `--force` for `--zzz` is worse than offering nothing: a user reads a tip as + // the CLI having understood them. clap's threshold, so clap's silence — and the rest of + // the message is the same either way. + assert_eq!( + rendered(&["use"], Error::UnknownFlag { token: b"--zzz" }), + "error: unexpected argument '--zzz' found\n\ + \n\ + Usage: ex use [-f --force] [--jobs ] [SHELLS]…\n\ + \n\ + For more information, try '--help'.\n" + ); + } + + #[test] + fn the_scores_are_the_ones_clap_would_compute() { + // Spot values for the algorithm itself, so a rewrite cannot quietly change which words + // count as similar. + assert!((jaro_winkler("--fore", "--force") - 0.972).abs() < 0.001); + assert_eq!(jaro_winkler("same", "same"), 1.0); + assert_eq!(jaro_winkler("", ""), 1.0); + assert_eq!(jaro_winkler("abc", ""), 0.0); + // No characters in common at all. + assert_eq!(jaro_winkler("abc", "xyz"), 0.0); + // A shared prefix pulls the score up, which is what makes it right for a mistyped flag. + assert!(jaro_winkler("--forc", "--force") > jaro_winkler("orce--", "--force")); + } + + #[test] + fn a_subcommand_and_a_value_get_the_same_treatment() { + // Two are close, so the plural — and in clap's order, which is ascending by score, so + // the *closest* comes last. That reads oddly and is what clap does. + let message = rendered(&[], Error::UnexpectedArg { token: b"usse" }); + assert!( + message.contains("tip: some similar subcommands exist: 'user', 'use'"), + "{message}" + ); + + // The singular is covered by the flag and value cases in this module, which name one + // each — here both commands begin `us`, so anything close to one is close to both. + + let owned = [ + std::ffi::OsString::from("use"), + std::ffi::OsString::from("nod"), + ]; + let argv: Vec<&std::ffi::OsStr> = owned.iter().map(|o| o.as_os_str()).collect(); + let message = render( + &SPEC, + &argv, + &Error::InvalidChoice { + name: "TOOL", + choices: &["node", "python"], + }, + Style::PLAIN, + ); + assert!( + message.contains("invalid value 'nod' for ''"), + "{message}" + ); + assert!( + message.contains("tip: a similar value exists: 'node'"), + "{message}" + ); + } + + #[test] + fn a_global_flag_is_suggested_inside_a_subcommand() { + // What the parser would have accepted there is what should be suggested there — the same + // rule the completions follow, for the same reason. + let message = rendered(&["use"], Error::UnknownFlag { token: b"--quie" }); + assert!( + message.contains("tip: a similar argument exists: '--quiet'"), + "{message}" + ); + } } diff --git a/benches/gate/tests/errors.rs b/benches/gate/tests/errors.rs index fe4845e34..e974b3ad1 100644 --- a/benches/gate/tests/errors.rs +++ b/benches/gate/tests/errors.rs @@ -114,3 +114,72 @@ fn the_usage_line_is_the_one_the_help_prints() { usage_argv::help::usage_line(&["mise", "config"], config) ); } + +#[test] +fn the_suggestion_is_the_one_clap_would_make() { + // Jaro-Winkler above 0.7, which is clap's rule — so the two suggest in the same cases and + // suggest the same thing. Checked against mise's real flags rather than a fixture, because + // what makes a suggestion good is the size of the set it was chosen from. + let cases = [ + (vec!["activate", "zsx"], "a similar value exists: 'zsh'"), + ( + vec!["config", "lss"], + "some similar subcommands exist: 'list', 'ls'", + ), + ]; + for (words, tip) in cases { + let ours = our_error(&words).unwrap_or_else(|| panic!("we parsed {words:?}")); + assert!(ours.contains(tip), "{words:?}\n{ours}"); + + // And clap says the same thing, where clap fails at all. + if let Some(theirs) = clap_error(&words) { + assert!(theirs.contains(tip), "clap on {words:?}:\n{theirs}"); + } + } +} + +#[test] +fn a_word_nothing_resembles_gets_no_tip() { + // The threshold earns its keep on a set this size: mise has 711 flags, and something is + // always vaguely similar to anything if the bar is low enough. + let ours = our_error(&["config", "zzzzzzzz"]).expect("a failure"); + assert!(!ours.contains("tip:"), "{ours}"); + if let Some(theirs) = clap_error(&["config", "zzzzzzzz"]) { + assert!(!theirs.contains("tip:"), "{theirs}"); + } +} + +#[test] +fn mise_accepts_an_unknown_flag_where_clap_refuses_one() { + // Not a rendering question, but the reason no flag typo appears above: mise's spec leaves + // `unknown_flags` at its default, which means a dash-prefixed word nothing matches is a + // *value*. clap refuses it. So `mise use --globa` is an error today and a tool named + // `--globa` under this parser. + // + // Recorded rather than worked around, because it is a decision for the adopting CLI — + // declaring `unknown_flags=error` on a command restores clap's behaviour and, with it, the + // typo suggestions this module produces for a flag. + let words = ["use", "--globa"]; + assert!( + clap_error(&words).is_some(), + "clap should refuse an unknown flag" + ); + assert!( + our_error(&words).is_none(), + "mise's spec should accept it as a value — if this fails, the spec changed and the \ + suggestion cases above can cover a flag again" + ); + + // The flag itself is there — it is the refusal that is not, which is what makes this a + // property of the spec rather than of the parser. + let has_global = shadow_mise::Cli::spec() + .root + .subcommands + .iter() + .find(|s| s.cmd.name == "use") + .expect("mise use") + .flags + .iter() + .any(|f| f.flag.longs.contains(&"global")); + assert!(has_global, "mise use declares --global"); +} From d75ac3cba0a10113a5b82c094848e395fb9141c3 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Sat, 15 Aug 2026 14:28:57 +0000 Subject: [PATCH 2/7] fix(argv): call a dash-prefixed word a flag, not a subcommand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `mise doctor --forc` read as "unrecognized subcommand '--forc'", which answers a question nobody asked — and it happened on exactly the commands where the mistake is easiest to make, the ones with subcommands, where a bare word *would* have been one. What the word looks like decides now, before anything about the command does: a dash-prefixed token is a flag the user got wrong, and gets the flag wording and a flag suggestion. clap says the same sentence for the same line, which the gate test holds. A bare word is still a subcommand, and a lone `-` is still a word — it is what several tools spell "standard input". Raised by jdx. Co-Authored-By: Claude Opus 5 --- argv/src/diagnostic.rs | 61 +++++++++++++++++++++++++++++++++--- benches/gate/tests/errors.rs | 12 +++++++ 2 files changed, 69 insertions(+), 4 deletions(-) diff --git a/argv/src/diagnostic.rs b/argv/src/diagnostic.rs index 6182956ae..eab79fc0d 100644 --- a/argv/src/diagnostic.rs +++ b/argv/src/diagnostic.rs @@ -385,10 +385,32 @@ pub fn render( Error::UnexpectedArg { token } => { with_usage = true; let word = String::from_utf8_lossy(token); - // A word where a subcommand was expected reads better as one — which is the same - // distinction clap draws between an unexpected argument and an unrecognized - // subcommand. - if cmd.subcommands.is_empty() { + // What the word *looks like* decides, before anything about the command does. A + // dash-prefixed token is a flag the user got wrong — telling them `--forc` is an + // unrecognized subcommand is answering a question they did not ask, and it happens + // on exactly the commands where the mistake is easiest to make: the ones with + // subcommands, where a bare word would have been one. + if word.starts_with('-') && word != "-" { + let _ = writeln!( + out, + "{} unexpected argument '{}' found", + style.error("error:"), + style.invalid(&word) + ); + let bare = word.trim_start_matches('-'); + let names: Vec<&str> = flags_in_scope(spec, cmd) + .flat_map(|meta| meta.flag.longs.iter().copied()) + .collect(); + let near: Vec = nearest(bare, names.into_iter()) + .into_iter() + .map(|name| format!("--{name}")) + .collect(); + out.push_str(&tip( + style, + "argument", + &near.iter().map(String::as_str).collect::>(), + )); + } else if cmd.subcommands.is_empty() { let _ = writeln!( out, "{} unexpected argument '{}' found", @@ -922,4 +944,35 @@ mod tests { "{message}" ); } + #[test] + fn a_dash_prefixed_word_is_a_flag_even_where_subcommands_exist() { + // The root has subcommands, so a bare word there is a subcommand — but `--forc` is not a + // subcommand anybody could have meant, and saying "unrecognized subcommand" answers a + // question the user did not ask. It happens on exactly the commands where the mistake is + // easiest to make. + let message = rendered(&[], Error::UnexpectedArg { token: b"--quie" }); + assert!( + message.starts_with("error: unexpected argument '--quie' found"), + "{message}" + ); + assert!( + message.contains("tip: a similar argument exists: '--quiet'"), + "{message}" + ); + assert!(!message.contains("subcommand"), "{message}"); + + // A bare word is still a subcommand, which is the other half of the same rule. + let message = rendered(&[], Error::UnexpectedArg { token: b"usse" }); + assert!( + message.starts_with("error: unrecognized subcommand 'usse'"), + "{message}" + ); + + // A lone `-` is a word, not a flag: it is what several tools spell "standard input". + let message = rendered(&[], Error::UnexpectedArg { token: b"-" }); + assert!( + message.starts_with("error: unrecognized subcommand '-'"), + "{message}" + ); + } } diff --git a/benches/gate/tests/errors.rs b/benches/gate/tests/errors.rs index e974b3ad1..3e96056c2 100644 --- a/benches/gate/tests/errors.rs +++ b/benches/gate/tests/errors.rs @@ -183,3 +183,15 @@ fn mise_accepts_an_unknown_flag_where_clap_refuses_one() { .any(|f| f.flag.longs.contains(&"global")); assert!(has_global, "mise use declares --global"); } + +#[test] +fn a_flag_typo_on_a_command_with_subcommands_reads_as_a_flag() { + // `mise doctor --forc` fails on both sides, and both should call it an unexpected *argument* + // — `doctor` has subcommands, so the tempting answer is "unrecognized subcommand", which is + // an answer to a question nobody asked. + let words = ["doctor", "--forc"]; + let ours = our_error(&words).expect("a failure"); + let theirs = clap_error(&words).expect("a failure"); + assert_eq!(first_line(&ours), first_line(&theirs), "\n{ours}\n{theirs}"); + assert!(!ours.contains("subcommand"), "{ours}"); +} From 96ae62096962f894998f78bc107ce79a2c7ca6e3 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Sat, 15 Aug 2026 15:20:59 +0000 Subject: [PATCH 3/7] fix(argv): score the way clap scores, and offer only flags that work here MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two from review, and the first is a correction to something I told jdx. clap does not use Jaro-Winkler. It uses plain `strsim::jaro`, and says why in its own source: "GH #4660: using `jaro` because `jaro_winkler` implementation in `strsim-rs` is wrong". Winkler's prefix bonus moves the bar — some words clear 0.7 with it and not without, and the ranking changes — so a module whose whole premise is that an adopter's users see the same tips has to use the same algorithm. The bonus is three lines away if it is ever wanted on both sides. And `flags_in_scope` walked the whole tree, treating every command it passed through as an ancestor, so a global declared on one branch was suggested under an unrelated one. A tip naming a flag the parser would refuse is worse than no tip. It follows the chain to the command now: its own flags, and from each real ancestor only what that ancestor declared global. The fixture had to change to see either. Its sibling command is declared *first*, so the walk to `use` passes through it — a sibling visited on the way is exactly what leaked — and the root now declares a flag of its own that is not global, which is the other half of the rule. Both mutations pass against the old fixture. Found by greptile and Cursor Bugbot. Co-Authored-By: Claude Opus 5 --- argv/src/diagnostic.rs | 186 +++++++++++++++++++++++++++++------------ 1 file changed, 134 insertions(+), 52 deletions(-) diff --git a/argv/src/diagnostic.rs b/argv/src/diagnostic.rs index eab79fc0d..70ea97fe0 100644 --- a/argv/src/diagnostic.rs +++ b/argv/src/diagnostic.rs @@ -102,42 +102,61 @@ fn flags_in_scope<'a>( spec: &'a Spec<'a>, cmd: &Command<'_>, ) -> impl Iterator> { - fn walk<'a>( + /// The chain of commands from the root down to `cmd`, or nothing if it is not below here. + /// + /// The chain and not the tree: the first version collected globals from every branch it + /// walked through, so a global declared on one command was suggested under an unrelated one + /// — a tip naming a flag the parser would refuse, which is worse than no tip. + fn chain<'a>( meta: &'a CommandMeta<'a>, cmd: &Command<'_>, - seen: &mut Vec<&'a crate::spec::FlagMeta<'a>>, - inside: bool, - ) { - let here = inside || core::ptr::eq(meta.cmd, cmd); - if here { - seen.extend(meta.flags.iter().filter(|f| !f.hide)); - } else { - // An ancestor contributes only what it declared global, which is the rule the parser - // follows on the way down. - seen.extend(meta.flags.iter().filter(|f| !f.hide && f.flag.global)); - } + path: &mut Vec<&'a CommandMeta<'a>>, + ) -> bool { + path.push(meta); if core::ptr::eq(meta.cmd, cmd) { - return; + return true; } for sub in meta.subcommands { - walk(sub, cmd, seen, false); + if chain(sub, cmd, path) { + return true; + } } + path.pop(); + false + } + + let mut path = Vec::new(); + if !chain(spec.root, cmd, &mut path) { + path.clear(); } - let mut seen = Vec::new(); - walk(spec.root, cmd, &mut seen, false); - seen.into_iter() + // The command's own flags, and from each ancestor only what it declared global — the rule + // the parser follows on the way down. + let depth = path.len(); + path.into_iter().enumerate().flat_map(move |(i, meta)| { + let own = i + 1 == depth; + meta.flags + .iter() + .filter(move |f| !f.hide && (own || f.flag.global)) + }) } /// How alike two words are, from 0 (nothing in common) to 1 (the same word). /// -/// Jaro-Winkler, which is what clap uses to decide whether to suggest something — so a CLI that -/// moves from clap gets the same suggestions rather than merely similar ones. It favours words -/// that agree at the start, which is right for a mistyped flag: `--fore` is a slip in `--force`, -/// not a different word. +/// Jaro, and deliberately not Jaro-Winkler. clap decides whether to suggest something with +/// `strsim::jaro`, and says why in its own source: /// -/// Written out rather than depended on, because this crate takes no dependencies and the whole -/// algorithm is thirty lines. -fn jaro_winkler(a: &str, b: &str) -> f64 { +/// ```text +/// // GH #4660: using `jaro` because `jaro_winkler` implementation in `strsim-rs` is wrong +/// ``` +/// +/// Winkler's variant adds a bonus for a shared prefix, which sounds right for a mistyped flag and +/// would move the bar: some words clear 0.7 with it and not without, and the ranking changes too. +/// Since the point of this module is that an adopter's users see the same tips they saw under +/// clap, the algorithm has to be the same one. The bonus is three lines away if it is ever wanted +/// on both sides. +/// +/// Written out rather than depended on, because this crate takes no dependencies. +fn jaro(a: &str, b: &str) -> f64 { let (a, b): (Vec, Vec) = (a.chars().collect(), b.chars().collect()); if a.is_empty() && b.is_empty() { return 1.0; @@ -185,19 +204,10 @@ fn jaro_winkler(a: &str, b: &str) -> f64 { } let matches = matches as f64; - let jaro = (matches / a.len() as f64 + (matches / a.len() as f64 + matches / b.len() as f64 + (matches - transpositions as f64 / 2.0) / matches) - / 3.0; - - // Winkler's part: a shared prefix of up to four characters pulls the score up. - let prefix = a - .iter() - .zip(b.iter()) - .take(4) - .take_while(|(x, y)| x == y) - .count() as f64; - jaro + prefix * 0.1 * (1.0 - jaro) + / 3.0 } /// Everything close enough to `typed` to be worth saying, in the order clap says them. @@ -213,7 +223,7 @@ fn jaro_winkler(a: &str, b: &str) -> f64 { /// both sides rather than on one. fn nearest<'a>(typed: &str, candidates: impl Iterator) -> Vec<&'a str> { let mut scored: Vec<(f64, &str)> = candidates - .map(|candidate| (jaro_winkler(typed, candidate), candidate)) + .map(|candidate| (jaro(typed, candidate), candidate)) .filter(|(score, _)| *score > 0.7) .collect(); scored.sort_by(|a, b| a.0.total_cmp(&b.0).then_with(|| a.1.cmp(b.1))); @@ -636,14 +646,31 @@ mod tests { ..Flag::BOOL }; /// A second command close to the same typo, so the plural wording is reachable. + static LOCAL: Flag = Flag { + key: 5, + name: "local", + longs: &["local"], + global: true, + ..Flag::BOOL + }; static USER: Command = Command { name: "user", + flags: &[&LOCAL], ..Command::EMPTY }; + /// Declared on the root and *not* global, so it belongs to the root alone. + static SETUP: Flag = Flag { + key: 6, + name: "setup", + longs: &["setup"], + ..Flag::BOOL + }; static ROOT: Command = Command { name: "ex", - flags: &[&QUIET], - subcommands: &[&USE, &USER], + flags: &[&QUIET, &SETUP], + // `user` first, so the walk to `use` passes through it: a sibling that is visited on the + // way is exactly what leaked into scope before. + subcommands: &[&USER, &USE], ..Command::EMPTY }; static USE_META: CommandMeta = CommandMeta { @@ -682,16 +709,28 @@ mod tests { static USER_META: CommandMeta = CommandMeta { cmd: &USER, about: Some("Manage users"), + flags: &[FlagMeta { + flag: &LOCAL, + help: Some("Only this checkout"), + ..FlagMeta::EMPTY + }], ..CommandMeta::EMPTY }; static ROOT_META: CommandMeta = CommandMeta { cmd: &ROOT, - flags: &[FlagMeta { - flag: &QUIET, - help: Some("Say less"), - ..FlagMeta::EMPTY - }], - subcommands: &[&USE_META, &USER_META], + flags: &[ + FlagMeta { + flag: &QUIET, + help: Some("Say less"), + ..FlagMeta::EMPTY + }, + FlagMeta { + flag: &SETUP, + help: Some("Set things up"), + ..FlagMeta::EMPTY + }, + ], + subcommands: &[&USER_META, &USE_META], ..CommandMeta::EMPTY }; static SPEC: Spec = Spec { @@ -886,15 +925,24 @@ mod tests { #[test] fn the_scores_are_the_ones_clap_would_compute() { // Spot values for the algorithm itself, so a rewrite cannot quietly change which words - // count as similar. - assert!((jaro_winkler("--fore", "--force") - 0.972).abs() < 0.001); - assert_eq!(jaro_winkler("same", "same"), 1.0); - assert_eq!(jaro_winkler("", ""), 1.0); - assert_eq!(jaro_winkler("abc", ""), 0.0); + // count as similar. `fore` against `force` is the ordinary case: five of six characters, + // in order. + assert!( + (jaro("fore", "force") - 0.933).abs() < 0.001, + "{}", + jaro("fore", "force") + ); + assert_eq!(jaro("same", "same"), 1.0); + assert_eq!(jaro("", ""), 1.0); + assert_eq!(jaro("abc", ""), 0.0); // No characters in common at all. - assert_eq!(jaro_winkler("abc", "xyz"), 0.0); - // A shared prefix pulls the score up, which is what makes it right for a mistyped flag. - assert!(jaro_winkler("--forc", "--force") > jaro_winkler("orce--", "--force")); + assert_eq!(jaro("abc", "xyz"), 0.0); + + // And *no* prefix bonus, which is the whole difference from Jaro-Winkler: Jaro counts + // matching characters and their order, not where the agreement falls, so dropping a + // word's last letter and dropping its first score alike. Under Winkler the first would + // win, and a different set of words would clear the bar than clap's. + assert_eq!(jaro("forc", "force"), jaro("orce", "force")); } #[test] @@ -975,4 +1023,38 @@ mod tests { "{message}" ); } + #[test] + fn a_siblings_global_is_not_offered_here() { + // `user` declares a global; it is a sibling of `use`, never an ancestor, so the parser + // would refuse `--local` inside `use`. A tip naming a flag that does not work is worse + // than no tip — and the first version of this walked the whole tree, collecting globals + // from every branch it passed through. + let message = rendered(&["use"], Error::UnknownFlag { token: b"--locl" }); + assert!(!message.contains("tip:"), "{message}"); + + // Inside `user` itself it is offered, which is what makes the absence above a rule + // rather than an oversight. + let message = rendered(&["user"], Error::UnknownFlag { token: b"--locl" }); + assert!( + message.contains("tip: a similar argument exists: '--local'"), + "{message}" + ); + + // And the root's global still reaches a subcommand, which is the case globals exist for. + let message = rendered(&["use"], Error::UnknownFlag { token: b"--quie" }); + assert!( + message.contains("tip: a similar argument exists: '--quiet'"), + "{message}" + ); + + // An ancestor's *non*-global flag does not: the root declares `--setup` for itself, and + // the parser would refuse it inside `use` exactly as it refuses a sibling's. + let message = rendered(&["use"], Error::UnknownFlag { token: b"--setu" }); + assert!(!message.contains("tip:"), "{message}"); + let message = rendered(&[], Error::UnknownFlag { token: b"--setu" }); + assert!( + message.contains("tip: a similar argument exists: '--setup'"), + "{message}" + ); + } } From 59601c1d4ccd5a6b48e35959f540d432691718bc Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Sat, 15 Aug 2026 15:46:14 +0000 Subject: [PATCH 4/7] fix(argv): a value attached to a flag is not part of its name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--fore=1`. The parser splits on the `=` before looking the name up, so the flag the user named is `--fore` — and the error was about `--fore=1`, which nobody typed. The half that would have gone quietly is the tip. `fore=1` scored against `force` falls under the 0.7 bar, so the suggestion vanished exactly where a mistyped value-taking flag is most likely to be written: attached form is what you use when the flag takes a value. clap 4 was run rather than remembered, and says both: error: unexpected argument '--fore' found tip: a similar argument exists: '--force' Long flags only. A short cluster is refused whole — `-xy` is not `-x` with a `y` attached — and clap keeps the `=` in a short flag's value. Found by Cursor Bugbot. Co-Authored-By: Claude Opus 5 --- argv/src/diagnostic.rs | 65 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 61 insertions(+), 4 deletions(-) diff --git a/argv/src/diagnostic.rs b/argv/src/diagnostic.rs index 70ea97fe0..be76af9fb 100644 --- a/argv/src/diagnostic.rs +++ b/argv/src/diagnostic.rs @@ -95,6 +95,26 @@ impl Style { } } +/// A flag as the user named it, without a value they attached to it. +/// +/// `--jobs=4` names `--jobs`; the parser splits on the `=` before looking the name up, so an +/// error about the whole token is about something nobody typed. Both halves of the message +/// depend on it: clap prints `'--fore'` for `--fore=1`, and it scores `fore` — with the value +/// left on, `fore=1` falls under the 0.7 bar and the tip disappears exactly where a mistyped +/// value-taking flag is most likely to be written. +/// +/// Long flags only. A short cluster is refused whole, so `-xy` is not `-x` with something +/// attached, and `-j=4` is a value clap keeps. +fn flag_named(token: &str) -> &str { + match token.strip_prefix("--") { + Some(body) => match body.find('=') { + Some(i) => &token[..i + 2], + None => token, + }, + None => token, + } +} + /// Every flag a word at this command could have named: its own, then any ancestor's globals. /// /// The same set the parser would have accepted, which is what makes a suggestion one that works. @@ -367,12 +387,13 @@ pub fn render( // The shape of the command line: clap shows a usage block for these. Error::UnknownFlag { token } => { with_usage = true; - let typed = String::from_utf8_lossy(token); + let whole = String::from_utf8_lossy(token); + let typed = flag_named(&whole); let _ = writeln!( out, "{} unexpected argument '{}' found", style.error("error:"), - style.invalid(&typed) + style.invalid(typed) ); // Scored without the dashes, and only then written back with them. Every flag // starts `--`, and the prefix bonus in Jaro-Winkler counts that agreement — so @@ -401,13 +422,16 @@ pub fn render( // on exactly the commands where the mistake is easiest to make: the ones with // subcommands, where a bare word would have been one. if word.starts_with('-') && word != "-" { + // Same rule as a refused flag: a value attached with `=` is not part of the + // name, and the word reaches here by the same spelling mistake. + let named = flag_named(&word); let _ = writeln!( out, "{} unexpected argument '{}' found", style.error("error:"), - style.invalid(&word) + style.invalid(named) ); - let bare = word.trim_start_matches('-'); + let bare = named.trim_start_matches('-'); let names: Vec<&str> = flags_in_scope(spec, cmd) .flat_map(|meta| meta.flag.longs.iter().copied()) .collect(); @@ -907,6 +931,39 @@ mod tests { ); } + #[test] + fn a_value_attached_to_a_flag_is_not_part_of_its_name() { + // `--fore=1`. The parser splits on the `=` before looking the name up, so the flag the + // user named is `--fore` and an error about `--fore=1` is about something nobody typed. + // + // Both halves matter, and clap 4 was run to check both rather than remembered: + // + // error: unexpected argument '--fore' found + // tip: a similar argument exists: '--force' + // + // The tip is the half that would have gone quietly: `fore=1` against `force` falls under + // the 0.7 bar, so leaving the value on loses the suggestion exactly where a mistyped + // value-taking flag is most likely to be written. + assert_eq!( + rendered(&["use"], Error::UnknownFlag { token: b"--fore=1" }), + "error: unexpected argument '--fore' found\n\ + \n\ + \x20 tip: a similar argument exists: '--force'\n\ + \n\ + Usage: ex use [-f --force] [--jobs ] [SHELLS]…\n\ + \n\ + For more information, try '--help'.\n" + ); + + // A short cluster is refused whole — `-xy` is not `-x` with a `y` attached — and clap + // keeps the `=` in a short flag's value, so the rule is for long flags only. + let message = rendered(&["use"], Error::UnknownFlag { token: b"-j=4" }); + assert!( + message.starts_with("error: unexpected argument '-j=4' found"), + "{message}" + ); + } + #[test] fn nothing_is_suggested_when_nothing_is_close() { // Offering `--force` for `--zzz` is worse than offering nothing: a user reads a tip as From b09e77fcd67f3779e8d1baac2ff92c0cf9e778da Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Sun, 16 Aug 2026 17:27:00 +0000 Subject: [PATCH 5/7] fix(argv): tell two mounts of one subcommand apart MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One `Subcommands` type mounted under two parents is one `Command` at one address — both parents splice the same `&'static [Command]`. So a lookup that searches the metadata tree for that address finds whichever mount comes first, and ex beta shared --betaglobl came back describing `ex alpha shared`: the wrong usage line, alpha's globals offered as suggestions, and beta's `--betaglobal` — the flag the user meant, and the only one the parser would have taken there — never mentioned. The command an error ends on does not identify itself. The route to it does, so the route is what gets carried: `path_taken` collects each `Command` event, and `resolve` walks the metadata down that route, matching each step among *that command's* children. A parent's own child list is unambiguous even when the child is shared. Found by greptile, whose report was about the suggestions; the usage line was wrong too, which the test asserts first. Not fixed here: `help::find` searches the same way, so `ex beta shared --help` prints `Usage: ex alpha shared`, and completions resolve a command the same way. Both predate this stack and neither is reachable from the diagnostics path. Reported separately rather than folded in — the fix is to carry a route through three more call sites. Co-Authored-By: Claude Opus 5 --- argv/src/diagnostic.rs | 128 ++++++++++--------- conformance/tests/shared_subcommand.rs | 166 +++++++++++++++++++++++++ 2 files changed, 233 insertions(+), 61 deletions(-) create mode 100644 conformance/tests/shared_subcommand.rs diff --git a/argv/src/diagnostic.rs b/argv/src/diagnostic.rs index be76af9fb..0a65b9279 100644 --- a/argv/src/diagnostic.rs +++ b/argv/src/diagnostic.rs @@ -118,41 +118,16 @@ fn flag_named(token: &str) -> &str { /// Every flag a word at this command could have named: its own, then any ancestor's globals. /// /// The same set the parser would have accepted, which is what makes a suggestion one that works. -fn flags_in_scope<'a>( - spec: &'a Spec<'a>, - cmd: &Command<'_>, -) -> impl Iterator> { - /// The chain of commands from the root down to `cmd`, or nothing if it is not below here. - /// - /// The chain and not the tree: the first version collected globals from every branch it - /// walked through, so a global declared on one command was suggested under an unrelated one - /// — a tip naming a flag the parser would refuse, which is worse than no tip. - fn chain<'a>( - meta: &'a CommandMeta<'a>, - cmd: &Command<'_>, - path: &mut Vec<&'a CommandMeta<'a>>, - ) -> bool { - path.push(meta); - if core::ptr::eq(meta.cmd, cmd) { - return true; - } - for sub in meta.subcommands { - if chain(sub, cmd, path) { - return true; - } - } - path.pop(); - false - } - - let mut path = Vec::new(); - if !chain(spec.root, cmd, &mut path) { - path.clear(); - } +fn flags_in_scope<'a, 'c>( + chain: &'c [&'a CommandMeta<'a>], +) -> impl Iterator> + use<'a, 'c> { // The command's own flags, and from each ancestor only what it declared global — the rule - // the parser follows on the way down. - let depth = path.len(); - path.into_iter().enumerate().flat_map(move |(i, meta)| { + // the parser follows on the way down. The chain and not the tree: an earlier version + // collected globals from every branch it walked through, so a global declared on one command + // was suggested under an unrelated one — a tip naming a flag the parser would refuse, which + // is worse than no tip. + let depth = chain.len(); + chain.iter().enumerate().flat_map(move |(i, meta)| { let own = i + 1 == depth; meta.flags .iter() @@ -341,24 +316,53 @@ fn value_bound_to( last } -/// The command the words reached, which is the one an error is about. +/// The commands the words went through, root first, ending at the one an error is about. /// /// Walked rather than carried on the error: only some variants know their command, and a caller /// that has just been handed an error has the argv it came from. The walk stops where the parse /// stopped, which is the command whose usage line belongs in the message. -fn command_reached<'t>(root: &'t Command<'t>, argv: &[&std::ffi::OsStr]) -> &'t Command<'t> { +/// +/// The whole path and not just its end, because the end does not identify itself. One +/// `Subcommands` type mounted under two parents is one `Command` in both — the same address — +/// so a search of the metadata tree for that address finds whichever mount comes first. That is +/// how `ex beta shared --betaglobl` came back describing `ex alpha shared`, suggesting alpha's +/// globals and not beta's. The route is the only thing that tells the two apart, so the route is +/// what gets carried. +fn path_taken<'t>(root: &'t Command<'t>, argv: &[&std::ffi::OsStr]) -> Vec<&'t Command<'t>> { + let mut path = vec![root]; let mut parser = crate::Parser::new(root, argv); while let Some(event) = parser.next_event() { - if event.is_err() { - break; + match event { + Ok(crate::Event::Command(cmd)) => path.push(cmd), + Ok(_) => {} + Err(_) => break, } } - parser.command() + path } -/// The path to a command, as a user would type it, and its metadata. -fn found<'a>(spec: &'a Spec<'a>, cmd: &Command<'_>) -> Option<(Vec<&'a str>, &'a CommandMeta<'a>)> { - crate::help::find(spec, cmd) +/// The metadata for each command along a path, and the path as a user would type it. +/// +/// Each step is matched among *that command's* children, which is what makes it unambiguous: +/// two mounts of one `Subcommands` type share an address, but a parent's own child list is its +/// own. Returns nothing if the path leaves this spec, which cannot happen for a path this module +/// produced and is not worth a panic if it ever does. +fn resolve<'a>( + spec: &'a Spec<'a>, + path: &[&Command<'_>], +) -> Option<(Vec<&'a str>, Vec<&'a CommandMeta<'a>>)> { + let mut names = vec![spec.bin.unwrap_or(spec.name)]; + let mut chain = vec![spec.root]; + for cmd in path.iter().skip(1) { + let here = chain.last()?; + let next = here + .subcommands + .iter() + .find(|sub| core::ptr::eq(sub.cmd, *cmd))?; + names.push(next.cmd.name); + chain.push(next); + } + Some((names, chain)) } /// Render `error` the way a user should read it. @@ -372,13 +376,19 @@ pub fn render( error: &Error<'_, '_>, style: Style, ) -> String { - let cmd = command_reached(spec.root.cmd, argv); - let path = found(spec, cmd) - .map(|(path, _)| path.join(" ")) + let taken = path_taken(spec.root.cmd, argv); + let cmd = *taken.last().expect("the root is always on the path"); + let resolved = resolve(spec, &taken); + let chain: &[&CommandMeta<'_>] = resolved.as_ref().map(|(_, c)| &c[..]).unwrap_or(&[]); + let here = chain.last().copied(); + let path = resolved + .as_ref() + .map(|(names, _)| names.join(" ")) .unwrap_or_else(|| spec.bin.unwrap_or(spec.name).to_string()); - let usage = found(spec, cmd) - .map(|(path, meta)| crate::help::usage_line(&path, meta)) - .unwrap_or_else(|| path.clone()); + let usage = match (&resolved, here) { + (Some((names, _)), Some(meta)) => crate::help::usage_line(names, meta), + _ => path.clone(), + }; let mut out = String::new(); let mut with_usage = false; @@ -400,7 +410,7 @@ pub fn render( // `--fore` came out similar to `--quiet`, which it is not. clap compares the bare // names for the same reason. let bare = typed.trim_start_matches('-'); - let names: Vec<&str> = flags_in_scope(spec, cmd) + let names: Vec<&str> = flags_in_scope(chain) .flat_map(|meta| meta.flag.longs.iter().copied()) .collect(); let near: Vec = nearest(bare, names.into_iter()) @@ -432,7 +442,7 @@ pub fn render( style.invalid(named) ); let bare = named.trim_start_matches('-'); - let names: Vec<&str> = flags_in_scope(spec, cmd) + let names: Vec<&str> = flags_in_scope(chain) .flat_map(|meta| meta.flag.longs.iter().copied()) .collect(); let near: Vec = nearest(bare, names.into_iter()) @@ -479,11 +489,7 @@ pub fn render( "{} the following required arguments were not provided:", style.error("error:") ); - let _ = writeln!( - out, - " {}", - style.valid(&shown(found(spec, cmd).map(|(_, meta)| meta), name)) - ); + let _ = writeln!(out, " {}", style.valid(&shown(here, name))); } Error::MissingSubcommand => { with_usage = true; @@ -503,8 +509,8 @@ pub fn render( .map(|l| format!("--{l}")) .or_else(|| flag.shorts.first().map(|s| format!("-{}", *s as char))) .unwrap_or_else(|| flag.name.to_string()); - let value = found(spec, cmd) - .and_then(|(_, meta)| { + let value = here + .and_then(|meta| { meta.flags .iter() .find(|m| core::ptr::eq(m.flag, *flag)) @@ -520,7 +526,7 @@ pub fn render( ); } Error::InvalidChoice { name, choices } => { - let shown_name = shown(found(spec, cmd).map(|(_, meta)| meta), name); + let shown_name = shown(here, name); match value_bound_to(spec.root.cmd, argv, name, choices) { Some(value) => { let _ = writeln!( @@ -558,7 +564,7 @@ pub fn render( "{} invalid value '{}' for '{}': {}", style.error("error:"), style.invalid(&invalid.value), - style.literal(&shown(found(spec, cmd).map(|(_, m)| m), invalid.name)), + style.literal(&shown(here, invalid.name)), invalid.reason ); } @@ -577,7 +583,7 @@ pub fn render( out, "{} {min} values required for '{}' but {got} were provided", style.error("error:"), - style.literal(&shown(found(spec, cmd).map(|(_, m)| m), name)) + style.literal(&shown(here, name)) ); } Error::VarTooMany { name, max, got } => { @@ -585,7 +591,7 @@ pub fn render( out, "{} {max} values allowed for '{}' but {got} were provided", style.error("error:"), - style.literal(&shown(found(spec, cmd).map(|(_, m)| m), name)) + style.literal(&shown(here, name)) ); } Error::ArgRequiresDoubleDash { arg } => { diff --git a/conformance/tests/shared_subcommand.rs b/conformance/tests/shared_subcommand.rs new file mode 100644 index 000000000..f876e7438 --- /dev/null +++ b/conformance/tests/shared_subcommand.rs @@ -0,0 +1,166 @@ +//! One `Subcommands` type mounted under two parents. +//! +//! Legal, and the tables say nothing about which mount you are in: both parents splice the same +//! `&'static [Command]`, so `alpha shared` and `beta shared` are one `Command` at one address. +//! A lookup that searches the metadata tree for that address finds whichever mount comes first, +//! which is not the one the user typed. +//! +//! The route is the only thing that tells them apart, so diagnostics resolve by the route the +//! parser took rather than by the command it ended on. + +use std::ffi::OsStr; + +use usage_argv::diagnostic::{render, Style}; +use usage_derive::{Args, Cli, Subcommands}; + +/// Do the shared thing +#[derive(Args)] +struct Shared { + /// A flag of its own + #[usage(long)] + thing: bool, +} + +#[derive(Subcommands)] +enum Both { + /// Do the shared thing + Shared(Shared), +} + +/// The first parent +#[derive(Args)] +struct Alpha { + /// Only alpha declares this + #[usage(long, global)] + alphaglobal: bool, + #[usage(subcommand)] + command: Option, +} + +/// The second parent +#[derive(Args)] +struct Beta { + /// Only beta declares this + #[usage(long, global)] + betaglobal: bool, + #[usage(subcommand)] + command: Option, +} + +#[derive(Subcommands)] +enum Top { + /// The first parent + Alpha(Box), + /// The second parent + Beta(Box), +} + +/// A tool that mounts one set of subcommands twice +#[derive(Cli)] +#[usage(bin = "ex", unknown_flags = "error")] +struct Ex { + #[usage(subcommand)] + command: Option, +} + +#[test] +fn the_two_mounts_really_are_one_command() { + // The premise, asserted rather than assumed: if these ever stop sharing an address the test + // below still passes, and would stop testing anything. + let root = Ex::command(); + let alpha = root.subcommands.iter().find(|c| c.name == "alpha").unwrap(); + let beta = root.subcommands.iter().find(|c| c.name == "beta").unwrap(); + let a = alpha + .subcommands + .iter() + .find(|c| c.name == "shared") + .unwrap(); + let b = beta + .subcommands + .iter() + .find(|c| c.name == "shared") + .unwrap(); + assert!( + core::ptr::eq(a, b), + "the two mounts no longer share a command, so this file tests nothing" + ); +} + +#[test] +fn an_error_under_the_second_mount_describes_the_second_mount() { + let owned: Vec<&OsStr> = ["beta", "shared", "--betaglobl"] + .iter() + .map(|s| OsStr::new(*s)) + .collect(); + let Err(err) = Ex::parse_from(&owned) else { + panic!("no such flag") + }; + let message = render(Ex::spec(), &owned, &err, Style::PLAIN); + + // The usage line named the wrong command entirely: `ex alpha shared`, for a line that says + // `beta`. + assert!( + message.contains("Usage: ex beta shared"), + "the usage line is for the command that was typed: {message}" + ); + // And the tip came from the wrong ancestor's globals, so the flag that would have worked + // went unmentioned while a flag the parser refuses here was on offer. + assert!( + message.contains("tip: a similar argument exists: '--betaglobal'"), + "{message}" + ); + assert!( + !message.contains("alphaglobal"), + "a global from an unrelated branch: {message}" + ); +} + +#[test] +fn the_first_mount_is_unaffected() { + // The half that passed before, and has to keep passing: resolving by route must not make the + // first mount resolve like the second. + let owned: Vec<&OsStr> = ["alpha", "shared", "--alphaglobl"] + .iter() + .map(|s| OsStr::new(*s)) + .collect(); + let Err(err) = Ex::parse_from(&owned) else { + panic!("no such flag") + }; + let message = render(Ex::spec(), &owned, &err, Style::PLAIN); + assert!(message.contains("Usage: ex alpha shared"), "{message}"); + assert!( + message.contains("tip: a similar argument exists: '--alphaglobal'"), + "{message}" + ); +} + +#[test] +fn the_fields_are_bound_under_either_parent() { + // Also what keeps every field read, which CI requires of a test CLI: a field nothing looks + // at is dead code, and silencing that would let a declaration rot unnoticed. + let owned: Vec<&OsStr> = ["beta", "--betaglobal", "shared", "--thing"] + .iter() + .map(|s| OsStr::new(*s)) + .collect(); + let ex = Ex::parse_from(&owned).expect("should parse"); + let Some(Top::Beta(beta)) = ex.command else { + panic!("expected beta") + }; + assert!(beta.betaglobal, "a global binds after its own command"); + let Some(Both::Shared(shared)) = beta.command else { + panic!("expected shared") + }; + assert!(shared.thing); + + // And the same enum under the other parent, which is the point of the file. + let owned: Vec<&OsStr> = ["alpha", "--alphaglobal", "shared"] + .iter() + .map(|s| OsStr::new(*s)) + .collect(); + let ex = Ex::parse_from(&owned).expect("should parse"); + let Some(Top::Alpha(alpha)) = ex.command else { + panic!("expected alpha") + }; + assert!(alpha.alphaglobal); + assert!(matches!(alpha.command, Some(Both::Shared(_)))); +} From ba71a98d7aea5ffaaf35c8eb4153d4ccbbe82efe Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Sun, 16 Aug 2026 17:52:05 +0000 Subject: [PATCH 6/7] fix(argv): four from review, and a build that only breaks for adopters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `use<'a, 'c>` is Rust 1.82 and this crate declares 1.80, so the lifetime capture is written the way 1.80 spells it. Nothing in CI builds at the MSRV, which is why a declaration nobody can honour compiled here. `flag_spelling` is gated with the code that calls it. Under `--features spec` alone nothing does, so it was dead — a warning, and this workspace makes warnings errors. Every one of the eight feature combinations is clean now, which is the check that was missing rather than the gate. `ConflictingFlags` and `ArgRequiresDoubleDash` were the two variants still printing the spec's name while the ones directly above and below them did not, so one argument could appear two ways in two messages from the same command. clap writes the dashes here too: error: the argument '--force' cannot be used with '--jobs ' And the gate test's comment still said Jaro-Winkler, which is the opposite of what the code does and of why: clap uses plain `jaro` because `strsim`'s `jaro_winkler` is wrong (clap GH #4660). A comment a reader trusts and that says the opposite is worse than none. Found by CodeRabbit. Co-Authored-By: Claude Opus 5 --- argv/src/diagnostic.rs | 33 +++++++++++++++++++++++++++++---- argv/src/help.rs | 5 +++-- benches/gate/tests/errors.rs | 5 +++-- 3 files changed, 35 insertions(+), 8 deletions(-) diff --git a/argv/src/diagnostic.rs b/argv/src/diagnostic.rs index 0a65b9279..f1222e9af 100644 --- a/argv/src/diagnostic.rs +++ b/argv/src/diagnostic.rs @@ -120,7 +120,7 @@ fn flag_named(token: &str) -> &str { /// The same set the parser would have accepted, which is what makes a suggestion one that works. fn flags_in_scope<'a, 'c>( chain: &'c [&'a CommandMeta<'a>], -) -> impl Iterator> + use<'a, 'c> { +) -> impl Iterator> + 'c { // The command's own flags, and from each ancestor only what it declared global — the rule // the parser follows on the way down. The chain and not the tree: an earlier version // collected globals from every branch it walked through, so a global declared on one command @@ -569,12 +569,14 @@ pub fn render( ); } Error::ConflictingFlags { name, other } => { + // Spelled by `help`, like every other name in this module — and like clap, which + // writes `the argument '--force' cannot be used with '--jobs '`. let _ = writeln!( out, "{} the argument '{}' cannot be used with '{}'", style.error("error:"), - style.invalid(name), - style.invalid(other) + style.invalid(&shown(here, name)), + style.invalid(&shown(here, other)) ); with_usage = true; } @@ -600,7 +602,7 @@ pub fn render( out, "{} '{}' can only be given after '{}'", style.error("error:"), - style.literal(arg.name), + style.literal(&shown(here, arg.name)), style.literal("--") ); } @@ -891,6 +893,29 @@ mod tests { // `--jobs`. let message = rendered(&["use"], Error::MissingRequired { name: "jobs" }); assert!(message.contains(" --jobs"), "{message}"); + + // Every variant, not most of them. These two printed the spec's name while the ones + // directly above and below them did not, so one argument could appear two ways in two + // messages from the same command — and clap writes the dashes here too: + // + // error: the argument '--force' cannot be used with '--jobs ' + let message = rendered( + &["use"], + Error::ConflictingFlags { + name: "force", + other: "jobs", + }, + ); + assert!( + message.contains("the argument '--force' cannot be used with '--jobs'"), + "{message}" + ); + + let message = rendered(&["use"], Error::ArgRequiresDoubleDash { arg: &SHELLS }); + assert!( + message.contains("'[SHELLS]…' can only be given"), + "{message}" + ); } #[test] diff --git a/argv/src/help.rs b/argv/src/help.rs index cc36026c8..20bd34185 100644 --- a/argv/src/help.rs +++ b/argv/src/help.rs @@ -378,10 +378,11 @@ fn annotations(out: &mut String, choices: &[&str], env: Option<&str>, default: & out.push('\n'); } -/// A flag as the flags section lists it, which includes its negation. /// How a usage line writes a flag: its first long form, or its short if that is all it has. /// -/// Shared with the diagnostics for the same reason as [`arg_usage`]. +/// Shared with the diagnostics for the same reason as [`arg_usage`], and gated with them: under +/// `spec` alone nothing calls it, and a `dead_code` warning is an error in this workspace. +#[cfg(feature = "diagnostics")] pub(crate) fn flag_spelling(meta: &FlagMeta<'_>) -> String { meta.flag .longs diff --git a/benches/gate/tests/errors.rs b/benches/gate/tests/errors.rs index 3e96056c2..8dbf8b40f 100644 --- a/benches/gate/tests/errors.rs +++ b/benches/gate/tests/errors.rs @@ -117,8 +117,9 @@ fn the_usage_line_is_the_one_the_help_prints() { #[test] fn the_suggestion_is_the_one_clap_would_make() { - // Jaro-Winkler above 0.7, which is clap's rule — so the two suggest in the same cases and - // suggest the same thing. Checked against mise's real flags rather than a fixture, because + // Jaro above 0.7, which is clap's rule — plain Jaro, because clap says in its own source + // that `strsim`'s `jaro_winkler` is wrong (GH #4660). So the two suggest in the same cases + // and suggest the same thing. Checked against mise's real flags rather than a fixture, because // what makes a suggestion good is the size of the set it was chosen from. let cases = [ (vec!["activate", "zsx"], "a similar value exists: 'zsh'"), From db1ac64f0548dc7022e3d6bc003b7e98e92b93cf Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Sun, 16 Aug 2026 18:41:33 +0000 Subject: [PATCH 7/7] fix(argv): suggest a negation, which is a name that works MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--no-color` is a spelling the parser accepts — through `find_negation` — and one the completions already offer. The suggestions scored only a flag's `longs`, so a near miss of a negation got silence, which is the one thing a tip should never be when a working name is right there. clap has no separate notion of a negation, so `--color` and `--no-color` are two arguments there and it suggests either. Measured rather than assumed: error: unexpected argument '--no-colr' found tip: a similar argument exists: '--no-color' Found by Cursor Bugbot. Co-Authored-By: Claude Opus 5 --- argv/src/diagnostic.rs | 49 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 43 insertions(+), 6 deletions(-) diff --git a/argv/src/diagnostic.rs b/argv/src/diagnostic.rs index f1222e9af..8f63ff37e 100644 --- a/argv/src/diagnostic.rs +++ b/argv/src/diagnostic.rs @@ -115,6 +115,16 @@ fn flag_named(token: &str) -> &str { } } +/// Every long spelling a flag answers to, its negation included. +/// +/// The parser takes `--no-color` through `find_negation`, and the completions offer it, so a +/// suggestion that leaves it out is the odd one — a near miss of a name that works gets silence. +/// clap has no separate notion of a negation, so the two forms are two arguments there and it +/// suggests either; matching that is the point. +fn long_spellings<'a>(meta: &'a crate::spec::FlagMeta<'a>) -> impl Iterator { + meta.flag.longs.iter().copied().chain(meta.flag.negate) +} + /// Every flag a word at this command could have named: its own, then any ancestor's globals. /// /// The same set the parser would have accepted, which is what makes a suggestion one that works. @@ -410,9 +420,7 @@ pub fn render( // `--fore` came out similar to `--quiet`, which it is not. clap compares the bare // names for the same reason. let bare = typed.trim_start_matches('-'); - let names: Vec<&str> = flags_in_scope(chain) - .flat_map(|meta| meta.flag.longs.iter().copied()) - .collect(); + let names: Vec<&str> = flags_in_scope(chain).flat_map(long_spellings).collect(); let near: Vec = nearest(bare, names.into_iter()) .into_iter() .map(|name| format!("--{name}")) @@ -442,9 +450,7 @@ pub fn render( style.invalid(named) ); let bare = named.trim_start_matches('-'); - let names: Vec<&str> = flags_in_scope(chain) - .flat_map(|meta| meta.flag.longs.iter().copied()) - .collect(); + let names: Vec<&str> = flags_in_scope(chain).flat_map(long_spellings).collect(); let near: Vec = nearest(bare, names.into_iter()) .into_iter() .map(|name| format!("--{name}")) @@ -645,6 +651,9 @@ mod tests { name: "force", longs: &["force"], shorts: b"f", + // A negation, because a flag's spellings are not only its `longs` and the parser takes + // this one — so a suggestion that cannot offer it is offering less than the CLI accepts. + negate: Some("no-force"), ..Flag::BOOL }; static JOBS: Flag = Flag { @@ -995,6 +1004,34 @@ mod tests { ); } + #[test] + fn a_negation_is_suggested_like_any_other_spelling() { + // `--no-force` is a name the parser accepts, through `find_negation`, and one the + // completions already offer. Scoring only `longs` left it out, so a near miss of a name + // that works got silence — and clap, which has no separate notion of a negation and + // sees two arguments, suggests it. Measured: + // + // error: unexpected argument '--no-colr' found + // tip: a similar argument exists: '--no-color' + let message = rendered( + &["use"], + Error::UnknownFlag { + token: b"--no-forc", + }, + ); + assert!( + message.contains("tip: a similar argument exists: '--no-force'"), + "{message}" + ); + + // And the plain form is still found, which is the half that already worked. + let message = rendered(&["use"], Error::UnknownFlag { token: b"--fore" }); + assert!( + message.contains("tip: a similar argument exists: '--force'"), + "{message}" + ); + } + #[test] fn nothing_is_suggested_when_nothing_is_close() { // Offering `--force` for `--zzz` is worse than offering nothing: a user reads a tip as