Fix some redaction bugs #112
Conversation
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.
This changes argh_derive to handle redacting types like:
```
struct Cmd {
#[argh(option)]
/// fooey
arg: Vec<String>,
}
```
Fixes google#111
| #[derive(FromArgs, Debug)] | ||
| /// Short description | ||
| struct Cmd { | ||
| #[argh(positional, arg_name = "speed")] |
There was a problem hiding this comment.
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 _?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
rust-lang/rust#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.
This makes two fixes to redaction. First, it corrects using redaction with repeated options, like
Vec<String>. Second, it changes redacted positional arguments to use thearg_nameif specified. In addition, it corrects all the outstanding warnings as well.Fixes #111