diff --git a/argv/src/help.rs b/argv/src/help.rs index 377bcfffd..9477971ed 100644 --- a/argv/src/help.rs +++ b/argv/src/help.rs @@ -939,6 +939,135 @@ pub fn find<'a>( walk(&mut path, &mut chain, spec.root, cmd).then_some((path, chain)) } +/// The entries for `--help` and `--version`, which the parser supplies and no spec declares. +/// +/// Listed because help is written for people: a reader looking for how to get help should find +/// it on the page. This reverses the rule these two used to follow — that a page lists exactly +/// what its spec declares — and the reason is that the spec has its own readers, and they are +/// not the ones reading this. +/// +/// Four spellings each, because a CLI may have claimed either form for itself. The parser +/// yields to a declaration (`in_scope` looks a command's own flags up first), so a page that +/// claimed otherwise would be describing a flag that never binds. +mod supplied { + use crate::spec::FlagMeta; + use crate::Flag; + + macro_rules! entry { + ($name:ident, $flag:ident, $key:expr, $label:expr, $longs:expr, $shorts:expr, $help:expr) => { + static $flag: Flag<'static> = Flag { + key: $key, + name: $label, + longs: $longs, + shorts: $shorts, + ..Flag::BOOL + }; + pub static $name: FlagMeta<'static> = FlagMeta { + flag: &$flag, + help: Some($help), + ..FlagMeta::EMPTY + }; + }; + } + + entry!( + HELP_BOTH, + HB, + crate::HELP_LONG_KEY, + "help", + &["help"], + b"h", + "Print help" + ); + entry!( + HELP_LONG_ONLY, + HL, + crate::HELP_LONG_KEY, + "help", + &["help"], + b"", + "Print help" + ); + // Named `h`, not `help`: the declared name is judged against the forms the entry shows, + // and a short-only entry called `help` reads as a renamed flag — it printed `help: -h`. + entry!( + HELP_SHORT_ONLY, + HS, + crate::HELP_SHORT_KEY, + "h", + &[], + b"h", + "Print help" + ); + entry!( + VERSION_BOTH, + VB, + crate::VERSION_LONG_KEY, + "version", + &["version"], + b"V", + "Print version" + ); + entry!( + VERSION_LONG_ONLY, + VL, + crate::VERSION_LONG_KEY, + "version", + &["version"], + b"", + "Print version" + ); + entry!( + VERSION_SHORT_ONLY, + VS, + crate::VERSION_SHORT_KEY, + "V", + &[], + b"V", + "Print version" + ); +} + +/// The supplied entries a page should list, given what the command already claims. +/// +/// `--version` only where the parser actually accepts it: on a command whose table says so, +/// which the derive sets on the root when a version is declared. A page offering one that the +/// parser would refuse is worse than a page that stays quiet. +fn supplied_entries(cmd: &Command<'_>, taken: &[String]) -> Vec<&'static FlagMeta<'static>> { + // Against the same set every other decision on this page uses, so a spelling claimed by a + // hidden declaration or by a negation is claimed here too. Offering a `--help` that + // something else binds is exactly the lie the model exists to prevent. + let pick = |long: &str, short: char, both, l, s| match ( + taken.contains(&format!("--{long}")), + taken.contains(&format!("-{short}")), + ) { + (true, true) => None, + (true, false) => Some(s), + (false, true) => Some(l), + (false, false) => Some(both), + }; + + let mut out = Vec::new(); + out.extend(pick( + "help", + 'h', + &supplied::HELP_BOTH, + &supplied::HELP_LONG_ONLY, + &supplied::HELP_SHORT_ONLY, + )); + // Only where the parser accepts one, which is the root of a CLI that declared a version. + if cmd.version { + out.extend(pick( + "version", + 'V', + &supplied::VERSION_BOTH, + &supplied::VERSION_LONG_ONLY, + &supplied::VERSION_SHORT_ONLY, + )); + } + out +} + /// Every flag a page should list, split into the command's own and the ones it inherits. /// /// The rule the parser follows on the way down, and the same one the diagnostics suggest @@ -1022,6 +1151,22 @@ fn own_and_global<'a>( }) .collect(); + // Last in the command's own section, which is where clap has them: they carry no + // `help_heading`, so a CLI that groups its flags gets them at the end of the ungrouped + // list rather than inside somebody's section. + // + // Given `taken` rather than the two lists: that set already counts hidden declarations and + // negations, and a `--help` the page offers while something else binds it is exactly the + // lie this whole model exists to prevent. + let mut own = own; + // Forms *and* negations: `long_flag` asks `find_negation` before it offers `--version`, + // so a declared negation beats a supplied flag even though it loses to a long. + let claimed: Vec = taken + .iter() + .cloned() + .chain(taken_negations.iter().cloned()) + .collect(); + own.extend(supplied_entries(here.cmd, &claimed)); (own, inherited) } diff --git a/conformance/tests/help_request.rs b/conformance/tests/help_request.rs index 8da0d2fb1..4d3d87fb9 100644 --- a/conformance/tests/help_request.rs +++ b/conformance/tests/help_request.rs @@ -100,13 +100,15 @@ fn asking_for_help_does_not_stop_the_cli_working() { } #[test] -fn a_help_flag_is_not_in_the_spec_it_renders() { - // The two are supplied by the parser and belong to no command's metadata, so they cannot - // appear in help output or in the emitted spec. A page advertising a `--help` that the spec - // it came from does not declare is a page that disagrees with its own CLI. +fn a_help_flag_is_listed_but_still_not_declared() { + // The split that replaced "a page lists exactly what its spec declares". The page names + // `-h, --help`, because help is written for people and someone looking for how to ask for + // help should find it there. The *spec* still does not declare it: the parser supplies it, + // and a spec claiming otherwise would have every reader inventing a flag its CLI never + // declared. let (_, page) = ask(&["--help"]); - assert!(!page.contains("--help"), "{page}"); - assert!(!page.contains("-h "), "{page}"); + assert!(page.contains("-h, --help"), "{page}"); + assert!(page.contains("Print help"), "{page}"); let kdl = Ex::to_kdl(); assert!(!kdl.contains("help\""), "{kdl}"); diff --git a/conformance/tests/version.rs b/conformance/tests/version.rs index c85af29df..c3937d822 100644 --- a/conformance/tests/version.rs +++ b/conformance/tests/version.rs @@ -187,24 +187,14 @@ fn a_declared_long_wins_too() { } #[test] -fn the_flag_is_not_in_the_help_page_or_the_spec() { - // Supplied rather than declared, exactly as `--help` is. A page listing a flag the spec - // does not declare is a page that disagrees with the spec it was rendered from. +fn the_flag_is_listed_but_still_not_declared() { + // Listed, because a reader looking for the version should find it where they are looking. + // Not *declared*: the parser supplies it, and a spec claiming otherwise would have every + // reader inventing a flag its CLI never wrote. let page = usage_argv::help::render(Versioned::spec(), Versioned::spec().root.cmd, true) .expect("a page"); - assert!(!page.contains("--version"), "{page}"); - - // Asserted on the metadata rather than by searching the page for `-V`: `go` declares one of - // its own, and it is listed — correctly — in the commands summary. What must be absent is a - // *root* flag nobody declared. - assert!( - !Versioned::spec() - .root - .flags - .iter() - .any(|f| f.flag.shorts.contains(&b'V') || f.flag.longs.contains(&"version")), - "the root lists a flag it does not declare" - ); + assert!(page.contains("-V, --version"), "{page}"); + assert!(page.contains("Print version"), "{page}"); let kdl = Versioned::to_kdl(); assert!(!kdl.contains("--version"), "{kdl}"); @@ -212,6 +202,40 @@ fn the_flag_is_not_in_the_help_page_or_the_spec() { assert!(kdl.contains(r#"version "1.2.3""#), "{kdl}"); } +#[test] +fn a_command_that_cannot_answer_does_not_offer() { + // `--version` is the root's, so a subcommand's page must not list one: the parser would + // refuse it there, and a page offering a flag that gets refused is worse than a quiet one. + let root = Versioned::spec().root.cmd; + let other = root + .subcommands + .iter() + .find(|c| c.name == "other") + .expect("other"); + let page = usage_argv::help::render(Versioned::spec(), other, true).expect("a page"); + assert!(!page.contains("--version"), "{page}"); + // And help, which every command does answer, is still there. + assert!(page.contains("-h, --help"), "{page}"); + + // Nor does a CLI that declares no version at all. + let page = usage_argv::help::render(Unversioned::spec(), Unversioned::spec().root.cmd, true) + .expect("a page"); + assert!(!page.contains("--version"), "{page}"); +} + +#[test] +fn a_claimed_spelling_is_not_offered_twice() { + // `OwnShort` takes `-V` for `--verbose`. The page must show the spelling that is still + // free and not claim the one that is taken, or it would describe a flag that never binds. + let page = usage_argv::help::render(OwnShort::spec(), OwnShort::spec().root.cmd, true) + .expect("a page"); + assert!(page.contains("--version"), "{page}"); + assert!( + !page.contains("-V, --version"), + "`-V` belongs to `--verbose` here: {page}" + ); +} + #[test] fn the_fields_are_bound() { let a = argv(&["--quiet"]); @@ -250,3 +274,46 @@ fn a_failure_renders_the_way_a_user_should_read_it() { "{message}" ); } + +/// Takes `--help` for itself, with a flag nobody can see +#[derive(Cli)] +#[usage(bin = "hidden-help", version = "1.0")] +struct HiddenHelp { + /// Hidden, and still binds — so the page must not offer `--help` as its own + #[usage(long = "help", hide)] + help_of_its_own: bool, + /// Its negation is `-V`'s long form, which counts as claiming it + #[usage(long = "quiet", negate = "--version")] + quiet: bool, +} + +#[test] +fn a_hidden_or_negated_claim_still_counts() { + // The parser looks a command's own flags up first and does not care whether they are + // shown, so a hidden `--help` binds and a negation named `--version` binds. Offering + // either as the supplied entry would describe an action that typing it does not perform. + let page = usage_argv::help::render(HiddenHelp::spec(), HiddenHelp::spec().root.cmd, true) + .expect("a page"); + let listing = page.split_once("\nFlags:").expect("a flags section").1; + + // `--help` is claimed, so only the short form is offered — and named after the form it + // shows, or it renders as a renamed flag: `help: -h`. + assert!(listing.contains(" -h "), "{page}"); + assert!(!listing.contains("help: -h"), "{page}"); + assert!(!listing.contains("-h, --help"), "{page}"); + + // `--version` is claimed by a negation, so only `-V` is offered. + assert!(!listing.contains("-V, --version"), "{page}"); +} + +#[test] +fn the_hidden_help_fields_are_bound() { + use std::ffi::OsStr; + let argv = ["--help"].map(OsStr::new); + let parsed = HiddenHelp::parse_from(&argv).expect("its own flag, not a help request"); + assert!(parsed.help_of_its_own); + + let argv = ["--version"].map(OsStr::new); + let parsed = HiddenHelp::parse_from(&argv).expect("its own negation, not a version request"); + assert!(!parsed.quiet); +} diff --git a/lib/src/docs/cli/mod.rs b/lib/src/docs/cli/mod.rs index 891b800e3..ebf7b0c73 100644 --- a/lib/src/docs/cli/mod.rs +++ b/lib/src/docs/cli/mod.rs @@ -20,12 +20,39 @@ pub fn render_help(spec: &Spec, cmd: &SpecCommand, long: bool) -> String { // // Listed nowhere before this: `communique generate` accepts `--config` from its root and // its page mentioned none of it — a flag a user can type and cannot discover. - let mut inherited = inherited_flags(spec, cmd, &docs_cmd.full_cmd); + let (mut inherited, ancestors_taken) = inherited_flags(spec, cmd, &docs_cmd.full_cmd); // One column over both lists, so the two sections read as one table with a rule through it // rather than two that happen to be adjacent. The width feeds the wrapping as well as the // padding — a continuation line is indented to sit under the description — so both lists // are laid out again once the width is known. + // Last in the command's own section, which is where clap has them: they carry no + // `help_heading`, so a CLI that groups its flags gets them at the end of the ungrouped + // list rather than inside somebody's section. + { + let supplied = supplied_flags(spec, cmd, &ancestors_taken, docs_cmd.full_cmd.is_empty()); + if !supplied.is_empty() { + match docs_cmd + .flag_groups + .iter_mut() + .find(|g| g.heading.is_none()) + { + Some(group) => group.items.extend(supplied), + // Inserted first, not pushed: `group_by_heading` sorts the unheaded group to + // the front and argv's `groups_section` emits it there, so a CLI that heads + // every one of its flags would otherwise get `Flags:` *after* the headed + // sections here and before them there. + None => docs_cmd.flag_groups.insert( + 0, + crate::docs::models::Group { + heading: None, + items: supplied, + }, + ), + } + } + } + let width = crate::docs::layout::get_terminal_width(); let col = crate::docs::layout::max_usage_width( docs_cmd @@ -52,6 +79,77 @@ pub fn render_help(spec: &Spec, cmd: &SpecCommand, long: bool) -> String { TERA.render(template, &ctx).unwrap().trim().to_string() + "\n" } +/// The entries for `--help` and `--version`, which the parser supplies and no spec declares. +/// +/// Listed because help is written for people: a reader looking for how to ask for help should +/// find it on the page. This reverses the rule these two used to follow — that a page lists +/// exactly what its spec declares — and the reason is that the spec has its own readers, and +/// they are not the ones reading this. +/// +/// `--version` only on the program's own page and only where a version is declared, which is +/// where a parser accepts one. Each spelling is dropped where the CLI claimed it, since a page +/// must not describe a flag that something else binds. +/// +/// The twin of `supplied_entries` in `usage-argv`'s `help` module; the gate over mise's spec is +/// what says the two agree. +fn supplied_flags( + spec: &Spec, + cmd: &SpecCommand, + ancestors_taken: &[String], + is_root: bool, +) -> Vec { + // The command's own spellings plus everything in scope above it — the set the inherited + // walk built, which counts hidden globals and negations. Rebuilding it from the *visible* + // inherited list lost both: a hidden ancestor that binds `--help` would have had the page + // offer it anyway. + let mut taken: Vec = ancestors_taken.to_vec(); + for f in &cmd.flags { + taken.extend(f.long.iter().map(|l| format!("--{l}"))); + taken.extend(f.short.iter().map(|s| format!("-{s}"))); + // Stored with its dashes here, unlike in usage-argv. + taken.extend(f.negate.clone()); + } + + let build = |name: &str, long: &str, short: char, help: &str| { + let long_free = !taken.contains(&format!("--{long}")); + let short_free = !taken.contains(&format!("-{short}")); + if !long_free && !short_free { + return None; + } + // Named after the form it shows: a short-only entry called `help` reads as a renamed + // flag and printed `help: -h`. + let name = if long_free { name } else { &short.to_string() }; + let mut flag = crate::SpecFlag { + name: name.to_string(), + long: if long_free { + vec![long.to_string()] + } else { + vec![] + }, + short: if short_free { vec![short] } else { vec![] }, + help: Some(help.to_string()), + ..Default::default() + }; + flag.usage = flag.usage(); + Some(crate::docs::models::SpecFlag::from(&flag)) + }; + + let mut out = Vec::new(); + // `disable_help` turns the parser's answer off — `is_help_arg` refuses the spelling + // outright — so a page that still listed it would describe an action nothing performs. + // The same rule as a claimed or hidden spelling, with the claim made by the spec itself. + // + // usage-argv has no equivalent: `disable_help` is a KDL-only word, so no spec that crate + // can hold ever carries one, and the two renderers cannot disagree about it. + if spec.disable_help != Some(true) { + out.extend(build("help", "help", 'h', "Print help")); + } + if is_root && spec.version.is_some() { + out.extend(build("version", "version", 'V', "Print version")); + } + out +} + /// Fit a list of flags to a column: how wide their names are, and where their help wraps. /// /// The same pass `SpecCommand::from` makes, run again once the width is known over *both* the @@ -88,14 +186,14 @@ fn inherited_flags( spec: &Spec, cmd: &SpecCommand, full_cmd: &[String], -) -> Vec { +) -> (Vec, Vec) { // Every ancestor, root first, which is the order a reader meets them walking down. let mut ancestors: Vec<&SpecCommand> = Vec::new(); let mut at = &spec.cmd; for name in full_cmd.iter().take(full_cmd.len().saturating_sub(1)) { ancestors.push(at); let Some(next) = at.subcommands.get(name) else { - return Vec::new(); + return (Vec::new(), Vec::new()); }; at = next; } @@ -164,7 +262,7 @@ fn inherited_flags( keep.push((f, long, short, negate)); } } - ancestors + let shown: Vec = ancestors .iter() .flat_map(|a| a.flags.iter()) .filter_map(|f| { @@ -184,7 +282,12 @@ fn inherited_flags( shown.usage = shown.usage(); crate::docs::models::SpecFlag::from(&shown) }) - .collect() + .collect(); + // The claim set travels with the result, forms and negations together: the supplied + // `--help` and `--version` entries lose to both, since `find_negation` runs before either + // is offered — even though a negation loses to a long. + taken.extend(taken_negations); + (shown, taken) } /// The command without anything marked `hide`. @@ -248,6 +351,33 @@ mod tests { use super::*; use insta::assert_snapshot; + #[test] + fn a_hidden_ancestor_claim_keeps_help_off_the_page() { + // `--help` is supplied by the parser, and a hidden global that declares it still binds + // first — `hide` keeps a flag off the page, not out of the parse. Deciding the supplied + // entries from the *visible* inherited list lost exactly that, and the page offered a + // `--help` that does something else. + let spec = crate::spec! { r#" +bin "ex" +flag "--help" global=#true hide=#true help="the CLI's own, and invisible" +cmd inner help="a command" { + flag "--plain" help="its own" +} + "# } + .unwrap(); + + let inner = spec.cmd.subcommands.get("inner").expect("inner"); + for long in [false, true] { + let page = super::render_help(&spec, inner, long); + assert!( + !page.contains("--help"), + "long={long}: a hidden ancestor binds this:\n{page}" + ); + // The short form is untouched, since nothing claimed it. + assert!(page.contains("-h"), "long={long}:\n{page}"); + } + } + #[test] fn a_long_beats_a_negation_however_far_away_it_is() { // A negation is stored *with* its dashes here and without them in usage-argv, so the @@ -349,6 +479,7 @@ cmd sneaky hide=#true help="a hidden command" Flags: --visible shown + -h, --help Print help "); } @@ -378,6 +509,7 @@ arg "" help="How to run" help_heading="Behaviour" Flags: --verbose Verbose output + -h, --help Print help Filtering: --filter Only matching @@ -401,6 +533,9 @@ flag "--filter " help="Only matching" help_heading="Filtering" assert_snapshot!(render_help(&spec, &spec.cmd, false), @" Usage: testcli [--filter ] + Flags: + -h, --help Print help + Filtering: --filter Only matching "); @@ -423,6 +558,7 @@ flag "--debug" help="Debug mode" --color Enable color output [env: MYCLI_COLOR] --verbose Verbose output [env: MYCLI_VERBOSE] --debug Debug mode + -h, --help Print help "); assert_snapshot!(render_help(&spec, &spec.cmd, true), @" @@ -434,6 +570,7 @@ flag "--debug" help="Debug mode" --verbose Verbose output [env: MYCLI_VERBOSE] --debug Debug mode + -h, --help Print help "); } @@ -456,9 +593,12 @@ arg "[default]" help="Arg with default value" default="default value" Output file [env: MY_OUTPUT] Extra arg without env [default] Arg with default value (default: default value) + + Flags: + -h, --help Print help "); - assert_snapshot!(render_help(&spec, &spec.cmd, true), @r" + assert_snapshot!(render_help(&spec, &spec.cmd, true), @" Usage: testcli … Arguments: @@ -469,6 +609,9 @@ arg "[default]" help="Arg with default value" default="default value" Extra arg without env [default] Arg with default value (default: default value) + + Flags: + -h, --help Print help "); } @@ -487,6 +630,7 @@ flag "--verbose" help="Verbose output" Flags: --compress / --no-compress Compress output --verbose Verbose output + -h, --help Print help "); assert_snapshot!(render_help(&spec, &spec.cmd, true), @" @@ -495,6 +639,7 @@ flag "--verbose" help="Verbose output" Flags: --compress / --no-compress Compress output --verbose Verbose output + -h, --help Print help "); } @@ -515,6 +660,7 @@ flag "--verbose" help="Enable verbose output" Flags: --verbose Enable verbose output + -h, --help Print help This text appears after the help "); @@ -539,6 +685,7 @@ flag "--verbose" help="Enable verbose output" Flags: --verbose Enable verbose output + -h, --help Print help short after "); @@ -550,6 +697,7 @@ flag "--verbose" help="Enable verbose output" Flags: --verbose Enable verbose output + -h, --help Print help This is the long version of after help "); @@ -570,6 +718,7 @@ example "testcli" header="Run normally" help="Just runs the tool" Flags: --verbose Enable verbose output + -h, --help Print help Examples: Run with verbose output: @@ -583,6 +732,7 @@ example "testcli" header="Run normally" help="Just runs the tool" Flags: --verbose Enable verbose output + -h, --help Print help Examples: Run with verbose output: @@ -609,6 +759,31 @@ flag "--verbose" help="Enable verbose output" Flags: --verbose Enable verbose output + -h, --help Print help + -V, --version Print version + "); + } + + #[test] + fn test_render_help_omits_help_when_disabled() { + // `disable_help` turns the parser's answer off, so the page must not offer it: the same + // rule as a spelling the CLI claimed, with the spec doing the claiming. `--version` + // stays, because nothing disabled that. + let spec = crate::spec! { r#" +bin "testcli" +version "1.2.3" +disable_help #true +flag "--verbose" help="Enable verbose output" + "# } + .unwrap(); + + assert_snapshot!(render_help(&spec, &spec.cmd, false), @" + testcli 1.2.3 + Usage: testcli [--verbose] + + Flags: + --verbose Enable verbose output + -V, --version Print version "); } @@ -628,6 +803,7 @@ flag "--verbose" help="Enable verbose output" Flags: --verbose Enable verbose output + -h, --help Print help "); // Long help should show author/license at the bottom @@ -636,6 +812,7 @@ flag "--verbose" help="Enable verbose output" Flags: --verbose Enable verbose output + -h, --help Print help Author: Test Author License: MIT @@ -651,13 +828,16 @@ cmd "new-cmd" help="Do something better" "# } .unwrap(); - assert_snapshot!(render_help(&spec, &spec.cmd, false), @r" + assert_snapshot!(render_help(&spec, &spec.cmd, false), @" Usage: testcli Commands: new-cmd Do something better old-cmd [deprecated: use new-cmd instead] Do something help Print this message or the help of the given subcommand(s) + + Flags: + -h, --help Print help "); } } diff --git a/lib/tests/parse.rs b/lib/tests/parse.rs index c91e2a2ea..1bef59720 100644 --- a/lib/tests/parse.rs +++ b/lib/tests/parse.rs @@ -114,6 +114,9 @@ arg_choices_help_short: Arguments: shorthelp [bash, fish, zsh] + +Flags: + -h, --help Print help "#, arg_choices_help_long: @@ -129,6 +132,9 @@ Arguments: fooo bar [possible values: bash, fish, zsh] + +Flags: + -h, --help Print help "#, flag_choices_help_short: @@ -140,6 +146,7 @@ flag_choices_help_short: Flags: --shell shorthelp [bash, fish, zsh] + -h, --help Print help "#, flag_choices_help_long: @@ -155,6 +162,7 @@ Flags: fooo bar [possible values: bash, fish, zsh] + -h, --help Print help "#, cmd_help_short: @@ -165,6 +173,9 @@ cmd_help_short: Commands: cmd shorthelp help Print this message or the help of the given subcommand(s) + +Flags: + -h, --help Print help "#, cmd_help_long: @@ -180,6 +191,9 @@ Commands: help Print this message or the help of the given subcommand(s) + +Flags: + -h, --help Print help "#, subcommand_help_short: @@ -192,6 +206,9 @@ subcommand_help_short: Commands: plugins install shorthelp help Print this message or the help of the given subcommand(s) + +Flags: + -h, --help Print help "#, flag_default: