From 8ca7f41e074529cf0ec100aebdff11bba1bd66ab Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 17 Aug 2026 00:12:42 +0000 Subject: [PATCH] feat(derive): say that a command cannot be run alone, which it knew and did not write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A bare `T` subcommand field requires a subcommand and an `Option` does not. The parser has always refused the invocation accordingly — that half was never missing — but `Spec::to_kdl` wrote nothing, so the emitted KDL described `usage generate` as a command a user could type on its own. That reaches past help text. Docs, manpages, completions and the SDK generators all read the emitted spec rather than the Rust type, so every one of them was offering a command that cannot run. Cold metadata, not a parse table: `subcommand_required` is not how a word binds, and the hot path never reads it. Written only where there is something to require, since `subcommand_required` on a command with no subcommands is a spec the linter reports and nothing could satisfy. Found by converting usage-cli itself to the derive and diffing the spec it prints against the one the clap bridge printed. `nesting`'s snapshot picks the change up on a fixture that was already shaped this way, which is the check that this is emission rather than a new rule. Not fixed here, and recorded in PLAN.md beside the root mount it resembles: the spec accepts the property only inside a `cmd` block, so a root that cannot run alone still has no way to say so. The clap bridge could not say it either, so nothing regressed. Co-Authored-By: Claude Opus 5 --- PLAN.md | 16 ++ argv/src/spec.rs | 15 ++ ...ted_spec_reads_like_a_handwritten_one.snap | 2 +- conformance/tests/subcommand_required.rs | 156 ++++++++++++++++++ derive/src/codegen.rs | 62 +++++++ 5 files changed, 250 insertions(+), 1 deletion(-) create mode 100644 conformance/tests/subcommand_required.rs 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,