fix!: Use value parsers for external subcommands

This changes the default type as well to encourage preserving the full
information for shelling out.  If people need UTF-8, then they can
change the value parser.

Fixes #3733

CHANGELOG.md

@@ -35,6 +35,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Leading dashes in `Arg::long` are no longer allowed (#3691)
 - Verify `required` is not used with conditional required settings (#3660)
 - Disallow more `value_names` than `number_of_values` (#2695)
+- Replaced `cmd.allow_invalid_for_utf8_external_subcommands` with `cmd.external_subcommand_value_parser` (#3733)
+- Changed the default type of `allow_external_subcommands` from `String` to `OsString`
 - *(assert)* Always enforce that version is specified when the `ArgAction::Version` is used
 - *(assert)* Add missing `#[track_caller]`s to make it easier to debug asserts
 - *(assert)* Ensure `overrides_with` IDs are valid

clap_derive/src/derives/subcommand.rs

@@ -171,14 +171,8 @@ fn gen_augment(
                     };
                     let subcommand = match subty_if_name(ty, "Vec") {
                         Some(subty) => {
-                            if is_simple_ty(subty, "OsString") {
+                            quote_spanned! { kind.span()=>
-                                quote_spanned! { kind.span()=>
+                                let #app_var = #app_var.external_subcommand_value_parser(clap::value_parser!(#subty));
-                                    let #app_var = #app_var.allow_external_subcommands(true).allow_invalid_utf8_for_external_subcommands(true);
-                                }
-                            } else {
-                                quote_spanned! { kind.span()=>
-                                    let #app_var = #app_var.allow_external_subcommands(true);
-                                }
                             }
                         }
 

examples/git.rs

@@ -9,7 +9,6 @@ fn cli() -> Command<'static> {
         .subcommand_required(true)
         .arg_required_else_help(true)
         .allow_external_subcommands(true)
-        .allow_invalid_utf8_for_external_subcommands(true)
         .subcommand(
             Command::new("clone")
                 .about("Clones repos")

src/builder/app_settings.rs

@@ -41,7 +41,6 @@ pub(crate) enum AppSettings {
     SubcommandRequired,
     AllowExternalSubcommands,
     Multicall,
-    AllowInvalidUtf8ForExternalSubcommands,
     SubcommandsNegateReqs,
     ArgsNegateSubcommands,
     SubcommandPrecedenceOverArg,
@@ -78,7 +77,6 @@ bitflags! {
         const TRAILING_VARARG                = 1 << 12;
         const NO_BIN_NAME                    = 1 << 13;
         const ALLOW_UNK_SC                   = 1 << 14;
-        const SC_UTF8_NONE                   = 1 << 15;
         const LEADING_HYPHEN                 = 1 << 16;
         const NO_POS_VALUES                  = 1 << 17;
         const NEXT_LINE_HELP                 = 1 << 18;
@@ -118,8 +116,6 @@ impl_settings! { AppSettings, AppFlags,
         => Flags::ARGS_NEGATE_SCS,
     AllowExternalSubcommands
         => Flags::ALLOW_UNK_SC,
-    AllowInvalidUtf8ForExternalSubcommands
-        => Flags::SC_UTF8_NONE,
     AllowHyphenValues
         => Flags::LEADING_HYPHEN,
     AllowNegativeNumbers

src/builder/command.rs

@@ -100,6 +100,7 @@ pub struct Command<'help> {
     current_disp_ord: Option<usize>,
     subcommand_value_name: Option<&'help str>,
     subcommand_heading: Option<&'help str>,
+    external_value_parser: Option<super::ValueParser>,
 }
 
 /// # Basic API
@@ -2673,6 +2674,7 @@ impl<'help> Command<'help> {
     /// # Examples
     ///
     /// ```rust
+    /// # use std::ffi::OsString;
     /// # use clap::Command;
     /// // Assume there is an external subcommand named "subcmd"
     /// let m = Command::new("myprog")
@@ -2685,7 +2687,7 @@ impl<'help> Command<'help> {
     /// // string argument name
     /// match m.subcommand() {
     ///     Some((external, ext_m)) => {
-    ///          let ext_args: Vec<_> = ext_m.get_many::<String>("").unwrap().collect();
+    ///          let ext_args: Vec<_> = ext_m.get_many::<OsString>("").unwrap().collect();
     ///          assert_eq!(external, "subcmd");
     ///          assert_eq!(ext_args, ["--option", "value", "-fff", "--flag"]);
     ///     },
@@ -2704,27 +2706,22 @@ impl<'help> Command<'help> {
         }
     }
 
-    /// Specifies that external subcommands that are invalid UTF-8 should *not* be treated as an error.
+    /// Specifies how to parse external subcommand arguments.
     ///
-    /// **NOTE:** Using external subcommand argument values with invalid UTF-8 requires using
+    /// The default parser is for `OsString`.  This can be used to switch it to `String` or another
-    /// [`ArgMatches::get_many::<OsString>`][crate::ArgMatches::get_many] for those particular
+    /// type.
-    /// arguments which may contain invalid UTF-8 values
     ///
     /// **NOTE:** Setting this requires [`Command::allow_external_subcommands`]
     ///
-    /// # Platform Specific
-    ///
-    /// Non Windows systems only
-    ///
     /// # Examples
     ///
     #[cfg_attr(not(unix), doc = " ```ignore")]
     #[cfg_attr(unix, doc = " ```")]
     /// # use std::ffi::OsString;
     /// # use clap::Command;
+    /// # use clap::value_parser;
     /// // Assume there is an external subcommand named "subcmd"
     /// let m = Command::new("myprog")
-    ///     .allow_invalid_utf8_for_external_subcommands(true)
     ///     .allow_external_subcommands(true)
     ///     .get_matches_from(vec![
     ///         "myprog", "subcmd", "--option", "value", "-fff", "--flag"
@@ -2742,13 +2739,35 @@ impl<'help> Command<'help> {
     /// }
     /// ```
     ///
+    /// ```
+    /// # use clap::Command;
+    /// # use clap::value_parser;
+    /// // Assume there is an external subcommand named "subcmd"
+    /// let m = Command::new("myprog")
+    ///     .external_subcommand_value_parser(value_parser!(String))
+    ///     .get_matches_from(vec![
+    ///         "myprog", "subcmd", "--option", "value", "-fff", "--flag"
+    ///     ]);
+    ///
+    /// // All trailing arguments will be stored under the subcommand's sub-matches using an empty
+    /// // string argument name
+    /// match m.subcommand() {
+    ///     Some((external, ext_m)) => {
+    ///          let ext_args: Vec<_> = ext_m.get_many::<String>("").unwrap().collect();
+    ///          assert_eq!(external, "subcmd");
+    ///          assert_eq!(ext_args, ["--option", "value", "-fff", "--flag"]);
+    ///     },
+    ///     _ => {},
+    /// }
+    /// ```
+    ///
     /// [`subcommands`]: crate::Command::subcommand()
-    pub fn allow_invalid_utf8_for_external_subcommands(self, yes: bool) -> Self {
+    pub fn external_subcommand_value_parser(
-        if yes {
+        mut self,
-            self.setting(AppSettings::AllowInvalidUtf8ForExternalSubcommands)
+        parser: impl Into<super::ValueParser>,
-        } else {
+    ) -> Self {
-            self.unset_setting(AppSettings::AllowInvalidUtf8ForExternalSubcommands)
+        self.external_value_parser = Some(parser.into());
-        }
+        self
     }
 
     /// Specifies that use of an argument prevents the use of [`subcommands`].
@@ -3604,31 +3623,22 @@ impl<'help> Command<'help> {
         self.is_set(AppSettings::AllowExternalSubcommands)
     }
 
-    /// Report whether [`Command::allow_invalid_utf8_for_external_subcommands`] is set
-    pub fn is_allow_invalid_utf8_for_external_subcommands_set(&self) -> bool {
-        self.is_set(AppSettings::AllowInvalidUtf8ForExternalSubcommands)
-    }
-
     /// Configured parser for values passed to an external subcommand
     ///
     /// # Example
     ///
     /// ```rust
     /// let cmd = clap::Command::new("raw")
-    ///     .allow_external_subcommands(true)
+    ///     .external_subcommand_value_parser(clap::value_parser!(String));
-    ///     .allow_invalid_utf8_for_external_subcommands(true);
     /// let value_parser = cmd.get_external_subcommand_value_parser();
     /// println!("{:?}", value_parser);
     /// ```
     pub fn get_external_subcommand_value_parser(&self) -> Option<&super::ValueParser> {
         if !self.is_allow_external_subcommands_set() {
             None
-        } else if self.is_allow_invalid_utf8_for_external_subcommands_set() {
-            static DEFAULT: super::ValueParser = super::ValueParser::os_string();
-            Some(&DEFAULT)
         } else {
-            static DEFAULT: super::ValueParser = super::ValueParser::string();
+            static DEFAULT: super::ValueParser = super::ValueParser::os_string();
-            Some(&DEFAULT)
+            Some(self.external_value_parser.as_ref().unwrap_or(&DEFAULT))
         }
     }
 
@@ -3763,6 +3773,10 @@ impl<'help> Command<'help> {
                 self.settings
                     .insert(AppSettings::SubcommandsNegateReqs.into());
             }
+            if self.external_value_parser.is_some() {
+                self.settings
+                    .insert(AppSettings::AllowExternalSubcommands.into());
+            }
 
             self._propagate();
             self._check_help_and_version();
@@ -4595,6 +4609,7 @@ impl<'help> Default for Command<'help> {
             current_disp_ord: Some(0),
             subcommand_value_name: Default::default(),
             subcommand_heading: Default::default(),
+            external_value_parser: Default::default(),
         }
     }
 }

src/builder/debug_asserts.rs

@@ -460,7 +460,6 @@ fn assert_app_flags(cmd: &Command) {
         };
     }
 
-    checker!(is_allow_invalid_utf8_for_external_subcommands_set requires is_allow_external_subcommands_set);
     checker!(is_multicall_set conflicts is_no_binary_name_set);
 }
 

src/error/kind.rs

@@ -211,7 +211,7 @@ pub enum ErrorKind {
     ///
     /// To allow arbitrary data
     /// - Set [`Arg::value_parser(value_parser!(OsString))`] for argument values
-    /// - Set [`Command::allow_invalid_utf8_for_external_subcommands`] for external-subcommand
+    /// - Set [`Command::external_subcommand_value_parser`] for external-subcommand
     ///   values
     ///
     /// # Platform Specific
@@ -237,7 +237,7 @@ pub enum ErrorKind {
     /// ```
     ///
     /// [`Arg::allow_invalid_utf8`]: crate::Arg::allow_invalid_utf8
-    /// [`Command::allow_invalid_utf8_for_external_subcommands`]: crate::Command::allow_invalid_utf8_for_external_subcommands
+    /// [`Command::external_subcommand_value_parser`]: crate::Command::external_subcommand_value_parser
     InvalidUtf8,
 
     /// Not a true "error" as it means `--help` or similar was used.

src/parser/matches/arg_matches.rs

@@ -705,6 +705,8 @@ impl ArgMatches {
     /// with pattern matching!
     ///
     /// ```rust
+    /// # use std::ffi::OsString;
+    /// # use std::ffi::OsStr;
     /// # use clap::Command;
     /// // Assume there is an external subcommand named "subcmd"
     /// let app_m = Command::new("myprog")
@@ -717,8 +719,8 @@ impl ArgMatches {
     /// // string argument name
     /// match app_m.subcommand() {
     ///     Some((external, sub_m)) => {
-    ///          let ext_args: Vec<&str> = sub_m.get_many::<String>("")
+    ///          let ext_args: Vec<&OsStr> = sub_m.get_many::<OsString>("")
-    ///             .unwrap().map(|s| s.as_str()).collect();
+    ///             .unwrap().map(|s| s.as_os_str()).collect();
     ///          assert_eq!(external, "subcmd");
     ///          assert_eq!(ext_args, ["--option", "value", "-fff", "--flag"]);
     ///     },
@@ -762,6 +764,7 @@ impl ArgMatches {
     /// with pattern matching!
     ///
     /// ```rust
+    /// # use std::ffi::OsString;
     /// # use clap::Command;
     /// // Assume there is an external subcommand named "subcmd"
     /// let mut app_m = Command::new("myprog")
@@ -774,7 +777,7 @@ impl ArgMatches {
     /// // string argument name
     /// match app_m.remove_subcommand() {
     ///     Some((external, mut sub_m)) => {
-    ///          let ext_args: Vec<String> = sub_m.remove_many("")
+    ///          let ext_args: Vec<OsString> = sub_m.remove_many("")
     ///             .expect("`file`is required")
     ///             .collect();
     ///          assert_eq!(external, "subcmd");

tests/builder/app_settings.rs

@@ -849,7 +849,6 @@ 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());
@@ -871,7 +870,6 @@ fn allow_ext_sc_when_sc_required() {
     let res = Command::new("clap-test")
         .version("v1.4.8")
         .allow_external_subcommands(true)
-        .allow_invalid_utf8_for_external_subcommands(true)
         .subcommand_required(true)
         .try_get_matches_from(vec!["clap-test", "external-cmd", "foo"]);
 
@@ -897,7 +895,6 @@ fn external_subcommand_looks_like_built_in() {
     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-update", "foo"]);
     assert!(res.is_ok(), "{}", res.unwrap_err());
@@ -922,7 +919,6 @@ 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());

tests/builder/utf8.rs

@@ -210,6 +210,7 @@ fn invalid_utf8_option_long_equals() {
 fn refuse_invalid_utf8_subcommand_with_allow_external_subcommands() {
     let m = Command::new("bad_utf8")
         .allow_external_subcommands(true)
+        .external_subcommand_value_parser(value_parser!(String))
         .try_get_matches_from(vec![
             OsString::from(""),
             OsString::from_vec(vec![0xe9]),
@@ -223,7 +224,6 @@ fn refuse_invalid_utf8_subcommand_with_allow_external_subcommands() {
 fn refuse_invalid_utf8_subcommand_when_args_are_allowed_with_allow_external_subcommands() {
     let m = Command::new("bad_utf8")
         .allow_external_subcommands(true)
-        .allow_invalid_utf8_for_external_subcommands(true)
         .try_get_matches_from(vec![
             OsString::from(""),
             OsString::from_vec(vec![0xe9]),
@@ -237,6 +237,7 @@ fn refuse_invalid_utf8_subcommand_when_args_are_allowed_with_allow_external_subc
 fn refuse_invalid_utf8_subcommand_args_with_allow_external_subcommands() {
     let m = Command::new("bad_utf8")
         .allow_external_subcommands(true)
+        .external_subcommand_value_parser(value_parser!(String))
         .try_get_matches_from(vec![
             OsString::from(""),
             OsString::from("subcommand"),
@@ -252,7 +253,6 @@ fn refuse_invalid_utf8_subcommand_args_with_allow_external_subcommands() {
 fn allow_invalid_utf8_subcommand_args_with_allow_external_subcommands() {
     let m = Command::new("bad_utf8")
         .allow_external_subcommands(true)
-        .allow_invalid_utf8_for_external_subcommands(true)
         .try_get_matches_from(vec![
             OsString::from(""),
             OsString::from("subcommand"),
@@ -289,7 +289,9 @@ fn allow_validated_utf8_value_of() {
 
 #[test]
 fn allow_validated_utf8_external_subcommand_values_of() {
-    let a = Command::new("test").allow_external_subcommands(true);
+    let a = Command::new("test")
+        .allow_external_subcommands(true)
+        .external_subcommand_value_parser(value_parser!(String));
     let m = a.try_get_matches_from(vec!["test", "cmd", "arg"]).unwrap();
     let (_ext, args) = m.subcommand().unwrap();
     args.get_many::<String>("").unwrap_or_default().count();
@@ -298,7 +300,9 @@ fn allow_validated_utf8_external_subcommand_values_of() {
 #[test]
 #[should_panic = "Mismatch between definition and access of ``. Could not downcast to std::ffi::os_str::OsString, need to downcast to alloc::string::String"]
 fn panic_validated_utf8_external_subcommand_values_of_os() {
-    let a = Command::new("test").allow_external_subcommands(true);
+    let a = Command::new("test")
+        .allow_external_subcommands(true)
+        .external_subcommand_value_parser(value_parser!(String));
     let m = a.try_get_matches_from(vec!["test", "cmd", "arg"]).unwrap();
     let (_ext, args) = m.subcommand().unwrap();
     args.get_many::<OsString>("").unwrap_or_default().count();
@@ -306,9 +310,7 @@ fn panic_validated_utf8_external_subcommand_values_of_os() {
 
 #[test]
 fn allow_invalid_utf8_external_subcommand_values_of_os() {
-    let a = Command::new("test")
+    let a = Command::new("test").allow_external_subcommands(true);
-        .allow_external_subcommands(true)
-        .allow_invalid_utf8_for_external_subcommands(true);
     let m = a.try_get_matches_from(vec!["test", "cmd", "arg"]).unwrap();
     let (_ext, args) = m.subcommand().unwrap();
     args.get_many::<OsString>("").unwrap_or_default().count();
@@ -317,9 +319,7 @@ fn allow_invalid_utf8_external_subcommand_values_of_os() {
 #[test]
 #[should_panic = "Mismatch between definition and access of ``. Could not downcast to alloc::string::String, need to downcast to std::ffi::os_str::OsString"]
 fn panic_invalid_utf8_external_subcommand_values_of() {
-    let a = Command::new("test")
+    let a = Command::new("test").allow_external_subcommands(true);
-        .allow_external_subcommands(true)
-        .allow_invalid_utf8_for_external_subcommands(true);
     let m = a.try_get_matches_from(vec!["test", "cmd", "arg"]).unwrap();
     let (_ext, args) = m.subcommand().unwrap();
     args.get_many::<String>("").unwrap_or_default().count();