-
Notifications
You must be signed in to change notification settings - Fork 893
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 panic in component list --toolchain stable
#3548
Conversation
Fixes a panic caused by the signature of the parser passed to Claps's `Arg.value_parser()` having PartialToolchainDesc, not matching `ArgMatches.get_one::<ResolvableToolchainName>()` in `explicit_or_dir_toolchain()` Also, rename `explicit_or_dir_toolchain()` -> `explicit_desc_or_dir_toolchain()` and consolidate its use across various CLI subcommands. Also fixes a similar panic in `rustup man --toolchain stable`
32449c3
to
0c5449d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks good to me!
How hard would it be to add a unit test for this?
I can't think of a way to unit test this. |
@majaha Do you think migrating to I imagine having something like the following will be better for cross-referencing: #[arg(..., value_parser = ...)]
toolchain: Toolchain, Type safety-wise I don't know much either, but maybe clap.rs already provides some sort of mechanism to avoid this kind of error? (cc @epage) PS: The ValueParser selection mechanism looks like a potential candidate to me. |
One way to unit test is to have |
As for avoiding this completely, its either don't use value parsers or use the derive with inferred value parsers. Value parsers were primarily designed / optimized for the derive API. |
Closes #3547
Fixes a panic caused by the signature of the parser passed to Claps's
Arg.value_parser()
having PartialToolchainDesc, not matchingArgMatches.get_one::<ResolvableToolchainName>()
inexplicit_or_dir_toolchain()
Also, renames
explicit_or_dir_toolchain()
->explicit_desc_or_dir_toolchain()
and consolidates its use across various CLI subcommands.Also fixes a similar panic in
rustup man --toolchain stable
It seems like Clap is very type-unsafe in this regard. I made a small effort to find other similar problems by eyeball, but there's just so many subcommands to cross-reference... Probably we need to have more rigorous testing of all CLI arguments that have a
.value_parser()
.