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

@@ -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)

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] <COMMAND>
 

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 {

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<Item = &KeyType> {
         self.keys.iter().map(|x| &x.key)

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(),

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]

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,
     );
 }