Repository: google/argh
Pull request #127 (2022-05-31 21:47:22)

allow alphanumeric strings in `long` attribute
Argh already allows usage of field names such as `ac97` but not if
specified in `long` attribute


Patch:
diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml
index 02b22a7..87f3c6c 100644
--- a/.github/workflows/rust.yml
+++ b/.github/workflows/rust.yml
@@ -4,16 +4,44 @@ on: [push, pull_request]
 
 jobs:
   build:
-
     runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v2
 
+    - name: Build
+      run: cargo build --workspace --examples --verbose
+
+    - name: Run tests
+      run: cargo test --workspace --verbose
+
+    - name: Run simple_example
+      run: cargo run --package argh --example simple_example two --fooey
+
+  rustfmt:
+    name: rustfmt
+    runs-on: ubuntu-latest
     steps:
     - uses: actions/checkout@v2
+
     - name: Run Rustfmt
       run: cargo fmt -- --check
-    - name: Build
-      run: cargo build --verbose
-    - name: Run tests
-      run: cargo test --verbose
+
+  clippy:
+    name: clippy
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v2
+
     - name: Run clippy
-      run: cargo clippy -- -D warnings
+      run: cargo clippy --version && cargo clippy --tests --workspace -- -D warnings
+
+  docs:
+    name: docs
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v1
+
+    - name: Check documentation
+      env:
+        RUSTDOCFLAGS: -D warnings
+      run: cargo doc --no-deps --document-private-items --workspace
diff --git a/README.md b/README.md
index 4e949e4..7e0089b 100644
--- a/README.md
+++ b/README.md
@@ -175,3 +175,16 @@ struct SubCommandTwo {
 ```
 
 NOTE: This is not an officially supported Google product.
+
+
+## How to debug the expanded derive marco for `argh`
+
+The `argh::FromArgs` derive marco can be debugged with the [cargo-expand](https://crates.io/crates/cargo-expand) crate.
+
+### Expand the derive marco in `examples/simple_example.rs`
+
+See [argh/examples/simple_example.rs](./argh/examples/simple_example.rs) for the example struct we wish to expand.
+
+First, install `cargo-expand` by running `cargo install cargo-expand`. Note this requires the nightly build of Rust.
+
+Once installed, run `cargo expand` with in the `argh` package and you can see the expanded code.
diff --git a/argh/Cargo.toml b/argh/Cargo.toml
index 2715895..39117bf 100644
--- a/argh/Cargo.toml
+++ b/argh/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "argh"
-version = "0.1.7"
+version = "0.1.8"
 authors = ["Taylor Cramer <cramertj@google.com>", "Benjamin Brittain <bwb@google.com>", "Erick Tryzelaar <etryzelaar@google.com>"]
 edition = "2018"
 keywords = ["args", "arguments", "derive", "cli"]
@@ -10,8 +10,9 @@ repository = "https://github.com/google/argh"
 readme = "README.md"
 
 [dependencies]
-argh_shared = { version = "0.1.7", path = "../argh_shared" }
-argh_derive = { version = "0.1.7", path = "../argh_derive" }
+argh_shared = { version = "0.1.8", path = "../argh_shared" }
+argh_derive = { version = "0.1.8", path = "../argh_derive" }
 
 [dev-dependencies]
 once_cell = "1.10.0"
+trybuild = "1.0.63"
diff --git a/argh/examples/simple_example.rs b/argh/examples/simple_example.rs
new file mode 100644
index 0000000..d977495
--- /dev/null
+++ b/argh/examples/simple_example.rs
@@ -0,0 +1,42 @@
+// Copyright (c) 2022 Google LLC All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+use {argh::FromArgs, std::fmt::Debug};
+
+#[derive(FromArgs, PartialEq, Debug)]
+/// Top-level command.
+struct TopLevel {
+    #[argh(subcommand)]
+    nested: MySubCommandEnum,
+}
+
+#[derive(FromArgs, PartialEq, Debug)]
+#[argh(subcommand)]
+enum MySubCommandEnum {
+    One(SubCommandOne),
+    Two(SubCommandTwo),
+}
+
+#[derive(FromArgs, PartialEq, Debug)]
+/// First subcommand.
+#[argh(subcommand, name = "one")]
+struct SubCommandOne {
+    #[argh(option)]
+    /// how many x
+    x: usize,
+}
+
+#[derive(FromArgs, PartialEq, Debug)]
+/// Second subcommand.
+#[argh(subcommand, name = "two")]
+struct SubCommandTwo {
+    #[argh(switch)]
+    /// whether to fooey
+    fooey: bool,
+}
+
+fn main() {
+    let toplevel: TopLevel = argh::from_env();
+    println!("{:#?}", toplevel);
+}
diff --git a/argh/src/lib.rs b/argh/src/lib.rs
index 5afc162..49ab386 100644
--- a/argh/src/lib.rs
+++ b/argh/src/lib.rs
@@ -166,6 +166,102 @@
 //!     fooey: bool,
 //! }
 //! ```
+//!
+//! You can also discover subcommands dynamically at runtime. To do this,
+//! declare subcommands as usual and add a variant to the enum with the
+//! `dynamic` attribute. Instead of deriving `FromArgs`, the value inside the
+//! dynamic variant should implement `DynamicSubCommand`.
+//!
+//! ```rust
+//! # use argh::CommandInfo;
+//! # use argh::DynamicSubCommand;
+//! # use argh::EarlyExit;
+//! # use argh::FromArgs;
+//! # use once_cell::sync::OnceCell;
+//!
+//! #[derive(FromArgs, PartialEq, Debug)]
+//! /// Top-level command.
+//! struct TopLevel {
+//!     #[argh(subcommand)]
+//!     nested: MySubCommandEnum,
+//! }
+//!
+//! #[derive(FromArgs, PartialEq, Debug)]
+//! #[argh(subcommand)]
+//! enum MySubCommandEnum {
+//!     Normal(NormalSubCommand),
+//!     #[argh(dynamic)]
+//!     Dynamic(Dynamic),
+//! }
+//!
+//! #[derive(FromArgs, PartialEq, Debug)]
+//! /// Normal subcommand.
+//! #[argh(subcommand, name = "normal")]
+//! struct NormalSubCommand {
+//!     #[argh(option)]
+//!     /// how many x
+//!     x: usize,
+//! }
+//!
+//! /// Dynamic subcommand.
+//! #[derive(PartialEq, Debug)]
+//! struct Dynamic {
+//!     name: String
+//! }
+//!
+//! impl DynamicSubCommand for Dynamic {
+//!     fn commands() -> &'static [&'static CommandInfo] {
+//!         static RET: OnceCell<Vec<&'static CommandInfo>> = OnceCell::new();
+//!         RET.get_or_init(|| {
+//!             let mut commands = Vec::new();
+//!
+//!             // argh needs the `CommandInfo` structs we generate to be valid
+//!             // for the static lifetime. We can allocate the structures on
+//!             // the heap with `Box::new` and use `Box::leak` to get a static
+//!             // reference to them. We could also just use a constant
+//!             // reference, but only because this is a synthetic example; the
+//!             // point of using dynamic commands is to have commands you
+//!             // don't know about until runtime!
+//!             commands.push(&*Box::leak(Box::new(CommandInfo {
+//!                 name: "dynamic_command",
+//!                 description: "A dynamic command",
+//!             })));
+//!
+//!             commands
+//!         })
+//!     }
+//!
+//!     fn try_redact_arg_values(
+//!         command_name: &[&str],
+//!         args: &[&str],
+//!     ) -> Option<Result<Vec<String>, EarlyExit>> {
+//!         for command in Self::commands() {
+//!             if command_name.last() == Some(&command.name) {
+//!                 // Process arguments and redact values here.
+//!                 if !args.is_empty() {
+//!                     return Some(Err("Our example dynamic command never takes arguments!"
+//!                                     .to_string().into()));
+//!                 }
+//!                 return Some(Ok(Vec::new()))
+//!             }
+//!         }
+//!         None
+//!     }
+//!
+//!     fn try_from_args(command_name: &[&str], args: &[&str]) -> Option<Result<Self, EarlyExit>> {
+//!         for command in Self::commands() {
+//!             if command_name.last() == Some(&command.name) {
+//!                 if !args.is_empty() {
+//!                     return Some(Err("Our example dynamic command never takes arguments!"
+//!                                     .to_string().into()));
+//!                 }
+//!                 return Some(Ok(Dynamic { name: command.name.to_string() }))
+//!             }
+//!         }
+//!         None
+//!     }
+//! }
+//! ```
 
 #![deny(missing_docs)]
 
@@ -465,15 +561,26 @@ pub trait DynamicSubCommand: Sized {
     /// Info about supported subcommands.
     fn commands() -> &'static [&'static CommandInfo];
 
-    /// Perform the function of `FromArgs::redact_arg_values` for this dynamic command. If the
-    /// command being processed is not recognized return `None`.
+    /// Perform the function of `FromArgs::redact_arg_values` for this dynamic
+    /// command.
+    ///
+    /// The full list of subcommands, ending with the subcommand that should be
+    /// dynamically recognized, is passed in `command_name`. If the command
+    /// passed is not recognized, this function should return `None`. Otherwise
+    /// it should return `Some`, and the value within the `Some` has the same
+    /// semantics as the return of `FromArgs::redact_arg_values`.
     fn try_redact_arg_values(
         command_name: &[&str],
         args: &[&str],
     ) -> Option<Result<Vec<String>, EarlyExit>>;
 
-    /// Perform the function of `FromArgs::from_args` for this dynamic command. If the command being
-    /// processed is not recognized return `None`.
+    /// Perform the function of `FromArgs::from_args` for this dynamic command.
+    ///
+    /// The full list of subcommands, ending with the subcommand that should be
+    /// dynamically recognized, is passed in `command_name`. If the command
+    /// passed is not recognized, this function should return `None`. Otherwise
+    /// it should return `Some`, and the value within the `Some` has the same
+    /// semantics as the return of `FromArgs::from_args`.
     fn try_from_args(command_name: &[&str], args: &[&str]) -> Option<Result<Self, EarlyExit>>;
 }
 
