Repository: google/argh (language: rust)

Issue:
redact_arg_values throws a parse error on Options of type Vec
In Fuchsia's ffx tool, there is a declaration of an Option type, --config that can be a Vec<String>.
from_args has no problem with invocations such as 

ffx --config x --config y

However, redact_arg_values throws a parse error in ParseSlotValue<Option> that a duplicate value, y, was provided. It seems to be mistaking the Option type for a variant that has one value.

This is preventing the majority of analytics use cases from working for ffx.


Discussion / hints:

Fix patch:
diff --git a/argh/Cargo.toml b/argh/Cargo.toml
index 80f71d3..ec92149 100644
--- a/argh/Cargo.toml
+++ b/argh/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "argh"
-version = "0.1.6"
+version = "0.1.7"
 authors = ["Taylor Cramer <cramertj@google.com>", "Benjamin Brittain <bwb@google.com>", "Erick Tryzelaar <etryzelaar@google.com>"]
 edition = "2018"
 keywords = ["args", "arguments", "derive", "cli"]
@@ -10,5 +10,5 @@ repository = "https://github.com/google/argh"
 readme = "README.md"
 
 [dependencies]
-argh_shared = { version = "0.1.6", path = "../argh_shared" }
-argh_derive = { version = "0.1.6", path = "../argh_derive" }
+argh_shared = { version = "0.1.7", path = "../argh_shared" }
+argh_derive = { version = "0.1.7", path = "../argh_derive" }
diff --git a/argh_derive/Cargo.toml b/argh_derive/Cargo.toml
index 39712d5..b74329d 100644
--- a/argh_derive/Cargo.toml
+++ b/argh_derive/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "argh_derive"
-version = "0.1.6"
+version = "0.1.7"
 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.5", path = "../argh_shared" }
+argh_shared = { version = "0.1.7", path = "../argh_shared" }
diff --git a/argh_derive/src/help.rs b/argh_derive/src/help.rs
index a8bbe49..5bf02b1 100644
--- a/argh_derive/src/help.rs
+++ b/argh_derive/src/help.rs
@@ -11,7 +11,6 @@ use {
     argh_shared::INDENT,
     proc_macro2::{Span, TokenStream},
     quote::quote,
-    syn::LitStr,
 };
 
 const SECTION_SEPARATOR: &str = "\n\n";
@@ -131,22 +130,13 @@ fn lits_section(out: &mut String, heading: &str, lits: &[syn::LitStr]) {
     }
 }
 
