From 09f38a06bf0df26ddb452107421bb51853cf10ea Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Sun, 16 Aug 2026 23:54:20 +0000 Subject: [PATCH 1/8] feat(derive): declare a group where the flags are declared MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The spec gained groups in the previous commit; this is the authoring surface for them, per the canonicality rule — spec first, then the derive lowers into it. Membership goes on the field and the two properties on the struct: #[usage(group("input", required))] struct Ex { #[usage(long, group = "input")] file: Option, #[usage(long, group = "input")] url: Option, } The struct line can be left out entirely when the group is a plain "at most one", which is the common case and should not need saying twice. Properties are named rather than assigned — `required`, not `required = true` — because that is how `long`, `global` and `count` are already written, and a property that is only ever on or off is said by naming it. Two things are compile errors rather than rules that quietly hold for nothing: a group with one member, which is a statement about that flag and belongs on the flag, and a declaration no field joins. Both name the group in the message. The checks read a default the way usage-lib does, which is the whole point of the spec being the definition: exclusivity counts what was supplied, requiredness asks whether a member ended up with a value. A group whose member has a default therefore generates no requiredness check at all — it could never fail — decided at compile time, as `requires` is. `usage-argv` gains `GroupMeta`, cold like the rest of the metadata, and `Error::MissingGroup`, which carries the members as members rather than as a rendered sentence: a caller rendering its own errors needs the list, and so will a completion that answers what would satisfy this. It fits inside `Error`'s existing 40 bytes, so the hot path's `Result` is unchanged. `gen-shadow` counts a `group` as dropped in both dialects. Both could express one — the derive with `group(…)`, clap with `ArgGroup` — so it is a gap in the shadow generator rather than in either target, and no spec in the fleet declares one yet. Counted so the report cannot claim to have expressed a whole spec that it did not. Co-Authored-By: Claude Opus 5 --- argv/src/diagnostic.rs | 14 +++ argv/src/lib.rs | 12 +++ argv/src/spec.rs | 46 +++++++++ conformance/tests/post_binding.rs | 102 +++++++++++++++++++ derive/src/codegen.rs | 139 +++++++++++++++++++++++++ derive/src/lib.rs | 23 +++++ derive/src/model.rs | 164 +++++++++++++++++++++++++++++- docs/spec/reference/group.md | 19 ++++ xtask/src/shadow.rs | 8 ++ 9 files changed, 524 insertions(+), 3 deletions(-) diff --git a/argv/src/diagnostic.rs b/argv/src/diagnostic.rs index a6709fd2a..04d799630 100644 --- a/argv/src/diagnostic.rs +++ b/argv/src/diagnostic.rs @@ -591,6 +591,20 @@ pub fn render( invalid.reason ); } + Error::MissingGroup { group, members } => { + with_usage = true; + // clap's own shape for a required group, which is the required-arguments + // message with the members listed under it. The group's name goes on the + // first line rather than into the list, since it is not something to type. + let _ = writeln!( + out, + "{} one of the following required arguments was not provided ({group}):", + style.error("error:") + ); + for member in *members { + let _ = writeln!(out, " {}", style.valid(&shown(here, member))); + } + } 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 '`. diff --git a/argv/src/lib.rs b/argv/src/lib.rs index 904fb0cf6..277fa1d54 100644 --- a/argv/src/lib.rs +++ b/argv/src/lib.rs @@ -454,6 +454,18 @@ pub enum Error<'t, 'v> { /// does not grow. A value that will not convert has already failed, and a message /// worth reading is worth one allocation. InvalidValue(::std::boxed::Box>), + /// A required group had none of its members given. + /// + /// Carries the members as members rather than as a rendered sentence: the caller + /// decides how to say it, and a completion asking what would satisfy this needs the + /// list rather than the prose. + MissingGroup { + /// The group's declared name, which appears in the message so a command with + /// several groups does not report the same sentence twice. + group: &'t str, + /// The flags that would satisfy it, as the declaration spells them. + members: &'t [&'t str], + }, /// A subcommand was required, and none was given. MissingSubcommand, /// `--help` or `-h` was given, and `cmd` is what it was asked about. diff --git a/argv/src/spec.rs b/argv/src/spec.rs index 2576009d3..7949a8fe7 100644 --- a/argv/src/spec.rs +++ b/argv/src/spec.rs @@ -390,6 +390,25 @@ pub const fn concat_aliases(groups: &[&[&'static str]]) -> [&'st out } +/// A set of one command's flags that relate to one another as a set. +/// +/// Pairwise `conflicts` can say "at most one of these", once per pair; what it cannot +/// say is that one of them is *needed*, which is a statement about the set. +#[derive(Debug, Clone, Copy)] +pub struct GroupMeta<'a> { + /// What the group is called. It appears in the message a failed check produces, and + /// it is how a reader tells two groups on one command apart. + pub name: &'a str, + /// The flags in the group, as selectors — `--long` or `-s`, the way every other + /// relationship names a flag. + pub members: &'a [&'a str], + /// Whether at least one member has to be given. + pub required: bool, + /// Whether more than one member may be given. False is what makes a bare group + /// mutual exclusion, as it does in clap. + pub multiple: bool, +} + /// What a command knows about itself beyond how it parses. #[derive(Debug, Clone, Copy)] pub struct CommandMeta<'a> { @@ -441,6 +460,11 @@ pub struct CommandMeta<'a> { pub args: &'a [ArgMeta<'a>], /// Metadata for `cmd.subcommands`, in the same order. pub subcommands: &'a [&'a CommandMeta<'a>], + /// Sets of this command's flags that relate to one another as a set. + /// + /// Cold like everything else here: a group is checked once the last token has been + /// read, by code the derive generates, and a successful parse never reads this. + pub groups: &'a [GroupMeta<'a>], } impl CommandMeta<'_> { @@ -460,6 +484,7 @@ impl CommandMeta<'_> { after_help: None, after_long_help: None, examples: &[], + groups: &[], flags: &[], args: &[], subcommands: &[], @@ -807,6 +832,11 @@ fn write_body( write_arg(out, arg, depth)?; } write_completion_types(out, meta, depth)?; + // After the flags and arguments they name, so a reader meets the members before the + // rule about them — the order usage-lib writes, so a round trip reads the same way. + for group in meta.groups { + write_group(out, group, depth)?; + } #[cfg(feature = "complete")] write_completers(out, meta, bin, depth)?; for sub in meta.subcommands { @@ -928,6 +958,22 @@ fn write_command( Ok(()) } +fn write_group(out: &mut String, group: &GroupMeta<'_>, depth: usize) -> core::fmt::Result { + indent(out, depth)?; + write!(out, "group {}", quoted(group.name))?; + for member in group.members { + write!(out, " {}", quoted(member))?; + } + if group.required { + out.push_str(" required=#true"); + } + if group.multiple { + out.push_str(" multiple=#true"); + } + out.push('\n'); + Ok(()) +} + fn write_example(out: &mut String, example: &Example<'_>, depth: usize) -> core::fmt::Result { indent(out, depth)?; write!(out, "example {}", quoted(example.code))?; diff --git a/conformance/tests/post_binding.rs b/conformance/tests/post_binding.rs index cc1e3a111..576e472b7 100644 --- a/conformance/tests/post_binding.rs +++ b/conformance/tests/post_binding.rs @@ -651,3 +651,105 @@ fn an_optional_collection_with_defaults_is_never_none() { // And one that declares no default still tells "never given" from "given nothing". assert_eq!(d.plain, None); } + +/// A CLI whose flags are grouped. +/// +/// `--file`/`--url`/`--stdin` are one exclusive, required group — exactly one source — +/// and `--json`/`--yaml` are an ordinary exclusive one, where saying nothing is fine. +#[derive(Cli)] +#[usage(bin = "grp")] +#[usage(group("input", required))] +struct Grp { + /// Read from a file + #[usage(long, group = "input")] + file: Option, + /// Read from a URL + #[usage(long, group = "input")] + url: Option, + /// Read from standard input + #[usage(short = 's', long, group = "input")] + stdin: bool, + /// Emit JSON + #[usage(long, group = "format")] + json: bool, + /// Emit YAML + #[usage(long, group = "format")] + yaml: bool, +} + +#[test] +fn a_required_group_needs_one_of_its_members() { + let a = argv([]); + assert!(matches!( + Grp::parse_from(&a), + Err(Error::MissingGroup { + group: "input", + members: ["--file", "--url", "--stdin"] + }) + )); + + let a = argv(["--url", "u"]); + assert_eq!( + Grp::parse_from(&a).expect("one member").url.as_deref(), + Some("u") + ); +} + +#[test] +fn two_members_of_an_exclusive_group_cannot_both_be_given() { + let a = argv(["--file", "f", "--stdin"]); + assert!(matches!( + Grp::parse_from(&a), + Err(Error::ConflictingFlags { + name: "stdin", + other: "file" + }) + )); + + // By its short form too, since the group is between flags rather than spellings. + let a = argv(["--url", "u", "-s"]); + assert!(matches!( + Grp::parse_from(&a), + Err(Error::ConflictingFlags { name: "stdin", .. }) + )); + + // One member alone still parses, and lands where it was declared. + let a = argv(["--file", "f"]); + let grp = Grp::parse_from(&a).expect("one member"); + assert_eq!(grp.file.as_deref(), Some("f")); + assert!(!grp.stdin); +} + +#[test] +fn a_group_that_is_not_required_may_be_left_alone() { + // `--json`/`--yaml` exclude each other and neither is needed. + let a = argv(["--url", "u"]); + let grp = Grp::parse_from(&a).expect("saying nothing about format is fine"); + assert!(!grp.json && !grp.yaml); + + let a = argv(["--url", "u", "--json"]); + assert!(Grp::parse_from(&a).expect("one of them").json); + + let a = argv(["--url", "u", "--json", "--yaml"]); + assert!(matches!( + Grp::parse_from(&a), + Err(Error::ConflictingFlags { .. }) + )); +} + +#[test] +fn a_group_reaches_the_emitted_spec_and_usage_lib_agrees() { + let kdl = Grp::to_kdl(); + assert!( + kdl.contains(r#"group "input" "--file" "--url" "--stdin" required=#true"#), + "{kdl}" + ); + assert!(kdl.contains(r#"group "format" "--json" "--yaml""#), "{kdl}"); + + // The reference implementation reads what the derive wrote, and enforces the same + // rule — which is the point of the spec being the definition rather than a summary. + let spec: usage::Spec = kdl.parse().expect("the emitted spec should parse"); + let group = spec.cmd.groups.iter().find(|g| g.name == "input").unwrap(); + assert!(group.required); + assert_eq!(group.members.len(), 3); +} diff --git a/derive/src/codegen.rs b/derive/src/codegen.rs index cb5e2de50..021e1d85e 100644 --- a/derive/src/codegen.rs +++ b/derive/src/codegen.rs @@ -123,6 +123,7 @@ pub fn emit(cli: &Cli) -> TokenStream { let tables = tables(cli); let table_decls = &tables.decls; let meta_table_decls = &tables.meta_decls; + let (group_meta_decl, group_meta_table_ref) = group_meta_table(cli); let flag_table_ref = &tables.flags; let arg_table_ref = &tables.args; let flag_meta_table_ref = &tables.flag_metas; @@ -274,6 +275,8 @@ pub fn emit(cli: &Cli) -> TokenStream { #(#arg_metas)* #meta_table_decls + #group_meta_decl + pub static ROOT_META: usage_argv::spec::CommandMeta = usage_argv::spec::CommandMeta { cmd: &ROOT, about: #about, @@ -287,6 +290,7 @@ pub fn emit(cli: &Cli) -> TokenStream { after_long_help: #after_long_help, flags: #flag_meta_table_ref, args: #arg_meta_table_ref, + groups: #group_meta_table_ref, #sub_metas ..usage_argv::spec::CommandMeta::EMPTY }; @@ -2339,6 +2343,7 @@ pub fn emit_args(cli: &Cli) -> TokenStream { let tables = tables(cli); let table_decls = &tables.decls; let meta_table_decls = &tables.meta_decls; + let (group_meta_decl, group_meta_table_ref) = group_meta_table(cli); let flag_table_ref = &tables.flags; let arg_table_ref = &tables.args; let flag_meta_table_ref = &tables.flag_metas; @@ -2410,6 +2415,8 @@ pub fn emit_args(cli: &Cli) -> TokenStream { #(#arg_metas)* #meta_table_decls + #group_meta_decl + pub static COMMAND_META: usage_argv::spec::CommandMeta = usage_argv::spec::CommandMeta { cmd: &COMMAND, effect: #effect, @@ -2425,6 +2432,7 @@ pub fn emit_args(cli: &Cli) -> TokenStream { after_long_help: #after_long_help, flags: #flag_meta_table_ref, args: #arg_meta_table_ref, + groups: #group_meta_table_ref, #sub_metas ..usage_argv::spec::CommandMeta::EMPTY }; @@ -2859,6 +2867,64 @@ fn displaced_guard(cli: &Cli, field: &Field) -> TokenStream { quote!(&& !partial.#overridden) } +/// The groups a command declares, in the order their first member is written. +/// +/// Membership lives on the fields and properties on the struct, so this is where the two +/// are joined — and it is the only place both are visible, which is why the emitted +/// metadata is built here rather than in the model. +fn declared_groups(cli: &Cli) -> Vec<(String, bool, bool, Vec)> { + let mut groups: Vec<(String, bool, bool, Vec)> = Vec::new(); + for field in &cli.fields { + let Some(name) = field.group.as_deref() else { + continue; + }; + let Some(selector) = Cli::selector_for_field(field) else { + continue; + }; + match groups.iter_mut().find(|(n, _, _, _)| n == name) { + Some((_, _, _, members)) => members.push(selector), + None => { + // An undeclared group takes the defaults, which is the common case: "at + // most one of these" needs no properties, and making it say so anyway + // would be ceremony. + let decl = cli.groups.iter().find(|d| d.name == name); + groups.push(( + name.to_string(), + decl.is_some_and(|d| d.required), + decl.is_some_and(|d| d.multiple), + vec![selector], + )); + } + } + } + groups +} + +/// The `static` array of group metadata, and the expression referring to it. +fn group_meta_table(cli: &Cli) -> (TokenStream, TokenStream) { + let groups = declared_groups(cli); + if groups.is_empty() { + return (quote!(), quote!(&[])); + } + let entries = groups.iter().map(|(name, required, multiple, members)| { + quote! { + ::usage_argv::spec::GroupMeta { + name: #name, + members: &[#(#members),*], + required: #required, + multiple: #multiple, + } + } + }); + let len = groups.len(); + ( + quote! { + pub static GROUP_METAS: [::usage_argv::spec::GroupMeta; #len] = [#(#entries),*]; + }, + quote!(&GROUP_METAS), + ) +} + /// Everything decided once the last token has been read. /// /// Ordered deliberately. The environment fills what argv left out, so it runs @@ -3167,6 +3233,78 @@ fn post_binding(cli: &Cli) -> TokenStream { }) }); + // Groups, checked once per group rather than per member: both questions a group asks + // — how many members were given, and whether that is enough — are about the set. + // + // The two halves read a default differently, deliberately, and the same way + // usage-lib does. Exclusivity counts what was supplied, or a defaulted member would + // collide with the sibling the user typed; requiredness asks whether a member ended + // up with a value, and a default is a value. + let group_checks = declared_groups(cli) + .into_iter() + .map(|(name, required, multiple, members)| { + let fields: Vec<&Field> = members + .iter() + .filter_map(|selector| cli.field_for_selector(selector)) + .collect(); + let given: Vec = fields + .iter() + .map(|f| { + let given = format_ident!("__given_{}", f.ident); + quote!(partial.#given) + }) + .collect(); + // A member with a default always has a value, so the group can never be + // unsatisfied. Decided here rather than at run time, as `requires` is. + let always_filled = fields.iter().any(|f| !f.default.is_empty()); + let exclusivity = (!multiple).then(|| { + // Reported as the first two that were given, which is the pair the user has + // to choose between. `ConflictingFlags` rather than a group-shaped error: + // what went wrong is that two flags were given together, which is exactly + // what that error says. + let names: Vec<&String> = fields.iter().map(|f| &f.name).collect(); + let pairs = (0..fields.len()).flat_map(|i| { + let (later, earlier) = (given.clone(), given.clone()); + let (later_names, earlier_names) = (names.clone(), names.clone()); + ((i + 1)..fields.len()) + .map(move |j| { + let (a, b) = (&earlier[i], &later[j]); + let (name_a, name_b) = (earlier_names[i], later_names[j]); + quote! { + if #a && #b { + return ::std::result::Result::Err( + ::usage_argv::Error::ConflictingFlags { + name: #name_b, + other: #name_a, + }, + ); + } + } + }) + .collect::>() + }); + quote!(#(#pairs)*) + }); + let requiredness = (required && !always_filled).then(|| { + let selectors = &members; + quote! { + if !(#(#given)||*) { + return ::std::result::Result::Err( + ::usage_argv::Error::MissingGroup { + group: #name, + members: &[#(#selectors),*], + }, + ); + } + } + }); + quote! { + #exclusivity + #requiredness + } + }) + .collect::>(); + // `required_if` and `required_unless` are the same question asked two ways: which // other flags decide whether this one had to be given. Neither needs to know the // order they arrived in — only whether they arrived — so both are answered here, @@ -3232,6 +3370,7 @@ fn post_binding(cli: &Cli) -> TokenStream { // more useful of the two answers when a conflict has also left something // unfilled, and it is the one usage-lib reports. #(#conflict_checks)* + #(#group_checks)* #(#requirement_checks)* #(#flattened_checks)* #(#required_checks)* diff --git a/derive/src/lib.rs b/derive/src/lib.rs index 1dba460a5..320a5daac 100644 --- a/derive/src/lib.rs +++ b/derive/src/lib.rs @@ -213,6 +213,7 @@ //! | `overrides = "--other"` | a flag this one displaces, the last given winning | //! | `conflicts = "--other"` | a flag this one cannot be given with | //! | `requires = "--other"` | a flag that must also be given when this one is | +//! | `group = "input"` | the group this flag is one of; see below | //! | `required_if = "--other"` | a flag whose presence makes this one necessary | //! | `required_unless = "--other"` | a flag whose presence makes this one unnecessary | //! @@ -221,6 +222,28 @@ //! is a compile error, which is the advantage of declaring a relationship in code: in a //! hand-written spec a typo'd selector is a relationship that quietly does not hold. //! +//! A **group** is the one relationship that is not written flag-to-flag, because what it +//! says is about the set: `required` means one of them is needed, and no rule on an +//! individual flag expresses that. Membership goes on the fields and the properties on the +//! struct, which may be left out entirely when the group is a plain "at most one": +//! +//! ```ignore +//! #[derive(Cli)] +//! #[usage(bin = "ex")] +//! #[usage(group("input", required))] +//! struct Ex { +//! #[usage(long, group = "input")] +//! file: Option, +//! #[usage(long, group = "input")] +//! url: Option, +//! } +//! ``` +//! +//! `required` means at least one member is needed and `multiple` means more than one may +//! be given, so a bare group is "at most one", `required` alone is "exactly one", and the +//! two together are "at least one" — clap's two properties, read the same way. A group +//! with one member, or a declaration no field joins, is a compile error. +//! //! They describe relationships *between flags*, so a positional cannot declare one — //! the spec records them on a flag and has nowhere to put them on an argument, and a //! check the emitted spec cannot describe would leave docs and completions saying diff --git a/derive/src/model.rs b/derive/src/model.rs index e56e689ba..203a19e47 100644 --- a/derive/src/model.rs +++ b/derive/src/model.rs @@ -96,9 +96,23 @@ pub struct Cli { /// Carried into the spec and nowhere else. The parser never runs it: a mount costs a /// subprocess, and completions are the cold path where that is affordable. pub mount: Option, + /// Groups declared on this command, with their properties. + /// + /// Membership is on the field — `#[usage(group = "input")]` — and only the two + /// properties live here, because a group that says nothing but "these three are + /// exclusive" should not need declaring twice. + pub groups: Vec, pub fields: Vec, } +/// A `#[usage(group("input", required))]` on the struct. +pub struct GroupDecl { + pub name: String, + pub required: bool, + pub multiple: bool, + pub span: Span, +} + /// One field, resolved to the thing it declares. pub struct Field { pub ident: syn::Ident, @@ -191,6 +205,10 @@ pub struct Field { /// one lives on the flag the rule is about, which is where clap puts it and where a /// reader looks for it. pub requires: Vec, + /// The group this flag belongs to, if any. Properties live on the group's own + /// declaration; membership lives here, because a field is where a reader looks to + /// see what a flag is part of. + pub group: Option, /// Flags whose presence makes this one necessary. pub required_if: Vec, /// Flags whose presence makes this one unnecessary. @@ -407,6 +425,7 @@ impl Cli { after_long_help: None, restart_token: None, mount: None, + groups: Vec::new(), fields: Vec::new(), }; @@ -474,14 +493,16 @@ impl Cli { } "restart_token" => cli.restart_token = Some(string_value(&meta)?), "mount" => cli.mount = Some(string_value(&meta)?), + "group" => cli.groups.push(group_decl(&meta)?), other => { return Err(syn::Error::new_spanned( path, format!( "unknown option `{other}` on a struct; usage::Cli takes \ `name`, `bin`, `version`, `usage`, `verbatim_doc_comment`, `unknown_flags`, \ - `default_subcommand`, `restart_token`, and `mount` here, \ - and the description comes from the doc comment" + `default_subcommand`, `restart_token`, `mount` and \ + `group` here, and the description comes from the doc \ + comment" ), )); } @@ -668,6 +689,20 @@ impl Cli { Ok(()) } + /// How a group names one of its member fields in the emitted spec. + /// + /// The long form when there is one, since that is how a spec refers to a flag + /// everywhere else; a short form otherwise, which selectors accept just as readily. + pub fn selector_for_field(field: &Field) -> Option { + let Kind::Flag { longs, shorts, .. } = &field.kind else { + return None; + }; + longs + .first() + .map(|long| format!("--{long}")) + .or_else(|| shorts.first().map(|short| format!("-{short}"))) + } + pub fn field_for_selector(&self, selector: &str) -> Option<&Field> { self.fields.iter().find(|field| { let Kind::Flag { @@ -817,6 +852,50 @@ impl Cli { } } + // Groups: every member is a flag, every declared group has members, and a group + // holds at least two of them — the same floor the spec enforces, checked here so + // it fails where it is written rather than when the spec is emitted. + let mut group_members: Vec<(&str, Vec<&Field>)> = Vec::new(); + for field in &self.fields { + let Some(name) = field.group.as_deref() else { + continue; + }; + if !matches!(field.kind, Kind::Flag { .. }) { + return Err(syn::Error::new( + field.span, + "`group` describes a relationship between flags, so the field needs \ + a `long` or a `short`", + )); + } + match group_members.iter_mut().find(|(n, _)| *n == name) { + Some((_, members)) => members.push(field), + None => group_members.push((name, vec![field])), + } + } + for (name, members) in &group_members { + if members.len() < 2 { + return Err(syn::Error::new( + members[0].span, + format!( + "group `{name}` has one flag in it; a rule about a single flag \ + belongs on that flag, as `required` or `requires`" + ), + )); + } + } + for decl in &self.groups { + if !group_members.iter().any(|(n, _)| *n == decl.name) { + return Err(syn::Error::new( + decl.span, + format!( + "group `{}` is declared and no field is in it; a field joins a \ + group with `#[usage(group = \"{}\")]`", + decl.name, decl.name + ), + )); + } + } + // Every relationship names a flag that exists. Resolving these at compile time // is the advantage of declaring them in code: a spec written by hand can only // find a typo'd selector at parse time, or never, since a selector naming @@ -951,6 +1030,7 @@ impl Field { overrides: Vec::new(), conflicts: Vec::new(), requires: Vec::new(), + group: None, required_if: Vec::new(), required_unless: Vec::new(), hide: false, @@ -1045,6 +1125,7 @@ impl Field { overrides: Vec::new(), conflicts: Vec::new(), requires: Vec::new(), + group: None, required_if: Vec::new(), required_unless: Vec::new(), hide: false, @@ -1102,6 +1183,7 @@ impl Field { let mut overrides: Vec = Vec::new(); let mut conflicts: Vec = Vec::new(); let mut requires: Vec = Vec::new(); + let mut group: Option = None; let mut required_if: Vec = Vec::new(); let mut required_unless: Vec = Vec::new(); @@ -1194,6 +1276,7 @@ impl Field { "overrides" => overrides = selectors(&meta)?, "conflicts" => conflicts = selectors(&meta)?, "requires" => requires = selectors(&meta)?, + "group" => group = Some(string_value(&meta)?), "required_if" => required_if = selectors(&meta)?, "required_unless" => required_unless = selectors(&meta)?, "value_enum" => value_enum = flag_value(&meta)?, @@ -1237,7 +1320,7 @@ impl Field { `short`, `negate`, `global`, `var`, `variadic`, \ `count`, `hide`, `arg`, `env`, `default`, `choices`, \ `var_min`, `var_max`, `value_enum`, `value_hint`, `overrides`, \ - `conflicts`, `requires`, `required_if`, \ + `conflicts`, `requires`, `group`, `required_if`, \ `required_unless`, `help_heading`, `value_name`, \ `verbatim_doc_comment`, \ `required`, and `double_dash`" @@ -1784,6 +1867,7 @@ impl Field { overrides, conflicts, requires, + group, required_if, required_unless, hide, @@ -1976,6 +2060,80 @@ fn string_value(meta: &Meta) -> syn::Result { /// than as field names, so a declaration reads the same in Rust as it does in KDL. /// Which flag each one names is resolved in [`Cli::check`], where every field is in /// view. +/// `group("input", required, multiple)` — a name, then any of the two properties. +/// +/// Hand-parsed rather than reusing [`selectors`], because the list is mixed: a string +/// literal for the name and bare idents for the properties. Spelling the properties as +/// idents rather than as `required = true` matches how `long`, `global` and `count` are +/// already written on a field — a property that is only ever on or off is said by naming +/// it. +fn group_decl(meta: &Meta) -> syn::Result { + let span = meta.path().span(); + let Meta::List(list) = meta else { + return Err(syn::Error::new_spanned( + meta.path(), + "a group is declared as `group(\"name\")`, with `required` and `multiple` \ + after the name if it needs them", + )); + }; + let mut name: Option = None; + let mut decl = GroupDecl { + name: String::new(), + required: false, + multiple: false, + span, + }; + list.parse_args_with(|input: syn::parse::ParseStream| { + while !input.is_empty() { + if input.peek(syn::LitStr) { + let lit: syn::LitStr = input.parse()?; + if name.is_some() { + return Err(syn::Error::new_spanned( + &lit, + "a group takes one name; its members are declared on the fields, \ + with `#[usage(group = \"…\")]`", + )); + } + name = Some(lit.value()); + } else { + let ident: syn::Ident = input.parse()?; + match ident.to_string().as_str() { + "required" => decl.required = true, + "multiple" => decl.multiple = true, + other => { + return Err(syn::Error::new_spanned( + &ident, + format!( + "unknown group property `{other}`; a group takes \ + `required` and `multiple`" + ), + )); + } + } + } + if input.is_empty() { + break; + } + input.parse::()?; + } + Ok(()) + })?; + let Some(name) = name else { + return Err(syn::Error::new( + span, + "a group needs a name, as in `group(\"input\", required)`", + )); + }; + if name.is_empty() { + return Err(syn::Error::new( + span, + "a group with no name answers to nothing", + )); + } + decl.name = name; + Ok(decl) +} + fn selectors(meta: &Meta) -> syn::Result> { let Meta::List(list) = meta else { return Ok(vec![string_value(meta)?]); diff --git a/docs/spec/reference/group.md b/docs/spec/reference/group.md index cee334f66..f5653cd81 100644 --- a/docs/spec/reference/group.md +++ b/docs/spec/reference/group.md @@ -84,6 +84,25 @@ Members are counted by the flag they name, not by the selector, so a group listi `-f` and `--file` holds one member and not two. Listing both is redundant rather than wrong, and a flag is never in conflict with itself. +## From the derive + +`#[derive(usage::Cli)]` writes the same group with membership on the fields and the +properties on the struct: + +```rust +#[usage(group("input", required))] +struct Ex { + #[usage(long, group = "input")] + file: Option, + #[usage(long, group = "input")] + url: Option, +} +``` + +The `#[usage(group(...))]` line can be left out when the group is a plain "at most one". +A group with fewer than two members, or a declaration no field joins, is a compile error +rather than a rule that quietly holds for nothing. + ## Coming from clap `ArgGroup` carries across, with `required` and `multiple` read the same way. A group that diff --git a/xtask/src/shadow.rs b/xtask/src/shadow.rs index dfb570bdb..4d098574f 100644 --- a/xtask/src/shadow.rs +++ b/xtask/src/shadow.rs @@ -314,6 +314,14 @@ fn emit_command(out: &mut String, cmd: &SpecCommand, ty: &Type, is_root: bool, r run.skipped.note("a command's second and later mounts"); } } + // Counted rather than emitted, in *both* dialects. Both can express a group — the + // derive with `group(…)` and clap with `ArgGroup` — so this is a gap in the shadow + // generator rather than in either target, and no spec in the fleet declares one yet + // for it to matter to. It is counted so the report cannot claim the shadow expressed + // a whole spec that it did not. + if !cmd.groups.is_empty() { + run.skipped.note("a `group` on a command"); + } for (_, sub, sub_ty) in &children { // `run` travels down unchanged; `default_subcommand` is read under an `is_root` // guard, so a child cannot pick up the root's. From ce6416a56592f0c7aca6a1d9095cba205f3244a5 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 17 Aug 2026 00:19:27 +0000 Subject: [PATCH 2/8] fix(derive): a flattened struct's group belongs in the spec it is enforced in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two from review of the derive surface. **Flatten dropped a group from the emitted spec.** A flattened struct's flags are joined into the parent's tables and its `check` runs, so a group declared inside it is *enforced* on the parent command — and the group metadata was not joined, so the emitted KDL described a CLI without a rule the CLI applies. Docs, completions and usage-lib would all have disagreed with the running program, which is exactly the drift the spec-as-definition rule exists to prevent. `concat_group_metas` joins them the way `concat_flag_metas` already joins the flags, at compile time, and the conformance test asserts both halves: the constraint holds, and it is in the KDL usage-lib parses back. **Two declarations of one group** were read first-match-wins, so the second one's `required` was written and never enforced. A compile error now, naming the group. The third finding — a mount replacing a command's flags leaving stale groups — was about the spec rather than the derive, and is fixed in the commit below it. Co-Authored-By: Claude Opus 5 --- argv/src/spec.rs | 43 +++++++++++++++++++++ conformance/tests/flatten.rs | 59 +++++++++++++++++++++++++++++ derive/src/codegen.rs | 36 +++++++++++++++++- derive/src/model.rs | 73 +++++++++++++++++++++++++++++++++++- 4 files changed, 208 insertions(+), 3 deletions(-) diff --git a/argv/src/spec.rs b/argv/src/spec.rs index 7949a8fe7..01b9da037 100644 --- a/argv/src/spec.rs +++ b/argv/src/spec.rs @@ -409,6 +409,49 @@ pub struct GroupMeta<'a> { pub multiple: bool, } +impl GroupMeta<'_> { + /// A group with nothing in it, for the array initialiser a const concat needs. + pub const EMPTY: GroupMeta<'static> = GroupMeta { + name: "", + members: &[], + required: false, + multiple: false, + }; +} + +/// Join groups of group metadata into one, at compile time. +/// +/// The same shape as [`concat_flag_metas`], and needed for the same reason: a flattened +/// struct's groups describe flags that are now in the parent's table, so they belong in +/// the parent's emitted spec. Without this a group declared on a flattened struct would +/// be enforced — the child's own `check` runs — and invisible to the KDL, which is +/// exactly the drift the spec-as-definition rule exists to prevent. +/// +/// `N` must be [`table_len`](crate::table_len) of the same groups. +pub const fn concat_group_metas( + groups: &[&[GroupMeta<'static>]], +) -> [GroupMeta<'static>; N] { + let mut out = [GroupMeta::EMPTY; N]; + let mut at = 0; + let mut g = 0; + while g < groups.len() { + let group = groups[g]; + let mut i = 0; + while i < group.len() { + out[at] = group[i]; + at += 1; + i += 1; + } + g += 1; + } + assert!( + at == N, + "`N` must be `table_len` of the same groups, or the metadata would describe a \ + group that does not exist" + ); + out +} + /// What a command knows about itself beyond how it parses. #[derive(Debug, Clone, Copy)] pub struct CommandMeta<'a> { diff --git a/conformance/tests/flatten.rs b/conformance/tests/flatten.rs index f043d03f7..3db34dce2 100644 --- a/conformance/tests/flatten.rs +++ b/conformance/tests/flatten.rs @@ -257,3 +257,62 @@ fn flattening_nests() { assert!(nested.outer.inner.listing.no_header); assert_eq!(nested.outer.inner.listing.what.as_deref(), Some("keys")); } + +/// A group declared inside a struct that gets flattened somewhere else. +#[derive(Args)] +#[usage(group("output", required))] +struct Emitting { + /// Emit JSON + #[usage(long, group = "output")] + json: bool, + /// Emit YAML + #[usage(long, group = "output")] + yaml: bool, +} + +#[derive(Cli)] +#[usage(bin = "fl")] +struct Flattened { + #[usage(flatten)] + emitting: Emitting, + /// Where to write + #[usage(long)] + out: Option, +} + +#[test] +fn a_flattened_structs_group_is_enforced_and_emitted() { + // Enforced: the child's own `check` runs, so the group holds on the command that + // flattened it. + let a = ["--json", "--out", "o"].map(OsStr::new); + let fl = Flattened::parse_from(&a).expect("one member"); + assert!(fl.emitting.json && !fl.emitting.yaml); + assert_eq!(fl.out.as_deref(), Some("o")); + + let a = ["--json", "--yaml"].map(OsStr::new); + assert!(matches!( + Flattened::parse_from(&a), + Err(Error::ConflictingFlags { .. }) + )); + + let a: [&OsStr; 0] = []; + assert!(matches!( + Flattened::parse_from(&a), + Err(Error::MissingGroup { + group: "output", + .. + }) + )); + + // And emitted, which is the half that can silently rot: the flags are joined into + // the parent's tables, so the group describing them has to be joined too, or the + // spec would describe a CLI without a rule the CLI enforces. + let kdl = Flattened::to_kdl(); + assert!( + kdl.contains(r#"group "output" "--json" "--yaml" required=#true"#), + "{kdl}" + ); + let spec: LibSpec = kdl.parse().expect("the emitted spec should parse"); + assert_eq!(spec.cmd.groups.len(), 1); + assert!(spec.cmd.groups[0].required); +} diff --git a/derive/src/codegen.rs b/derive/src/codegen.rs index 021e1d85e..6143ad1a1 100644 --- a/derive/src/codegen.rs +++ b/derive/src/codegen.rs @@ -2901,9 +2901,28 @@ fn declared_groups(cli: &Cli) -> Vec<(String, bool, bool, Vec)> { } /// The `static` array of group metadata, and the expression referring to it. +/// +/// A flattened struct's groups are joined in, the way its flags and their metadata are: +/// the child enforces them through its own `check`, and its flags are in *this* command's +/// table, so its groups describe this command and belong in this command's emitted KDL. +/// Leaving them out would enforce a rule the spec does not mention — the drift the +/// spec-as-definition rule exists to prevent. fn group_meta_table(cli: &Cli) -> (TokenStream, TokenStream) { let groups = declared_groups(cli); - if groups.is_empty() { + let flattened: Vec = cli + .fields + .iter() + .filter_map(|f| { + let Kind::Flatten { ty } = &f.kind else { + return None; + }; + // Named directly, as the flag and argument tables beside this one are: the + // generated items live in the user's own scope now rather than in a module + // above it, so there is no path to rewrite. + Some(quote!(<#ty as ::usage_argv::spec::CommandArgs>::META.groups)) + }) + .collect(); + if groups.is_empty() && flattened.is_empty() { return (quote!(), quote!(&[])); } let entries = groups.iter().map(|(name, required, multiple, members)| { @@ -2917,9 +2936,22 @@ fn group_meta_table(cli: &Cli) -> (TokenStream, TokenStream) { } }); let len = groups.len(); + if flattened.is_empty() { + return ( + quote! { + pub static GROUP_METAS: [::usage_argv::spec::GroupMeta; #len] = [#(#entries),*]; + }, + quote!(&GROUP_METAS), + ); + } ( quote! { - pub static GROUP_METAS: [::usage_argv::spec::GroupMeta; #len] = [#(#entries),*]; + pub static OWN_GROUP_METAS: [::usage_argv::spec::GroupMeta; #len] = [#(#entries),*]; + const GROUP_META_GROUPS: &[&[::usage_argv::spec::GroupMeta<'static>]] = + &[&OWN_GROUP_METAS, #(#flattened),*]; + static GROUP_METAS: [::usage_argv::spec::GroupMeta<'static>; + ::usage_argv::table_len(GROUP_META_GROUPS)] = + ::usage_argv::spec::concat_group_metas(GROUP_META_GROUPS); }, quote!(&GROUP_METAS), ) diff --git a/derive/src/model.rs b/derive/src/model.rs index 203a19e47..a1daa6cc0 100644 --- a/derive/src/model.rs +++ b/derive/src/model.rs @@ -883,7 +883,20 @@ impl Cli { )); } } - for decl in &self.groups { + for (i, decl) in self.groups.iter().enumerate() { + // Two declarations of one group would be read first-match-wins, so the second + // one's properties would be silently dropped — a `required` written and not + // enforced, which is worse than not being able to write it. + if self.groups[..i].iter().any(|d| d.name == decl.name) { + return Err(syn::Error::new( + decl.span, + format!( + "group `{}` is declared twice; one declaration carries all of \ + its properties", + decl.name + ), + )); + } if !group_members.iter().any(|(n, _)| *n == decl.name) { return Err(syn::Error::new( decl.span, @@ -3520,6 +3533,64 @@ mod tests { assert!(err.contains("takes no value"), "unhelpful message: {err}"); } + #[test] + fn a_group_is_declared_once_and_joined_by_at_least_two_flags() { + // Two declarations would be read first-match-wins, so the second one's + // properties would be silently dropped — a `required` written and not enforced. + let err = rejection( + r#" + #[usage(group("input", required))] + #[usage(group("input", multiple))] + struct Ex { + #[usage(long, group = "input")] + file: Option, + #[usage(long, group = "input")] + url: Option, + } + "#, + ); + assert!(err.contains("declared twice"), "unhelpful message: {err}"); + + // A group of one is a statement about that flag. + let err = rejection( + r#" + struct Ex { + #[usage(long, group = "input")] + file: Option, + } + "#, + ); + assert!(err.contains("one flag in it"), "unhelpful message: {err}"); + + // And a declaration nothing joins holds for nothing. + let err = rejection( + r#" + #[usage(group("input", required))] + struct Ex { + #[usage(long)] + file: Option, + } + "#, + ); + assert!( + err.contains("no field is in it"), + "unhelpful message: {err}" + ); + + // A positional cannot be in one, as it cannot hold any other relationship. + let err = rejection( + r#" + struct Ex { + #[usage(group = "input")] + target: String, + #[usage(long, group = "input")] + url: Option, + } + "#, + ); + assert!(err.contains("between flags"), "unhelpful message: {err}"); + } + #[test] fn an_alias_cannot_name_a_sibling() { // The parser takes the first table entry that matches, so a name claimed twice From eb473a348815e84b7c8e2f2c0172cb62a67dc6f2 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 17 Aug 2026 04:44:40 +0000 Subject: [PATCH 3/8] fix(derive): a group name is claimed once, and a conflict still answers first MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three from review of the group surface. **A parent and a flattened child could both declare `input`.** Each struct checks only its own members, so one member from either side satisfied neither exclusion, and the emitted KDL carried two `group "input"` nodes saying different things. Neither expansion can see the other, so it is caught where the joined tables are visible — beside the duplicate-key, duplicate-flag-form and unfillable-argument assertions in `Spec::to_kdl`, which exist for exactly this class of collision. **Requiredness could answer before a later group's exclusivity.** Emitted as one block per group, an unsatisfied `input` reported `MissingGroup` before `format` noticed it had been given two members — and before a flattened child's conflicts ran at all. Two passes now: every exclusivity check, then every requiredness one, which is the order the rest of `check` already promises. What the user typed wrong is more useful than what they left out. **A group could still end up nameless.** `group("")` on the struct was refused and two fields saying `group = ""` formed the same nameless group by the back door. The fourth finding — duplicate group declarations dropping the second one's properties — was already fixed before it was posted; the compile error and its test are in the commit under this one. Co-Authored-By: Claude Opus 5 --- argv/src/spec.rs | 29 +++++++++++++++++++++ conformance/tests/post_binding.rs | 43 +++++++++++++++++++++++++++++++ derive/src/codegen.rs | 17 ++++++++---- derive/src/model.rs | 23 +++++++++++++++++ 4 files changed, 107 insertions(+), 5 deletions(-) diff --git a/argv/src/spec.rs b/argv/src/spec.rs index 01b9da037..739ab4c6d 100644 --- a/argv/src/spec.rs +++ b/argv/src/spec.rs @@ -78,6 +78,27 @@ fn duplicate_flag_form(cmd: &Command<'_>) -> Option { .find_map(|sub| duplicate_flag_form(sub)) } +/// A group name that two declarations on the same command both claim, if any. +/// +/// Within one struct the derive catches this, and `#[usage(flatten)]` joins declarations +/// from two expansions that cannot see each other — so a parent and the struct it +/// flattens can each declare `input`, and each then checks only its own members. One +/// member from either side would satisfy neither exclusion, and the emitted KDL would +/// carry two `group "input"` nodes saying different things. +/// +/// Checked here for the same reason duplicate flag forms are: this is where the joined +/// tables are visible. +fn duplicate_group_name(meta: &CommandMeta<'_>) -> Option { + let mut names: std::vec::Vec<&str> = meta.groups.iter().map(|g| g.name).collect(); + names.sort_unstable(); + if let Some(pair) = names.windows(2).find(|pair| pair[0] == pair[1]) { + return Some(pair[0].to_string()); + } + meta.subcommands + .iter() + .find_map(|sub| duplicate_group_name(sub)) +} + /// An argument that no word could ever reach, if any. /// /// An unbounded variadic takes every remaining word, so what follows it can never be filled — @@ -715,6 +736,14 @@ impl Spec<'_> { the parent and the struct it flattens each declared it.", duplicate_flag_form(self.root.cmd) ); + debug_assert!( + duplicate_group_name(self.root).is_none(), + "two groups on the same command are called {:?}, so each would enforce only \ + its own members and one from either side would satisfy neither. With \ + `flatten` this is the collision neither expansion can see: the parent and \ + the struct it flattens each declared it. Give one of them another name.", + duplicate_group_name(self.root) + ); debug_assert!( unfillable_arg(self.root.cmd).is_none(), "no word could ever reach the argument {:?}, because an unbounded variadic before \ diff --git a/conformance/tests/post_binding.rs b/conformance/tests/post_binding.rs index 576e472b7..3532cdafd 100644 --- a/conformance/tests/post_binding.rs +++ b/conformance/tests/post_binding.rs @@ -753,3 +753,46 @@ fn a_group_reaches_the_emitted_spec_and_usage_lib_agrees() { assert!(group.required); assert_eq!(group.members.len(), 3); } + +/// Two groups on one command, one required and one exclusive. +#[derive(Cli)] +#[usage(bin = "ex4")] +#[usage(group("input", required))] +struct TwoGroups { + #[usage(long, group = "input")] + file: Option, + #[usage(long, group = "input")] + url: Option, + #[usage(long, group = "format")] + json: bool, + #[usage(long, group = "format")] + yaml: bool, +} + +#[test] +fn a_conflict_answers_before_an_unsatisfied_group_does() { + // Both are wrong here: `input` has no member, and `format` has two. The conflict is + // the more useful answer — it says which flag not to have typed, where the other + // asks for one more — and it is the order the rest of the checks already follow. + let a = argv(["--json", "--yaml"]); + assert!( + matches!( + TwoGroups::parse_from(&a), + Err(Error::ConflictingFlags { .. }) + ), + "the exclusivity of a later group should answer before an earlier group's requiredness" + ); + + // With the conflict gone, the unsatisfied group is what is left to say. + let a = argv(["--json"]); + assert!(matches!( + TwoGroups::parse_from(&a), + Err(Error::MissingGroup { group: "input", .. }) + )); + + // And with both satisfied, the values land where they were declared. + let a = argv(["--file", "f", "--yaml"]); + let two = TwoGroups::parse_from(&a).expect("one from each group"); + assert_eq!(two.file.as_deref(), Some("f")); + assert!(two.url.is_none() && two.yaml && !two.json); +} diff --git a/derive/src/codegen.rs b/derive/src/codegen.rs index 6143ad1a1..0b8a9574c 100644 --- a/derive/src/codegen.rs +++ b/derive/src/codegen.rs @@ -3330,12 +3330,18 @@ fn post_binding(cli: &Cli) -> TokenStream { } } }); - quote! { - #exclusivity - #requiredness - } + (exclusivity, requiredness) }) .collect::>(); + // Two passes rather than one block per group, because the order between *kinds* of + // check is the one this function promises: what the user typed wrong before what + // they left out. Emitted together, an earlier group's `MissingGroup` would answer + // before a later group's `ConflictingFlags` — and before a flattened child's, since + // those run later still. + let group_exclusivity_checks: Vec = + group_checks.iter().filter_map(|(e, _)| e.clone()).collect(); + let group_required_checks: Vec = + group_checks.iter().filter_map(|(_, r)| r.clone()).collect(); // `required_if` and `required_unless` are the same question asked two ways: which // other flags decide whether this one had to be given. Neither needs to know the @@ -3402,10 +3408,11 @@ fn post_binding(cli: &Cli) -> TokenStream { // more useful of the two answers when a conflict has also left something // unfilled, and it is the one usage-lib reports. #(#conflict_checks)* - #(#group_checks)* + #(#group_exclusivity_checks)* #(#requirement_checks)* #(#flattened_checks)* #(#required_checks)* + #(#group_required_checks)* #(#relationship_required_checks)* #(#choice_checks)* #(#bound_checks)* diff --git a/derive/src/model.rs b/derive/src/model.rs index a1daa6cc0..14941c398 100644 --- a/derive/src/model.rs +++ b/derive/src/model.rs @@ -867,6 +867,16 @@ impl Cli { a `long` or a `short`", )); } + // `group("")` on the struct is refused as nameless; two fields saying + // `group = ""` would otherwise form the same nameless group by the back + // door, and it would be emitted and reported with nothing to call it. + if name.is_empty() { + return Err(syn::Error::new( + field.span, + "a group with no name answers to nothing; give it one, as \ + `group = \"input\"`", + )); + } match group_members.iter_mut().find(|(n, _)| *n == name) { Some((_, members)) => members.push(field), None => group_members.push((name, vec![field])), @@ -3589,6 +3599,19 @@ mod tests { "#, ); assert!(err.contains("between flags"), "unhelpful message: {err}"); + + // A group with no name answers to nothing, whichever way it is written. + let err = rejection( + r#" + struct Ex { + #[usage(long, group = "")] + file: Option, + #[usage(long, group = "")] + url: Option, + } + "#, + ); + assert!(err.contains("no name"), "unhelpful message: {err}"); } #[test] From 36eb46415ec0c1821cc5ce312cb8c76ec7ac4a9f Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 17 Aug 2026 10:23:26 +0000 Subject: [PATCH 4/8] fix(argv): reject duplicate groups in release builds --- argv/src/spec.rs | 2 +- conformance/tests/flatten.rs | 31 +++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/argv/src/spec.rs b/argv/src/spec.rs index 739ab4c6d..5a826329a 100644 --- a/argv/src/spec.rs +++ b/argv/src/spec.rs @@ -736,7 +736,7 @@ impl Spec<'_> { the parent and the struct it flattens each declared it.", duplicate_flag_form(self.root.cmd) ); - debug_assert!( + assert!( duplicate_group_name(self.root).is_none(), "two groups on the same command are called {:?}, so each would enforce only \ its own members and one from either side would satisfy neither. With \ diff --git a/conformance/tests/flatten.rs b/conformance/tests/flatten.rs index 3db34dce2..d89718d91 100644 --- a/conformance/tests/flatten.rs +++ b/conformance/tests/flatten.rs @@ -316,3 +316,34 @@ fn a_flattened_structs_group_is_enforced_and_emitted() { assert_eq!(spec.cmd.groups.len(), 1); assert!(spec.cmd.groups[0].required); } + +#[derive(Args)] +#[usage(group("input"))] +#[allow(dead_code)] +struct ChildInput { + #[usage(long, group = "input")] + json: bool, + #[usage(long, group = "input")] + yaml: bool, +} + +#[derive(Cli)] +#[usage(bin = "fl", group("input"))] +#[allow(dead_code)] +struct DuplicateFlattenedGroup { + #[usage(flatten)] + child: ChildInput, + #[usage(long, group = "input")] + file: bool, + #[usage(long, group = "input")] + url: bool, +} + +#[test] +#[should_panic(expected = "two groups on the same command are called")] +fn a_flattened_group_name_collision_is_rejected_in_every_build() { + // The two derive expansions cannot see each other's declarations. The joined metadata + // can, and emitting it must reject the collision with an ordinary assertion so release + // builds cannot write two independent groups with the same name. + DuplicateFlattenedGroup::to_kdl(); +} From f78f43a383049e192f275376c7322bce97731856 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 17 Aug 2026 12:19:28 +0000 Subject: [PATCH 5/8] fix(argv): validate flattened groups during parsing --- argv/src/diagnostic.rs | 77 +++++++++++++++++++++++++++++++++++- argv/src/spec.rs | 28 +++++++++++++ conformance/tests/flatten.rs | 31 --------------- 3 files changed, 104 insertions(+), 32 deletions(-) diff --git a/argv/src/diagnostic.rs b/argv/src/diagnostic.rs index 04d799630..b79f6b214 100644 --- a/argv/src/diagnostic.rs +++ b/argv/src/diagnostic.rs @@ -288,6 +288,41 @@ fn shown<'a>(meta: Option<&'a CommandMeta<'a>>, name: &str) -> String { name.to_string() } +/// A group member is stored as a selector (`--file` or `-f`), not a field name. +fn group_member_shown(meta: Option<&CommandMeta<'_>>, selector: &str) -> String { + let Some(meta) = meta else { + return selector.to_string(); + }; + let found = meta.flags.iter().find(|flag| { + flag.flag + .longs + .iter() + .any(|long| selector == format!("--{long}")) + || flag + .flag + .shorts + .iter() + .any(|short| selector == format!("-{}", *short as char)) + || flag + .flag + .negate + .is_some_and(|negate| selector == format!("--{negate}")) + }); + found + .map(|flag| { + let mut shown = crate::help::flag_spelling(flag); + if flag.flag.takes_value { + let name = flag.value_name.unwrap_or(flag.flag.name); + let _ = write!(shown, " <{name}>"); + if flag.flag.variadic { + shown.push('…'); + } + } + shown + }) + .unwrap_or_else(|| selector.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 @@ -602,7 +637,7 @@ pub fn render( style.error("error:") ); for member in *members { - let _ = writeln!(out, " {}", style.valid(&shown(here, member))); + let _ = writeln!(out, " {}", style.valid(&group_member_shown(here, member))); } } Error::ConflictingFlags { name, other } => { @@ -832,6 +867,46 @@ mod tests { assert_eq!(line, crate::help::usage_line(&["ex", "use"], &USE_META)); } + #[test] + fn a_missing_group_lists_value_taking_members_completely() { + static FILE: Flag = Flag { + name: "file", + longs: &["file"], + takes_value: true, + ..Flag::BOOL + }; + static ROOT: Command = Command { + name: "grouped", + flags: &[&FILE], + ..Command::EMPTY + }; + static META: CommandMeta = CommandMeta { + cmd: &ROOT, + flags: &[FlagMeta { + flag: &FILE, + value_name: Some("PATH"), + ..FlagMeta::EMPTY + }], + ..CommandMeta::EMPTY + }; + static SPEC: Spec = Spec { + name: "grouped", + bin: Some("grouped"), + root: &META, + ..Spec::EMPTY + }; + let message = render( + &SPEC, + &[], + &Error::MissingGroup { + group: "input", + members: &["--file"], + }, + Style::PLAIN, + ); + assert!(message.contains(" --file "), "{message}"); + } + #[test] fn a_missing_subcommand_prints_the_choices() { let message = rendered(&[], Error::MissingSubcommand); diff --git a/argv/src/spec.rs b/argv/src/spec.rs index 5a826329a..e12981c60 100644 --- a/argv/src/spec.rs +++ b/argv/src/spec.rs @@ -459,6 +459,17 @@ pub const fn concat_group_metas( let group = groups[g]; let mut i = 0; while i < group.len() { + // This function initialises a generated `static`, so a collision across a parent + // and a flattened child is rejected while the adopter compiles. Leaving this to + // `to_kdl` let direct parsing enforce two independent groups with the same name. + let mut seen = 0; + while seen < at { + assert!( + !crate::str_eq(out[seen].name, group[i].name), + "two flattened groups on one command have the same name" + ); + seen += 1; + } out[at] = group[i]; at += 1; i += 1; @@ -1853,3 +1864,20 @@ mod tests { assert_eq!(placeholder("BUMP", false, true), "[BUMP]"); } } +#[test] +#[should_panic(expected = "two flattened groups on one command have the same name")] +fn concatenating_group_metadata_rejects_duplicate_names() { + static LEFT: [GroupMeta; 1] = [GroupMeta { + name: "input", + members: &["--file", "--url"], + required: false, + multiple: false, + }]; + static RIGHT: [GroupMeta; 1] = [GroupMeta { + name: "input", + members: &["--json", "--yaml"], + required: false, + multiple: false, + }]; + let _ = concat_group_metas::<2>(&[&LEFT, &RIGHT]); +} diff --git a/conformance/tests/flatten.rs b/conformance/tests/flatten.rs index d89718d91..3db34dce2 100644 --- a/conformance/tests/flatten.rs +++ b/conformance/tests/flatten.rs @@ -316,34 +316,3 @@ fn a_flattened_structs_group_is_enforced_and_emitted() { assert_eq!(spec.cmd.groups.len(), 1); assert!(spec.cmd.groups[0].required); } - -#[derive(Args)] -#[usage(group("input"))] -#[allow(dead_code)] -struct ChildInput { - #[usage(long, group = "input")] - json: bool, - #[usage(long, group = "input")] - yaml: bool, -} - -#[derive(Cli)] -#[usage(bin = "fl", group("input"))] -#[allow(dead_code)] -struct DuplicateFlattenedGroup { - #[usage(flatten)] - child: ChildInput, - #[usage(long, group = "input")] - file: bool, - #[usage(long, group = "input")] - url: bool, -} - -#[test] -#[should_panic(expected = "two groups on the same command are called")] -fn a_flattened_group_name_collision_is_rejected_in_every_build() { - // The two derive expansions cannot see each other's declarations. The joined metadata - // can, and emitting it must reject the collision with an ordinary assertion so release - // builds cannot write two independent groups with the same name. - DuplicateFlattenedGroup::to_kdl(); -} From 9ee557c234a1418aade467f82d82546f43a526b8 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 17 Aug 2026 12:51:55 +0000 Subject: [PATCH 6/8] fix(derive): route group code through facade --- derive/src/codegen.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/derive/src/codegen.rs b/derive/src/codegen.rs index 0b8a9574c..c7935363d 100644 --- a/derive/src/codegen.rs +++ b/derive/src/codegen.rs @@ -2919,7 +2919,7 @@ fn group_meta_table(cli: &Cli) -> (TokenStream, TokenStream) { // Named directly, as the flag and argument tables beside this one are: the // generated items live in the user's own scope now rather than in a module // above it, so there is no path to rewrite. - Some(quote!(<#ty as ::usage_argv::spec::CommandArgs>::META.groups)) + Some(quote!(<#ty as usage_argv::spec::CommandArgs>::META.groups)) }) .collect(); if groups.is_empty() && flattened.is_empty() { @@ -2927,7 +2927,7 @@ fn group_meta_table(cli: &Cli) -> (TokenStream, TokenStream) { } let entries = groups.iter().map(|(name, required, multiple, members)| { quote! { - ::usage_argv::spec::GroupMeta { + usage_argv::spec::GroupMeta { name: #name, members: &[#(#members),*], required: #required, @@ -2939,19 +2939,19 @@ fn group_meta_table(cli: &Cli) -> (TokenStream, TokenStream) { if flattened.is_empty() { return ( quote! { - pub static GROUP_METAS: [::usage_argv::spec::GroupMeta; #len] = [#(#entries),*]; + pub static GROUP_METAS: [usage_argv::spec::GroupMeta; #len] = [#(#entries),*]; }, quote!(&GROUP_METAS), ); } ( quote! { - pub static OWN_GROUP_METAS: [::usage_argv::spec::GroupMeta; #len] = [#(#entries),*]; - const GROUP_META_GROUPS: &[&[::usage_argv::spec::GroupMeta<'static>]] = + pub static OWN_GROUP_METAS: [usage_argv::spec::GroupMeta; #len] = [#(#entries),*]; + const GROUP_META_GROUPS: &[&[usage_argv::spec::GroupMeta<'static>]] = &[&OWN_GROUP_METAS, #(#flattened),*]; - static GROUP_METAS: [::usage_argv::spec::GroupMeta<'static>; - ::usage_argv::table_len(GROUP_META_GROUPS)] = - ::usage_argv::spec::concat_group_metas(GROUP_META_GROUPS); + static GROUP_METAS: [usage_argv::spec::GroupMeta<'static>; + usage_argv::table_len(GROUP_META_GROUPS)] = + usage_argv::spec::concat_group_metas(GROUP_META_GROUPS); }, quote!(&GROUP_METAS), ) @@ -3305,7 +3305,7 @@ fn post_binding(cli: &Cli) -> TokenStream { quote! { if #a && #b { return ::std::result::Result::Err( - ::usage_argv::Error::ConflictingFlags { + usage_argv::Error::ConflictingFlags { name: #name_b, other: #name_a, }, @@ -3322,7 +3322,7 @@ fn post_binding(cli: &Cli) -> TokenStream { quote! { if !(#(#given)||*) { return ::std::result::Result::Err( - ::usage_argv::Error::MissingGroup { + usage_argv::Error::MissingGroup { group: #name, members: &[#(#selectors),*], }, From 16f3fb504c6d1f34a58f2620f522d64a979a5460 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 17 Aug 2026 19:22:20 +0000 Subject: [PATCH 7/8] fix(conformance): fill the group metadata in the spec-built tables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `conformance/src/tables.rs` arrived on main while this branch was open, and it builds a `CommandMeta` field by field — so the `groups` this branch adds to that struct left it one short. Filled the same way the neighbouring lists are, with each selector leaked on its own: a group names flags in selector form, and the metadata holds them that way. Co-Authored-By: Claude Opus 5 --- conformance/src/tables.rs | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/conformance/src/tables.rs b/conformance/src/tables.rs index 6a31926a2..6714d19cf 100644 --- a/conformance/src/tables.rs +++ b/conformance/src/tables.rs @@ -24,8 +24,8 @@ //! turned up a rule the two implementations disagree about that nothing had recorded. use usage::spec::cmd::SpecExample; -use usage::{Spec, SpecArg, SpecCommand, SpecComplete, SpecFlag}; -use usage_argv::spec::{ArgMeta, CommandMeta, Effect, Example, FlagMeta}; +use usage::{Spec, SpecArg, SpecCommand, SpecComplete, SpecFlag, SpecGroup}; +use usage_argv::spec::{ArgMeta, CommandMeta, Effect, Example, FlagMeta, GroupMeta}; use usage_argv::{Arg, Command, DoubleDash, Flag, UnknownFlags as ArgvUnknownFlags}; /// A command's two tables, built together so the metadata can borrow the parse table. @@ -144,6 +144,7 @@ pub fn build( after_help: opt(&cmd.after_help), after_long_help: opt(&cmd.after_help_long), examples: examples(&cmd.examples), + groups: groups(&cmd.groups), flags: Box::leak(flag_metas.into_boxed_slice()), args: Box::leak(arg_metas.into_boxed_slice()), subcommands: Box::leak( @@ -413,6 +414,30 @@ fn example(e: &SpecExample) -> Example<'static> { } } +/// The command's groups, as usage-argv's cold model of them. +/// +/// Selectors are leaked one at a time rather than joined: a group names flags the way every +/// other relationship does, and the metadata holds them in that form. +fn groups(list: &[SpecGroup]) -> &'static [GroupMeta<'static>] { + Box::leak( + list.iter() + .map(|g| GroupMeta { + name: leak(&g.name), + members: Box::leak( + g.members + .iter() + .map(|m| leak(m)) + .collect::>() + .into_boxed_slice(), + ), + required: g.required, + multiple: g.multiple, + }) + .collect::>() + .into_boxed_slice(), + ) +} + fn examples(list: &[SpecExample]) -> &'static [Example<'static>] { Box::leak( list.iter() From e1c52e39be6cc1e79f8fb672e71f4a4077fa4680 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 17 Aug 2026 19:33:52 +0000 Subject: [PATCH 8/8] fix(derive): a flattened struct's groups land where the field was written MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The group metadata put every group this struct declares before every group a flattened field brought in, whatever order the fields were written in. So a group declared *below* a flattened field came out above the ones that field carried, and the emitted KDL — which is what docs and anything reading the spec go by — disagreed with the source. Built in one walk over the fields now, flushing the run of local groups at each flattened field and splicing the child's in there. That is how the flag and argument tables beside it are already built, and for the same reason; groups were the one table not following it. A group whose members straddle a flattened field keeps its whole member list and sits where its *first* member was written, which is the order `declared_groups` already establishes. Co-Authored-By: Claude Opus 5 --- conformance/tests/flatten.rs | 58 +++++++++++++++++++++++++++ derive/src/codegen.rs | 77 +++++++++++++++++++++++++++--------- 2 files changed, 116 insertions(+), 19 deletions(-) diff --git a/conformance/tests/flatten.rs b/conformance/tests/flatten.rs index 3db34dce2..fd1dce109 100644 --- a/conformance/tests/flatten.rs +++ b/conformance/tests/flatten.rs @@ -316,3 +316,61 @@ fn a_flattened_structs_group_is_enforced_and_emitted() { assert_eq!(spec.cmd.groups.len(), 1); assert!(spec.cmd.groups[0].required); } + +/// A second group to flatten, so a struct can hold one on each side of one. +#[allow(dead_code)] +#[derive(Args)] +#[usage(group("format"))] +struct Formatting { + /// Compact output + #[usage(long, group = "format")] + compact: bool, + /// Pretty output + #[usage(long, group = "format")] + pretty: bool, +} + +/// A group before the flattened field, and another after it. +#[allow(dead_code)] +#[derive(Cli)] +#[usage(bin = "ord", group("source"), group("sink"))] +struct GroupOrder { + /// Read from a file + #[usage(long, group = "source")] + file: Option, + /// Read from a URL + #[usage(long, group = "source")] + url: Option, + #[usage(flatten)] + formatting: Formatting, + /// Write to a file + #[usage(long, group = "sink")] + out: Option, + /// Write to stdout + #[usage(long, group = "sink")] + stdout: bool, +} + +#[test] +fn a_flattened_structs_groups_land_where_the_field_was_written() { + // The order the flag and argument tables already promise, on the groups beside them: + // a flattened struct's groups splice in at the field, rather than after everything this + // struct declares. Emitting all the local ones first put `sink` — written below the + // flattened field — above the group that field brought in. + let kdl = GroupOrder::to_kdl(); + let at = |name: &str| kdl.find(name).unwrap_or_else(|| panic!("{name} in {kdl}")); + assert!( + at("\"source\"") < at("\"format\"") && at("\"format\"") < at("\"sink\""), + "groups follow the fields that declare them: {kdl}" + ); + + let spec: usage::Spec = kdl.parse().expect("the emitted spec should parse"); + assert_eq!( + spec.cmd + .groups + .iter() + .map(|g| g.name.as_str()) + .collect::>(), + ["source", "format", "sink"], + ); +} diff --git a/derive/src/codegen.rs b/derive/src/codegen.rs index c7935363d..d88e6e7ab 100644 --- a/derive/src/codegen.rs +++ b/derive/src/codegen.rs @@ -2909,23 +2909,22 @@ fn declared_groups(cli: &Cli) -> Vec<(String, bool, bool, Vec)> { /// spec-as-definition rule exists to prevent. fn group_meta_table(cli: &Cli) -> (TokenStream, TokenStream) { let groups = declared_groups(cli); - let flattened: Vec = cli - .fields + // Where each group's first member was written, which is the position `declared_groups` + // already orders them by. A group whose members straddle a flattened field still belongs + // where it *starts*, so it keeps its whole member list rather than being split in two. + let first_member_at: Vec = groups .iter() - .filter_map(|f| { - let Kind::Flatten { ty } = &f.kind else { - return None; - }; - // Named directly, as the flag and argument tables beside this one are: the - // generated items live in the user's own scope now rather than in a module - // above it, so there is no path to rewrite. - Some(quote!(<#ty as usage_argv::spec::CommandArgs>::META.groups)) + .map(|(name, _, _, _)| { + cli.fields + .iter() + .position(|f| { + f.group.as_deref() == Some(name.as_str()) + && Cli::selector_for_field(f).is_some() + }) + .unwrap_or(usize::MAX) }) .collect(); - if groups.is_empty() && flattened.is_empty() { - return (quote!(), quote!(&[])); - } - let entries = groups.iter().map(|(name, required, multiple, members)| { + let entry = |(name, required, multiple, members): &(String, bool, bool, Vec)| { quote! { usage_argv::spec::GroupMeta { name: #name, @@ -2934,9 +2933,50 @@ fn group_meta_table(cli: &Cli) -> (TokenStream, TokenStream) { multiple: #multiple, } } - }); - let len = groups.len(); - if flattened.is_empty() { + }; + + // One walk over the fields, so a flattened struct's groups land where the field was + // written rather than after everything this struct declares — the same interleaving the + // flag and argument tables are built with, and visible in the same places their order is. + let mut parts: Vec = Vec::new(); + let mut run: Vec = Vec::new(); + let mut emitted = vec![false; groups.len()]; + let mut any_flattened = false; + for (i, field) in cli.fields.iter().enumerate() { + let Kind::Flatten { ty } = &field.kind else { + continue; + }; + any_flattened = true; + for (g, group) in groups.iter().enumerate() { + if !emitted[g] && first_member_at[g] < i { + emitted[g] = true; + run.push(entry(group)); + } + } + if !run.is_empty() { + let entries = std::mem::take(&mut run); + parts.push(quote!(&[#(#entries),*])); + } + // Named directly, as the flag and argument tables beside this one are: the + // generated items live in the user's own scope now rather than in a module + // above it, so there is no path to rewrite. + parts.push(quote!(<#ty as usage_argv::spec::CommandArgs>::META.groups)); + } + for (g, group) in groups.iter().enumerate() { + if !emitted[g] { + run.push(entry(group)); + } + } + if !run.is_empty() { + parts.push(quote!(&[#(#run),*])); + } + + if parts.is_empty() { + return (quote!(), quote!(&[])); + } + if !any_flattened { + let len = groups.len(); + let entries = groups.iter().map(entry); return ( quote! { pub static GROUP_METAS: [usage_argv::spec::GroupMeta; #len] = [#(#entries),*]; @@ -2946,9 +2986,8 @@ fn group_meta_table(cli: &Cli) -> (TokenStream, TokenStream) { } ( quote! { - pub static OWN_GROUP_METAS: [usage_argv::spec::GroupMeta; #len] = [#(#entries),*]; const GROUP_META_GROUPS: &[&[usage_argv::spec::GroupMeta<'static>]] = - &[&OWN_GROUP_METAS, #(#flattened),*]; + &[#(#parts),*]; static GROUP_METAS: [usage_argv::spec::GroupMeta<'static>; usage_argv::table_len(GROUP_META_GROUPS)] = usage_argv::spec::concat_group_metas(GROUP_META_GROUPS);