Merge pull request #3703 from epage/external

fix(parser): Disambiguate whether built-in subcommands are escaped (unstable)

clap_derive/src/derives/subcommand.rs

@@ -484,9 +484,17 @@ fn gen_from_arg_matches(
             Unnamed(..) => abort_call_site!("{}: tuple enums are not supported", variant.ident),
         };
 
-        quote! {
-            if #sub_name == #subcommand_name_var {
-                return ::std::result::Result::Ok(#name :: #variant_name #constructor_block)
+        if cfg!(feature = "unstable-v4") {
+            quote! {
+                if #sub_name == #subcommand_name_var && !#sub_arg_matches_var.is_present("") {
+                    return ::std::result::Result::Ok(#name :: #variant_name #constructor_block)
+                }
+            }
+        } else {
+            quote! {
+                if #sub_name == #subcommand_name_var {
+                    return ::std::result::Result::Ok(#name :: #variant_name #constructor_block)
+                }
             }
         }
     });

src/build/command.rs

@@ -2709,11 +2709,16 @@ impl<'help> App<'help> {
 
     /// Assume unexpected positional arguments are a [`subcommand`].
     ///
+    /// Arguments will be stored in the `""` argument in the [`ArgMatches`]
+    ///
     /// **NOTE:** Use this setting with caution,
     /// as a truly unexpected argument (i.e. one that is *NOT* an external subcommand)
     /// will **not** cause an error and instead be treated as a potential subcommand.
     /// One should check for such cases manually and inform the user appropriately.
     ///
+    /// **NOTE:** A built-in subcommand will be parsed as an external subcommand when escaped with
+    /// `--`.
+    ///
     /// # Examples
     ///
     /// ```rust

src/parse/arg_matcher.rs

@@ -30,7 +30,11 @@ impl ArgMatcher {
             //
             // See clap-rs/clap#3263
             #[cfg(debug_assertions)]
+            #[cfg(not(feature = "unstable-v4"))]
             disable_asserts: _cmd.is_allow_external_subcommands_set(),
+            #[cfg(debug_assertions)]
+            #[cfg(feature = "unstable-v4")]
+            disable_asserts: false,
             ..Default::default()
         })
     }
@@ -85,6 +89,7 @@ impl ArgMatcher {
         }
     }
 
+    #[cfg(not(feature = "unstable-v4"))]
     pub(crate) fn get_mut(&mut self, arg: &Id) -> Option<&mut MatchedArg> {
         self.0.args.get_mut(arg)
     }
@@ -142,6 +147,19 @@ impl ArgMatcher {
         ma.inc_occurrences();
     }
 
