Merge #2157

2157: Suggest using subcommand when supplied after `--` r=pksunkara a=ldm0



Co-authored-by: Donough Liu <ldm2993593805@163.com>

src/parse/errors.rs

@@ -828,6 +828,27 @@ impl Error {
         }
     }
 
+    pub(crate) fn unnecessary_double_dash(arg: String, usage: String, color: ColorChoice) -> Self {
+        let mut c = Colorizer::new(true, color);
+
+        start_error(&mut c, "Found argument '");
+        c.warning(arg.clone());
+        c.none("' which wasn't expected, or isn't valid in this context");
+
+        c.none(format!(
+            "\n\nIf you tried to supply `{}` as a subcommand, remove the '--' before it.",
+            arg
+        ));
+        put_usage(&mut c, usage);
+        try_help(&mut c);
+
+        Error {
+            message: c,
+            kind: ErrorKind::UnknownArgument,
+            info: vec![arg],
+        }
+    }
+
     pub(crate) fn argument_not_found_auto(arg: String) -> Self {
         let mut c = Colorizer::new(true, ColorChoice::Auto);
 

src/parse/parser.rs

@@ -350,8 +350,6 @@ impl<'help, 'app> Parser<'help, 'app> {
         // Verify all positional assertions pass
         self._build();
 
-        let has_args = self.has_args();
-
         let mut subcmd_name: Option<String> = None;
         let mut keep_state = false;
         let mut external_subcommand = false;
@@ -681,18 +679,9 @@ impl<'help, 'app> Parser<'help, 'app> {
                 external_subcommand = true;
 
                 break;
-            } else if !((self.is_set(AS::AllowLeadingHyphen)
-                || self.is_set(AS::AllowNegativeNumbers))
-                && arg_os.starts_with("-"))
-                && !self.is_set(AS::InferSubcommands)
+            } else if !self.has_args()
+                || self.is_set(AS::InferSubcommands) && self.has_subcommands()
             {
-                return Err(ClapError::unknown_argument(
-                    arg_os.to_string_lossy().to_string(),
-                    None,
-                    Usage::new(self).create_usage_with_title(&[]),
-                    self.app.color(),
-                ));
-            } else if !has_args || self.is_set(AS::InferSubcommands) && self.has_subcommands() {
                 let cands = suggestions::did_you_mean(
                     &*arg_os.to_string_lossy(),
                     self.app.all_subcommand_names(),
@@ -721,6 +710,22 @@ impl<'help, 'app> Parser<'help, 'app> {
                         self.app.color(),
                     ));
                 }
+            } else if self.is_set(AS::TrailingValues) {
+                // If argument followed by a `--`
+                if self.possible_subcommand(&arg_os).is_some() {
+                    return Err(ClapError::unnecessary_double_dash(
+                        arg_os.to_string_lossy().to_string(),
+                        Usage::new(self).create_usage_with_title(&[]),
+                        self.app.color(),
+                    ));
+                } else {
+                    return Err(ClapError::unknown_argument(
+                        arg_os.to_string_lossy().to_string(),
+                        None,
+                        Usage::new(self).create_usage_with_title(&[]),
+                        self.app.color(),
+                    ));
+                }
             } else {
                 return Err(ClapError::unknown_argument(
                     arg_os.to_string_lossy().to_string(),
@@ -1192,18 +1197,16 @@ impl<'help, 'app> Parser<'help, 'app> {
         // Update the curent index
         self.cur_idx.set(self.cur_idx.get() + 1);
 
-        let mut val = None;
         debug!("Parser::parse_long_arg: Does it contain '='...");
         let matches;
-        let arg = if full_arg.contains_byte(b'=') {
+        let (arg, val) = if full_arg.contains_byte(b'=') {
             matches = full_arg.trim_start_matches(b'-');
             let (p0, p1) = matches.split_at_byte(b'=');
             debug!("Yes '{:?}'", p1);
-            val = Some(p1);
-            p0
+            (p0, Some(p1))
         } else {
             debug!("No");
-            full_arg.trim_start_matches(b'-')
+            (full_arg.trim_start_matches(b'-'), None)
         };
         if let Some(opt) = self.app.args.get(&KeyType::Long(arg.to_os_string())) {
             debug!(

tests/subcommands.rs

@@ -93,6 +93,16 @@ USAGE:
 
 For more information try --help";
 
+static SUBCMD_AFTER_DOUBLE_DASH: &str =
+    "error: Found argument 'subcmd' which wasn't expected, or isn't valid in this context
+
+If you tried to supply `subcmd` as a subcommand, remove the '--' before it.
+
+USAGE:
+    app [SUBCOMMAND]
+
+For more information try --help";
+
 #[test]
 fn subcommand() {
     let m = App::new("test")
@@ -382,3 +392,15 @@ fn subcommand_placeholder_test() {
         .unwrap()
         .contains("TEST_HEADER:"));
 }
+
+#[test]
+fn subcommand_used_after_double_dash() {
+    let app = App::new("app").subcommand(App::new("subcmd"));
+
+    assert!(utils::compare_output(
+        app,
+        "app -- subcmd",
+        SUBCMD_AFTER_DOUBLE_DASH,
+        true
+    ));
+}