-fn get_positional_name(field: &StructField<'_>) -> String {
-    return field
-        .attrs
-        .arg_name
-        .as_ref()
-        .map(LitStr::value)
-        .unwrap_or_else(|| field.name.to_string());
-}
-
 /// Add positional arguments like `[<foo>...]` to a help format string.
 fn positional_usage(out: &mut String, field: &StructField<'_>) {
     if !field.optionality.is_required() {
         out.push('[');
     }
     out.push('<');
-    let name = get_positional_name(field);
+    let name = field.arg_name();
     out.push_str(&name);
     if field.optionality == Optionality::Repeating {
         out.push_str("...");
@@ -219,7 +209,7 @@ Add a doc comment or an `#[argh(description = \"...\")]` attribute.",
 /// Describes a positional argument like this:
 ///  hello       positional argument description
 fn positional_description(out: &mut String, field: &StructField<'_>) {
-    let field_name = get_positional_name(field);
+    let field_name = field.arg_name();
 
     let mut description = String::from("");
     if let Some(desc) = &field.attrs.description {
diff --git a/argh_derive/src/lib.rs b/argh_derive/src/lib.rs
index 221425f..a123d4e 100644
--- a/argh_derive/src/lib.rs
+++ b/argh_derive/src/lib.rs
@@ -16,7 +16,7 @@ use {
     proc_macro2::{Span, TokenStream},
     quote::{quote, quote_spanned, ToTokens},
     std::str::FromStr,
-    syn::spanned::Spanned,
+    syn::{spanned::Spanned, LitStr},
 };
 
 mod errors;
@@ -203,6 +203,10 @@ impl<'a> StructField<'a> {
 
         Some(StructField { field, attrs, kind, optionality, ty_without_wrapper, name, long_name })
     }
+
+    pub(crate) fn arg_name(&self) -> String {
+        self.attrs.arg_name.as_ref().map(LitStr::value).unwrap_or_else(|| self.name.to_string())
+    }
 }
 
 /// Implements `FromArgs` and `TopLevelCommand` or `SubCommand` for a `#[derive(FromArgs)]` struct.
@@ -622,8 +626,17 @@ fn declare_local_storage_for_redacted_fields<'a>(
                 }
             }
             FieldKind::Option => {
+                let field_slot_type = match field.optionality {
+                    Optionality::Repeating => {
+                        quote! { std::vec::Vec<String> }
+                    }
+                    Optionality::None | Optionality::Optional | Optionality::Defaulted(_) => {
+                        quote! { std::option::Option<String> }
+                    }
+                };
+
                 quote! {
-                    let mut #field_name: argh::ParseValueSlotTy::<Option<String>, String> =
+                    let mut #field_name: argh::ParseValueSlotTy::<#field_slot_type, String> =
                         argh::ParseValueSlotTy {
                         slot: std::default::Default::default(),
                         parse_func: |arg, _| { Ok(arg.to_string()) },
@@ -640,12 +653,12 @@ fn declare_local_storage_for_redacted_fields<'a>(
                     }
                 };
 
-                let long_name = field.name.to_string();
+                let arg_name = field.arg_name();
                 quote! {
                     let mut #field_name: argh::ParseValueSlotTy::<#field_slot_type, String> =
                         argh::ParseValueSlotTy {
                         slot: std::default::Default::default(),
-                        parse_func: |_, _| { Ok(#long_name.to_string()) },
+                        parse_func: |_, _| { Ok(#arg_name.to_string()) },
                     };
                 }
             }
@@ -664,13 +677,27 @@ fn unwrap_redacted_fields<'a>(
         let field_name = field.name;
 
         match field.kind {
-            FieldKind::Switch | FieldKind::Option => {
+            FieldKind::Switch => {
                 quote! {
                     if let Some(__field_name) = #field_name.slot {
                         __redacted.push(__field_name);
                     }
                 }
             }
+            FieldKind::Option => match field.optionality {
+                Optionality::Repeating => {
+                    quote! {
+                        __redacted.extend(#field_name.slot.into_iter());
+                    }
+                }
+                Optionality::None | Optionality::Optional | Optionality::Defaulted(_) => {
+                    quote! {
+                        if let Some(__field_name) = #field_name.slot {
+                            __redacted.push(__field_name);
+                        }
+                    }
+                }
+            },
             FieldKind::Positional => {
                 quote! {
                     __redacted.extend(#field_name.slot.into_iter());
@@ -718,7 +745,7 @@ fn append_missing_requirements<'a>(
         match field.kind {
             FieldKind::Switch => unreachable!("switches are always optional"),
             FieldKind::Positional => {
-                let name = field.name.to_string();
+                let name = field.arg_name();
                 quote! {
                     if #field_name.slot.is_none() {
                         #mri.missing_positional_arg(#name)
diff --git a/argh_shared/Cargo.toml b/argh_shared/Cargo.toml
index 2c24892..1f7e694 100644
--- a/argh_shared/Cargo.toml
+++ b/argh_shared/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "argh_shared"
-version = "0.1.6"
+version = "0.1.7"
 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/lib.rs b/argh/tests/lib.rs
index 5b1ac7a..fe8c858 100644
--- a/argh/tests/lib.rs
+++ b/argh/tests/lib.rs
@@ -176,7 +176,7 @@ fn missing_option_value() {
     struct Cmd {
         #[argh(option)]
         /// fooey
-        msg: String,
+        _msg: String,
     }
 
     let e = Cmd::from_args(&["cmdname"], &["--msg"])
@@ -539,10 +539,10 @@ mod fuchsia_commandline_tools_rubric {
     struct TwoSwitches {
         #[argh(switch, short = 'a')]
         /// a
-        a: bool,
+        _a: bool,
         #[argh(switch, short = 'b')]
         /// b
-        b: bool,
+        _b: bool,
     }
 
     /// Running switches together is not allowed
@@ -558,7 +558,7 @@ mod fuchsia_commandline_tools_rubric {
     struct OneOption {
         #[argh(option)]
         /// some description
-        foo: String,
+        _foo: String,
     }
 
     /// Do not use an equals punctuation or similar to separate the key and value.
@@ -666,7 +666,7 @@ mod fuchsia_commandline_tools_rubric {
     /// A type for testing `--help`/`help`
     struct HelpTopLevel {
         #[argh(subcommand)]
-        sub: HelpFirstSub,
+        _sub: HelpFirstSub,
     }
 
     #[derive(FromArgs, Debug)]
@@ -674,7 +674,7 @@ mod fuchsia_commandline_tools_rubric {
     /// First subcommmand for testing `help`.
     struct HelpFirstSub {
         #[argh(subcommand)]
-        sub: HelpSecondSub,
+        _sub: HelpSecondSub,
     }
 
     #[derive(FromArgs, Debug)]
@@ -912,7 +912,7 @@ fn redact_arg_values_no_args() {
     struct Cmd {
         #[argh(option)]
         /// a msg param
-        msg: Option<String>,
+        _msg: Option<String>,
     }
 
     let actual = Cmd::redact_arg_values(&["program-name"], &[]).unwrap();
@@ -926,13 +926,41 @@ fn redact_arg_values_optional_arg() {
     struct Cmd {
         #[argh(option)]
         /// a msg param
-        msg: Option<String>,
+        _msg: Option<String>,
     }
 
     let actual = Cmd::redact_arg_values(&["program-name"], &["--msg", "hello"]).unwrap();
     assert_eq!(actual, &["program-name", "--msg"]);
 }
 
+#[test]
+fn redact_arg_values_optional_arg_short() {
+    #[derive(FromArgs, Debug)]
+    /// Short description
+    struct Cmd {
+        #[argh(option, short = 'm')]
+        /// a msg param
+        _msg: Option<String>,
+    }
+
+    let actual = Cmd::redact_arg_values(&["program-name"], &["-m", "hello"]).unwrap();
+    assert_eq!(actual, &["program-name", "-m"]);
+}
+
+#[test]
+fn redact_arg_values_optional_arg_long() {
+    #[derive(FromArgs, Debug)]
+    /// Short description
+    struct Cmd {
+        #[argh(option, long = "my-msg")]
+        /// a msg param
+        _msg: Option<String>,
+    }
+
+    let actual = Cmd::redact_arg_values(&["program-name"], &["--my-msg", "hello"]).unwrap();
+    assert_eq!(actual, &["program-name", "--my-msg"]);
+}
+
 #[test]
 fn redact_arg_values_two_option_args() {
     #[derive(FromArgs, Debug)]
@@ -940,11 +968,11 @@ fn redact_arg_values_two_option_args() {
     struct Cmd {
         #[argh(option)]
         /// a msg param
-        msg: String,
+        _msg: String,
 
         #[argh(option)]
         /// a delivery param
-        delivery: String,
+        _delivery: String,
     }
 
     let actual =
@@ -960,11 +988,11 @@ fn redact_arg_values_option_one_optional_args() {
     struct Cmd {
         #[argh(option)]
         /// a msg param
-        msg: String,
+        _msg: String,
 
         #[argh(option)]
         /// a delivery param
-        delivery: Option<String>,
+        _delivery: Option<String>,
     }
 
     let actual =
@@ -976,6 +1004,24 @@ fn redact_arg_values_option_one_optional_args() {
     assert_eq!(actual, &["program-name", "--msg"]);
 }
 
+#[test]
+fn redact_arg_values_option_repeating() {
+    #[derive(FromArgs, Debug)]
+    /// Short description
+    struct Cmd {
+        #[argh(option)]
+        /// fooey
+        _msg: Vec<String>,
+    }
+
+    let actual = Cmd::redact_arg_values(&["program-name"], &[]).unwrap();
+    assert_eq!(actual, &["program-name"]);
+
+    let actual =
+        Cmd::redact_arg_values(&["program-name"], &["--msg", "abc", "--msg", "xyz"]).unwrap();
+    assert_eq!(actual, &["program-name", "--msg", "--msg"]);
+}
+
 #[test]
 fn redact_arg_values_switch() {
     #[derive(FromArgs, Debug)]
@@ -983,7 +1029,7 @@ fn redact_arg_values_switch() {
     struct Cmd {
         #[argh(switch, short = 'f')]
         /// speed of cmd
-        faster: bool,
+        _faster: bool,
     }
 
     let actual = Cmd::redact_arg_values(&["program-name"], &["--faster"]).unwrap();
@@ -998,6 +1044,7 @@ fn redact_arg_values_positional() {
     #[derive(FromArgs, Debug)]
     /// Short description
     struct Cmd {
+        #[allow(unused)]
         #[argh(positional)]
         /// speed of cmd
         speed: u8,
@@ -1007,14 +1054,28 @@ fn redact_arg_values_positional() {
     assert_eq!(actual, &["program-name", "speed"]);
 }
 
+#[test]
+fn redact_arg_values_positional_arg_name() {
+    #[derive(FromArgs, Debug)]
+    /// Short description
+    struct Cmd {
+        #[argh(positional, arg_name = "speed")]
+        /// speed of cmd
+        _speed: u8,
+    }
+
+    let actual = Cmd::redact_arg_values(&["program-name"], &["5"]).unwrap();
+    assert_eq!(actual, &["program-name", "speed"]);
+}
+
 #[test]
 fn redact_arg_values_positional_repeating() {
     #[derive(FromArgs, Debug)]
     /// Short description
     struct Cmd {
-        #[argh(positional)]
+        #[argh(positional, arg_name = "speed")]
         /// speed of cmd
-        speed: Vec<u8>,
+        _speed: Vec<u8>,
     }
 
     let actual = Cmd::redact_arg_values(&["program-name"], &["5", "6"]).unwrap();
@@ -1026,9 +1087,9 @@ fn redact_arg_values_positional_err() {
     #[derive(FromArgs, Debug)]
     /// Short description
     struct Cmd {
-        #[argh(positional)]
+        #[argh(positional, arg_name = "speed")]
         /// speed of cmd
-        speed: u8,
+        _speed: u8,
     }
 
     let actual = Cmd::redact_arg_values(&["program-name"], &[]).unwrap_err();
@@ -1046,13 +1107,13 @@ fn redact_arg_values_two_positional() {
     #[derive(FromArgs, Debug)]
     /// Short description
     struct Cmd {
-        #[argh(positional)]
+        #[argh(positional, arg_name = "speed")]
         /// speed of cmd
-        speed: u8,
+        _speed: u8,
 
-        #[argh(positional)]
+        #[argh(positional, arg_name = "direction")]
         /// direction
-        direction: String,
+        _direction: String,
     }
 
     let actual = Cmd::redact_arg_values(&["program-name"], &["5", "north"]).unwrap();
@@ -1064,13 +1125,13 @@ fn redact_arg_values_positional_option() {
     #[derive(FromArgs, Debug)]
     /// Short description
     struct Cmd {
-        #[argh(positional)]
+        #[argh(positional, arg_name = "speed")]
         /// speed of cmd
-        speed: u8,
+        _speed: u8,
 
         #[argh(option)]
         /// direction
-        direction: String,
+        _direction: String,
     }
 
     let actual = Cmd::redact_arg_values(&["program-name"], &["5", "--direction", "north"]).unwrap();
@@ -1082,13 +1143,13 @@ fn redact_arg_values_positional_optional_option() {
     #[derive(FromArgs, Debug)]
     /// Short description
     struct Cmd {
-        #[argh(positional)]
+        #[argh(positional, arg_name = "speed")]
         /// speed of cmd
-        speed: u8,
+        _speed: u8,
 
         #[argh(option)]
         /// direction
-        direction: Option<String>,
+        _direction: Option<String>,
     }
 
     let actual = Cmd::redact_arg_values(&["program-name"], &["5"]).unwrap();
@@ -1100,13 +1161,13 @@ fn redact_arg_values_subcommand() {
     #[derive(FromArgs, Debug)]
     /// Short description
     struct Cmd {
-        #[argh(positional)]
+        #[argh(positional, arg_name = "speed")]
         /// speed of cmd
-        speed: u8,
+        _speed: u8,
 
         #[argh(subcommand)]
         /// means of transportation
-        means: MeansSubcommand,
+        _means: MeansSubcommand,
     }
 
     #[derive(FromArgs, Debug)]
@@ -1124,7 +1185,7 @@ fn redact_arg_values_subcommand() {
     struct WalkingSubcommand {
         #[argh(option)]
         /// a song to listen to
-        music: String,
+        _music: String,
     }
 
     #[derive(FromArgs, Debug)]
@@ -1146,13 +1207,13 @@ fn redact_arg_values_subcommand_with_space_in_name() {
     #[derive(FromArgs, Debug)]
     /// Short description
     struct Cmd {
-        #[argh(positional)]
+        #[argh(positional, arg_name = "speed")]
         /// speed of cmd
-        speed: u8,
+        _speed: u8,
 
         #[argh(subcommand)]
         /// means of transportation
-        means: MeansSubcommand,
+        _means: MeansSubcommand,
     }
 
     #[derive(FromArgs, Debug)]
@@ -1169,7 +1230,7 @@ fn redact_arg_values_subcommand_with_space_in_name() {
     struct WalkingSubcommand {
         #[argh(option)]
         /// a song to listen to
-        music: String,
+        _music: String,
     }
 
     #[derive(FromArgs, Debug)]
@@ -1227,3 +1288,21 @@ fn redact_arg_values_produces_errors_with_bad_arguments() {
         }),
     );
 }
+
+#[test]
+fn redact_arg_values_does_not_warn_if_used() {
+    #[forbid(unused)]
+    #[derive(FromArgs, Debug)]
+    /// Short description
+    struct Cmd {
+        #[argh(positional)]
+        /// speed of cmd
+        speed: u8,
+    }
+
+    let cmd = Cmd::from_args(&["program-name"], &["5"]).unwrap();
+    assert_eq!(cmd.speed, 5);
+
+    let actual = Cmd::redact_arg_values(&["program-name"], &["5"]).unwrap();
+    assert_eq!(actual, &["program-name", "speed"]);
+}