+    #[cfg(feature = "unstable-v4")]
+    pub(crate) fn inc_occurrence_of_external(&mut self, allow_invalid_utf8: bool) {
+        let id = &Id::empty_hash();
+        debug!(
+            "ArgMatcher::inc_occurrence_of_external: id={:?}, allow_invalid_utf8={}",
+            id, allow_invalid_utf8
+        );
+        let ma = self.entry(id).or_insert(MatchedArg::new());
+        ma.update_ty(ValueSource::CommandLine);
+        ma.invalid_utf8_allowed(allow_invalid_utf8);
+        ma.inc_occurrences();
+    }
+
     pub(crate) fn add_val_to(&mut self, arg: &Id, val: OsString, ty: ValueSource, append: bool) {
         if append {
             self.append_val_to(arg, val, ty);

src/parse/parser.rs

@@ -428,26 +428,34 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
 
                 // Collect the external subcommand args
                 let mut sc_m = ArgMatcher::new(self.cmd);
+                let allow_invalid_utf8 = self
+                    .cmd
+                    .is_allow_invalid_utf8_for_external_subcommands_set();
+                #[cfg(feature = "unstable-v4")]
+                {
+                    sc_m.inc_occurrence_of_external(allow_invalid_utf8);
+                }
 
                 for v in raw_args.remaining(&mut args_cursor) {
-                    let allow_invalid_utf8 = self
-                        .cmd
-                        .is_allow_invalid_utf8_for_external_subcommands_set();
                     if !allow_invalid_utf8 && v.to_str().is_none() {
                         return Err(ClapError::invalid_utf8(
                             self.cmd,
                             Usage::new(self.cmd).create_usage_with_title(&[]),
                         ));
                     }
+                    let external_id = &Id::empty_hash();
                     sc_m.add_val_to(
-                        &Id::empty_hash(),
+                        external_id,
                         v.to_os_string(),
                         ValueSource::CommandLine,
                         false,
                     );
-                    sc_m.get_mut(&Id::empty_hash())
-                        .expect("just inserted")
-                        .invalid_utf8_allowed(allow_invalid_utf8);
+                    #[cfg(not(feature = "unstable-v4"))]
+                    {
+                        sc_m.get_mut(external_id)
+                            .expect("just inserted")
+                            .invalid_utf8_allowed(allow_invalid_utf8);
+                    }
                 }
 
                 matcher.subcommand(SubCommand {

tests/builder/app_settings.rs

@@ -851,6 +851,46 @@ fn issue_1093_allow_ext_sc() {
     utils::assert_output(cmd, "clap-test --help", ALLOW_EXT_SC, false);
 }
 
+#[test]
+#[cfg(not(feature = "unstable-v4"))]
+fn allow_ext_sc_empty_args() {
+    let res = Command::new("clap-test")
+        .version("v1.4.8")
+        .allow_external_subcommands(true)
+        .allow_invalid_utf8_for_external_subcommands(true)
+        .try_get_matches_from(vec!["clap-test", "external-cmd"]);
+
+    assert!(res.is_ok(), "{}", res.unwrap_err());
+
+    match res.unwrap().subcommand() {
+        Some((name, args)) => {
+            assert_eq!(name, "external-cmd");
+            assert_eq!(args.values_of_lossy(""), None);
+        }
+        _ => unreachable!(),
+    }
+}
+
+#[test]
+#[cfg(feature = "unstable-v4")]
+fn allow_ext_sc_empty_args() {
+    let res = Command::new("clap-test")
+        .version("v1.4.8")
+        .allow_external_subcommands(true)
+        .allow_invalid_utf8_for_external_subcommands(true)
+        .try_get_matches_from(vec!["clap-test", "external-cmd"]);
+
+    assert!(res.is_ok(), "{}", res.unwrap_err());
+
+    match res.unwrap().subcommand() {
+        Some((name, args)) => {
+            assert_eq!(name, "external-cmd");
+            assert_eq!(args.values_of_lossy(""), Some(vec![]));
+        }
+        _ => unreachable!(),
+    }
+}
+
 #[test]
 fn allow_ext_sc_when_sc_required() {
     let res = Command::new("clap-test")
@@ -890,6 +930,25 @@ fn external_subcommand_looks_like_built_in() {
     }
 }
 
+#[test]
+fn built_in_subcommand_escaped() {
+    let res = Command::new("cargo")
+        .version("1.26.0")
+        .allow_external_subcommands(true)
+        .allow_invalid_utf8_for_external_subcommands(true)
+        .subcommand(Command::new("install"))
+        .try_get_matches_from(vec!["cargo", "--", "install", "foo"]);
+    assert!(res.is_ok(), "{}", res.unwrap_err());
+    let m = res.unwrap();
+    match m.subcommand() {
+        Some((name, args)) => {
+            assert_eq!(name, "install");
+            assert_eq!(args.values_of_lossy(""), Some(vec!["foo".to_string()]));
+        }
+        _ => panic!("external_subcommand didn't work"),
+    }
+}
+
 #[test]
 fn aaos_flags() {
     // flags

tests/derive/subcommands.rs

@@ -547,3 +547,59 @@ fn skip_subcommand() {
     let res = Opt::try_parse_from(&["test", "skip"]);
     assert_eq!(res.unwrap_err().kind(), clap::ErrorKind::UnknownArgument,);
 }
+
+#[test]
+#[cfg(feature = "unstable-v4")]
+fn built_in_subcommand_escaped() {
+    #[derive(Debug, PartialEq, Parser)]
+    enum Command {
+        Install {
+            arg: Option<String>,
+        },
+        #[clap(external_subcommand)]
+        Custom(Vec<String>),
+    }
+
+    assert_eq!(
+        Command::try_parse_from(&["test", "install", "arg"]).unwrap(),
+        Command::Install {
+            arg: Some(String::from("arg"))
+        }
+    );
+    assert_eq!(
+        Command::try_parse_from(&["test", "--", "install"]).unwrap(),
+        Command::Custom(vec![String::from("install")])
+    );
+    assert_eq!(
+        Command::try_parse_from(&["test", "--", "install", "arg"]).unwrap(),
+        Command::Custom(vec![String::from("install"), String::from("arg")])
+    );
+}
+
+#[test]
+#[cfg(not(feature = "unstable-v4"))]
+fn built_in_subcommand_escaped() {
+    #[derive(Debug, PartialEq, Parser)]
+    enum Command {
+        Install {
+            arg: Option<String>,
+        },
+        #[clap(external_subcommand)]
+        Custom(Vec<String>),
+    }
+
+    assert_eq!(
+        Command::try_parse_from(&["test", "install", "arg"]).unwrap(),
+        Command::Install {
+            arg: Some(String::from("arg"))
+        }
+    );
+    assert_eq!(
+        Command::try_parse_from(&["test", "--", "install"]).unwrap(),
+        Command::Install { arg: None }
+    );
+    assert_eq!(
+        Command::try_parse_from(&["test", "--", "install", "arg"]).unwrap(),
+        Command::Install { arg: None }
+    );
+}