Repository: google/argh (language: rust)

Issue:
prevent duplicate long names
Fixes #31 


Discussion / hints:
This looks good, but it needs a test.

Fix patch:
diff --git a/argh/Cargo.toml b/argh/Cargo.toml
index b730843..39117bf 100644
--- a/argh/Cargo.toml
+++ b/argh/Cargo.toml
@@ -15,3 +15,4 @@ argh_derive = { version = "0.1.8", path = "../argh_derive" }
 
 [dev-dependencies]
 once_cell = "1.10.0"
+trybuild = "1.0.63"
diff --git a/argh_derive/src/errors.rs b/argh_derive/src/errors.rs
index a5b69e6..cfad383 100644
--- a/argh_derive/src/errors.rs
+++ b/argh_derive/src/errors.rs
@@ -146,6 +146,11 @@ impl Errors {
         self.push(syn::Error::new(span, msg));
     }
 
+    /// Issue an error spanning over the given syntax tree node.
+    pub fn err_span_tokens<T: ToTokens>(&self, tokens: T, msg: &str) {
+        self.push(syn::Error::new_spanned(tokens, msg));
+    }
+
     /// Push a `syn::Error` onto the list of errors to issue.
     pub fn push(&self, err: syn::Error) {
         self.errors.borrow_mut().push(err);
diff --git a/argh_derive/src/lib.rs b/argh_derive/src/lib.rs
index c35a269..2e5f1ec 100644
--- a/argh_derive/src/lib.rs
+++ b/argh_derive/src/lib.rs
@@ -15,7 +15,7 @@ use {
     },
     proc_macro2::{Span, TokenStream},
     quote::{quote, quote_spanned, ToTokens},
-    std::str::FromStr,
+    std::{collections::HashMap, str::FromStr},
     syn::{spanned::Spanned, LitStr},
 };
 
@@ -233,6 +233,7 @@ fn impl_from_args_struct(
         })
         .collect();
 
+    ensure_unique_names(errors, &fields);
     ensure_only_last_positional_is_optional(errors, &fields);
 
     let impl_span = Span::call_site();
@@ -505,6 +506,39 @@ fn ensure_only_last_positional_is_optional(errors: &Errors, fields: &[StructFiel
     }
 }
 
