Skip to content

Fix some redaction bugs #112

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
erickt merged 4 commits into from
Dec 8, 2021
Merged

Fix some redaction bugs #112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f102494
Use arg_name for redacted positionals
erickt Dec 7, 2021
69a8bb6
Fix redacting Vec<T> options
erickt Dec 7, 2021
2a6bddc
Use #[allow(unused)]
erickt Dec 8, 2021
903d4ff
Make sure we don't warn when fields are used
erickt Dec 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions argh/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"]
Expand All @@ -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" }
149 changes: 114 additions & 35 deletions argh/tests/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ fn missing_option_value() {
struct Cmd {
#[argh(option)]
/// fooey
msg: String,
_msg: String,
}

let e = Cmd::from_args(&["cmdname"], &["--msg"])
Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand Down Expand Up @@ -666,15 +666,15 @@ mod fuchsia_commandline_tools_rubric {
/// A type for testing `--help`/`help`
struct HelpTopLevel {
#[argh(subcommand)]
sub: HelpFirstSub,
_sub: HelpFirstSub,
}

#[derive(FromArgs, Debug)]
#[argh(subcommand, name = "first")]
/// First subcommmand for testing `help`.
struct HelpFirstSub {
#[argh(subcommand)]
sub: HelpSecondSub,
_sub: HelpSecondSub,
}

#[derive(FromArgs, Debug)]
Expand Down Expand Up @@ -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();
Expand All @@ -926,25 +926,53 @@ 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)]
/// Short description
struct Cmd {
#[argh(option)]
/// a msg param
msg: String,
_msg: String,

#[argh(option)]
/// a delivery param
delivery: String,
_delivery: String,
}

let actual =
Expand All @@ -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 =
Expand All @@ -976,14 +1004,32 @@ 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)]
/// Short description
struct Cmd {
#[argh(switch, short = 'f')]
/// speed of cmd
faster: bool,
_faster: bool,
}

let actual = Cmd::redact_arg_values(&["program-name"], &["--faster"]).unwrap();
Expand All @@ -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,
Expand All @@ -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")]
Copy link

@richkadel richkadel Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed offline, I have no problem with the implementation of an arg_name override, but it highlights a new issue that seems to underly the need for arg_name, I assume?

You changed most (all?) of the fields in your test structs (at least in this commit) to add an underscore to the fields. I'm assuming you did this because you are getting warnings that these fields are unused?

This sounds very related to the problems that we've seen in other Rust code bases after rust-lang/rust#85200 landed. Many downstream structs that depended on Debug in order to generate logging output (among other things) were forced to either add the leading underscore or add #[allow(unused)]. (Personally, I was not a fan of the code breakage that caused, but the decision-makers felt the cost was worth it.)

I'm not sure if that change or some other change broke how argh works, but, from what you told me, it looks like the change happened between rustc 1.56 and 1.57.

I would like to know if you can track down the origin of the change in 1.57 to see if there is a better, internal workaround in the argh macro that could cause the attributed field to get marked "used", and avoid the warning?

If that's not feasible, and you feel it's OK to just pass the burden down to the users of argh, I can buy that argument, but as a workaround, would you consider making the default value of arg_name (if not supplied) strip the leading underscore, if there is one, so the implicit result is as if the user supplied the arg_name that way?

And if you can do that, is that the only reason to implement an arg_name parameter to the argh() attribute? If you have some other good use cases, can you modify your tests to demonstrate a different example, using arg_name in some useful way other than for stripping the _?

Copy link
Collaborator Author

@erickt erickt Dec 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You changed most (all?) of the fields in your test structs (at least in this commit) to add an underscore to the fields. I'm assuming you did this because you are getting warnings that these fields are unused?

Yeah, I updated these fields to be prefixed with _ to silence the unused warnings. I'm guessing the source of this was rust-lang/rust#85200, especially since that landed in 1.57.

I would like to know if you can track down the origin of the change in 1.57 to see if there is a better, internal workaround in the argh macro that could cause the attributed field to get marked "used", and avoid the warning?

I don't think we have a great way to do that. The problem we're running into is that all these tests are doing is calling FromArgs::redact_arg_values, which is just using the same argument parser that FromArgs::from_args uses, but we don't actually store any of the values in the FromArgs implementation, so those fields never get used.

To avoid the warning, we could change the derived FromArgs::redact_arg_values to parse the arguments and throw them away, but I'm a little hesitant on doing this, since other implementations of FromArgs would observe these calls, and they could have side effects. There'd also be some overhead in doing this, which the optimizer might not be able to get rid of if the impls have side effects.

Perhaps a better way of doing this would be to get rid of FromArgs::redact_arg_values, and replace it with FromArgs::from_args_with_redacted_args(). That function would return the parsed value and the redacted CLI arguments.

However, I'm not sure how painful this is going to be in practice. In order to trip over this, users would need to have an impl of FromArgs, and the only use of it is calling redact_arg_values. I'm struggling to see how users would encounter this outside of testing argh.

I can buy that argument, but as a workaround, would you consider making the default value of arg_name (if not supplied) strip the leading underscore, if there is one, so the implicit result is as if the user supplied the arg_name that way?

Sure, I filed #113 to track this. I think we can implement that in a future PR though.

And if you can do that, is that the only reason to implement an arg_name parameter to the argh() attribute? If you have some other good use cases, can you modify your tests to demonstrate a different example, using arg_name in some useful way other than for stripping the _?

Yeah there are other use cases for arg_name. I had a test case that used this here: https://github.com/google/argh/pull/112/files#diff-16442fbfe34613b50b21a4a6a5211ed1ff15a68ebba662d38059ca44ce7d1ae2R951

Copy link

@richkadel richkadel Dec 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK cool. Well, if #113 gets implemented, then most of the arg_name = ... can (and probably should) be removed, because they would be redundant. So your "my-msg" example might be the only one left behind to demonstrate how arg_name changes the base behavior.

/// 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();
Expand All @@ -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();
Expand All @@ -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();
Expand All @@ -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();
Expand All @@ -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();
Expand All @@ -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)]
Expand All @@ -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)]
Expand All @@ -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)]
Expand All @@ -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)]
Expand Down Expand Up @@ -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"]);
}
4 changes: 2 additions & 2 deletions argh_derive/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
Expand All @@ -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" }
Loading