From f1576eb5362aaa17970fb1de18104705050b1fc4 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Wed, 12 Aug 2026 01:15:57 +0000 Subject: [PATCH 1/2] feat(derive): declare which flags conflict and which require each other MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three relationships a CLI has to be able to state, and the ones that need only whether a flag was given rather than what order the flags arrived in: `conflicts`, `required_if`, `required_unless`. Each takes a flag the way the spec names one — `"--long"` or `"-s"` — and several as a list. A selector naming no flag on the command is a compile error, which is the point of writing a relationship in code: in a hand-written spec a typo'd selector is a relationship that quietly does not hold, and nothing ever says so. Naming its own field is refused for the same reason. The checks read the `__given_*` flags after the environment fallback has run, so a value from the environment counts the same as a typed one — clap does that too, and an asymmetric rule would make the same pair of flags a conflict or not depending on which side happened to be typed. Conflicts are reported before required-ness: when giving two flags that cannot go together has also left something unfilled, "you gave both" is the more useful of the two answers. `overrides` is still missing, and now for the only reason left: it is about which flag came last, which nothing records yet. The stale claim that `env` is unread and required-ness unreported is gone from the derive docs, along with a duplicated bullet. --- PLAN.md | 20 +- argv/src/lib.rs | 10 + argv/src/spec.rs | 16 ++ conformance/tests/derive.rs | 63 ++++++ conformance/tests/post_binding.rs | 96 +++++++++ ...roundtrip__the_emitted_spec_is_stable.snap | 6 +- conformance/tests/spec_roundtrip.rs | 34 +++- derive/src/codegen.rs | 90 +++++++++ derive/src/lib.rs | 30 +-- derive/src/model.rs | 184 +++++++++++++++++- 10 files changed, 520 insertions(+), 29 deletions(-) diff --git a/PLAN.md b/PLAN.md index dd4614c7e..5c15914be 100644 --- a/PLAN.md +++ b/PLAN.md @@ -112,9 +112,9 @@ manpages, and SDKs — never a runtime dependency of somebody else's program. paragraph short, whole block long), and spec emission, from usage-native attributes. Unsupported field types are a compile error rather than a surprise, and the messages point at the offending field. -- [ ] **Subcommands in the derive** — one command per struct today. Needs an enum - of variants, cross-type table references, and a nested command path through - the parse function. +- [x] **Subcommands in the derive** — an enum of variants, each holding the struct + that declares its flags, nested to any depth. A command is selected by its + position in the parent's table, found from the address the parser hands back. - [ ] **Typed values** — fields are text today (`String`, `Option`, `Vec`, `bool`, counting integers). Parsing into other types needs an error type for value conversion, which is also where `env`, required-ness, @@ -123,10 +123,11 @@ manpages, and SDKs — never a runtime dependency of somebody else's program. (`requires`/`conflicts`/`overrides`/`required_unless`), `var`, `count`, `env`, defaults, delimiters, the `double_dash` modes, global flags, flatten, boxed subcommand variants, headings, `cfg`-gated variants. -- [ ] **The post-binding layer** — `required`, `choices`, `env` fallback, - defaults, `var_min`/`var_max`, `overrides`. These need a value's type, so - they belong with the derive rather than in the parser. Closes the 24 corpus - vectors usage-argv reports as out of scope. +- [ ] **The post-binding layer** — `required`, `choices`, `env` fallback, defaults, + `var_min`/`var_max`, `conflicts`, `required_if` and `required_unless` are done; + `overrides` is what remains, and it is the one that needs arrival order rather + than only whether a flag was given. These need a value's type, so they belong + with the derive rather than in the parser. ### Spec gaps found on the way @@ -140,6 +141,11 @@ carrying them rather than being worked around. - [x] **Rendering headings** — help output and generated markdown both group by heading now. Unheaded entries keep the default section and come first, and a heading with nothing visible in it produces no section. +- [x] **`conflicts`** — the spec could say `overrides`, `required_if` and + `required_unless`, but not that two flags must not be given together, so the + forty `conflicts_with` relationships mise declares in clap were being dropped + by the bridge. Now a flag property, enforced by usage-lib, and a value from + the environment counts on both sides of the check as clap does. - [ ] **A mount on the root command** — the spec accepts `mount` only inside a `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 diff --git a/argv/src/lib.rs b/argv/src/lib.rs index 3cb56f767..e10ed9c33 100644 --- a/argv/src/lib.rs +++ b/argv/src/lib.rs @@ -319,6 +319,16 @@ pub enum Error<'t, 'v> { max: usize, got: usize, }, + /// Two flags declared to conflict were both given. + /// + /// Carries both names because either one alone reads as a puzzle: which flag is + /// unwelcome depends entirely on what else is on the command line. + ConflictingFlags { + /// The flag whose declaration names the conflict. + name: &'t str, + /// The flag it cannot be given with, as the declaration spells it. + other: &'t str, + }, /// A subcommand was required, and none was given. MissingSubcommand, } diff --git a/argv/src/spec.rs b/argv/src/spec.rs index 367955ae7..05901d9a7 100644 --- a/argv/src/spec.rs +++ b/argv/src/spec.rs @@ -143,6 +143,14 @@ pub struct FlagMeta<'a> { pub var_max: Option, /// Flags this one displaces when both are given. pub overrides: &'a [&'a str], + /// Flags that cannot be given alongside this one. + /// + /// Where [`overrides`](Self::overrides) resolves a collision by letting the last + /// flag win, this reports it: the combination has no meaning, so honouring one + /// side silently would hide a mistake. + pub conflicts: &'a [&'a str], + /// Flags that make this one necessary. + pub required_if: &'a [&'a str], /// Flags that make this one unnecessary. pub required_unless: &'a [&'a str], /// Heading to list this flag under in help output. Presentational: it groups @@ -168,6 +176,8 @@ impl FlagMeta<'_> { var_min: None, var_max: None, overrides: &[], + conflicts: &[], + required_if: &[], required_unless: &[], help_heading: None, effect: None, @@ -484,6 +494,8 @@ fn write_flag(out: &mut String, meta: &FlagMeta<'_>, depth: usize) -> core::fmt: } write_single_default(out, meta.default)?; write_single_list(out, "overrides", meta.overrides)?; + write_single_list(out, "conflicts", meta.conflicts)?; + write_single_list(out, "required_if", meta.required_if)?; write_single_list(out, "required_unless", meta.required_unless)?; let has_children = meta.long_help.is_some() @@ -491,6 +503,8 @@ fn write_flag(out: &mut String, meta: &FlagMeta<'_>, depth: usize) -> core::fmt: || !meta.choices.is_empty() || meta.default.len() > 1 || meta.overrides.len() > 1 + || meta.conflicts.len() > 1 + || meta.required_if.len() > 1 || meta.required_unless.len() > 1; if !has_children { out.push('\n'); @@ -505,6 +519,8 @@ fn write_flag(out: &mut String, meta: &FlagMeta<'_>, depth: usize) -> core::fmt: } write_many_defaults(out, meta.default, inner)?; write_many_list(out, "overrides", meta.overrides, inner)?; + write_many_list(out, "conflicts", meta.conflicts, inner)?; + write_many_list(out, "required_if", meta.required_if, inner)?; write_many_list(out, "required_unless", meta.required_unless, inner)?; if meta.flag.takes_value { indent(out, inner)?; diff --git a/conformance/tests/derive.rs b/conformance/tests/derive.rs index 4be88b426..e842f404d 100644 --- a/conformance/tests/derive.rs +++ b/conformance/tests/derive.rs @@ -554,3 +554,66 @@ mod docs_example { assert!(Cli::to_kdl().contains(r#"flag "-j --jobs""#)); } } + +/// A CLI whose flags relate to one another. +/// +/// Separate from `Ex` so the relationships do not have to hold for every other test's +/// command line. Enforcement is checked in `post_binding.rs`; what matters here is +/// that the declarations reach the spec, since a conflict nobody wrote down is a +/// conflict `usage g markdown` and the completions cannot mention. +#[derive(Cli, Debug)] +#[usage(bin = "rel")] +struct Related { + /// Read from a file + #[usage(long, required_unless("--url", "--stdin"))] + file: Option, + /// Read from a URL + #[usage(long)] + url: Option, + /// Read from standard input + #[usage(short = 's', long, conflicts("--file", "--url"))] + stdin: bool, + /// Where to write + #[usage(long, required_if = "--stdin")] + out: Option, +} + +#[test] +fn flag_relationships_reach_the_spec() { + let spec: LibSpec = Related::to_kdl().parse().expect("valid spec"); + let flag = |name: &str| { + spec.cmd + .flags + .iter() + .find(|f| f.name == name) + .unwrap_or_else(|| panic!("--{name} should be in the spec")) + .clone() + }; + + assert_eq!( + flag("stdin").conflicts, + vec!["--file".to_string(), "--url".to_string()] + ); + assert_eq!(flag("out").required_if, vec!["--stdin".to_string()]); + assert_eq!( + flag("file").required_unless, + vec!["--url".to_string(), "--stdin".to_string()] + ); + + // Written as the declaration spelled them, not normalized to the field name: a + // selector is how the spec refers to a flag, and `-s` would be just as valid. + assert!( + Related::to_kdl().contains(r#"conflicts "--file" "--url""#), + "{}", + Related::to_kdl() + ); + + // And the same declaration still parses: a satisfied set of relationships is + // invisible, which is the point. + let a = argv(["--stdin", "--out", "o"]); + let rel = Related::parse_from(&a).expect("should parse"); + assert!(rel.stdin); + assert_eq!(rel.out.as_deref(), Some("o")); + assert!(rel.file.is_none()); + assert!(rel.url.is_none()); +} diff --git a/conformance/tests/post_binding.rs b/conformance/tests/post_binding.rs index 260f521fd..7cd770973 100644 --- a/conformance/tests/post_binding.rs +++ b/conformance/tests/post_binding.rs @@ -200,3 +200,99 @@ fn the_checks_reach_the_spec_too() { let target = spec.cmd.args.iter().find(|a| a.name == "target").unwrap(); assert!(target.required); } + +/// A CLI whose flags relate to each other. +/// +/// `--stdin` conflicts with `--file` and `--url`; `--out` is required when writing +/// from `--stdin`; and reading has to come from somewhere, so `--file` is required +/// unless one of the other two says where. +#[derive(Cli)] +#[usage(bin = "rel")] +struct Rel { + /// Read from a file + #[usage(long, required_unless("--url", "--stdin"))] + file: Option, + /// Read from a URL + #[usage(long)] + url: Option, + /// Read from standard input + #[usage(long, conflicts("--file", "--url"), short = 's')] + stdin: bool, + /// Where to write + #[usage(long, required_if = "--stdin")] + out: Option, +} + +#[test] +fn conflicting_flags_cannot_both_be_given() { + let a = argv(["--stdin", "--out", "o", "--file", "f"]); + assert!(matches!( + Rel::parse_from(&a), + Err(Error::ConflictingFlags { + name: "stdin", + other: "file" + }) + )); + + // The other target of the same declaration, and by its short form, since the + // conflict is between flags rather than between spellings. + let a = argv(["-s", "--out", "o", "--url", "u"]); + assert!(matches!( + Rel::parse_from(&a), + Err(Error::ConflictingFlags { + name: "stdin", + other: "url" + }) + )); + + // Either side alone is fine. + let a = argv(["--stdin", "--out", "o"]); + assert!(Rel::parse_from(&a).expect("should parse").stdin); + let a = argv(["--file", "f"]); + assert_eq!( + Rel::parse_from(&a).expect("should parse").file.as_deref(), + Some("f") + ); +} + +#[test] +fn a_conflict_is_reported_before_what_it_left_unfilled() { + // `--stdin` without `--out` is also a missing-required error. The conflict is the + // more useful answer: it says which flag not to have typed, where the other would + // ask for one more. + let a = argv(["--stdin", "--file", "f"]); + assert!(matches!( + Rel::parse_from(&a), + Err(Error::ConflictingFlags { name: "stdin", .. }) + )); +} + +#[test] +fn required_if_applies_only_when_the_other_flag_is_given() { + let a = argv(["--stdin"]); + assert!(matches!( + Rel::parse_from(&a), + Err(Error::MissingRequired { name: "out" }) + )); + + // Without `--stdin`, `--out` is optional. + let a = argv(["--file", "f"]); + assert!(Rel::parse_from(&a).expect("should parse").out.is_none()); +} + +#[test] +fn required_unless_is_satisfied_by_the_other_flag() { + // Neither given: `--file` is missing. + let a = argv([]); + assert!(matches!( + Rel::parse_from(&a), + Err(Error::MissingRequired { name: "file" }) + )); + + // `--url` stands in for it. + let a = argv(["--url", "u"]); + assert_eq!( + Rel::parse_from(&a).expect("should parse").url.as_deref(), + Some("u") + ); +} diff --git a/conformance/tests/snapshots/spec_roundtrip__the_emitted_spec_is_stable.snap b/conformance/tests/snapshots/spec_roundtrip__the_emitted_spec_is_stable.snap index c77fe23b9..aded98f7c 100644 --- a/conformance/tests/snapshots/spec_roundtrip__the_emitted_spec_is_stable.snap +++ b/conformance/tests/snapshots/spec_roundtrip__the_emitted_spec_is_stable.snap @@ -15,7 +15,7 @@ flag "-j --jobs" help="how many jobs, and a quote: \"" global=#true help_heading } flag "--color" help="colorize output" negate="--no-color" default="true" flag "-v --verbose" hide=#true count=#true -flag "--include" help="patterns to include" var=#true var_min=1 var_max=5 overrides="--exclude" { +flag "--include" help="patterns to include" var=#true var_min=1 var_max=5 overrides="--exclude" required_if="--verbose" { arg "..." } flag "--shell" required=#true { @@ -24,7 +24,7 @@ flag "--shell" required=#true { choices "bash" "zsh" "fish" } } -flag "--prune" help="delete anything unused" effect="destructive" { +flag "--prune" help="delete anything unused" effect="destructive" conflicts="--force" { long_help "Deletes things.\u{1b}[0m Carefully." overrides "--keep" "--dry-run" } @@ -33,6 +33,8 @@ flag "--paths" { "/usr/bin" "/usr/local/bin" } + conflicts "--include" "--prune" + required_if "--force" "--prune" arg "" } arg "[file]" help="the file" help_heading="Input" env="EX_FILE" default="a.txt" diff --git a/conformance/tests/spec_roundtrip.rs b/conformance/tests/spec_roundtrip.rs index 96781290b..b712ff3a4 100644 --- a/conformance/tests/spec_roundtrip.rs +++ b/conformance/tests/spec_roundtrip.rs @@ -264,6 +264,8 @@ static ROOT_META: CommandMeta = CommandMeta { var_min: Some(1), var_max: Some(5), overrides: &["--exclude"], + // One target, which the writer puts on the node as a property. + required_if: &["--verbose"], ..FlagMeta::EMPTY }, FlagMeta { @@ -284,13 +286,17 @@ static ROOT_META: CommandMeta = CommandMeta { long_help: Some("Deletes things.\u{1b}[0m Carefully."), effect: Some(Effect::Destructive), overrides: &["--keep", "--dry-run"], + conflicts: &["--force"], ..FlagMeta::EMPTY }, - // More than one default, which cannot be written as a property. + // More than one default, which cannot be written as a property. Neither can + // more than one conflict or condition, so they go in the same child block. FlagMeta { flag: &PATHS, value_name: Some("path"), default: &["/usr/bin", "/usr/local/bin"], + conflicts: &["--include", "--prune"], + required_if: &["--force", "--prune"], ..FlagMeta::EMPTY }, ], @@ -406,6 +412,32 @@ fn variadic_bounds_and_overrides_survive() { assert_eq!(include.var_min, Some(1)); assert_eq!(include.var_max, Some(5)); assert_eq!(include.overrides, vec!["--exclude".to_string()]); + assert_eq!(include.required_if, vec!["--verbose".to_string()]); +} + +#[test] +fn conflicts_and_conditions_survive_in_both_spellings() { + // One target is a property on the node and several are a child block, so each + // list has to be read back in the form the writer chose for it. + let spec = parsed(); + let flag = |name: &str| { + spec.cmd + .flags + .iter() + .find(|f| f.name == name) + .unwrap_or_else(|| panic!("--{name} should be in the spec")) + .clone() + }; + + assert_eq!(flag("prune").conflicts, vec!["--force".to_string()]); + assert_eq!( + flag("paths").conflicts, + vec!["--include".to_string(), "--prune".to_string()] + ); + assert_eq!( + flag("paths").required_if, + vec!["--force".to_string(), "--prune".to_string()] + ); } #[test] diff --git a/derive/src/codegen.rs b/derive/src/codegen.rs index 384cbfa07..9574f2372 100644 --- a/derive/src/codegen.rs +++ b/derive/src/codegen.rs @@ -319,6 +319,11 @@ fn flag_meta(i: usize, field: &Field) -> TokenStream { let required = field.shape == Shape::Required; let choices = choices_tokens(field); let (var_min, var_max) = bounds_tokens(field); + // Written as declared, in the spec's own spelling, so the emitted KDL says what + // the struct says. + let conflicts = &field.conflicts; + let required_if = &field.required_if; + let required_unless = &field.required_unless; quote! { pub static #name: FlagMeta = FlagMeta { @@ -335,6 +340,9 @@ fn flag_meta(i: usize, field: &Field) -> TokenStream { choices: #choices, var_min: #var_min, var_max: #var_max, + conflicts: &[#(#conflicts),*], + required_if: &[#(#required_if),*], + required_unless: &[#(#required_unless),*], ..FlagMeta::EMPTY }; } @@ -1244,9 +1252,91 @@ fn post_binding(cli: &Cli) -> TokenStream { }) }); + // A conflict asks whether two flags both ended up with a value, which is why it + // reads `__given_*` rather than the fields themselves: a `bool` flag that was given + // as `false` is still a flag the user asked for. Env fallback has already run, so a + // value from the environment counts the same as a typed one — clap does that too, + // and an asymmetric rule would make the same pair a conflict or not depending on + // which side happened to be typed. + let conflict_checks = cli.fields.iter().flat_map(move |f| { + let given = format_ident!("__given_{}", f.ident); + let name = &f.name; + f.conflicts.iter().filter_map(move |selector| { + // Resolved in the model, which rejects a selector naming nothing. + let other = cli.field_for_selector(selector)?; + let other_given = format_ident!("__given_{}", other.ident); + let other_name = &other.name; + Some(quote! { + if partial.#given && partial.#other_given { + return ::std::result::Result::Err( + ::usage_argv::Error::ConflictingFlags { + name: #name, + other: #other_name, + }, + ); + } + }) + }) + }); + + // `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, + // beside plain required-ness, from the same `__given_*` flags. + let relationship_required_checks = cli.fields.iter().filter_map(move |f| { + if f.required_if.is_empty() && f.required_unless.is_empty() { + return None; + } + let given = format_ident!("__given_{}", f.ident); + let name = &f.name; + let selector_given = |selector: &String| { + let other = cli.field_for_selector(selector)?; + let other_given = format_ident!("__given_{}", other.ident); + Some(quote!(partial.#other_given)) + }; + let if_given: Vec<_> = f.required_if.iter().filter_map(selector_given).collect(); + let unless_given: Vec<_> = f + .required_unless + .iter() + .filter_map(selector_given) + .collect(); + // Absent, with nothing standing in for it: a default or an environment + // variable has already filled the field and set `__given_*`. + let missing = quote! { + return ::std::result::Result::Err( + ::usage_argv::Error::MissingRequired { name: #name }, + ); + }; + let required_if = (!if_given.is_empty()).then(|| { + quote! { + if #(#if_given)||* { + #missing + } + } + }); + let required_unless = (!unless_given.is_empty()).then(|| { + quote! { + if !(#(#unless_given)||*) { + #missing + } + } + }); + Some(quote! { + if !partial.#given { + #required_if + #required_unless + } + }) + }); + quote! { #(#env_fallbacks)* + // Before required-ness: "you gave two flags that cannot go together" is the + // more useful of the two answers when a conflict has also left something + // unfilled, and it is the one usage-lib reports. + #(#conflict_checks)* #(#required_checks)* + #(#relationship_required_checks)* #(#choice_checks)* #(#bound_checks)* #sub_check diff --git a/derive/src/lib.rs b/derive/src/lib.rs index bcfc070cd..c81b74ce1 100644 --- a/derive/src/lib.rs +++ b/derive/src/lib.rs @@ -138,26 +138,30 @@ //! | `hide` | keep it out of help and completions | //! | `double_dash = "required"` | a positional only fillable after `--` | //! | `arg` | force a field to be positional | +//! | `conflicts = "--other"` | a flag this one cannot be given with | +//! | `required_if = "--other"` | a flag whose presence makes this one necessary | +//! | `required_unless = "--other"` | a flag whose presence makes this one unnecessary | +//! +//! The last three name a flag the way the spec does — `"--long"` or `"-s"` — and take +//! several as a list: `conflicts("--file", "--url")`. A selector naming no flag on the +//! command 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. //! //! # What this version does not do //! //! Published early on purpose, so it can be used and argued with — but these are //! real limits, not omissions from the docs. //! -//! - **`overrides` and `required_unless`.** Both are about one flag's effect on -//! another, which needs to know the order they arrived in; nothing records that -//! yet. -//! - **Typed values.** Everything is text: a field is a `String`, an -//! `Option`, a `Vec`, a `bool`, or a counting integer. Parsing -//! into a richer type needs an error for a value that will not convert. +//! - **`overrides`.** Unlike the other relationships, it is not about whether two +//! flags were both given but about which came *last*, and nothing records arrival +//! order yet. //! - **Typed values.** Fields are `bool`, `String`, `Option`, -//! `Vec`, or an unsigned integer with `count`. Anything else is a -//! compile error rather than a surprise, because converting a value is also -//! where required-ness, `choices`, and bounds get enforced, and that layer does -//! not exist yet. -//! - **Enforce what it records.** `default` and `env` are written into the spec -//! and `default` is applied, but `env` is not read, and a missing required value -//! is not reported. Same reason. +//! `Vec`, or an unsigned integer with `count`. Anything else is a compile +//! error rather than a surprise: converting a value needs somewhere to report one +//! that will not convert, and that is a layer this version does not have. +//! - **Flattening.** A struct cannot yet borrow another struct's flags, so a set of +//! options shared by several commands has to be repeated. use proc_macro::TokenStream; use syn::{parse_macro_input, DeriveInput}; diff --git a/derive/src/model.rs b/derive/src/model.rs index 9c465e493..a197c9692 100644 --- a/derive/src/model.rs +++ b/derive/src/model.rs @@ -51,6 +51,13 @@ pub struct Field { pub choices: Vec, pub var_min: Option, pub var_max: Option, + /// Flags this one cannot be given with. Checked after the parse: whether a flag + /// is unwelcome depends on the whole command line, not on the token itself. + pub conflicts: Vec, + /// Flags whose presence makes this one necessary. + pub required_if: Vec, + /// Flags whose presence makes this one unnecessary. + pub required_unless: Vec, pub hide: bool, /// Whether the flag may be given more than once, taking one value each time. /// @@ -196,6 +203,35 @@ impl Cli { Ok(cli) } + /// The field a flag selector names, as the spec spells one: `--long` or `-s`. + /// + /// A negation counts, since `--no-color` is another way to name the field `--color` + /// declared — the two share one place to record whether they were given. + pub fn field_for_selector(&self, selector: &str) -> Option<&Field> { + self.fields.iter().find(|field| { + let Kind::Flag { + longs, + shorts, + negate, + .. + } = &field.kind + else { + return false; + }; + match selector.strip_prefix("--") { + Some(long) => longs.iter().chain(negate.iter()).any(|l| l == long), + // A short is one character; `-abc` is three flags rather than a name. + None => selector + .strip_prefix('-') + .and_then(|rest| { + let mut chars = rest.chars(); + chars.next().filter(|_| chars.next().is_none()) + }) + .is_some_and(|short| shorts.contains(&short)), + } + }) + } + /// Reject declarations that would compile into a CLI nobody could use. fn check(&self) -> syn::Result<()> { let mut seen_long: Vec<(&str, Span)> = Vec::new(); @@ -265,6 +301,36 @@ impl Cli { } } } + + // 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 + // nothing quietly holds no relationship at all. + for field in &self.fields { + for (option, selectors) in [ + ("conflicts", &field.conflicts), + ("required_if", &field.required_if), + ("required_unless", &field.required_unless), + ] { + for selector in selectors { + let Some(target) = self.field_for_selector(selector) else { + return Err(syn::Error::new( + field.span, + format!( + "`{option} = \"{selector}\"` names no flag on this \ + command; write it as the spec does, `--long` or `-s`" + ), + )); + }; + if target.ident == field.ident { + return Err(syn::Error::new( + field.span, + format!("`{option} = \"{selector}\"` names its own field"), + )); + } + } + } + } Ok(()) } } @@ -301,10 +367,8 @@ impl Field { return Ok(None); } - // `Option` says the subcommand may be left out. A bare `T` cannot be - // satisfied by this version — nothing yet reports "a subcommand is - // required", which is a post-binding question. - // `Option` may be left unfilled; a bare `T` requires one. + // `Option` says the subcommand may be left out; a bare `T` requires one, + // reported once the last token has been read. let name = type_name(&field.ty); let (ty, optional) = match name .strip_prefix("Option<") @@ -330,6 +394,9 @@ impl Field { choices: Vec::new(), var_min: None, var_max: None, + conflicts: Vec::new(), + required_if: Vec::new(), + required_unless: Vec::new(), hide: false, repeatable: false, span, @@ -369,6 +436,9 @@ impl Field { let mut choices: Vec = Vec::new(); let mut var_min: Option = None; let mut var_max: Option = None; + let mut conflicts: Vec = Vec::new(); + let mut required_if: Vec = Vec::new(); + let mut required_unless: Vec = Vec::new(); for attr in attrs(&field.attrs) { for meta in nested(attr)? { @@ -428,6 +498,12 @@ impl Field { )); } } + // Both spellings the spec has: one target as a value, several as a + // list. A flag selector never contains a comma, so unlike `choices` + // there is nothing to lose by accepting the shorter form. + "conflicts" => conflicts = selectors(&meta)?, + "required_if" => required_if = selectors(&meta)?, + "required_unless" => required_unless = selectors(&meta)?, "var_min" => var_min = Some(int_value(&meta)?), "var_max" => var_max = Some(int_value(&meta)?), "default" => default = Some(string_value(&meta)?), @@ -453,8 +529,9 @@ impl Field { format!( "unknown option `{other}`; a field takes `name`, `long`, \ `short`, `negate`, `global`, `var`, `variadic`, \ - `count`, `hide`, `arg`, `env`, `default`, \ - `help_heading`, and `double_dash`" + `count`, `hide`, `arg`, `env`, `default`, `choices`, \ + `var_min`, `var_max`, `conflicts`, `required_if`, \ + `required_unless`, `help_heading`, and `double_dash`" ), )); } @@ -713,6 +790,9 @@ impl Field { choices, var_min, var_max, + conflicts, + required_if, + required_unless, hide, repeatable, span, @@ -819,6 +899,25 @@ fn string_value(meta: &Meta) -> syn::Result { } } +/// One flag selector as a value, or several as a list. +/// +/// Selectors are written the way the spec writes them — `"--stdin"`, `"-s"` — rather +/// 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. +fn selectors(meta: &Meta) -> syn::Result> { + match meta { + Meta::List(list) => Ok(list + .parse_args_with( + syn::punctuated::Punctuated::::parse_terminated, + )? + .into_iter() + .map(|lit| lit.value()) + .collect()), + _ => Ok(vec![string_value(meta)?]), + } +} + fn int_value(meta: &Meta) -> syn::Result { let value = &meta.require_name_value()?.value; match value { @@ -1074,3 +1173,76 @@ impl Variant { }) } } + +#[cfg(test)] +mod tests { + use super::Cli; + + fn cli(body: &str) -> syn::Result { + Cli::from_input(&syn::parse_str::(body).expect("valid Rust")) + } + + /// The message a bad declaration produces, which is the part worth asserting on: + /// `Cli` is not `Debug`, and the error is what the user sees. + fn rejection(body: &str) -> String { + match cli(body) { + Ok(_) => panic!("should not have compiled"), + Err(e) => e.to_string(), + } + } + + #[test] + fn a_selector_resolves_by_long_short_or_negation() { + let cli = cli(r#" + struct Ex { + #[usage(long, negate = "no-color")] + color: bool, + #[usage(short = 'f', long)] + force: bool, + } + "#) + .expect("should compile"); + + let named = |selector: &str| { + cli.field_for_selector(selector) + .map(|f| f.ident.to_string()) + }; + assert_eq!(named("--color").as_deref(), Some("color")); + assert_eq!(named("--no-color").as_deref(), Some("color")); + assert_eq!(named("-f").as_deref(), Some("force")); + assert_eq!(named("--force").as_deref(), Some("force")); + assert_eq!(named("--nope"), None); + // Not a name: `-fx` is two flags bundled, and a bare word is not a selector. + assert_eq!(named("-fx"), None); + assert_eq!(named("force"), None); + } + + #[test] + fn a_selector_naming_nothing_is_a_compile_error() { + let err = rejection( + r#" + struct Ex { + #[usage(long, conflicts = "--stdout")] + out: Option, + } + "#, + ); + assert!(err.contains("names no flag"), "unhelpful message: {err}"); + } + + #[test] + fn a_selector_naming_its_own_field_is_a_compile_error() { + let err = rejection( + r#" + struct Ex { + #[usage(long, required_unless = "--out")] + out: Option, + } + "#, + ); + assert!( + err.contains("names its own field"), + "unhelpful message: {err}" + ); + } +} From b3d833d4f2969d6006e640216bfdccadf5b68f66 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Wed, 12 Aug 2026 02:03:31 +0000 Subject: [PATCH 2/2] fix(derive): keep a relationship from claiming more than the spec can say MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found four ways a declaration could mean less, or more, than it read as. A positional could declare `conflicts`, `required_if` or `required_unless`, and the check was generated — but the spec records these on a flag and has nowhere to put them on an argument, so the running binary enforced something its own spec, docs and completions could not describe. Refused now, like `count` and `negate` are. `required_unless` on a bare `String` was accepted and could never take effect: the shape-based required check runs first and does not know about the condition, so the declared exception was unreachable. It needs somewhere to put "absent", which means an `Option`. A field with a `default` was reported missing by `required_if` and `required_unless` even though it was already filled. Applying a default does not set `__given_*`; plain required-ness has always skipped defaulted fields, and usage-lib does too. And `conflicts()` with nothing in it compiled into no relationship at all, which is a declaration that reads as though it does something. --- conformance/tests/post_binding.rs | 13 ++++ derive/src/codegen.rs | 5 ++ derive/src/lib.rs | 15 ++-- derive/src/model.rs | 116 +++++++++++++++++++++++++++--- 4 files changed, 135 insertions(+), 14 deletions(-) diff --git a/conformance/tests/post_binding.rs b/conformance/tests/post_binding.rs index 7cd770973..52f17d075 100644 --- a/conformance/tests/post_binding.rs +++ b/conformance/tests/post_binding.rs @@ -221,6 +221,9 @@ struct Rel { /// Where to write #[usage(long, required_if = "--stdin")] out: Option, + /// How many at once + #[usage(long, default = "4", required_if = "--stdin")] + jobs: Option, } #[test] @@ -280,6 +283,16 @@ fn required_if_applies_only_when_the_other_flag_is_given() { assert!(Rel::parse_from(&a).expect("should parse").out.is_none()); } +#[test] +fn a_default_satisfies_a_condition_that_would_have_required_the_flag() { + // `--jobs` is required when `--stdin` is given, and it has a default — so it is + // already filled and no condition can make it missing. Plain required-ness works + // the same way, and so does usage-lib. + let a = argv(["--stdin", "--out", "o"]); + let rel = Rel::parse_from(&a).expect("should parse"); + assert_eq!(rel.jobs.as_deref(), Some("4")); +} + #[test] fn required_unless_is_satisfied_by_the_other_flag() { // Neither given: `--file` is missing. diff --git a/derive/src/codegen.rs b/derive/src/codegen.rs index 9574f2372..4f59c4391 100644 --- a/derive/src/codegen.rs +++ b/derive/src/codegen.rs @@ -1287,6 +1287,11 @@ fn post_binding(cli: &Cli) -> TokenStream { if f.required_if.is_empty() && f.required_unless.is_empty() { return None; } + // A field with a default is already filled, so no condition can make it + // missing. Plain required-ness skips these too, and so does usage-lib. + if f.default.is_some() { + return None; + } let given = format_ident!("__given_{}", f.ident); let name = &f.name; let selector_given = |selector: &String| { diff --git a/derive/src/lib.rs b/derive/src/lib.rs index c81b74ce1..9423dd01d 100644 --- a/derive/src/lib.rs +++ b/derive/src/lib.rs @@ -142,11 +142,16 @@ //! | `required_if = "--other"` | a flag whose presence makes this one necessary | //! | `required_unless = "--other"` | a flag whose presence makes this one unnecessary | //! -//! The last three name a flag the way the spec does — `"--long"` or `"-s"` — and take -//! several as a list: `conflicts("--file", "--url")`. A selector naming no flag on the -//! command 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. +//! These name a flag the way the spec does — `"--long"` or `"-s"` — and take several +//! as a list: `conflicts("--file", "--url")`. A selector naming no flag on the command +//! 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. +//! +//! 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 +//! something else. `required_unless` also needs somewhere to put "absent", so it takes +//! an `Option` rather than a bare `String`. //! //! # What this version does not do //! diff --git a/derive/src/model.rs b/derive/src/model.rs index a197c9692..3717df18b 100644 --- a/derive/src/model.rs +++ b/derive/src/model.rs @@ -719,6 +719,35 @@ impl Field { "`negate` names a second long form, so the field needs a `long`", )); } + // Relationships hold between flags. The spec records them on a flag and has no + // place for them on an argument, so accepting one here would enforce something + // the emitted spec cannot say — and docs and completions would describe a + // different CLI from the one that runs. + for (option, selectors) in [ + ("conflicts", &conflicts), + ("required_if", &required_if), + ("required_unless", &required_unless), + ] { + if !selectors.is_empty() && !is_flag { + return Err(syn::Error::new( + span, + format!( + "`{option}` describes a relationship between flags, so the field \ + needs a `long` or a `short`" + ), + )); + } + } + // `required_unless` says the field may be absent when another flag stands in for + // it. A bare `String` has nowhere to put absent, so its type would keep claiming + // the value is mandatory and the exception could never take effect. + if !required_unless.is_empty() && shape == Shape::Required { + return Err(syn::Error::new( + span, + "`required_unless` says this may be left out, so the field needs \ + somewhere to put \"absent\": make it an `Option`", + )); + } // A `Vec` flag collects, so it is repeatable whether or not it says so — // unless it is `variadic`, which is the other way of collecting. Emitting @@ -906,16 +935,29 @@ fn string_value(meta: &Meta) -> syn::Result { /// Which flag each one names is resolved in [`Cli::check`], where every field is in /// view. fn selectors(meta: &Meta) -> syn::Result> { - match meta { - Meta::List(list) => Ok(list - .parse_args_with( - syn::punctuated::Punctuated::::parse_terminated, - )? - .into_iter() - .map(|lit| lit.value()) - .collect()), - _ => Ok(vec![string_value(meta)?]), + let Meta::List(list) = meta else { + return Ok(vec![string_value(meta)?]); + }; + let found: Vec = list + .parse_args_with( + syn::punctuated::Punctuated::::parse_terminated, + )? + .into_iter() + .map(|lit| lit.value()) + .collect(); + // An empty list compiles into no relationship at all, which is a declaration that + // reads as though it does something. + if found.is_empty() { + return Err(syn::Error::new_spanned( + meta.path(), + format!( + "`{}` needs at least one flag, as in `{}(\"--other\")`", + ident_of(meta.path()), + ident_of(meta.path()) + ), + )); } + Ok(found) } fn int_value(meta: &Meta) -> syn::Result { @@ -1230,6 +1272,62 @@ mod tests { assert!(err.contains("names no flag"), "unhelpful message: {err}"); } + #[test] + fn a_relationship_needs_a_flag_to_hold_between() { + // The spec records these on a flag and has nowhere to put them on an argument, + // so enforcing one here would describe a CLI the emitted spec does not. + let err = rejection( + r#" + struct Ex { + #[usage(long)] + force: bool, + #[usage(conflicts = "--force")] + file: String, + } + "#, + ); + assert!( + err.contains("relationship between flags"), + "unhelpful message: {err}" + ); + } + + #[test] + fn required_unless_needs_somewhere_to_put_absent() { + // A bare `String` is always filled, so the exception could never take effect: + // the shape says mandatory and the attribute says conditional. + let err = rejection( + r#" + struct Ex { + #[usage(long)] + url: Option, + #[usage(long, required_unless = "--url")] + file: String, + } + "#, + ); + assert!( + err.contains("make it an `Option`"), + "unhelpful message: {err}" + ); + } + + #[test] + fn an_empty_relationship_list_is_a_compile_error() { + let err = rejection( + r#" + struct Ex { + #[usage(long, conflicts())] + file: Option, + } + "#, + ); + assert!( + err.contains("needs at least one flag"), + "unhelpful message: {err}" + ); + } + #[test] fn a_selector_naming_its_own_field_is_a_compile_error() { let err = rejection(