From b7d13dfb88d6517843ec71630e8c0ef44e20b8ad Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 15 Sep 2022 10:27:16 -0500 Subject: [PATCH] fix(parser): Prefer invalid subcommands over invalid args Just having `--help` or `--version` can make us get invalid args instead of invalid subcommands. It doesn't make sense to do this unless positionals are used. Even then it might not make sense but this is at least a step in the right direction. Unsure how I feel about this being backported to clap 3. It most likely would be fine? This was noticed while looking into #4218 --- CHANGELOG.md | 1 + examples/derive_ref/interop_tests.md | 4 ++-- src/builder/command.rs | 6 +++--- src/mkeymap.rs | 5 ----- src/parser/parser.rs | 6 +++++- tests/builder/app_settings.rs | 2 +- tests/derive/subcommands.rs | 2 +- 7 files changed, 13 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 894b1ea5cb2..9495823ce1a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -294,6 +294,7 @@ Behavior Changes - *(version)* Use `Command::display_name` rather than `Command::bin_name` (#3966) - *(parser)* Always fill in `""` argument for external subcommands (#3263) - *(parser)* Short flags now have higher precedence than hyphen values with `Arg::allow_hyphen_values`, like `Command::allow_hyphen_values` (#4187) +- *(parser)* Prefer `InvalidSubcommand` over `UnknownArgument` in more cases (#4219) - *(derive)* Detect escaped external subcommands that look like built-in subcommands (#3703) - *(derive)* Leave `Arg::id` as `verbatim` casing (#3282) - *(derive)* Default to `#[clap(value_parser, action)]` instead of `#[clap(parse)]` (#3827) diff --git a/examples/derive_ref/interop_tests.md b/examples/derive_ref/interop_tests.md index c891654e827..87bd7602252 100644 --- a/examples/derive_ref/interop_tests.md +++ b/examples/derive_ref/interop_tests.md @@ -85,7 +85,7 @@ For more information try --help ```console $ interop_augment_subcommands unknown ? failed -error: Found argument 'unknown' which wasn't expected, or isn't valid in this context +error: The subcommand 'unknown' wasn't recognized Usage: interop_augment_subcommands[EXE] [COMMAND] @@ -189,7 +189,7 @@ Cli { ```console $ interop_hand_subcommand unknown ? failed -error: Found argument 'unknown' which wasn't expected, or isn't valid in this context +error: The subcommand 'unknown' wasn't recognized Usage: interop_hand_subcommand[EXE] [OPTIONS] diff --git a/src/builder/command.rs b/src/builder/command.rs index e842ffb8295..ac834d46a8a 100644 --- a/src/builder/command.rs +++ b/src/builder/command.rs @@ -1166,7 +1166,7 @@ impl Command { /// "myprog", "help" /// ]); /// assert!(res.is_err()); - /// assert_eq!(res.unwrap_err().kind(), ErrorKind::UnknownArgument); + /// assert_eq!(res.unwrap_err().kind(), ErrorKind::InvalidSubcommand); /// ``` /// /// [`subcommand`]: crate::Command::subcommand() @@ -4313,8 +4313,8 @@ impl Command { } #[inline] - pub(crate) fn has_args(&self) -> bool { - !self.args.is_empty() + pub(crate) fn has_positionals(&self) -> bool { + self.get_positionals().next().is_some() } pub(crate) fn has_visible_subcommands(&self) -> bool { diff --git a/src/mkeymap.rs b/src/mkeymap.rs index 3c660171331..82a82e1f800 100644 --- a/src/mkeymap.rs +++ b/src/mkeymap.rs @@ -113,11 +113,6 @@ impl MKeyMap { .map(|k| &self.args[k.index]) } - /// Find out if the map have no arg. - pub(crate) fn is_empty(&self) -> bool { - self.args.is_empty() - } - /// Return iterators of all keys. pub(crate) fn keys(&self) -> impl Iterator { self.keys.iter().map(|x| &x.key) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index e78715ddd26..d736d0cfdf0 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -492,6 +492,7 @@ impl<'cmd> Parser<'cmd> { ); } } + let candidates = suggestions::did_you_mean( &arg_os.display().to_string(), self.cmd.all_subcommand_names(), @@ -513,8 +514,10 @@ impl<'cmd> Parser<'cmd> { Usage::new(self.cmd).create_usage_with_title(&[]), ); } + // If the argument must be a subcommand. - if !self.cmd.has_args() || self.cmd.is_infer_subcommands_set() && self.cmd.has_subcommands() + if self.cmd.has_subcommands() + && (!self.cmd.has_positionals() || self.cmd.is_infer_subcommands_set()) { return ClapError::unrecognized_subcommand( self.cmd, @@ -522,6 +525,7 @@ impl<'cmd> Parser<'cmd> { Usage::new(self.cmd).create_usage_with_title(&[]), ); } + ClapError::unknown_argument( self.cmd, arg_os.display().to_string(), diff --git a/tests/builder/app_settings.rs b/tests/builder/app_settings.rs index a3125373518..723e15d6b07 100644 --- a/tests/builder/app_settings.rs +++ b/tests/builder/app_settings.rs @@ -600,7 +600,7 @@ fn disable_help_subcommand() { .try_get_matches_from(vec!["", "help"]); assert!(result.is_err()); let err = result.err().unwrap(); - assert_eq!(err.kind(), ErrorKind::UnknownArgument); + assert_eq!(err.kind(), ErrorKind::InvalidSubcommand); } #[test] diff --git a/tests/derive/subcommands.rs b/tests/derive/subcommands.rs index 2ed688485cb..ad42616a9be 100644 --- a/tests/derive/subcommands.rs +++ b/tests/derive/subcommands.rs @@ -551,7 +551,7 @@ fn skip_subcommand() { let res = Opt::try_parse_from(&["test", "skip"]); assert_eq!( res.unwrap_err().kind(), - clap::error::ErrorKind::UnknownArgument, + clap::error::ErrorKind::InvalidSubcommand, ); }