fix(argv): reject duplicate flags

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:

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

conformance/tests/post_binding.rs

@@ -373,7 +373,7 @@ struct Ovr {
     #[usage(long)]
     url: Option<String>,
     /// 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
@@ -505,6 +536,28 @@ struct Defaulted {
     plain: Option<Vec<String>>,
 }
 
+#[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

derive/src/codegen.rs

@@ -1138,6 +1138,27 @@ 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);
+        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;
+                }
+            }
+        }
+    });
     let body = match field.shape {
         // `negated` is what distinguishes `--color` from `--no-color`.
         Shape::Bool => quote!(partial.#ident = !negated;),
@@ -1158,6 +1179,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 +1194,27 @@ 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
+}
+
+/// 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:
@@ -1184,9 +1227,14 @@ fn displacements(cli: &Cli, field: &Field) -> Vec<TokenStream> {
             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
@@ -1297,7 +1345,15 @@ 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,)
+        });
+        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
@@ -1617,10 +1673,20 @@ 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,)
+        });
+        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
@@ -2668,6 +2734,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 +3075,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.