From 7f8b07eda4769f4431a6a657f7cbac5f72479f50 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 17 Aug 2026 03:04:19 +0000 Subject: [PATCH 1/2] fix(argv): reject duplicate flags --- argv/src/diagnostic.rs | 15 +++++++++++ argv/src/lib.rs | 5 ++++ conformance/tests/post_binding.rs | 22 ++++++++++++++++ derive/src/codegen.rs | 42 ++++++++++++++++++++++++++++++- 4 files changed, 83 insertions(+), 1 deletion(-) diff --git a/argv/src/diagnostic.rs b/argv/src/diagnostic.rs index f3e99ea01..8fb024c1a 100644 --- a/argv/src/diagnostic.rs +++ b/argv/src/diagnostic.rs @@ -498,6 +498,15 @@ pub fn render( ); let _ = writeln!(out, " {}", style.valid(&shown(here, name))); } + Error::DuplicateFlag { name } => { + with_usage = true; + let _ = writeln!( + out, + "{} the argument '{}' cannot be used multiple times", + style.error("error:"), + style.invalid(&shown(here, name)) + ); + } Error::MissingSubcommand => { with_usage = true; let _ = writeln!( @@ -906,6 +915,12 @@ mod tests { let message = rendered(&["use"], Error::MissingRequired { name: "jobs" }); assert!(message.contains(" --jobs"), "{message}"); + let message = rendered(&["use"], Error::DuplicateFlag { name: "jobs" }); + assert!( + message.contains("the argument '--jobs' cannot be used multiple times"), + "{message}" + ); + // Every variant, not most of them. These two printed the spec's name while the ones // directly above and below them did not, so one argument could appear two ways in two // messages from the same command — and clap writes the dashes here too: diff --git a/argv/src/lib.rs b/argv/src/lib.rs index 9d5eab1d2..5bf1850d3 100644 --- a/argv/src/lib.rs +++ b/argv/src/lib.rs @@ -394,6 +394,11 @@ pub enum Error<'t, 'v> { /// The flag or argument's name, as the spec calls it. name: &'t str, }, + /// A flag that is not repeatable was given more than once. + DuplicateFlag { + /// The flag's name, as the spec calls it. + name: &'t str, + }, /// A value was given that is not among the declared choices. /// /// Carries the choices rather than the offending value: rendering the value means diff --git a/conformance/tests/post_binding.rs b/conformance/tests/post_binding.rs index 7c0f0d96a..e34191376 100644 --- a/conformance/tests/post_binding.rs +++ b/conformance/tests/post_binding.rs @@ -505,6 +505,28 @@ struct Defaulted { plain: Option>, } +#[test] +fn a_plain_flag_cannot_be_given_twice() { + let a = argv(["--jobs", "2", "--jobs", "3"]); + assert!(matches!( + Defaulted::parse_from(&a), + Err(usage_argv::Error::DuplicateFlag { name: "jobs" }) + )); + + let a = argv(["--all-events", "--all-events"]); + assert!(matches!( + Defaulted::parse_from(&a), + Err(usage_argv::Error::DuplicateFlag { name: "all-events" }) + )); +} + +#[test] +fn a_repeatable_flag_still_accepts_several_occurrences() { + let a = argv(["--fs-events", "access", "--fs-events", "remove"]); + let parsed = Defaulted::parse_from(&a).expect("var permits another occurrence"); + assert_eq!(parsed.fs_events, ["access", "remove"]); +} + #[test] fn a_collecting_flag_starts_out_holding_its_defaults() { // Absent: all of them, in the order written. A `Vec` is the one shape that can hold diff --git a/derive/src/codegen.rs b/derive/src/codegen.rs index e649cb108..9ccead5d1 100644 --- a/derive/src/codegen.rs +++ b/derive/src/codegen.rs @@ -1138,6 +1138,14 @@ fn flag_arm(cli: &Cli, i: usize, field: &Field) -> TokenStream { let overridden = format_ident!("__overridden_{}", ident); quote!(partial.#overridden = false;) }); + let duplicate = rejects_duplicate(field).then(|| { + let duplicated = format_ident!("__duplicated_{}", ident); + quote! { + if partial.#given { + partial.#duplicated = true; + } + } + }); let body = match field.shape { // `negated` is what distinguishes `--color` from `--no-color`. Shape::Bool => quote!(partial.#ident = !negated;), @@ -1158,6 +1166,7 @@ fn flag_arm(cli: &Cli, i: usize, field: &Field) -> TokenStream { // without this, one command's flag would fill another's field. `static` items // have distinct addresses, so this is exact. #key if ::core::ptr::eq(*flag, &#table) => { + #duplicate #body partial.#given = true; // Given again after having lost: it is standing once more, which matters @@ -1172,6 +1181,16 @@ fn flag_arm(cli: &Cli, i: usize, field: &Field) -> TokenStream { } } +/// Whether another occurrence is a command-line mistake rather than another value. +/// +/// Counts and collections repeat by definition, and `var` explicitly opts a value-taking flag +/// into repetition. Every other flag matches clap's default of one occurrence. +fn rejects_duplicate(field: &Field) -> bool { + matches!(field.kind, Kind::Flag { .. }) + && !matches!(field.shape, Shape::Count | Shape::Many) + && !field.repeatable +} + /// Undoing the flags a flag displaces, as statements to run once it has been bound. /// /// Both directions. `overrides` is declared on one flag and holds between the two: @@ -1297,7 +1316,11 @@ fn partial_struct(cli: &Cli) -> TokenStream { let overridden = format_ident!("__overridden_{}", ident); quote!(pub #overridden: bool,) }); - Some(quote!(pub #ident: #ty, pub #given: bool, #overridden)) + let duplicated = rejects_duplicate(f).then(|| { + let duplicated = format_ident!("__duplicated_{}", ident); + quote!(pub #duplicated: bool,) + }); + Some(quote!(pub #ident: #ty, pub #given: bool, #overridden #duplicated)) }); // No derived `Default`: `start` is what produces a fresh partial, because a @@ -1617,10 +1640,15 @@ fn partial_defaults(cli: &Cli) -> TokenStream { let overridden = format_ident!("__overridden_{}", ident); quote!(#overridden: false,) }); + let duplicated = rejects_duplicate(f).then(|| { + let duplicated = format_ident!("__duplicated_{}", ident); + quote!(#duplicated: false,) + }); Some(quote! { #ident: ::std::default::Default::default(), #given: false, #overridden + #duplicated }) }); // Only the fields that declare one: `Partial`'s own initializer has already put @@ -2668,6 +2696,17 @@ fn post_binding(cli: &Cli) -> TokenStream { <#ty as ::usage_argv::spec::CommandArgs>::check(&mut partial.#ident)?; }) }); + let duplicate_checks = cli.fields.iter().filter(|f| rejects_duplicate(f)).map(|f| { + let duplicated = format_ident!("__duplicated_{}", f.ident); + let name = &f.name; + quote! { + if partial.#duplicated { + return ::std::result::Result::Err( + ::usage_argv::Error::DuplicateFlag { name: #name }, + ); + } + } + }); // Applied here rather than in `start`, and this is not a detail: `start` builds the // partial for *every* command in the CLI, selected or not, so a declared default was // costing a `String` per default per command — 60 allocations to parse a bare `mise`, @@ -2998,6 +3037,7 @@ fn post_binding(cli: &Cli) -> TokenStream { // the order `start` used to give them. #(#declared_defaults)* #(#env_fallbacks)* + #(#duplicate_checks)* // 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. From 407362e095c3be0a02725b8a38c040ef06da118f Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 17 Aug 2026 03:58:37 +0000 Subject: [PATCH 2/2] fix(derive): reset duplicate state for overrides --- conformance/tests/post_binding.rs | 33 +++++++++++++++++++++- derive/src/codegen.rs | 46 ++++++++++++++++++++++++++++--- 2 files changed, 74 insertions(+), 5 deletions(-) diff --git a/conformance/tests/post_binding.rs b/conformance/tests/post_binding.rs index e34191376..cc1e3a111 100644 --- a/conformance/tests/post_binding.rs +++ b/conformance/tests/post_binding.rs @@ -373,7 +373,7 @@ struct Ovr { #[usage(long)] url: Option, /// Colorize output, unless told otherwise - #[usage(long, default = "true", overrides = "--plain")] + #[usage(long, negate = "--no-color", default = "true", overrides = "--plain")] color: bool, /// No decoration at all #[usage(long)] @@ -401,6 +401,37 @@ fn the_last_of_two_overriding_flags_wins() { assert!(!ovr.stdin, "displaced by the flag that came after it"); } +#[test] +fn a_later_override_clears_an_earlier_duplicate() { + let a = argv(["--file", "a", "--file", "b", "--stdin"]); + let ovr = Ovr::parse_from(&a).expect("the final overriding flag should win"); + assert!(ovr.stdin); + assert_eq!(ovr.file, None); +} + +#[test] +fn positive_and_negative_spellings_override_instead_of_duplicate() { + let a = argv(["--color", "--no-color"]); + assert!( + !Ovr::parse_from(&a) + .expect("the negative form should win") + .color + ); + + let a = argv(["--no-color", "--color"]); + assert!( + Ovr::parse_from(&a) + .expect("the positive form should win") + .color + ); + + let a = argv(["--no-color", "--no-color"]); + assert!(matches!( + Ovr::parse_from(&a), + Err(usage_argv::Error::DuplicateFlag { name: "color" }) + )); +} + #[test] fn a_displaced_flag_goes_back_to_its_default_rather_than_to_nothing() { // `--color` defaults to on. `--plain` displaces it, and what it displaces it to diff --git a/derive/src/codegen.rs b/derive/src/codegen.rs index 9ccead5d1..d72f07511 100644 --- a/derive/src/codegen.rs +++ b/derive/src/codegen.rs @@ -1140,9 +1140,22 @@ fn flag_arm(cli: &Cli, i: usize, field: &Field) -> TokenStream { }); let duplicate = rejects_duplicate(field).then(|| { let duplicated = format_ident!("__duplicated_{}", ident); - quote! { - if partial.#given { - partial.#duplicated = true; + if has_negate(field) { + let negated = format_ident!("__negated_{}", ident); + quote! { + if partial.#given { + // The positive and negative spellings override one another: the + // last of `--color --no-color` wins just like an explicit + // `overrides` pair. Repeating the same spelling is still an error. + partial.#duplicated = partial.#negated == negated; + } + partial.#negated = negated; + } + } else { + quote! { + if partial.#given { + partial.#duplicated = true; + } } } }); @@ -1191,6 +1204,17 @@ fn rejects_duplicate(field: &Field) -> bool { && !field.repeatable } +/// Whether a boolean flag has a negative spelling that overrides its positive one. +fn has_negate(field: &Field) -> bool { + matches!( + &field.kind, + Kind::Flag { + negate: Some(_), + .. + } + ) +} + /// Undoing the flags a flag displaces, as statements to run once it has been bound. /// /// Both directions. `overrides` is declared on one flag and holds between the two: @@ -1203,9 +1227,14 @@ fn displacements(cli: &Cli, field: &Field) -> Vec { let reset = reset_to_default(other); let given = format_ident!("__given_{}", other.ident); let overridden = format_ident!("__overridden_{}", other.ident); + let duplicated = rejects_duplicate(other).then(|| { + let duplicated = format_ident!("__duplicated_{}", other.ident); + quote!(partial.#duplicated = false;) + }); quote! { #reset partial.#given = false; + #duplicated // Remembered, not just cleared: without this the environment fallback // would refill the flag that lost and mark it given again, and a // displaced `String` would be reported missing. usage-lib keeps the @@ -1320,7 +1349,11 @@ fn partial_struct(cli: &Cli) -> TokenStream { let duplicated = format_ident!("__duplicated_{}", ident); quote!(pub #duplicated: bool,) }); - Some(quote!(pub #ident: #ty, pub #given: bool, #overridden #duplicated)) + let negated = has_negate(f).then(|| { + let negated = format_ident!("__negated_{}", ident); + quote!(pub #negated: bool,) + }); + Some(quote!(pub #ident: #ty, pub #given: bool, #overridden #duplicated #negated)) }); // No derived `Default`: `start` is what produces a fresh partial, because a @@ -1644,11 +1677,16 @@ fn partial_defaults(cli: &Cli) -> TokenStream { let duplicated = format_ident!("__duplicated_{}", ident); quote!(#duplicated: false,) }); + let negated = has_negate(f).then(|| { + let negated = format_ident!("__negated_{}", ident); + quote!(#negated: false,) + }); Some(quote! { #ident: ::std::default::Default::default(), #given: false, #overridden #duplicated + #negated }) }); // Only the fields that declare one: `Partial`'s own initializer has already put