From f102494b8937e1909b63615eee13c5fe6d70f08c Mon Sep 17 00:00:00 2001 From: Erick Tryzelaar Date: Tue, 7 Dec 2021 14:59:02 -0500 Subject: [PATCH 1/4] Use arg_name for redacted positionals This changes redacted positional arguments from using the field name to using the arg name. This allows the field name to be changed without impacting the redaction. In addition, this fixes all the outstanding warnings. --- argh/tests/lib.rs | 113 +++++++++++++++++++++++++++------------- argh_derive/src/help.rs | 14 +---- argh_derive/src/lib.rs | 12 +++-- 3 files changed, 88 insertions(+), 51 deletions(-) diff --git a/argh/tests/lib.rs b/argh/tests/lib.rs index 5b1ac7a..fc4fc13 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, + _msg: Option, } 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, + _msg: Option, } 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, + } + + 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, + } + + 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, + _delivery: Option, } let actual = @@ -983,7 +1011,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 +1026,7 @@ fn redact_arg_values_positional() { #[derive(FromArgs, Debug)] /// Short description struct Cmd { + #[allow(dead_code)] #[argh(positional)] /// speed of cmd speed: u8, @@ -1007,14 +1036,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, + _speed: Vec, } let actual = Cmd::redact_arg_values(&["program-name"], &["5", "6"]).unwrap(); @@ -1026,9 +1069,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 +1089,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 +1107,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 +1125,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, + _direction: Option, } let actual = Cmd::redact_arg_values(&["program-name"], &["5"]).unwrap(); @@ -1100,13 +1143,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 +1167,7 @@ fn redact_arg_values_subcommand() { struct WalkingSubcommand { #[argh(option)] /// a song to listen to - music: String, + _music: String, } #[derive(FromArgs, Debug)] @@ -1146,13 +1189,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 +1212,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)] 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 `[...]` 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..9a11077 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. @@ -640,12 +644,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()) }, }; } } @@ -718,7 +722,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) From 69a8bb6a3318b82e26e17e7c94c2d9b4b84530bd Mon Sep 17 00:00:00 2001 From: Erick Tryzelaar Date: Tue, 7 Dec 2021 15:39:31 -0500 Subject: [PATCH 2/4] Fix redacting Vec options This changes argh_derive to handle redacting types like: ``` struct Cmd { #[argh(option)] /// fooey arg: Vec, } ``` Fixes #111 --- argh/Cargo.toml | 6 +++--- argh/tests/lib.rs | 18 ++++++++++++++++++ argh_derive/Cargo.toml | 4 ++-- argh_derive/src/lib.rs | 27 +++++++++++++++++++++++++-- argh_shared/Cargo.toml | 2 +- 5 files changed, 49 insertions(+), 8 deletions(-) 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 ", "Benjamin Brittain ", "Erick Tryzelaar "] 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/tests/lib.rs b/argh/tests/lib.rs index fc4fc13..810e2bc 100644 --- a/argh/tests/lib.rs +++ b/argh/tests/lib.rs @@ -1004,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, + } + + 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)] 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 ", "Benjamin Brittain ", "Erick Tryzelaar "] 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/lib.rs b/argh_derive/src/lib.rs index 9a11077..a123d4e 100644 --- a/argh_derive/src/lib.rs +++ b/argh_derive/src/lib.rs @@ -626,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 } + } + Optionality::None | Optionality::Optional | Optionality::Defaulted(_) => { + quote! { std::option::Option } + } + }; + quote! { - let mut #field_name: argh::ParseValueSlotTy::, 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()) }, @@ -668,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()); 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 ", "Benjamin Brittain ", "Erick Tryzelaar "] edition = "2018" license = "BSD-3-Clause" From 2a6bddc3b3df04f0c3834b3788a76c3ed6960a7b Mon Sep 17 00:00:00 2001 From: Erick Tryzelaar Date: Wed, 8 Dec 2021 13:19:58 -0500 Subject: [PATCH 3/4] Use #[allow(unused)] --- argh/tests/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/argh/tests/lib.rs b/argh/tests/lib.rs index 810e2bc..5c4ace6 100644 --- a/argh/tests/lib.rs +++ b/argh/tests/lib.rs @@ -1044,7 +1044,7 @@ fn redact_arg_values_positional() { #[derive(FromArgs, Debug)] /// Short description struct Cmd { - #[allow(dead_code)] + #[allow(unused)] #[argh(positional)] /// speed of cmd speed: u8, From 903d4ff7d01362da3afc32475dc3a049d8059e42 Mon Sep 17 00:00:00 2001 From: Erick Tryzelaar Date: Wed, 8 Dec 2021 13:39:17 -0500 Subject: [PATCH 4/4] Make sure we don't warn when fields are used https://github.com/rust-lang/rust/pull/85200 changed rust to emit more unused warnings if fields in a struct are ultimately never read. This adds a test to make sure that we don't experience these warnings. --- argh/tests/lib.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/argh/tests/lib.rs b/argh/tests/lib.rs index 5c4ace6..fe8c858 100644 --- a/argh/tests/lib.rs +++ b/argh/tests/lib.rs @@ -1288,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"]); +}