diff --git a/argh_derive/Cargo.toml b/argh_derive/Cargo.toml
index b74329d..0c9eb8e 100644
--- a/argh_derive/Cargo.toml
+++ b/argh_derive/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "argh_derive"
-version = "0.1.7"
+version = "0.1.8"
 authors = ["Taylor Cramer <cramertj@google.com>", "Benjamin Brittain <bwb@google.com>", "Erick Tryzelaar <etryzelaar@google.com>"]
 edition = "2018"
 license = "BSD-3-Clause"
@@ -16,4 +16,4 @@ heck = "0.3.1"
 proc-macro2 = "1.0"
 quote = "1.0"
 syn = "1.0"
-argh_shared = { version = "0.1.7", path = "../argh_shared" }
+argh_shared = { version = "0.1.8", path = "../argh_shared" }
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/help.rs b/argh_derive/src/help.rs
index 917c781..276b706 100644
--- a/argh_derive/src/help.rs
+++ b/argh_derive/src/help.rs
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
+use std::fmt::Write;
 use {
     crate::{
         errors::Errors,
@@ -26,6 +27,7 @@ pub(crate) fn help(
     fields: &[StructField<'_>],
     subcommand: Option<&StructField<'_>>,
 ) -> TokenStream {
+    #![allow(clippy::format_push_string)]
     let mut format_lit = "Usage: {command_name}".to_string();
 
     let positional = fields.iter().filter(|f| f.kind == FieldKind::Positional);
@@ -108,7 +110,7 @@ pub(crate) fn help(
         for (code, text) in &ty_attrs.error_codes {
             format_lit.push('\n');
             format_lit.push_str(INDENT);
-            format_lit.push_str(&format!("{} {}", code, text.value()));
+            write!(format_lit, "{} {}", code, text.value()).unwrap();
         }
     }
 
@@ -225,7 +227,7 @@ fn positional_description(out: &mut String, field: &StructField<'_>) {
 }
 
 fn positional_description_format(out: &mut String, name: &str, description: &str) {
-    let info = argh_shared::CommandInfo { name: &*name, description };
+    let info = argh_shared::CommandInfo { name, description };
     argh_shared::write_description(out, &info);
 }
 
@@ -255,6 +257,6 @@ fn option_description_format(
     }
     name.push_str(long_with_leading_dashes);
 
-    let info = argh_shared::CommandInfo { name: &*name, description };
+    let info = argh_shared::CommandInfo { name: &name, description };
     argh_shared::write_description(out, &info);
 }
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 =
diff --git a/argh_derive/src/parse_attrs.rs b/argh_derive/src/parse_attrs.rs
index f229862..212b184 100644
--- a/argh_derive/src/parse_attrs.rs
+++ b/argh_derive/src/parse_attrs.rs
@@ -170,7 +170,7 @@ impl FieldAttrs {
         if !value.is_ascii() {
             errors.err(long, "Long names must be ASCII");
         }
-        if !value.chars().all(|c| c.is_lowercase() || c == '-') {
+        if !value.chars().all(|c| c.is_lowercase() || c == '-' || c.is_ascii_digit()) {
             errors.err(long, "Long names must be lowercase");
         }
     }
diff --git a/argh_shared/Cargo.toml b/argh_shared/Cargo.toml
index 1f7e694..ca7e61c 100644
--- a/argh_shared/Cargo.toml
+++ b/argh_shared/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "argh_shared"
-version = "0.1.7"
+version = "0.1.8"
 authors = ["Taylor Cramer <cramertj@google.com>", "Benjamin Brittain <bwb@google.com>", "Erick Tryzelaar <etryzelaar@google.com>"]
 edition = "2018"
 license = "BSD-3-Clause"


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/lib.rs b/argh/tests/lib.rs
index c4e58a9..9005a09 100644
--- a/argh/tests/lib.rs
+++ b/argh/tests/lib.rs
@@ -1519,3 +1519,17 @@ fn subcommand_does_not_panic() {
         argh::EarlyExit { output: "no subcommand name".into(), status: Err(()) },
     );
 }
+
+#[test]
+fn long_alphanumeric() {
+    #[derive(FromArgs)]
+    /// Short description
+    struct Cmd {
+        #[argh(option, long = "ac97")]
+        /// fooey
+        ac97: String,
+    }
+
+    let cmd = Cmd::from_args(&["cmdname"], &["--ac97", "bar"]).unwrap();
+    assert_eq!(cmd.ac97, "bar");
+}
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,
+   | |_____________^