diff --git a/PLAN.md b/PLAN.md index 12b2e68ee..433164961 100644 --- a/PLAN.md +++ b/PLAN.md @@ -257,6 +257,22 @@ tasks --usage"`, so task names are meant to come from running that. usage-argv d `cmd` block, so a CLI whose _top-level_ subcommands are discovered by running something cannot say so. Worth deciding whether that is a gap or a deliberate restriction. +- [x] **`subcommand_required`, which the derive knew and did not say** — a bare `T` + subcommand field requires a subcommand and an `Option` does not, and the parser + has always refused the invocation accordingly. `Spec::to_kdl` wrote neither, so the + emitted KDL described a group command as one a user could run alone — and help, docs, + completions and the SDK generators all read that rather than the type. Cold metadata, + since it is not how a word binds. Found by converting usage-cli itself to the derive + and diffing the spec it prints against the clap bridge's. One shape could have made the + answer a lie — a `#[usage(flatten)]` group declaring subcommands of its own, which + flatten leaves behind while the group's `build` still demands one — and is a compile + error now, asserted during const evaluation in the parent's expansion, where the group + is only a type. +- [ ] **`subcommand_required` on the root command** — the same restriction as the root + mount, and found beside it: the spec accepts the property only inside a `cmd` block, + so a CLI whose _root_ cannot be run alone has no way to say so. The clap bridge could + not say it either, so nothing regressed — but a bare `T` subcommand field on the root + is now a thing the derive knows and the spec has nowhere to put. - [x] **Three things a spec could say that the derive could not** — a flag's value name (`--tool ` came back as `--tool `, since the flag's own name was all there diff --git a/argv/src/spec.rs b/argv/src/spec.rs index e85debdf1..93ac3bced 100644 --- a/argv/src/spec.rs +++ b/argv/src/spec.rs @@ -386,6 +386,15 @@ pub struct CommandMeta<'a> { /// A token that starts a fresh invocation of this command, such as mise's /// `:::`. pub restart_token: Option<&'a str>, + /// Whether this command cannot be run on its own: naming it and stopping is an + /// error, and one of its subcommands has to follow. + /// + /// Cold metadata rather than a parse table, because it is not how a word binds — + /// the derive already refuses the invocation from the type, a bare `T` subcommand + /// field against an `Option`. It is here so the emitted spec can say it, since + /// everything reading that spec — help, docs, completions — otherwise describes a + /// command as runnable when it is not. + pub subcommand_required: bool, /// Text printed above the usage line, and below everything else. /// /// The spec's `before_help`/`after_help` and their long forms. mise puts an Examples @@ -415,6 +424,7 @@ impl CommandMeta<'_> { effect: None, mount: None, restart_token: None, + subcommand_required: false, before_help: None, before_long_help: None, after_help: None, @@ -782,6 +792,11 @@ fn write_command( if let Some(token) = meta.restart_token { write!(out, " restart_token={}", quoted(token))?; } + // Only where there is something to require. A command with no subcommands cannot + // demand one, and the spec's own reader treats the pair as a mistake. + if meta.subcommand_required && !meta.cmd.subcommands.is_empty() { + out.push_str(" subcommand_required=#true"); + } out.push_str(" {\n"); let inner = depth + 1; diff --git a/conformance/tests/snapshots/nesting__the_emitted_spec_reads_like_a_handwritten_one.snap b/conformance/tests/snapshots/nesting__the_emitted_spec_reads_like_a_handwritten_one.snap index be014830c..9e1a50f05 100644 --- a/conformance/tests/snapshots/nesting__the_emitted_spec_reads_like_a_handwritten_one.snap +++ b/conformance/tests/snapshots/nesting__the_emitted_spec_reads_like_a_handwritten_one.snap @@ -6,7 +6,7 @@ name "ex" bin "ex" about "A tool with commands inside commands" flag "-v --verbose" help="Say more" global=#true -cmd "settings" help="Manage settings" { +cmd "settings" help="Manage settings" subcommand_required=#true { flag "--file" help="Which settings file" { arg "" } diff --git a/conformance/tests/subcommand_required.rs b/conformance/tests/subcommand_required.rs new file mode 100644 index 000000000..844082fdb --- /dev/null +++ b/conformance/tests/subcommand_required.rs @@ -0,0 +1,156 @@ +//! A command that cannot be run on its own, said in the spec it emits. +//! +//! The derive has always known this — a bare `T` subcommand field requires a subcommand and an +//! `Option` does not, and the parser refuses the invocation either way — but `Spec::to_kdl` +//! did not write it, so the emitted KDL described `usage generate` as a command a user could +//! type alone. Found by converting usage-cli itself to the derive and diffing the spec it +//! prints against the one the clap bridge used to print. +//! +//! It matters past help text: docs, manpages, completions and the SDK generators all read the +//! emitted spec, and a command they think is runnable is one they offer. +//! +//! One shape could make the answer a lie, and is refused at compile time instead: a +//! `#[usage(flatten)]` group that declares subcommands of its own. Flatten joins flags and +//! arguments into the parent's tables and leaves subcommands behind, so the group's `build` +//! demanded one that no word could select while the parent — reading its own fields, which is +//! all an expansion can see — reported `subcommand_required=false`. `flatten_checks` in the +//! derive asserts the group's `COMMAND` has no subcommands, during const evaluation in the +//! parent's expansion, so that CLI stops compiling rather than shipping a command nobody can +//! run. There is no compile-fail harness here; the refusal is verified by hand, and the +//! working shape below is what keeps the rest of it honest. + +use usage::Spec as LibSpec; +use usage_derive::{Args, Cli, Subcommands}; + +#[derive(Args)] +struct Leaf { + /// Say more + #[usage(long)] + verbose: bool, +} + +#[derive(Subcommands)] +enum Inner { + /// The only thing under either parent + Leaf(Box), +} + +/// A group that is nothing but its subcommands +#[derive(Args)] +struct Strict { + #[usage(subcommand)] + command: Inner, +} + +#[derive(Subcommands)] +enum InnerToo { + /// The only thing under this one + Leaf(Box), +} + +#[derive(Args)] +struct LeafToo { + /// Say more + #[usage(long)] + verbose: bool, +} + +/// A command that does something itself, and has subcommands too +#[derive(Args)] +struct Loose { + #[usage(subcommand)] + command: Option, +} + +#[derive(Subcommands)] +enum Commands { + Strict(Box), + Loose(Box), +} + +#[derive(Cli)] +#[usage(bin = "ex")] +struct Ex { + #[usage(subcommand)] + command: Option, +} + +fn spec() -> LibSpec { + Ex::to_kdl().parse().expect("valid spec") +} + +#[test] +fn a_command_that_cannot_run_alone_says_so() { + let spec = spec(); + let strict = spec.cmd.subcommands.get("strict").expect("declared"); + assert!( + strict.subcommand_required, + "a bare `T` subcommand field means one has to follow: {}", + Ex::to_kdl() + ); +} + +#[test] +fn a_command_that_can_stays_quiet() { + // Not merely absent from the KDL — read back as false, which is what a consumer asks. + // Writing it unconditionally would be the same bug in the other direction. + let spec = spec(); + let loose = spec.cmd.subcommands.get("loose").expect("declared"); + assert!(!loose.subcommand_required); + assert!( + !Ex::to_kdl().contains("cmd \"loose\" subcommand_required"), + "an `Option` field writes nothing: {}", + Ex::to_kdl() + ); +} + +#[test] +fn a_leaf_command_never_claims_it() { + // `subcommand_required` on a command with no subcommands is a spec the linter reports and + // nothing could satisfy, so the writer holds both conditions rather than just the flag. + let kdl = Ex::to_kdl(); + let leaf_lines: Vec<&str> = kdl + .lines() + .filter(|line| line.trim_start().starts_with("cmd \"leaf\"")) + .collect(); + assert_eq!(leaf_lines.len(), 2, "one under each parent: {kdl}"); + for line in leaf_lines { + assert!(!line.contains("subcommand_required"), "{line}"); + } +} + +#[test] +fn the_parser_and_the_spec_agree() { + // The property is emission-only, so this is the half that was already true: the derive + // refuses the invocation from the type. If these ever disagree, the spec is describing a + // grammar the binary does not have. + use std::ffi::OsStr; + let argv = [OsStr::new("strict")]; + assert!( + Ex::parse_from(&argv).is_err(), + "`strict` alone is not an invocation" + ); + + let argv = [OsStr::new("loose")]; + let ex = Ex::parse_from(&argv).expect("`loose` alone is, and says so both ways"); + assert!(matches!(ex.command, Some(Commands::Loose(_)))); + + // And both still route to what is under them, which is what makes the distinction about + // requiredness rather than about reachability. + let argv = ["strict", "leaf", "--verbose"].map(OsStr::new); + let Some(Commands::Strict(strict)) = Ex::parse_from(&argv).expect("should parse").command + else { + panic!("expected `strict`") + }; + let Inner::Leaf(leaf) = strict.command; + assert!(leaf.verbose); + + let argv = ["loose", "leaf", "--verbose"].map(OsStr::new); + let Some(Commands::Loose(loose)) = Ex::parse_from(&argv).expect("should parse").command else { + panic!("expected `loose`") + }; + let Some(InnerToo::Leaf(leaf)) = loose.command else { + panic!("expected `leaf`") + }; + assert!(leaf.verbose); +} diff --git a/derive/src/codegen.rs b/derive/src/codegen.rs index 5a680cf4b..6fd2be312 100644 --- a/derive/src/codegen.rs +++ b/derive/src/codegen.rs @@ -41,6 +41,19 @@ pub fn emit(cli: &Cli) -> TokenStream { let default_subcommand = option_str(cli.default_subcommand.as_deref()); let restart_token = option_str(cli.restart_token.as_deref()); let mount = option_str(cli.mount.as_deref()); + // A bare `T` subcommand field says the command cannot run alone; an `Option` says it + // can. The parser already refuses the invocation from the type — this is so the emitted + // spec says it too, since help, docs and completions read that rather than the type. + let flatten_checks = flatten_checks(cli); + let subcommand_required = cli.fields.iter().any(|f| { + matches!( + f.kind, + Kind::Subcommand { + optional: false, + .. + } + ) + }); let before_help = option_str(cli.before_help.as_deref()); let before_long_help = option_str(cli.before_long_help.as_deref()); let after_help = option_str(cli.after_help.as_deref()); @@ -186,6 +199,7 @@ pub fn emit(cli: &Cli) -> TokenStream { clippy::needless_update )] const _: () = { + #flatten_checks #keys #(#flag_tables)* #(#arg_tables)* @@ -214,6 +228,7 @@ pub fn emit(cli: &Cli) -> TokenStream { about: #about, long_about: #long_about, restart_token: #restart_token, + subcommand_required: #subcommand_required, mount: #mount, before_help: #before_help, before_long_help: #before_long_help, @@ -441,6 +456,38 @@ pub fn emit(cli: &Cli) -> TokenStream { } } +/// A compile-time refusal of a flattened group that declares subcommands. +/// +/// Flatten joins one struct's flags and arguments into another's tables. Subcommands are not +/// joined — the parent's table has no entry for them — but the group's own `build` still +/// demands one, so the shape compiles into a command that cannot be run and whose emitted +/// spec says it can: `subcommand_required` reads the parent's own fields and sees none. +/// +/// Refused rather than supported, as `Option` flatten is: joining two commands' subcommand +/// sets needs a rule for which enum a word selects from, and nothing in the fleet asks for +/// one. Checked in the *parent's* expansion, where the group is only a type — its `COMMAND` +/// is a `const`, so the parent can look at it during const evaluation without the two +/// expansions ever seeing each other. Written as the user wrote it, like every other use of a +/// flattened type here: the tables are emitted beside their struct now rather than in a module +/// above it, so there is no path to rewrite. +fn flatten_checks(cli: &Cli) -> TokenStream { + let checks = cli.fields.iter().filter_map(|f| { + let Kind::Flatten { ty } = &f.kind else { + return None; + }; + Some(quote! { + const _: () = ::core::assert!( + <#ty as ::usage_argv::spec::CommandArgs>::COMMAND.subcommands.is_empty(), + "a flattened group cannot declare subcommands: flatten joins flags and \ + arguments into the parent's tables and leaves subcommands behind, so the \ + command would require one that no word could select. Declare the \ + `subcommand` field on the command's own struct instead." + ); + }) + }); + quote!(#(#checks)*) +} + /// The completion entry points, for a CLI that asked for them. /// /// Two pieces: a function that answers a request, and the line in `parse` that notices one. Both @@ -2065,6 +2112,19 @@ pub fn emit_args(cli: &Cli) -> TokenStream { let command_key = key_ident("COMMAND", None); let restart_token = option_str(cli.restart_token.as_deref()); let mount = option_str(cli.mount.as_deref()); + // A bare `T` subcommand field says the command cannot run alone; an `Option` says it + // can. The parser already refuses the invocation from the type — this is so the emitted + // spec says it too, since help, docs and completions read that rather than the type. + let flatten_checks = flatten_checks(cli); + let subcommand_required = cli.fields.iter().any(|f| { + matches!( + f.kind, + Kind::Subcommand { + optional: false, + .. + } + ) + }); let before_help = option_str(cli.before_help.as_deref()); let before_long_help = option_str(cli.before_long_help.as_deref()); let after_help = option_str(cli.after_help.as_deref()); @@ -2142,6 +2202,7 @@ pub fn emit_args(cli: &Cli) -> TokenStream { clippy::needless_update )] const _: () = { + #flatten_checks #keys #(#flag_tables)* #(#arg_tables)* @@ -2166,6 +2227,7 @@ pub fn emit_args(cli: &Cli) -> TokenStream { about: #about, long_about: #long_about, restart_token: #restart_token, + subcommand_required: #subcommand_required, mount: #mount, before_help: #before_help, before_long_help: #before_long_help,