Merge pull request #4567 from epage/error

fix(parser): Be less confusing with args/subcommand conflicts
clap-rs · Dec 21, 2022 · b941a3e · b941a3e
2 parents a72f962 + 453ac0b
commit b941a3e
Show file tree

Hide file tree

Showing 3 changed files with 95 additions and 71 deletions.
diff --git a/src/parser/parser.rs b/src/parser/parser.rs
@@ -502,33 +502,35 @@ impl<'cmd> Parser<'cmd> {
             }
         }
 
-        let candidates = suggestions::did_you_mean(
-            &arg_os.display().to_string(),
-            self.cmd.all_subcommand_names(),
-        );
-        // If the argument looks like a subcommand.
-        if !candidates.is_empty() {
-            return ClapError::invalid_subcommand(
-                self.cmd,
-                arg_os.display().to_string(),
-                candidates,
-                self.cmd
-                    .get_bin_name()
-                    .unwrap_or_else(|| self.cmd.get_name())
-                    .to_owned(),
-                Usage::new(self.cmd).create_usage_with_title(&[]),
+        if !(self.cmd.is_args_conflicts_with_subcommands_set() && valid_arg_found) {
+            let candidates = suggestions::did_you_mean(
+                &arg_os.display().to_string(),
+                self.cmd.all_subcommand_names(),
             );
-        }
+            // If the argument looks like a subcommand.
+            if !candidates.is_empty() {
+                return ClapError::invalid_subcommand(
+                    self.cmd,
+                    arg_os.display().to_string(),
+                    candidates,
+                    self.cmd
+                        .get_bin_name()
+                        .unwrap_or_else(|| self.cmd.get_name())
+                        .to_owned(),
+                    Usage::new(self.cmd).create_usage_with_title(&[]),
+                );
+            }
 
-        // If the argument must be a subcommand.
-        if self.cmd.has_subcommands()
-            && (!self.cmd.has_positionals() || self.cmd.is_infer_subcommands_set())
-        {
-            return ClapError::unrecognized_subcommand(
-                self.cmd,
-                arg_os.display().to_string(),
-                Usage::new(self.cmd).create_usage_with_title(&[]),
-            );
+            // If the argument must be a subcommand.
+            if self.cmd.has_subcommands()
+                && (!self.cmd.has_positionals() || self.cmd.is_infer_subcommands_set())
+            {
+                return ClapError::unrecognized_subcommand(
+                    self.cmd,
+                    arg_os.display().to_string(),
+                    Usage::new(self.cmd).create_usage_with_title(&[]),
+                );
+            }
         }
 
         let suggested_trailing_arg = !trailing_values

diff --git a/tests/builder/app_settings.rs b/tests/builder/app_settings.rs
@@ -654,52 +654,6 @@ Options:
     utils::assert_output(cmd, "clap-test --help", REQUIRE_EQUALS, false);
 }
 
-#[test]
-fn args_negate_subcommands_one_level() {
-    let res = Command::new("disablehelp")
-        .args_conflicts_with_subcommands(true)
-        .subcommand_negates_reqs(true)
-        .arg(arg!(<arg1> "some arg"))
-        .arg(arg!(<arg2> "some arg"))
-        .subcommand(
-            Command::new("sub1").subcommand(Command::new("sub2").subcommand(Command::new("sub3"))),
-        )
-        .try_get_matches_from(vec!["", "pickles", "sub1"]);
-    assert!(res.is_ok(), "error: {:?}", res.unwrap_err().kind());
-    let m = res.unwrap();
-    assert_eq!(
-        m.get_one::<String>("arg2").map(|v| v.as_str()),
-        Some("sub1")
-    );
-}
-
-#[test]
-fn args_negate_subcommands_two_levels() {
-    let res = Command::new("disablehelp")
-        .args_conflicts_with_subcommands(true)
-        .subcommand_negates_reqs(true)
-        .arg(arg!(<arg1> "some arg"))
-        .arg(arg!(<arg2> "some arg"))
-        .subcommand(
-            Command::new("sub1")
-                .args_conflicts_with_subcommands(true)
-                .subcommand_negates_reqs(true)
-                .arg(arg!(<arg> "some"))
-                .arg(arg!(<arg2> "some"))
-                .subcommand(Command::new("sub2").subcommand(Command::new("sub3"))),
-        )
-        .try_get_matches_from(vec!["", "sub1", "arg", "sub2"]);
-    assert!(res.is_ok(), "error: {:?}", res.unwrap_err().kind());
-    let m = res.unwrap();
-    assert_eq!(
-        m.subcommand_matches("sub1")
-            .unwrap()
-            .get_one::<String>("arg2")
-            .map(|v| v.as_str()),
-        Some("sub2")
-    );
-}
-
 #[test]
 fn propagate_vals_down() {
     let m = Command::new("myprog")

diff --git a/tests/builder/conflicts.rs b/tests/builder/conflicts.rs
@@ -655,6 +655,74 @@ fn exclusive_with_required() {
     cmd.clone().try_get_matches_from(["bug", "--test"]).unwrap();
 }
 
+#[test]
+fn args_negate_subcommands_one_level() {
+    let res = Command::new("disablehelp")
+        .args_conflicts_with_subcommands(true)
+        .subcommand_negates_reqs(true)
+        .arg(arg!(<arg1> "some arg"))
+        .arg(arg!(<arg2> "some arg"))
+        .subcommand(
+            Command::new("sub1").subcommand(Command::new("sub2").subcommand(Command::new("sub3"))),
+        )
+        .try_get_matches_from(vec!["", "pickles", "sub1"]);
+    assert!(res.is_ok(), "error: {:?}", res.unwrap_err().kind());
+    let m = res.unwrap();
+    assert_eq!(
+        m.get_one::<String>("arg2").map(|v| v.as_str()),
+        Some("sub1")
+    );
+}
+
+#[test]
+fn args_negate_subcommands_two_levels() {
+    let res = Command::new("disablehelp")
+        .args_conflicts_with_subcommands(true)
+        .subcommand_negates_reqs(true)
+        .arg(arg!(<arg1> "some arg"))
+        .arg(arg!(<arg2> "some arg"))
+        .subcommand(
+            Command::new("sub1")
+                .args_conflicts_with_subcommands(true)
+                .subcommand_negates_reqs(true)
+                .arg(arg!(<arg> "some"))
+                .arg(arg!(<arg2> "some"))
+                .subcommand(Command::new("sub2").subcommand(Command::new("sub3"))),
+        )
+        .try_get_matches_from(vec!["", "sub1", "arg", "sub2"]);
+    assert!(res.is_ok(), "error: {:?}", res.unwrap_err().kind());
+    let m = res.unwrap();
+    assert_eq!(
+        m.subcommand_matches("sub1")
+            .unwrap()
+            .get_one::<String>("arg2")
+            .map(|v| v.as_str()),
+        Some("sub2")
+    );
+}
+
+#[test]
+#[cfg(feature = "error-context")]
+fn subcommand_conflict_error_message() {
+    static CONFLICT_ERR: &str = "\
+error: Found argument 'sub1' which wasn't expected, or isn't valid in this context
+
+Usage: test [OPTIONS]
+       test <COMMAND>
+
+For more information try '--help'
+";
+
+    let cmd = Command::new("test")
+        .args_conflicts_with_subcommands(true)
+        .arg(arg!(-p --place <"place id"> "Place ID to open"))
+        .subcommand(
+            Command::new("sub1").subcommand(Command::new("sub2").subcommand(Command::new("sub3"))),
+        );
+
+    utils::assert_output(cmd, "test --place id sub1", CONFLICT_ERR, true);
+}
+
 #[test]
 fn subcommand_conflict_negates_required() {
     let cmd = Command::new("test")