diff --git a/argv/src/diagnostic.rs b/argv/src/diagnostic.rs index 6cda8645c..8f63ff37e 100644 --- a/argv/src/diagnostic.rs +++ b/argv/src/diagnostic.rs @@ -95,6 +95,173 @@ 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 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. +fn flags_in_scope<'a, 'c>( + chain: &'c [&'a CommandMeta<'a>], +) -> 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 + // 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() + .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, and deliberately not Jaro-Winkler. clap decides whether to suggest something with +/// `strsim::jaro`, and says why in its own source: +/// +/// ```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; + } + 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; + (matches / a.len() as f64 + + matches / b.len() as f64 + + (matches - transpositions as f64 / 2.0) / matches) + / 3.0 +} + +/// 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(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 @@ -159,24 +326,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. @@ -190,13 +386,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; @@ -205,20 +407,60 @@ pub fn render( // The shape of the command line: clap shows a usage block for these. Error::UnknownFlag { token } => { with_usage = true; + let whole = String::from_utf8_lossy(token); + let typed = flag_named(&whole); 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(chain).flat_map(long_spellings).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; 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 != "-" { + // 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(named) + ); + let bare = named.trim_start_matches('-'); + 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}")) + .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", @@ -232,6 +474,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 } => { @@ -241,11 +495,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; @@ -265,8 +515,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)) @@ -282,7 +532,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!( @@ -306,6 +556,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!( @@ -313,17 +570,19 @@ 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 ); } 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; } @@ -332,7 +591,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 } => { @@ -340,7 +599,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 } => { @@ -349,7 +608,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("--") ); } @@ -392,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 { @@ -417,9 +679,39 @@ 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 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", - subcommands: &[&USE], + 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 { @@ -455,9 +747,31 @@ mod tests { ], ..CommandMeta::EMPTY }; + 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, - subcommands: &[&USE_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 { @@ -473,18 +787,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 @@ -600,6 +902,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] @@ -623,10 +948,238 @@ 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 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 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 + // 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. `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("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] + 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}" + ); + } + #[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}" + ); + } + #[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}" + ); + } } 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 fe4845e34..8dbf8b40f 100644 --- a/benches/gate/tests/errors.rs +++ b/benches/gate/tests/errors.rs @@ -114,3 +114,85 @@ 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 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'"), + ( + 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"); +} + +#[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}"); +} 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(_)))); +}