Skip to content
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.

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

Fix some redaction bugs #112

Merged
merged 4 commits into from
Dec 8, 2021
Merged

Fix some redaction bugs #112

merged 4 commits into from
Dec 8, 2021

Conversation

erickt
Copy link
Collaborator

@erickt erickt commented Dec 7, 2021

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 the arg_name if specified. In addition, it corrects all the outstanding warnings as well.

Fixes #111

@erickt erickt requested a review from richkadel December 7, 2021 20:50
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")]
Copy link
Contributor

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

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
Contributor

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.

argh/tests/lib.rs Outdated Show resolved Hide resolved
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.
@erickt erickt merged commit f1f85d2 into google:master Dec 8, 2021
@erickt erickt deleted the bump branch December 8, 2021 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

redact_arg_values throws a parse error on Options of type Vec
2 participants