Repository: clap-rs/clap
Pull request #4219 (2022-09-15 15:29:09)

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

<!--
Thanks for helping out!

Please link the appropriate issue from your PR.

If you don't have an issue, we'd recommend starting with one first so the PR can focus on the
implementation (unless its an obvious bug or documentation fix that will have
little conversation).
-->



Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 894b1ea5..9495823c 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/CONTRIBUTING.md b/CONTRIBUTING.md
index fedb381b..4318c908 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -65,7 +65,7 @@ Some practices to avoid breaking changes
 As we work towards [a more flexible architecture](https://github.com/clap-rs/clap/discussions/3476), we hope to support multiple major versions to help ease users through the upgrade churn.
 
 | Version                                              | Status        | Support | End-of-Life |
-|------------------------------------------------------|---------------|---------|-------------|
+|------------------------------------------------------|---------------|---------|
 | [v4](https://github.com/clap-rs/clap/tree/master)    | active        | Features and bug fixes target `master` by default | TBD |
 | [v3](https://github.com/clap-rs/clap/tree/v3-master) | maintenance   | Accepting trivial cherry-picks from `master` (i.e. minimal conflict resolution) by contributors | TBD |
 | [v2](https://github.com/clap-rs/clap/tree/v2-master) | deprecated    | Only accepting fixes for ecosystem-wide critical bugs | TBD |
diff --git a/src/builder/command.rs b/src/builder/command.rs
index e842ffb8..ac834d46 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 3c660171..82a82e1f 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<Item = &KeyType> {
         self.keys.iter().map(|x| &x.key)
diff --git a/src/parser/parser.rs b/src/parser/parser.rs
index e78715dd..d736d0cf 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(),


Test patch:
diff --git a/examples/derive_ref/interop_tests.md b/examples/derive_ref/interop_tests.md
index c891654e..87bd7602 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] <COMMAND>
 
diff --git a/tests/builder/app_settings.rs b/tests/builder/app_settings.rs
index a3125373..723e15d6 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 2ed68848..ad42616a 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,
     );
 }