From 0794fbdfa2efbc0538ea9719dfe9870d42c37de3 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Sat, 15 Aug 2026 14:20:39 +0000 Subject: [PATCH 1/2] feat(argv): say what went wrong the way clap says it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Today a parse failure reaches a user as `format!("{e:?}")` — `UnknownFlag { token: [45, 45, 102, 111, 114, 101] }`. mise's users read clap's errors, and the help on either side of this is already byte-identical to usage-lib's, so the error text is the last thing an adopter's users would notice changing. The aim is that they do not. The skeleton was measured from clap 4 rather than remembered, ANSI codes included: bold red for `error:`, yellow for what was typed, green for what would have worked, bold underline for `Usage:`. Which failures carry a usage block follows clap too — the ones about the shape of the command line do, the ones about a single value do not. Two deliberate departures, both stated in the module. The usage line is *ours*, the same one `--help` prints, because an error that disagrees with the help about how a command is spelled is worse than one that disagrees with clap. And a name is shown the way a usage line writes it — ``, `[SHELL_TYPE]` — which is what clap does and what the spec's own bare names are not. `Error::InvalidChoice` names the argument and not the offending value, deliberately: owning it would allocate on the one path this crate promises not to, and the doc comment says diagnostics are a separate layer. This is that layer, so it recovers the value by walking argv — which it can, because by then the parse has failed. Held against clap over mise's real spec: for six command lines that fail on both sides, the first line — the sentence a user actually reads — is identical, the footer is identical, and a usage block appears in the same cases. Colour follows the conventions rather than a flag: `NO_COLOR` wins over everything, `CLICOLOR_FORCE` is the other direction, and otherwise it depends on whether stderr is a terminal. Co-Authored-By: Claude Opus 5 --- argv/Cargo.toml | 3 + argv/src/diagnostic.rs | 546 +++++++++++++++++++++++++++++++++++ argv/src/lib.rs | 2 + benches/gate/Cargo.toml | 2 +- benches/gate/tests/errors.rs | 116 ++++++++ conformance/Cargo.toml | 2 +- 6 files changed, 669 insertions(+), 2 deletions(-) create mode 100644 argv/src/diagnostic.rs create mode 100644 benches/gate/tests/errors.rs diff --git a/argv/Cargo.toml b/argv/Cargo.toml index eacf8b70b..1cbedbbca 100644 --- a/argv/Cargo.toml +++ b/argv/Cargo.toml @@ -22,6 +22,9 @@ spec = [] # separate from `spec` because a CLI can want one without the other: completion needs # to split a line whether or not the binary can also emit its own spec. complete = ["spec"] +# What a user reads when a command line does not parse. Off by default like the rest: a CLI +# that renders its own errors should not carry these strings, and the parser stays a parser. +diagnostics = ["spec"] [package.metadata.release] shared-version = true diff --git a/argv/src/diagnostic.rs b/argv/src/diagnostic.rs new file mode 100644 index 000000000..f435624e6 --- /dev/null +++ b/argv/src/diagnostic.rs @@ -0,0 +1,546 @@ +//! What a user reads when a command line does not parse. +//! +//! Held to clap's shape on purpose. mise's users read clap's errors today, and the help output on +//! either side of this is already byte-identical to usage-lib's — so the error text is the last +//! thing an adopter's users would notice changing, and the aim is that they do not. +//! +//! The skeleton, measured from clap 4 rather than remembered: +//! +//! ```text +//! error: unexpected argument '--fore' found +//! +//! tip: a similar argument exists: '--force' +//! +//! Usage: mise use [OPTIONS] [TOOL@VERSION]… +//! +//! For more information, try '--help'. +//! ``` +//! +//! Two deliberate departures. The usage line is *ours* — the same one `--help` prints, rendered +//! from the spec — because an error that disagrees with the help about how a command is spelled is +//! worse than one that disagrees with clap. And which errors carry a usage block follows clap: +//! the ones about the shape of the command line do, the ones about a single value do not. + +use core::fmt::Write as _; + +use crate::spec::{CommandMeta, Spec}; +use crate::{Command, Error}; + +/// Whether to colour, and what with. +/// +/// The codes are clap's, so that a terminal shows the same thing: bold red for `error:`, yellow +/// for the offending text, green for a suggestion and for what was expected, bold underline for +/// `Usage:`. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct Style { + coloured: bool, +} + +impl Style { + /// Plain text, for a pipe or a test. + pub const PLAIN: Style = Style { coloured: false }; + /// Coloured, whatever the terminal is. + pub const COLOURED: Style = Style { coloured: true }; + + /// Colour when stderr is a terminal and the environment has not asked otherwise. + /// + /// `NO_COLOR` wins over everything, per the convention: a user who sets it has said once, for + /// every program, that they do not want this. `CLICOLOR_FORCE` is the other direction, for a + /// pipe that ends up somewhere that does render colour. + pub fn auto() -> Style { + use std::io::IsTerminal as _; + let forced = std::env::var_os("CLICOLOR_FORCE").is_some_and(|v| v != "0"); + let refused = std::env::var_os("NO_COLOR").is_some_and(|v| !v.is_empty()); + if refused { + return Style::PLAIN; + } + if forced || std::io::stderr().is_terminal() { + Style::COLOURED + } else { + Style::PLAIN + } + } + + fn wrap(self, code: &str, text: &str) -> String { + if self.coloured { + format!("\u{1b}[{code}m{text}\u{1b}[0m") + } else { + text.to_string() + } + } + + /// `error:`, and anything else that is the failure itself. + fn error(self, text: &str) -> String { + self.wrap("1m\u{1b}[31", text) + } + + /// What the user typed that did not work. + fn invalid(self, text: &str) -> String { + self.wrap("33", text) + } + + /// What would have worked: a suggestion, a possible value, a missing argument. + fn valid(self, text: &str) -> String { + self.wrap("32", text) + } + + /// A heading, such as `Usage:`. + fn heading(self, text: &str) -> String { + self.wrap("1m\u{1b}[4", text) + } + + /// Something to be typed as it is written. + fn literal(self, text: &str) -> String { + self.wrap("1", text) + } +} + +/// A name as a usage line writes it: `` when it must be filled, `[TOOL]` when it need not. +/// +/// The error carries the spec's name for a thing; a user reads the form the help shows. Looked up +/// on the command rather than guessed, and left alone when it names a flag — a flag already reads +/// as itself. +fn shown<'a>(meta: Option<&'a CommandMeta<'a>>, name: &str) -> String { + let Some(meta) = meta else { + return name.to_string(); + }; + match meta.args.iter().find(|a| a.arg.name == name) { + Some(arg) if arg.required => format!("<{name}>"), + Some(_) => format!("[{name}]"), + None => name.to_string(), + } +} + +/// The word that was bound to a named argument, recovered from argv. +/// +/// The parse itself does not carry it: an error that owned the offending text would allocate on +/// the one path this crate promises not to, so [`Error::InvalidChoice`] names the argument and +/// stops. Recovering it here is what that promise assumes — the diagnostics are a layer that may +/// do the work, and by the time one is being written the parse has already failed. +fn value_bound_to(root: &Command<'_>, argv: &[&std::ffi::OsStr], name: &str) -> Option { + let mut parser = crate::Parser::new(root, argv); + let mut found = None; + while let Some(event) = parser.next_event() { + match event { + Ok(crate::Event::Arg { arg, value }) if arg.name == name => { + found = Some(String::from_utf8_lossy(value).into_owned()); + } + Ok(crate::Event::Flag { + flag, + value: Some(value), + .. + }) if flag.name == name => { + found = Some(String::from_utf8_lossy(value).into_owned()); + } + Ok(_) => {} + Err(_) => break, + } + } + found +} + +/// The command the words reached, which is 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> { + let mut parser = crate::Parser::new(root, argv); + while let Some(event) = parser.next_event() { + if event.is_err() { + break; + } + } + parser.command() +} + +/// 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) +} + +/// Render `error` the way a user should read it. +/// +/// `argv` is what was being parsed, which is how the message finds the command to show a usage +/// line for. A [`Error::Help`] renders as nothing: it is not a failure, and a caller that has not +/// handled it before reaching here has a bug this cannot paper over. +pub fn render( + spec: &Spec<'_>, + argv: &[&std::ffi::OsStr], + error: &Error<'_, '_>, + style: Style, +) -> String { + let cmd = command_reached(spec.root.cmd, argv); + let path = found(spec, cmd) + .map(|(path, _)| path.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 mut out = String::new(); + let mut with_usage = false; + + match error { + // The shape of the command line: clap shows a usage block for these. + Error::UnknownFlag { token } => { + with_usage = true; + let _ = writeln!( + out, + "{} unexpected argument '{}' found", + style.error("error:"), + style.invalid(&String::from_utf8_lossy(token)) + ); + } + 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() { + let _ = writeln!( + out, + "{} unexpected argument '{}' found", + style.error("error:"), + style.invalid(&word) + ); + } else { + let _ = writeln!( + out, + "{} unrecognized subcommand '{}'", + style.error("error:"), + style.invalid(&word) + ); + } + } + Error::MissingRequired { name } => { + with_usage = true; + let _ = writeln!( + out, + "{} the following required arguments were not provided:", + style.error("error:") + ); + let _ = writeln!( + out, + " {}", + style.valid(&shown(found(spec, cmd).map(|(_, meta)| meta), name)) + ); + } + Error::MissingSubcommand => { + with_usage = true; + let _ = writeln!( + out, + "{} '{}' requires a subcommand but one was not provided", + style.error("error:"), + style.invalid(&path) + ); + } + + // About one value: clap shows no usage block, on the grounds that the shape was right. + Error::MissingFlagValue { flag } => { + let name = flag + .longs + .first() + .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)| { + meta.flags + .iter() + .find(|m| core::ptr::eq(m.flag, *flag)) + .and_then(|m| m.value_name) + }) + .map(|v| format!(" <{v}>")) + .unwrap_or_default(); + let _ = writeln!( + out, + "{} a value is required for '{}' but none was supplied", + style.error("error:"), + style.invalid(&format!("{name}{value}")) + ); + } + Error::InvalidChoice { name, choices } => { + let shown_name = shown(found(spec, cmd).map(|(_, meta)| meta), name); + match value_bound_to(spec.root.cmd, argv, name) { + Some(value) => { + let _ = writeln!( + out, + "{} invalid value '{}' for '{}'", + style.error("error:"), + style.invalid(&value), + style.literal(&shown_name) + ); + } + // Nothing in argv bound to it, which means the value came from somewhere else — + // an environment variable, or a default the spec declared. + None => { + let _ = writeln!( + out, + "{} invalid value for '{}'", + style.error("error:"), + style.literal(&shown_name) + ); + } + } + let listed: Vec = choices.iter().map(|c| style.valid(c)).collect(); + let _ = writeln!(out, " [possible values: {}]", listed.join(", ")); + } + Error::InvalidValue(invalid) => { + let _ = writeln!( + out, + "{} invalid value '{}' for '{}': {}", + style.error("error:"), + style.invalid(&invalid.value), + style.literal(invalid.name), + invalid.reason + ); + } + Error::ConflictingFlags { name, other } => { + let _ = writeln!( + out, + "{} the argument '{}' cannot be used with '{}'", + style.error("error:"), + style.invalid(name), + style.invalid(other) + ); + with_usage = true; + } + Error::VarTooFew { name, min, got } => { + let _ = writeln!( + out, + "{} {min} values required for '{}' but {got} were provided", + style.error("error:"), + style.literal(name) + ); + } + Error::VarTooMany { name, max, got } => { + let _ = writeln!( + out, + "{} {max} values allowed for '{}' but {got} were provided", + style.error("error:"), + style.literal(name) + ); + } + Error::ArgRequiresDoubleDash { arg } => { + with_usage = true; + let _ = writeln!( + out, + "{} '{}' can only be given after '{}'", + style.error("error:"), + style.literal(arg.name), + style.literal("--") + ); + } + Error::TooDeep => { + let _ = writeln!( + out, + "{} this command line nests deeper than the parser goes", + style.error("error:") + ); + } + // Not a failure. A caller reaching here with one has skipped handling it, and inventing a + // message would hide that rather than help. + Error::Help { .. } => return String::new(), + } + + if with_usage { + let _ = writeln!( + out, + "\n{} {}", + style.heading("Usage:"), + style.literal(&usage) + ); + } + let _ = writeln!( + out, + "\nFor more information, try '{}'.", + style.literal("--help") + ); + out +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::spec::{ArgMeta, FlagMeta}; + use crate::{Arg, Flag}; + + static FORCE: Flag = Flag { + key: 1, + name: "force", + longs: &["force"], + shorts: b"f", + ..Flag::BOOL + }; + static JOBS: Flag = Flag { + key: 2, + name: "jobs", + longs: &["jobs"], + ..Flag::VALUE + }; + static TOOL: Arg = Arg { + key: 3, + name: "TOOL", + ..Arg::REQUIRED + }; + static USE: Command = Command { + name: "use", + flags: &[&FORCE, &JOBS], + args: &[&TOOL], + ..Command::EMPTY + }; + static ROOT: Command = Command { + name: "ex", + subcommands: &[&USE], + ..Command::EMPTY + }; + static USE_META: CommandMeta = CommandMeta { + cmd: &USE, + about: Some("Use a tool"), + flags: &[ + FlagMeta { + flag: &FORCE, + help: Some("Force it"), + ..FlagMeta::EMPTY + }, + FlagMeta { + flag: &JOBS, + help: Some("How many"), + value_name: Some("JOBS"), + ..FlagMeta::EMPTY + }, + ], + args: &[ArgMeta { + arg: &TOOL, + help: Some("Which tool"), + required: true, + ..ArgMeta::EMPTY + }], + ..CommandMeta::EMPTY + }; + static ROOT_META: CommandMeta = CommandMeta { + cmd: &ROOT, + subcommands: &[&USE_META], + ..CommandMeta::EMPTY + }; + static SPEC: Spec = Spec { + name: "ex", + bin: Some("ex"), + root: &ROOT_META, + ..Spec::EMPTY + }; + + fn rendered(words: &[&str], error: Error<'static, 'static>) -> String { + let owned: Vec = words.iter().map(std::ffi::OsString::from).collect(); + let argv: Vec<&std::ffi::OsStr> = owned.iter().map(|o| o.as_os_str()).collect(); + 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 ] \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 + // help about how a command is written is worse than one that disagrees with clap. + let message = rendered(&["use"], Error::UnknownFlag { token: b"--fore" }); + let line = message + .lines() + .find_map(|l| l.strip_prefix("Usage: ")) + .expect("a usage line"); + assert_eq!(line, crate::help::usage_line(&["ex", "use"], &USE_META)); + } + + #[test] + fn a_missing_value_names_what_it_wanted() { + // No usage block: the shape of the command line was right, one value was missing — which + // is the distinction clap draws too. + assert_eq!( + rendered(&["use"], Error::MissingFlagValue { flag: &JOBS }), + "error: a value is required for '--jobs ' but none was supplied\n\ + \n\ + For more information, try '--help'.\n" + ); + } + + #[test] + fn a_word_where_a_subcommand_was_expected_says_so() { + // The root has subcommands, so an unexpected word there is an unrecognized subcommand; + // inside `use`, which has none, the same error is an unexpected argument. + let at_root = rendered(&[], Error::UnexpectedArg { token: b"nonesuch" }); + assert!( + at_root.starts_with("error: unrecognized subcommand 'nonesuch'"), + "{at_root}" + ); + let in_use = rendered(&["use"], Error::UnexpectedArg { token: b"extra" }); + assert!( + in_use.starts_with("error: unexpected argument 'extra' found"), + "{in_use}" + ); + } + + #[test] + fn a_required_argument_is_listed_the_way_clap_lists_it() { + assert_eq!( + rendered(&["use"], Error::MissingRequired { name: "" }), + "error: the following required arguments were not provided:\n \ + \n\ + \n\ + Usage: ex use [-f --force] [--jobs ] \n\ + \n\ + For more information, try '--help'.\n" + ); + } + + #[test] + fn colour_is_the_same_codes_clap_uses() { + // Measured from clap 4 rather than remembered: bold red for `error:`, yellow for what was + // typed, bold underline for `Usage:`, bold for what to type. + let owned = [std::ffi::OsString::from("use")]; + let argv: Vec<&std::ffi::OsStr> = owned.iter().map(|o| o.as_os_str()).collect(); + let message = render( + &SPEC, + &argv, + &Error::UnknownFlag { token: b"--fore" }, + Style::COLOURED, + ); + assert!( + message.starts_with("\u{1b}[1m\u{1b}[31merror:\u{1b}[0m"), + "{message:?}" + ); + assert!(message.contains("\u{1b}[33m--fore\u{1b}[0m"), "{message:?}"); + assert!( + message.contains("\u{1b}[1m\u{1b}[4mUsage:\u{1b}[0m"), + "{message:?}" + ); + // And nothing at all when plain, which is what a pipe or a test gets. + assert!(!rendered(&["use"], Error::UnknownFlag { token: b"--fore" }).contains('\u{1b}')); + } + + #[test] + fn a_help_request_renders_nothing() { + // It is not a failure, and a caller that reaches here with one has skipped handling it — + // which a message would hide rather than help. + assert_eq!( + rendered( + &["use"], + Error::Help { + cmd: &USE, + long: true + } + ), + "" + ); + } +} diff --git a/argv/src/lib.rs b/argv/src/lib.rs index e68ee77d6..60deef8d7 100644 --- a/argv/src/lib.rs +++ b/argv/src/lib.rs @@ -85,6 +85,8 @@ use std::ffi::{OsStr, OsString}; #[cfg(feature = "complete")] pub mod complete; +#[cfg(feature = "diagnostics")] +pub mod diagnostic; #[cfg(feature = "complete")] pub mod script; diff --git a/benches/gate/Cargo.toml b/benches/gate/Cargo.toml index 99647ec1c..a1720f0db 100644 --- a/benches/gate/Cargo.toml +++ b/benches/gate/Cargo.toml @@ -13,7 +13,7 @@ publish = false clap = { version = "4", features = ["derive", "env"] } shadow-mise = { path = "../shadows/mise" } shadow-mise-clap = { path = "../shadows/mise-clap" } -usage-argv = { path = "../../argv", features = ["spec", "complete"] } +usage-argv = { path = "../../argv", features = ["spec", "complete", "diagnostics"] } # The oracle for help rendering. A dev-dependency because nothing the gate *builds* needs it: # it is here so the shadow's rendered help can be compared against usage-lib's over mise's diff --git a/benches/gate/tests/errors.rs b/benches/gate/tests/errors.rs new file mode 100644 index 000000000..fe4845e34 --- /dev/null +++ b/benches/gate/tests/errors.rs @@ -0,0 +1,116 @@ +//! Does a failure read the way clap's reads, at mise's scale? +//! +//! An adopter's users read clap's errors today. The help either side of this is already +//! byte-identical to usage-lib's, so the error text is the last thing they would notice changing +//! — and the way to know is to fail the same command line on both sides and compare. +//! +//! Not byte-equality, deliberately. The usage line is *ours*, rendered from the spec exactly as +//! `--help` renders it, because an error that disagrees with the help about how a command is +//! spelled is worse than one that disagrees with clap. So what is compared is the first line — +//! the sentence a user actually reads — plus the shape around it. + +use clap::Parser; +use std::ffi::{OsStr, OsString}; +use usage_argv::diagnostic::{render, Style}; + +/// clap's message for a command line, without colour. +fn clap_error(words: &[&str]) -> Option { + let mut argv = vec!["mise"]; + argv.extend_from_slice(words); + match shadow_mise_clap::Cli::try_parse_from(&argv) { + Ok(_) => None, + Err(e) => Some(e.render().to_string()), + } +} + +/// Ours for the same command line. +fn our_error(words: &[&str]) -> Option { + let owned: Vec = words.iter().map(OsString::from).collect(); + let argv: Vec<&OsStr> = owned.iter().map(|o| o.as_os_str()).collect(); + match shadow_mise::Cli::parse_from(&argv) { + Ok(_) => None, + Err(error) => Some(render( + shadow_mise::Cli::spec(), + &argv, + &error, + Style::PLAIN, + )), + } +} + +fn first_line(message: &str) -> &str { + message.lines().next().unwrap_or_default() +} + +#[test] +fn the_sentence_a_user_reads_is_the_one_clap_wrote() { + // Each of these fails on both sides, and the failure is the same failure — so the line naming + // it should be the same line. + for words in [ + vec!["use", "--jobs"], + vec!["config", "nonesuch"], + vec!["plugins", "link"], + vec!["activate", "zsx"], + vec!["settings", "get"], + vec!["alias", "get"], + ] { + let theirs = clap_error(&words).unwrap_or_else(|| panic!("clap parsed {words:?}")); + let ours = our_error(&words).unwrap_or_else(|| panic!("we parsed {words:?}")); + assert_eq!( + first_line(&ours), + first_line(&theirs), + "\n{words:?}\n ours: {ours}\n theirs: {theirs}" + ); + } +} + +#[test] +fn the_message_ends_the_way_clap_ends_one() { + let words = ["config", "nonesuch"]; + let ours = our_error(&words).expect("a failure"); + let theirs = clap_error(&words).expect("a failure"); + let footer = "For more information, try '--help'."; + assert!(ours.trim_end().ends_with(footer), "{ours}"); + assert!(theirs.trim_end().ends_with(footer), "{theirs}"); +} + +#[test] +fn a_usage_block_appears_where_clap_shows_one() { + // The line differs — ours is the spec's — but whether there *is* one is a decision about how + // much to say, and that should match. + for words in [ + vec!["config", "nonesuch"], + vec!["plugins", "link"], + vec!["use", "--jobs"], + vec!["activate", "zsx"], + ] { + let ours = our_error(&words).expect("a failure"); + let theirs = clap_error(&words).expect("a failure"); + assert_eq!( + ours.contains("\nUsage: "), + theirs.contains("\nUsage: "), + "{words:?}\n ours: {ours}\n theirs: {theirs}" + ); + } +} + +#[test] +fn the_usage_line_is_the_one_the_help_prints() { + // Which is the deliberate difference from clap: an error and a help page describing the same + // command differently is the confusing kind of inconsistency. + let ours = our_error(&["config", "nonesuch"]).expect("a failure"); + let line = ours + .lines() + .find_map(|l| l.strip_prefix("Usage: ")) + .expect("a usage line"); + let root = shadow_mise::Cli::spec().root; + let config = root + .subcommands + .iter() + .find(|s| s.cmd.name == "config") + .expect("mise config"); + assert_eq!( + line, + usage_argv::help::usage_line(&["mise", "config"], config) + ); +} diff --git a/conformance/Cargo.toml b/conformance/Cargo.toml index ab05f7abb..f3e864066 100644 --- a/conformance/Cargo.toml +++ b/conformance/Cargo.toml @@ -15,7 +15,7 @@ license = { workspace = true } kdl = "6" serde = { version = "1", features = ["derive"] } serde_json = "1" -usage-argv = { workspace = true, features = ["spec", "complete"] } +usage-argv = { workspace = true, features = ["spec", "complete", "diagnostics"] } usage-config = { workspace = true } usage-lib = { workspace = true } From 9a40c8f71c037bfff5b5864ea88e19c56a2df8b6 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Sat, 15 Aug 2026 15:24:29 +0000 Subject: [PATCH 2/2] fix(argv): name a thing in an error the way the help names it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four from review, all the same root: the error text was deciding for itself how to spell an argument instead of asking the module that already knows. `help` now shares the two rules it had — how a usage line writes an argument, and how it writes a flag — and every error goes through them. So a default makes an argument optional in both places, a variadic keeps its ellipsis, an argument that needs a separator keeps its `[-- …]`, and a flag is spelled with its dashes: the spec calls it `jobs`, a user reads `--jobs`. `InvalidValue`, `VarTooFew` and `VarTooMany` printed the bare name while two other variants did not, so the same argument could appear as `TOOL` in one message and `` in another. And the value an invalid-choice error names is now the one that was *refused*. A variadic may be given several, the check stops at the first that is not allowed, and reporting whichever came last named a value that was perfectly good while leaving the wrong one unmentioned. Found by greptile and Cursor Bugbot. Co-Authored-By: Claude Opus 5 --- argv/src/diagnostic.rs | 150 ++++++++++++++++++++++++++++++++--------- argv/src/help.rs | 23 ++++++- 2 files changed, 140 insertions(+), 33 deletions(-) diff --git a/argv/src/diagnostic.rs b/argv/src/diagnostic.rs index f435624e6..6cda8645c 100644 --- a/argv/src/diagnostic.rs +++ b/argv/src/diagnostic.rs @@ -95,20 +95,30 @@ impl Style { } } -/// A name as a usage line writes it: `` when it must be filled, `[TOOL]` when it need not. +/// 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. Looked up -/// on the command rather than guessed, and left alone when it names a flag — a flag already reads -/// as itself. +/// The error carries the spec's name for a thing; a user reads the form the help shows. Both come +/// from `help` rather than being decided again here — an error and the page above it describing +/// one argument differently is the confusing kind of inconsistency, and rewriting the rule is how +/// that happens. A flag is spelled with its dashes, which is the whole of what `--jobs` versus +/// `jobs` is about. fn shown<'a>(meta: Option<&'a CommandMeta<'a>>, name: &str) -> String { let Some(meta) = meta else { return name.to_string(); }; - match meta.args.iter().find(|a| a.arg.name == name) { - Some(arg) if arg.required => format!("<{name}>"), - Some(_) => format!("[{name}]"), - None => name.to_string(), + if let Some(arg) = meta.args.iter().find(|a| a.arg.name == name) { + return crate::help::arg_usage(arg); } + // A flag can be named by an error too — a missing required one, or a value that is not among + // its choices — and the spec's name for it has no dashes. + if let Some(flag) = meta + .flags + .iter() + .find(|f| f.flag.name == name || f.value_name == Some(name)) + { + return crate::help::flag_spelling(flag); + } + name.to_string() } /// The word that was bound to a named argument, recovered from argv. @@ -117,26 +127,36 @@ fn shown<'a>(meta: Option<&'a CommandMeta<'a>>, name: &str) -> String { /// the one path this crate promises not to, so [`Error::InvalidChoice`] names the argument and /// stops. Recovering it here is what that promise assumes — the diagnostics are a layer that may /// do the work, and by the time one is being written the parse has already failed. -fn value_bound_to(root: &Command<'_>, argv: &[&std::ffi::OsStr], name: &str) -> Option { +fn value_bound_to( + root: &Command<'_>, + argv: &[&std::ffi::OsStr], + name: &str, + refused: &[&str], +) -> Option { let mut parser = crate::Parser::new(root, argv); - let mut found = None; + let mut last = None; while let Some(event) = parser.next_event() { - match event { - Ok(crate::Event::Arg { arg, value }) if arg.name == name => { - found = Some(String::from_utf8_lossy(value).into_owned()); - } + let value = match event { + Ok(crate::Event::Arg { arg, value }) if arg.name == name => value, Ok(crate::Event::Flag { flag, value: Some(value), .. - }) if flag.name == name => { - found = Some(String::from_utf8_lossy(value).into_owned()); - } - Ok(_) => {} + }) if flag.name == name => value, + Ok(_) => continue, Err(_) => break, + }; + let value = String::from_utf8_lossy(value).into_owned(); + // The *offending* one, not the last. A repeatable flag or a variadic argument may be + // given several values, and the check refuses the first that is not allowed — reporting + // whichever came last would name a value that is perfectly good and leave the wrong one + // unmentioned. + if !refused.is_empty() && !refused.contains(&value.as_str()) { + return Some(value); } + last = Some(value); } - found + last } /// The command the words reached, which is the one an error is about. @@ -263,7 +283,7 @@ pub fn render( } Error::InvalidChoice { name, choices } => { let shown_name = shown(found(spec, cmd).map(|(_, meta)| meta), name); - match value_bound_to(spec.root.cmd, argv, name) { + match value_bound_to(spec.root.cmd, argv, name, choices) { Some(value) => { let _ = writeln!( out, @@ -293,7 +313,7 @@ pub fn render( "{} invalid value '{}' for '{}': {}", style.error("error:"), style.invalid(&invalid.value), - style.literal(invalid.name), + style.literal(&shown(found(spec, cmd).map(|(_, m)| m), invalid.name)), invalid.reason ); } @@ -312,7 +332,7 @@ pub fn render( out, "{} {min} values required for '{}' but {got} were provided", style.error("error:"), - style.literal(name) + style.literal(&shown(found(spec, cmd).map(|(_, m)| m), name)) ); } Error::VarTooMany { name, max, got } => { @@ -320,7 +340,7 @@ pub fn render( out, "{} {max} values allowed for '{}' but {got} were provided", style.error("error:"), - style.literal(name) + style.literal(&shown(found(spec, cmd).map(|(_, m)| m), name)) ); } Error::ArgRequiresDoubleDash { arg } => { @@ -385,10 +405,16 @@ mod tests { name: "TOOL", ..Arg::REQUIRED }; + /// Variadic and choice-bearing, so "which value was refused" has a wrong answer available. + static SHELLS: Arg = Arg { + key: 7, + name: "SHELLS", + ..Arg::VAR + }; static USE: Command = Command { name: "use", flags: &[&FORCE, &JOBS], - args: &[&TOOL], + args: &[&TOOL, &SHELLS], ..Command::EMPTY }; static ROOT: Command = Command { @@ -412,12 +438,21 @@ mod tests { ..FlagMeta::EMPTY }, ], - args: &[ArgMeta { - arg: &TOOL, - help: Some("Which tool"), - required: true, - ..ArgMeta::EMPTY - }], + args: &[ + ArgMeta { + arg: &TOOL, + help: Some("Which tool"), + required: true, + ..ArgMeta::EMPTY + }, + ArgMeta { + arg: &SHELLS, + help: Some("Which shells"), + choices: &["bash", "zsh"], + required: false, + ..ArgMeta::EMPTY + }, + ], ..CommandMeta::EMPTY }; static ROOT_META: CommandMeta = CommandMeta { @@ -444,7 +479,7 @@ mod tests { rendered(&["use"], Error::UnknownFlag { token: b"--fore" }), "error: unexpected argument '--fore' found\n\ \n\ - Usage: ex use [-f --force] [--jobs ] \n\ + Usage: ex use [-f --force] [--jobs ] [SHELLS]…\n\ \n\ For more information, try '--help'.\n" ); @@ -497,7 +532,7 @@ mod tests { "error: the following required arguments were not provided:\n \ \n\ \n\ - Usage: ex use [-f --force] [--jobs ] \n\ + Usage: ex use [-f --force] [--jobs ] [SHELLS]…\n\ \n\ For more information, try '--help'.\n" ); @@ -543,4 +578,55 @@ mod tests { "" ); } + #[test] + fn a_name_is_spelled_the_way_the_help_spells_it() { + // One rule, taken from `help` rather than decided again here: an error and the page above + // it describing the same argument differently is the confusing kind of inconsistency. + let message = rendered(&["use"], Error::MissingRequired { name: "TOOL" }); + assert!(message.contains(" "), "{message}"); + + // A variadic keeps its ellipsis, exactly as the usage line writes it. + let message = rendered( + &["use"], + Error::VarTooFew { + name: "SHELLS", + min: 2, + got: 1, + }, + ); + assert!(message.contains("'[SHELLS]…'"), "{message}"); + + // And a *flag* is spelled with its dashes: the spec calls it `jobs`, a user reads + // `--jobs`. + let message = rendered(&["use"], Error::MissingRequired { name: "jobs" }); + assert!(message.contains(" --jobs"), "{message}"); + } + + #[test] + fn the_value_named_is_the_one_that_was_refused() { + // A variadic given several values: the check refuses the first that is not allowed, so + // naming whichever came last would name a value that is perfectly good and leave the + // wrong one unmentioned. + let owned = [ + std::ffi::OsString::from("use"), + std::ffi::OsString::from("node"), + std::ffi::OsString::from("fsh"), + std::ffi::OsString::from("zsh"), + ]; + let argv: Vec<&std::ffi::OsStr> = owned.iter().map(|o| o.as_os_str()).collect(); + let message = render( + &SPEC, + &argv, + &Error::InvalidChoice { + name: "SHELLS", + choices: &["bash", "zsh"], + }, + Style::PLAIN, + ); + assert!(message.contains("invalid value 'fsh'"), "{message}"); + assert!( + !message.contains("'zsh'\n"), + "named a value that was fine: {message}" + ); + } } diff --git a/argv/src/help.rs b/argv/src/help.rs index 159c5b5d2..cc36026c8 100644 --- a/argv/src/help.rs +++ b/argv/src/help.rs @@ -185,7 +185,11 @@ fn demanded(meta: &ArgMeta<'_>) -> bool { meta.required && meta.default.is_empty() } -fn arg_usage(meta: &ArgMeta<'_>) -> String { +/// How a usage line writes an argument: ``, `[TOOL]`, `[TOOL]…`, `[-- COMMAND]…`. +/// +/// Shared with the diagnostics, which name the same argument in an error and must not spell it +/// differently from the page above it. +pub(crate) fn arg_usage(meta: &ArgMeta<'_>) -> String { let arg = meta.arg; let mut out = String::new(); let (open, close) = if demanded(meta) { @@ -375,6 +379,23 @@ fn annotations(out: &mut String, choices: &[&str], env: Option<&str>, default: & } /// 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`]. +pub(crate) fn flag_spelling(meta: &FlagMeta<'_>) -> String { + meta.flag + .longs + .first() + .map(|long| format!("--{long}")) + .or_else(|| { + meta.flag + .shorts + .first() + .map(|short| format!("-{}", *short as char)) + }) + .unwrap_or_else(|| meta.flag.name.to_string()) +} + fn display_usage(meta: &FlagMeta<'_>) -> String { let usage = flag_usage(meta); match meta.flag.negate {