+/// Ensures that only one short or long name is used.
+fn ensure_unique_names(errors: &Errors, fields: &[StructField<'_>]) {
+    let mut seen_short_names = HashMap::new();
+    let mut seen_long_names = HashMap::new();
+
+    for field in fields {
+        if let Some(short_name) = &field.attrs.short {
+            let short_name = short_name.value();
+            if let Some(first_use_field) = seen_short_names.get(&short_name) {
+                errors.err_span_tokens(
+                    first_use_field,
+                    &format!("The short name of \"-{}\" was already used here.", short_name),
+                );
+                errors.err_span_tokens(&field.field, "Later usage here.");
+            }
+
+            seen_short_names.insert(short_name, &field.field);
+        }
+
+        if let Some(long_name) = &field.long_name {
+            if let Some(first_use_field) = seen_long_names.get(&long_name) {
+                errors.err_span_tokens(
+                    *first_use_field,
+                    &format!("The long name of \"{}\" was already used here.", long_name),
+                );
+                errors.err_span_tokens(&field.field, "Later usage here.");
+            }
+
+            seen_long_names.insert(long_name, field.field);
+        }
+    }
+}
+
 /// Implement `argh::TopLevelCommand` or `argh::SubCommand` as appropriate.
 fn top_or_sub_cmd_impl(errors: &Errors, name: &syn::Ident, type_attrs: &TypeAttrs) -> TokenStream {
     let description =


Test patch:
diff --git a/argh/tests/compiletest.rs b/argh/tests/compiletest.rs
new file mode 100644
index 0000000..bd72ecd
--- /dev/null
+++ b/argh/tests/compiletest.rs
@@ -0,0 +1,5 @@
+#[test]
+fn ui() {
+    let t = trybuild::TestCases::new();
+    t.compile_fail("tests/ui/**/*.rs");
+}
diff --git a/argh/tests/ui/duplicate-name/duplicate-long-name.rs b/argh/tests/ui/duplicate-name/duplicate-long-name.rs
new file mode 100644
index 0000000..550a7aa
--- /dev/null
+++ b/argh/tests/ui/duplicate-name/duplicate-long-name.rs
@@ -0,0 +1,21 @@
+/// Command
+#[derive(argh::FromArgs)]
+struct Cmd {
+    /// foo1
+    #[argh(option, long = "foo")]
+    foo1: u32,
+
+    /// foo2
+    #[argh(option, long = "foo")]
+    foo2: u32,
+
+    /// bar1
+    #[argh(option, long = "bar")]
+    bar1: u32,
+
+    /// bar2
+    #[argh(option, long = "bar")]
+    bar2: u32,
+}
+
+fn main() {}
diff --git a/argh/tests/ui/duplicate-name/duplicate-long-name.stderr b/argh/tests/ui/duplicate-name/duplicate-long-name.stderr
new file mode 100644
index 0000000..697f36b
--- /dev/null
+++ b/argh/tests/ui/duplicate-name/duplicate-long-name.stderr
@@ -0,0 +1,31 @@
+error: The long name of "--foo" was already used here.
+ --> tests/ui/duplicate-name/duplicate-long-name.rs:4:5
+  |
+4 | /     /// foo1
+5 | |     #[argh(option, long = "foo")]
+6 | |     foo1: u32,
+  | |_____________^
+
+error: Later usage here.
+  --> tests/ui/duplicate-name/duplicate-long-name.rs:8:5
+   |
+8  | /     /// foo2
+9  | |     #[argh(option, long = "foo")]
+10 | |     foo2: u32,
+   | |_____________^
+
+error: The long name of "--bar" was already used here.
+  --> tests/ui/duplicate-name/duplicate-long-name.rs:12:5
+   |
+12 | /     /// bar1
+13 | |     #[argh(option, long = "bar")]
+14 | |     bar1: u32,
+   | |_____________^
+
+error: Later usage here.
+  --> tests/ui/duplicate-name/duplicate-long-name.rs:16:5
+   |
+16 | /     /// bar2
+17 | |     #[argh(option, long = "bar")]
+18 | |     bar2: u32,
+   | |_____________^
diff --git a/argh/tests/ui/duplicate-name/duplicate-short-name.rs b/argh/tests/ui/duplicate-name/duplicate-short-name.rs
new file mode 100644
index 0000000..a52bddb
--- /dev/null
+++ b/argh/tests/ui/duplicate-name/duplicate-short-name.rs
@@ -0,0 +1,21 @@
+/// Command
+#[derive(argh::FromArgs)]
+struct Cmd {
+    /// foo1
+    #[argh(option, short = 'f')]
+    foo1: u32,
+
+    /// foo2
+    #[argh(option, short = 'f')]
+    foo2: u32,
+
+    /// bar1
+    #[argh(option, short = 'b')]
+    bar1: u32,
+
+    /// bar2
+    #[argh(option, short = 'b')]
+    bar2: u32,
+}
+
+fn main() {}
diff --git a/argh/tests/ui/duplicate-name/duplicate-short-name.stderr b/argh/tests/ui/duplicate-name/duplicate-short-name.stderr
new file mode 100644
index 0000000..9262a7d
--- /dev/null
+++ b/argh/tests/ui/duplicate-name/duplicate-short-name.stderr
@@ -0,0 +1,31 @@
+error: The short name of "-f" was already used here.
+ --> tests/ui/duplicate-name/duplicate-short-name.rs:4:5
+  |
+4 | /     /// foo1
+5 | |     #[argh(option, short = 'f')]
+6 | |     foo1: u32,
+  | |_____________^
+
+error: Later usage here.
+  --> tests/ui/duplicate-name/duplicate-short-name.rs:8:5
+   |
+8  | /     /// foo2
+9  | |     #[argh(option, short = 'f')]
+10 | |     foo2: u32,
+   | |_____________^
+
+error: The short name of "-b" was already used here.
+  --> tests/ui/duplicate-name/duplicate-short-name.rs:12:5
+   |
+12 | /     /// bar1
+13 | |     #[argh(option, short = 'b')]
+14 | |     bar1: u32,
+   | |_____________^
+
+error: Later usage here.
+  --> tests/ui/duplicate-name/duplicate-short-name.rs:16:5
+   |
+16 | /     /// bar2
+17 | |     #[argh(option, short = 'b')]
+18 | |     bar2: u32,
+   | |_____________^