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

better compile time validation of value_parser types #4830

Open
2 tasks done
rbtcollins opened this issue Apr 10, 2023 · 2 comments
Open
2 tasks done

better compile time validation of value_parser types #4830

rbtcollins opened this issue Apr 10, 2023 · 2 comments
Labels
C-enhancement Category: Raise on the bar on expectations

Comments

@rbtcollins
Copy link

Please complete the following tasks

Clap Version

3.2.23

Describe your use case

Trying to parse toolchain arguments for rustup as a custom type rather than just String. We have about 5 different sorts of toolchains that can be parsed. Official ones from channels, custom names, official-or-custom, 'none'-or-official-or-custom.

When the clap value parser type (e.g. CustomToolchainNameParser) and the code querying the argument have a mismatched type, there is a silent failure (or perhaps we're misusing things?)

e.g. in the install code path I had this:

    if let Ok(Some(names)) = m.try_get_many::<PartialToolchainDesc>("toolchain") {

and the definition for install has this :

                        .value_parser(NonEmptyStringValueParser)

Then the type id check in verify_arg_t fails, but this is then caught in try_get_many - which I think we're using to handle optional values. But this is a different sort of failure - the failure is in the code that was compiled, and should be detectable at compile time.

Describe the solution you'd like

Our buggy code should fail to compile. Or perhaps we're doing things wrong and the docs should help us understand that.

Alternatives, if applicable

No response

Additional Context

No response

@rbtcollins rbtcollins added the C-enhancement Category: Raise on the bar on expectations label Apr 10, 2023
@epage
Copy link
Member

epage commented Apr 10, 2023

Except in some specific cases (#3912, #3864), the derive API handles all of this for you at compile-time

When using ArgMatches::get_*, we will panic at runtime. This just requires exercising all ArgMatches::get_* calls in tests and you know things are valid. You could use try_get_one::<CustomToolchainNameParser as TypedValueParser>::Value>("toolchain") to help (if those were even the right types and args involved, I also don't know if you can shorten that for associated value accesses)

Instead of either of the above, rustup is using ArgMatches::try_get_* and its unclear from the Issue what rustup does in the case ofErr(MatchesError::Downcast {...} }) .

I'm not too sure what more clap can do to help in this case or how the docs could be modified to help people who have explicitly chosen to go off the beaten path and then (making assumptions here) ignoring the error we are reporting about this case.

@rbtcollins
Copy link
Author

I'm very happy to change rustup to use clap better - its a very old codebase and we have a lot of places things could be better :).

Poking at this further, I've found

Mismatch between definition and access of `toolchain`. Could not downcast to rustup::toolchain::ResolvableToolchainName, need to downcast to rustup::dist::dist::PartialToolchainDesc 

happens at runtime, which is great.

I think the original failure occurred after I changed the get_* calls, but had not yet added the value_parser() calls to the clap App definition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Raise on the bar on expectations
Projects
None yet
Development

No branches or pull requests

2 participants