Repository: clap-rs/clap (rust)

Issue:
Implement `#[clap(external_subcommand)]`
I'm not exporting it directly from structopt because it doesn't seem to work, but I'll get to it later.

Structopt issue: https://github.com/TeXitoi/structopt/issues/130

Chosen implementation:
```rust
enum Opt {
  #[clap(name = "known")]
  KnownCommand {
    // …
  },

  #[clap(external_subcommand)]
  Other(Vec<String>),
}
```

Pull request description:
External subcommand
<!--
If your PR closes some issues, please write `Closes #XXXX`
where `XXXX` is the number of the issue you want to fix.
Each issue goes on it's own line.
-->
Closes #1672 

This is _almost_ done, but blocked on the undetected bug in 3.0: if both `AllowExternalSubcommands` and `SubcommandRequiredElseHelp` are set, externals subcommand doesn't count and exit triggers. Will get to it tomorrow.

Patch:
diff --git a/benches/05_ripgrep.rs b/benches/05_ripgrep.rs
index 7a5f2858..6fa12e77 100644
--- a/benches/05_ripgrep.rs
+++ b/benches/05_ripgrep.rs
@@ -934,7 +934,7 @@ lazy_static! {
     };
 }
 
-fn validate_number(s: &str) -> Result<(), String> {
+fn validate_number(s: String) -> Result<(), String> {
     s.parse::<usize>()
         .map(|_| ())
         .map_err(|err| err.to_string())
diff --git a/clap_derive/src/derives/attrs.rs b/clap_derive/src/attrs.rs
similarity index 97%
rename from clap_derive/src/derives/attrs.rs
rename to clap_derive/src/attrs.rs
index 2220da16..860fcc2f 100644
--- a/clap_derive/src/derives/attrs.rs
+++ b/clap_derive/src/attrs.rs
@@ -12,7 +12,10 @@
 // commit#ea76fa1b1b273e65e3b0b1046643715b49bec51f which is licensed under the
 // MIT/Apache 2.0 license.
 
-use super::{doc_comments::process_doc_comment, parse::*, spanned::Sp, ty::Ty};
+use crate::{
+    parse::*,
+    utils::{process_doc_comment, Sp, Ty},
+};
 
 use std::env;
 
@@ -38,6 +41,7 @@ pub enum Kind {
     Subcommand(Sp<Ty>),
     Flatten,
     Skip(Option<Expr>),
+    ExternalSubcommand,
 }
 
 #[derive(Clone)]
@@ -296,6 +300,11 @@ impl Attrs {
                     self.set_kind(kind);
                 }
 
+                ExternalSubcommand(ident) => {
+                    let kind = Sp::new(Kind::ExternalSubcommand, ident.span());
+                    self.set_kind(kind);
+                }
+
                 Flatten(ident) => {
                     let kind = Sp::new(Kind::Flatten, ident.span());
                     self.set_kind(kind);
@@ -422,7 +431,7 @@ impl Attrs {
         match &*res.kind {
             Kind::Subcommand(_) => abort!(res.kind.span(), "subcommand is only allowed on fields"),
             Kind::Skip(_) => abort!(res.kind.span(), "skip is only allowed on fields"),
-            Kind::Arg(_) | Kind::Flatten => res,
+            Kind::Arg(_) | Kind::Flatten | Kind::ExternalSubcommand => res,
         }
     }
 
@@ -457,6 +466,13 @@ impl Attrs {
                     );
                 }
             }
+
+            Kind::ExternalSubcommand => {
+                abort! { res.kind.span(),
+                    "`external_subcommand` can be used only on enum variants"
+                }
+            }
+
             Kind::Subcommand(_) => {
                 if res.has_custom_parser {
                     abort!(
@@ -568,7 +584,7 @@ impl Attrs {
         } else {
             abort!(
                 kind.span(),
-                "subcommand, flatten and skip cannot be used together"
+                "`subcommand`, `flatten`, `external_subcommand` and `skip` cannot be used together"
             );
         }
     }
diff --git a/clap_derive/src/derives/arg_enum.rs b/clap_derive/src/derives/arg_enum.rs
index 88f021a8..2b3c1f70 100644
--- a/clap_derive/src/derives/arg_enum.rs
+++ b/clap_derive/src/derives/arg_enum.rs
@@ -7,8 +7,11 @@
 // <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
-use crate::derives::attrs::{Attrs, Name, DEFAULT_CASING, DEFAULT_ENV_CASING};
-use crate::derives::spanned::Sp;
+
+use crate::{
+    attrs::{Attrs, Name, DEFAULT_CASING, DEFAULT_ENV_CASING},
+    utils::Sp,
+};
 
 use proc_macro2::{Span, TokenStream};
 use quote::quote;
diff --git a/clap_derive/src/derives/clap.rs b/clap_derive/src/derives/clap.rs
index b16163e8..9c454e11 100644
--- a/clap_derive/src/derives/clap.rs
+++ b/clap_derive/src/derives/clap.rs
@@ -12,7 +12,11 @@
 // commit#ea76fa1b1b273e65e3b0b1046643715b49bec51f which is licensed under the
 // MIT/Apache 2.0 license.
 
-use super::{arg_enum, dummies, from_argmatches, into_app, subcommand};
+use crate::{
+    derives::{arg_enum, from_argmatches, into_app, subcommand},
+    dummies,
+};
+
 use proc_macro2::TokenStream;
 use proc_macro_error::abort_call_site;
 use quote::quote;
diff --git a/clap_derive/src/derives/from_argmatches.rs b/clap_derive/src/derives/from_argmatches.rs
index df7b2713..06dc125d 100644
--- a/clap_derive/src/derives/from_argmatches.rs
+++ b/clap_derive/src/derives/from_argmatches.rs
@@ -12,10 +12,14 @@
 // commit#ea76fa1b1b273e65e3b0b1046643715b49bec51f which is licensed under the
 // MIT/Apache 2.0 license.
 use proc_macro2::TokenStream;
+use proc_macro_error::abort;
 use quote::{quote, quote_spanned};
 use syn::{punctuated::Punctuated, spanned::Spanned, Field, Ident, Token, Type};
 
-use super::{sub_type, subty_if_name, Attrs, Kind, ParserKind, Ty};
+use crate::{
+    attrs::{Attrs, Kind, ParserKind},
+    utils::{sub_type, subty_if_name, Ty},
+};
 
 pub fn gen_for_struct(
     struct_name: &Ident,
@@ -89,7 +93,12 @@ pub fn gen_constructor(
         );
         let field_name = field.ident.as_ref().unwrap();
         let kind = attrs.kind();
-        match &*attrs.kind() {
+        match &*kind {
+            Kind::ExternalSubcommand => {
+                abort! { kind.span(),
+                    "`external_subcommand` can be used only on enum variants"
+                }
+            }
             Kind::Subcommand(ty) => {
                 let subcmd_type = match (**ty, sub_type(&field.ty)) {
                     (Ty::Option, Some(sub_type)) => sub_type,
diff --git a/clap_derive/src/derives/into_app.rs b/clap_derive/src/derives/into_app.rs
index 38a64204..b5aecdb7 100644
--- a/clap_derive/src/derives/into_app.rs
+++ b/clap_derive/src/derives/into_app.rs
@@ -19,9 +19,9 @@ use proc_macro_error::abort;
 use quote::{quote, quote_spanned};
 use syn::{punctuated::Punctuated, spanned::Spanned, Attribute, Field, Ident, Token, Type};
 
-use super::{
-    spanned::Sp, sub_type, subty_if_name, ty::Ty, Attrs, GenOutput, Kind, Name, ParserKind,
-    DEFAULT_CASING, DEFAULT_ENV_CASING,
+use crate::{
+    attrs::{Attrs, GenOutput, Kind, Name, ParserKind, DEFAULT_CASING, DEFAULT_ENV_CASING},
+    utils::{sub_type, subty_if_name, Sp, Ty},
 };
 
 pub fn gen_for_struct(
@@ -179,7 +179,7 @@ pub fn gen_app_augmentation(
         );
         let kind = attrs.kind();
         match &*kind {
-            Kind::Subcommand(_) | Kind::Skip(_) => None,
+            Kind::Subcommand(_) | Kind::Skip(_) | Kind::ExternalSubcommand => None,
             Kind::Flatten => {
                 let ty = &field.ty;
                 Some(quote_spanned! { kind.span()=>
@@ -205,13 +205,13 @@ pub fn gen_app_augmentation(
                     _ if attrs.is_enum() => quote!(),
                     ParserKind::TryFromStr => quote_spanned! { func.span()=>
                         .validator(|s| {
-                            #func(s)
+                            #func(s.as_str())
                             .map(|_: #convert_type| ())
                             .map_err(|e| e.to_string())
                         })
                     },
                     ParserKind::TryFromOsStr => quote_spanned! { func.span()=>
-                        .validator_os(|s| #func(s).map(|_: #convert_type| ()))
+                        .validator_os(|s| #func(&s).map(|_: #convert_type| ()))
                     },
                     _ => quote!(),
                 };
diff --git a/clap_derive/src/derives/mod.rs b/clap_derive/src/derives/mod.rs
index e3afdc33..8fe7aed2 100644
--- a/clap_derive/src/derives/mod.rs
+++ b/clap_derive/src/derives/mod.rs
@@ -12,20 +12,9 @@
 // commit#ea76fa1b1b273e65e3b0b1046643715b49bec51f which is licensed under the
 // MIT/Apache 2.0 license.
 mod arg_enum;
-pub mod attrs;
 mod clap;
-mod doc_comments;
-mod dummies;
 mod from_argmatches;
 mod into_app;
-pub mod parse;
-pub mod spanned;
 mod subcommand;
-pub mod ty;
 
-pub use self::attrs::{
-    Attrs, CasingStyle, GenOutput, Kind, Name, Parser, ParserKind, DEFAULT_CASING,
-    DEFAULT_ENV_CASING,
-};
 pub use self::clap::derive_clap;
-pub use self::ty::{sub_type, subty_if_name, Ty};
diff --git a/clap_derive/src/derives/subcommand.rs b/clap_derive/src/derives/subcommand.rs
index 739bfc73..ccee7c6c 100644
--- a/clap_derive/src/derives/subcommand.rs
+++ b/clap_derive/src/derives/subcommand.rs
@@ -1,5 +1,8 @@
-use crate::derives::attrs::{Attrs, Kind, Name, DEFAULT_CASING, DEFAULT_ENV_CASING};
-use crate::derives::{from_argmatches, into_app, spanned::Sp};
+use crate::{
+    attrs::{Attrs, Kind, Name, DEFAULT_CASING, DEFAULT_ENV_CASING},
+    derives::{from_argmatches, into_app},
+    utils::{is_simple_ty, subty_if_name, Sp},
+};
 
 use proc_macro2::{Ident, Span, TokenStream};
 use proc_macro_error::{abort, abort_call_site};
@@ -46,62 +49,72 @@ fn gen_augment_subcommands(
 ) -> TokenStream {
     use syn::Fields::*;
 
-    let subcommands = variants.iter().map(|variant| {
-        let attrs = Attrs::from_struct(
-            variant.span(),
-            &variant.attrs,
-            Name::Derived(variant.ident.clone()),
-            parent_attribute.casing(),
-            parent_attribute.env_casing(),
-        );
-        let kind = attrs.kind();
-        match &*kind {
-            Kind::Flatten => match variant.fields {
-                Unnamed(FieldsUnnamed { ref unnamed, .. }) if unnamed.len() == 1 => {
-                    let ty = &unnamed[0];
-                    quote! {
-                        let app = <#ty as ::clap::Subcommand>::augment_subcommands(app);
+    let subcommands: Vec<_> = variants
+        .iter()
+        .map(|variant| {
+            let attrs = Attrs::from_struct(
+                variant.span(),
+                &variant.attrs,
+                Name::Derived(variant.ident.clone()),
+                parent_attribute.casing(),
+                parent_attribute.env_casing(),
+            );
+            let kind = attrs.kind();
+
+            match &*kind {
+                Kind::ExternalSubcommand => {
+                    quote_spanned! { kind.span()=>
+                        let app = app.setting(::clap::AppSettings::AllowExternalSubcommands);
                     }
                 }
-                _ => abort!(
-                    variant,
-                    "`flatten` is usable only with single-typed tuple variants"
-                ),
-            },
-
-            _ => {
-                let app_var = Ident::new("subcommand", Span::call_site());
-                let arg_block = match variant.fields {
-                    Named(ref fields) => {
-                        into_app::gen_app_augmentation(&fields.named, &app_var, &attrs)
-                    }
-                    Unit => quote!( #app_var ),
+
+                Kind::Flatten => match variant.fields {
                     Unnamed(FieldsUnnamed { ref unnamed, .. }) if unnamed.len() == 1 => {
                         let ty = &unnamed[0];
-                        quote_spanned! { ty.span()=>
-                            {
-                                <#ty as ::clap::IntoApp>::augment_clap(#app_var)
-                            }
+                        quote! {
+                            let app = <#ty as ::clap::Subcommand>::augment_subcommands(app);
                         }
                     }
-                    Unnamed(..) => {
-                        abort!(variant, "non single-typed tuple enums are not supported")
-                    }
-                };
+                    _ => abort!(
+                        variant,
+                        "`flatten` is usable only with single-typed tuple variants"
+                    ),
+                },
 
-                let name = attrs.cased_name();
-                let from_attrs = attrs.top_level_methods();
-                let version = attrs.version();
-                quote! {
-                    let app = app.subcommand({
-                        let #app_var = ::clap::App::new(#name);
-                        let #app_var = #arg_block;
-                        #app_var#from_attrs#version
-                    });
+                _ => {
+                    let app_var = Ident::new("subcommand", Span::call_site());
+                    let arg_block = match variant.fields {
+                        Named(ref fields) => {
+                            into_app::gen_app_augmentation(&fields.named, &app_var, &attrs)
+                        }
+                        Unit => quote!( #app_var ),
+                        Unnamed(FieldsUnnamed { ref unnamed, .. }) if unnamed.len() == 1 => {
+                            let ty = &unnamed[0];
+                            quote_spanned! { ty.span()=>
+                                {
+                                    <#ty as ::clap::IntoApp>::augment_clap(#app_var)
+                                }
+                            }
+                        }
+                        Unnamed(..) => {
+                            abort!(variant, "non single-typed tuple enums are not supported")
+                        }
+                    };
+
+                    let name = attrs.cased_name();
+                    let from_attrs = attrs.top_level_methods();
+                    let version = attrs.version();
+                    quote! {
+                        let app = app.subcommand({
+                            let #app_var = ::clap::App::new(#name);
+                            let #app_var = #arg_block;
+                            #app_var#from_attrs#version
+                        });
+                    }
                 }
             }
-        }
-    });
+        })
+        .collect();
 
     let app_methods = parent_attribute.top_level_methods();
     let version = parent_attribute.version();
@@ -120,9 +133,12 @@ fn gen_from_subcommand(
     parent_attribute: &Attrs,
 ) -> TokenStream {
     use syn::Fields::*;
+
+    let mut ext_subcmd = None;
+
     let (flatten_variants, variants): (Vec<_>, Vec<_>) = variants
         .iter()
-        .map(|variant| {
+        .filter_map(|variant| {
             let attrs = Attrs::from_struct(
                 variant.span(),
                 &variant.attrs,
@@ -130,7 +146,56 @@ fn gen_from_subcommand(
                 parent_attribute.casing(),
                 parent_attribute.env_casing(),
             );
-            (variant, attrs)
+
+            if let Kind::ExternalSubcommand = &*attrs.kind() {
+                if ext_subcmd.is_some() {
+                    abort!(
+                        attrs.kind().span(),
+                        "Only one variant can be marked with `external_subcommand`, \
+                         this is the second"
+                    );
+                }
+
+                let ty = match variant.fields {
+                    Unnamed(ref fields) if fields.unnamed.len() == 1 => &fields.unnamed[0].ty,
+
+                    _ => abort!(
+                        variant,
+                        "The enum variant marked with `external_attribute` must be \
+                         a single-typed tuple, and the type must be either `Vec<String>` \
+                         or `Vec<OsString>`."
+                    ),
+                };
+
+                let (span, str_ty, values_of) = match subty_if_name(ty, "Vec") {
+                    Some(subty) => {
+                        if is_simple_ty(subty, "String") {
+                            (
+                                subty.span(),
+                                quote!(::std::string::String),
+                                quote!(values_of),
+                            )
+                        } else {
+                            (
+                                subty.span(),
+                                quote!(::std::ffi::OsString),
+                                quote!(values_of_os),
+                            )
+                        }
+                    }
+
+                    None => abort!(
+                        ty.span(),
+                        "The type must be either `Vec<String>` or `Vec<OsString>` \
+                         to be used with `external_subcommand`."
+                    ),
+                };
+
+                ext_subcmd = Some((span, &variant.ident, str_ty, values_of));
+                None
+            } else {
+                Some((variant, attrs))
+            }
         })
         .partition(|(_, attrs)| {
             let kind = attrs.kind();
@@ -177,16 +242,45 @@ fn gen_from_subcommand(
         }
     });
 
+    let wildcard = match ext_subcmd {
+        Some((span, var_name, str_ty, values_of)) => quote_spanned! { span=>
+            ("", ::std::option::Option::None) => ::std::option::Option::None,
+
+            (external, Some(matches)) => {
+                ::std::option::Option::Some(#name::#var_name(
+                    ::std::iter::once(#str_ty::from(external))
+                    .chain(
+                        matches.#values_of("").into_iter().flatten().map(#str_ty::from)
+                    )
+                    .collect::<::std::vec::Vec<_>>()
+                ))
+            }
+
+            (external, None) => {
+                ::std::option::Option::Some(#name::#var_name({
+                    let mut v = ::std::vec::Vec::with_capacity(1);
+                    v.push(#str_ty::from(external));
+                    v
+                }))
+            }
+        },
+
+        None => quote!(_ => None),
+    };
+
     quote! {
         fn from_subcommand<'b>(
             name: &'b str,
             sub: Option<&'b ::clap::ArgMatches>) -> Option<Self>
         {
             match (name, sub) {
-                #( #match_arms ),*,
+                #( #match_arms, )*
                 other => {
-                    #( #child_subcommands )*;
-                    None
+                    #( #child_subcommands )else*
+
+                    match other {
+                        #wildcard
+                    }
                 }
             }
         }
diff --git a/clap_derive/src/derives/dummies.rs b/clap_derive/src/dummies.rs
similarity index 100%
rename from clap_derive/src/derives/dummies.rs
rename to clap_derive/src/dummies.rs
diff --git a/clap_derive/src/lib.rs b/clap_derive/src/lib.rs
index 4bb373ae..68244122 100644
--- a/clap_derive/src/lib.rs
+++ b/clap_derive/src/lib.rs
@@ -20,7 +20,11 @@ extern crate proc_macro;
 
 use proc_macro_error::proc_macro_error;
 
+mod attrs;
 mod derives;
+mod dummies;
+mod parse;
+mod utils;
 
 // /// It is required to have this seperate and specificly defined.
 // #[proc_macro_derive(ArgEnum, attributes(case_sensitive))]
diff --git a/clap_derive/src/derives/parse.rs b/clap_derive/src/parse.rs
similarity index 98%
rename from clap_derive/src/derives/parse.rs
rename to clap_derive/src/parse.rs
index bbdc4777..04fc2453 100644
--- a/clap_derive/src/derives/parse.rs
+++ b/clap_derive/src/parse.rs
@@ -19,6 +19,7 @@ pub enum ClapAttr {
     ArgEnum(Ident),
     Subcommand(Ident),
     VerbatimDocComment(Ident),
+    ExternalSubcommand(Ident),
 
     // ident [= "string literal"]
     About(Ident, Option<LitStr>),
@@ -170,7 +171,7 @@ impl Parse for ClapAttr {
                 "flatten" => Ok(Flatten(name)),
                 "arg_enum" => Ok(ArgEnum(name)),
                 "subcommand" => Ok(Subcommand(name)),
-
+                "external_subcommand" => Ok(ExternalSubcommand(name)),
                 "verbatim_doc_comment" => Ok(VerbatimDocComment(name)),
 
                 "default_value" => Ok(DefaultValue(name, None)),
diff --git a/clap_derive/src/derives/doc_comments.rs b/clap_derive/src/utils/doc_comments.rs
similarity index 99%
rename from clap_derive/src/derives/doc_comments.rs
rename to clap_derive/src/utils/doc_comments.rs
index 7f8822f8..49c794c9 100644
--- a/clap_derive/src/derives/doc_comments.rs
+++ b/clap_derive/src/utils/doc_comments.rs
@@ -3,7 +3,8 @@
 //! #[derive(Clap)] works in terms of "paragraphs". Paragraph is a sequence of
 //! non-empty adjacent lines, delimited by sequences of blank (whitespace only) lines.
 
-use super::attrs::Method;
+use crate::attrs::Method;
+
 use quote::{format_ident, quote};
 use std::iter;
 
diff --git a/clap_derive/src/utils/mod.rs b/clap_derive/src/utils/mod.rs
new file mode 100644
index 00000000..89691332
--- /dev/null
+++ b/clap_derive/src/utils/mod.rs
@@ -0,0 +1,9 @@
+mod doc_comments;
+mod spanned;
+mod ty;
+
+pub use self::{
+    doc_comments::process_doc_comment,
+    spanned::Sp,
+    ty::{is_simple_ty, sub_type, subty_if_name, Ty},
+};
diff --git a/clap_derive/src/derives/spanned.rs b/clap_derive/src/utils/spanned.rs
similarity index 97%
rename from clap_derive/src/derives/spanned.rs
rename to clap_derive/src/utils/spanned.rs
index adde02dd..c8ba8062 100644
--- a/clap_derive/src/derives/spanned.rs
+++ b/clap_derive/src/utils/spanned.rs
@@ -81,7 +81,7 @@ impl<T: AsRef<str>> AsRef<str> for Sp<T> {
 impl<T: ToTokens> ToTokens for Sp<T> {
     fn to_tokens(&self, stream: &mut TokenStream) {
         // this is the simplest way out of correct ones to change span on
-        // arbitrary token tree I can come up with
+        // arbitrary token tree I could come up with
         let tt = self.val.to_token_stream().into_iter().map(|mut tt| {
             tt.set_span(self.span.clone());
             tt
diff --git a/clap_derive/src/derives/ty.rs b/clap_derive/src/utils/ty.rs
similarity index 97%
rename from clap_derive/src/derives/ty.rs
rename to clap_derive/src/utils/ty.rs
index b046ac6d..c2cec83f 100644
--- a/clap_derive/src/derives/ty.rs
+++ b/clap_derive/src/utils/ty.rs
@@ -84,7 +84,7 @@ pub fn subty_if_name<'a>(ty: &'a syn::Type, name: &str) -> Option<&'a syn::Type>
     subty_if(ty, |seg| seg.ident == name)
 }
 
-fn is_simple_ty(ty: &syn::Type, name: &str) -> bool {
+pub fn is_simple_ty(ty: &syn::Type, name: &str) -> bool {
     only_last_segment(ty)
         .map(|segment| {
             if let PathArguments::None = segment.arguments {
diff --git a/examples/15_custom_validator.rs b/examples/15_custom_validator.rs
index e8255950..976816eb 100644
--- a/examples/15_custom_validator.rs
+++ b/examples/15_custom_validator.rs
@@ -21,7 +21,7 @@ fn main() {
     println!("The .PNG file is: {}", matches.value_of("input").unwrap());
 }
 
-fn is_png(val: &str) -> Result<(), String> {
+fn is_png(val: String) -> Result<(), String> {
     // val is the argument value passed in by the user
     // val has type of String.
     if val.ends_with(".png") {
diff --git a/examples/18_builder_macro.rs b/examples/18_builder_macro.rs
index 38bc2973..ddc355cc 100644
--- a/examples/18_builder_macro.rs
+++ b/examples/18_builder_macro.rs
@@ -5,7 +5,7 @@ use clap::clap_app;
 
 fn main() {
     // Validation example testing that a file exists
-    let file_exists = |path: &str| {
+    let file_exists = |path| {
         if std::fs::metadata(path).is_ok() {
             Ok(())
         } else {
diff --git a/src/build/app/mod.rs b/src/build/app/mod.rs
index 3f4136ed..2cebbd4c 100644
--- a/src/build/app/mod.rs
+++ b/src/build/app/mod.rs
@@ -1711,13 +1711,6 @@ impl<'b> App<'b> {
                 "Global arguments cannot be required.\n\n\t'{}' is marked as both global and required",
                 arg.name
             );
-
-            // validators
-            assert!(
-                arg.validator.is_none() || arg.validator_os.is_none(),
-                "Argument '{}' has both `validator` and `validator_os` set which is not allowed",
-                arg.name
-            );
         }
 
         for group in &self.groups {
diff --git a/src/build/arg/mod.rs b/src/build/arg/mod.rs
index 7a92a037..8024595f 100644
--- a/src/build/arg/mod.rs
+++ b/src/build/arg/mod.rs
@@ -25,7 +25,7 @@ use crate::{
     INTERNAL_ERROR_MSG,
 };
 
-type Validator = Rc<dyn Fn(&str) -> Result<(), String>>;
+type Validator = Rc<dyn Fn(String) -> Result<(), String>>;
 type ValidatorOs = Rc<dyn Fn(&OsStr) -> Result<(), String>>;
 
 /// The abstract representation of a command line argument. Used to set all the options and
@@ -1927,7 +1927,7 @@ impl<'help> Arg<'help> {
     ///
     /// ```rust
     /// # use clap::{App, Arg};
-    /// fn has_at(v: &str) -> Result<(), String> {
+    /// fn has_at(v: String) -> Result<(), String> {
     ///     if v.contains("@") { return Ok(()); }
     ///     Err(String::from("The value did not contain the required @ sigil"))
     /// }
@@ -1947,7 +1947,7 @@ impl<'help> Arg<'help> {
     /// [`Rc`]: https://doc.rust-lang.org/std/rc/struct.Rc.html
     pub fn validator<F, O, E>(mut self, f: F) -> Self
     where
-        F: Fn(&str) -> Result<O, E> + 'static,
+        F: Fn(String) -> Result<O, E> + 'static,
         E: ToString,
     {
         self.validator = Some(Rc::new(move |s| {
diff --git a/src/parse/errors.rs b/src/parse/errors.rs
index c5215d19..2fde154f 100644
--- a/src/parse/errors.rs
+++ b/src/parse/errors.rs
@@ -130,7 +130,7 @@ pub enum ErrorKind {
     ///
     /// ```rust
     /// # use clap::{App, Arg, ErrorKind};
-    /// fn is_numeric(val: &str) -> Result<(), String> {
+    /// fn is_numeric(val: String) -> Result<(), String> {
     ///     match val.parse::<i64>() {
     ///         Ok(..) => Ok(()),
     ///         Err(..) => Err(String::from("Value wasn't a number!")),
diff --git a/src/parse/parser.rs b/src/parse/parser.rs
index 7fff483d..d764458c 100644
--- a/src/parse/parser.rs
+++ b/src/parse/parser.rs
@@ -381,6 +381,7 @@ where
         let has_args = self.has_args();
 
         let mut subcmd_name: Option<String> = None;
+        let mut external_subcommand = false;
         let mut needs_val_of: ParseResult = ParseResult::NotFound;
         let mut pos_counter = 1;
         let mut replace: Option<&[&str]> = None;
@@ -677,6 +678,8 @@ where
                     matches: sc_m.into_inner(),
                 });
 
+                external_subcommand = true;
+
                 break;
             } else if !((self.is_set(AS::AllowLeadingHyphen)
                 || self.is_set(AS::AllowNegativeNumbers))
@@ -718,28 +721,30 @@ where
             }
         }
 
-        if let Some(ref pos_sc_name) = subcmd_name {
-            let sc_name = find_subcmd!(self.app, *pos_sc_name)
-                .expect(INTERNAL_ERROR_MSG)
-                .name
-                .clone();
-            self.parse_subcommand(&sc_name, matcher, it)?;
-        } else if self.is_set(AS::SubcommandRequired) {
-            let bn = self.app.bin_name.as_ref().unwrap_or(&self.app.name);
-            return Err(ClapError::missing_subcommand(
-                bn,
-                &Usage::new(self).create_usage_with_title(&[]),
-                self.app.color(),
-            )?);
-        } else if self.is_set(AS::SubcommandRequiredElseHelp) {
-            debug!("Parser::get_matches_with: SubcommandRequiredElseHelp=true");
-            let message = self.write_help_err()?;
-            return Err(ClapError {
-                cause: String::new(),
-                message,
-                kind: ErrorKind::MissingArgumentOrSubcommand,
-                info: None,
-            });
+        if !external_subcommand {
+            if let Some(ref pos_sc_name) = subcmd_name {
+                let sc_name = find_subcmd!(self.app, *pos_sc_name)
+                    .expect(INTERNAL_ERROR_MSG)
+                    .name
+                    .clone();
+                self.parse_subcommand(&sc_name, matcher, it)?;
+            } else if self.is_set(AS::SubcommandRequired) {
+                let bn = self.app.bin_name.as_ref().unwrap_or(&self.app.name);
+                return Err(ClapError::missing_subcommand(
+                    bn,
+                    &Usage::new(self).create_usage_with_title(&[]),
+                    self.app.color(),
+                )?);
+            } else if self.is_set(AS::SubcommandRequiredElseHelp) {
+                debug!("Parser::get_matches_with: SubcommandRequiredElseHelp=true");
+                let message = self.write_help_err()?;
+                return Err(ClapError {
+                    cause: String::new(),
+                    message,
+                    kind: ErrorKind::MissingArgumentOrSubcommand,
+                    info: None,
+                });
+            }
         }
 
         self.remove_overrides(matcher);
diff --git a/src/parse/validator.rs b/src/parse/validator.rs
index 84571287..c03ccad8 100644
--- a/src/parse/validator.rs
+++ b/src/parse/validator.rs
@@ -137,7 +137,7 @@ impl<'b, 'c, 'z> Validator<'b, 'c, 'z> {
             }
             if let Some(ref vtor) = arg.validator {
                 debug!("Validator::validate_arg_values: checking validator...");
-                if let Err(e) = vtor(&*val.to_string_lossy()) {
+                if let Err(e) = vtor(val.to_string_lossy().into_owned()) {
                     debug!("error");
                     return Err(Error::value_validation(Some(arg), &e, self.p.app.color())?);
                 } else {
diff --git a/src/util/argstr.rs b/src/util/argstr.rs
index 098712dc..7ad80b64 100644
--- a/src/util/argstr.rs
+++ b/src/util/argstr.rs
@@ -1,12 +1,12 @@
 use std::{
     borrow::Cow,
     ffi::{OsStr, OsString},
+    fmt::{self, Debug},
     str,
 };
 
 use os_str_bytes::{OsStrBytes, OsStringBytes};
 
-#[derive(Debug)]
 pub(crate) struct ArgStr<'a>(Cow<'a, [u8]>);
 
 impl<'a> ArgStr<'a> {
@@ -127,6 +127,12 @@ impl<'a> ArgStr<'a> {
     }
 }
 
+impl<'a> Debug for ArgStr<'a> {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "{:?}", self.to_string_lossy())
+    }
+}
+
 impl<'a> PartialEq<str> for ArgStr<'a> {
     fn eq(&self, other: &str) -> bool {
         self.0 == other.as_bytes()


Test patch:
diff --git a/clap_derive/tests/subcommands.rs b/clap_derive/tests/subcommands.rs
index f012b3a5..9fbb6323 100644
--- a/clap_derive/tests/subcommands.rs
+++ b/clap_derive/tests/subcommands.rs
@@ -161,6 +161,96 @@ fn test_tuple_commands() {
     assert!(!output.contains("Not shown"));
 }
 
+#[test]
+fn external_subcommand() {
+    #[derive(Debug, PartialEq, Clap)]
+    struct Opt {
+        #[clap(subcommand)]
+        sub: Subcommands,
+    }
+
+    #[derive(Debug, PartialEq, Clap)]
+    enum Subcommands {
+        Add,
+        Remove,
+        #[clap(external_subcommand)]
+        Other(Vec<String>),
+    }
+
+    assert_eq!(
+        Opt::parse_from(&["test", "add"]),
+        Opt {
+            sub: Subcommands::Add
+        }
+    );
+
+    assert_eq!(
+        Opt::parse_from(&["test", "remove"]),
+        Opt {
+            sub: Subcommands::Remove
+        }
+    );
+
+    assert!(Opt::try_parse_from(&["test"]).is_err());
+
+    assert_eq!(
+        Opt::try_parse_from(&["test", "git", "status"]).unwrap(),
+        Opt {
+            sub: Subcommands::Other(vec!["git".into(), "status".into()])
+        }
+    );
+}
+
+#[test]
+fn external_subcommand_os_string() {
+    use std::ffi::OsString;
+
+    #[derive(Debug, PartialEq, Clap)]
+    struct Opt {
+        #[clap(subcommand)]
+        sub: Subcommands,
+    }
+
+    #[derive(Debug, PartialEq, Clap)]
+    enum Subcommands {
+        #[clap(external_subcommand)]
+        Other(Vec<OsString>),
+    }
+
+    assert_eq!(
+        Opt::try_parse_from(&["test", "git", "status"]).unwrap(),
+        Opt {
+            sub: Subcommands::Other(vec!["git".into(), "status".into()])
+        }
+    );
+
+    assert!(Opt::try_parse_from(&["test"]).is_err());
+}
+
+#[test]
+fn external_subcommand_optional() {
+    #[derive(Debug, PartialEq, Clap)]
+    struct Opt {
+        #[clap(subcommand)]
+        sub: Option<Subcommands>,
+    }
+
+    #[derive(Debug, PartialEq, Clap)]
+    enum Subcommands {
+        #[clap(external_subcommand)]
+        Other(Vec<String>),
+    }
+
+    assert_eq!(
+        Opt::try_parse_from(&["test", "git", "status"]).unwrap(),
+        Opt {
+            sub: Some(Subcommands::Other(vec!["git".into(), "status".into()]))
+        }
+    );
+
+    assert_eq!(Opt::try_parse_from(&["test"]).unwrap(), Opt { sub: None });
+}
+
 // #[test]
 // #[ignore] // FIXME (@CreepySkeleton)
 // fn enum_in_enum_subsubcommand() {
diff --git a/clap_derive/tests/ui/external_subcommand_misuse.rs b/clap_derive/tests/ui/external_subcommand_misuse.rs
new file mode 100644
index 00000000..19d24258
--- /dev/null
+++ b/clap_derive/tests/ui/external_subcommand_misuse.rs
@@ -0,0 +1,11 @@
+use clap::Clap;
+
+#[derive(Clap, Debug)]
+struct Opt {
+    #[clap(external_subcommand)]
+    field: String,
+}
+
+fn main() {
+    let _ = Opt::parse();
+}
diff --git a/clap_derive/tests/ui/external_subcommand_misuse.stderr b/clap_derive/tests/ui/external_subcommand_misuse.stderr
new file mode 100644
index 00000000..14540920
--- /dev/null
+++ b/clap_derive/tests/ui/external_subcommand_misuse.stderr
@@ -0,0 +1,5 @@
+error: `external_subcommand` can be used only on enum variants
+ --> $DIR/external_subcommand_misuse.rs:5:12
+  |
+5 |     #[clap(external_subcommand)]
+  |            ^^^^^^^^^^^^^^^^^^^
diff --git a/clap_derive/tests/ui/external_subcommand_wrong_type.rs b/clap_derive/tests/ui/external_subcommand_wrong_type.rs
new file mode 100644
index 00000000..e8233ef6
--- /dev/null
+++ b/clap_derive/tests/ui/external_subcommand_wrong_type.rs
@@ -0,0 +1,26 @@
+use clap::Clap;
+use std::ffi::CString;
+
+#[derive(Clap, Debug)]
+enum Opt {
+    #[clap(external_subcommand)]
+    Other(Vec<CString>),
+}
+
+#[derive(Clap, Debug)]
+enum Opt2 {
+    #[clap(external_subcommand)]
+    Other(String),
+}
+
+#[derive(Clap, Debug)]
+enum Opt3 {
+    #[clap(external_subcommand)]
+    Other { a: String },
+}
+
+fn main() {
+    let _ = Opt::parse();
+    let _ = Opt2::parse();
+    let _ = Opt3::parse();
+}
diff --git a/clap_derive/tests/ui/external_subcommand_wrong_type.stderr b/clap_derive/tests/ui/external_subcommand_wrong_type.stderr
new file mode 100644
index 00000000..d4ff9026
--- /dev/null
+++ b/clap_derive/tests/ui/external_subcommand_wrong_type.stderr
@@ -0,0 +1,21 @@
+error: The type must be either `Vec<String>` or `Vec<OsString>` to be used with `external_subcommand`.
+  --> $DIR/external_subcommand_wrong_type.rs:13:11
+   |
+13 |     Other(String),
+   |           ^^^^^^
+
+error: The enum variant marked with `external_attribute` must be a single-typed tuple, and the type must be either `Vec<String>` or `Vec<OsString>`.
+  --> $DIR/external_subcommand_wrong_type.rs:18:5
+   |
+18 | /     #[clap(external_subcommand)]
+19 | |     Other { a: String },
+   | |_______________________^
+
+error[E0308]: mismatched types
+ --> $DIR/external_subcommand_wrong_type.rs:7:15
+  |
+7 |     Other(Vec<CString>),
+  |               ^^^^^^^ expected struct `std::ffi::CString`, found struct `std::ffi::OsString`
+  |
+  = note: expected struct `std::vec::Vec<std::ffi::CString>`
+             found struct `std::vec::Vec<std::ffi::OsString>`
diff --git a/clap_derive/tests/ui/multiple_external_subcommand.rs b/clap_derive/tests/ui/multiple_external_subcommand.rs
new file mode 100644
index 00000000..3cd342a3
--- /dev/null
+++ b/clap_derive/tests/ui/multiple_external_subcommand.rs
@@ -0,0 +1,21 @@
+use clap::Clap;
+
+#[derive(Clap, Debug)]
+struct Opt {
+    #[clap(subcommand)]
+    cmd: Command,
+}
+
+#[derive(Clap, Debug)]
+enum Command {
+    #[clap(external_subcommand)]
+    Run(Vec<String>),
+
+    #[clap(external_subcommand)]
+    Other(Vec<String>),
+}
+
+fn main() {
+    let opt = Opt::parse();
+    println!("{:?}", opt);
+}
diff --git a/clap_derive/tests/ui/multiple_external_subcommand.stderr b/clap_derive/tests/ui/multiple_external_subcommand.stderr
new file mode 100644
index 00000000..270fd36d
--- /dev/null
+++ b/clap_derive/tests/ui/multiple_external_subcommand.stderr
@@ -0,0 +1,5 @@
+error: Only one variant can be marked with `external_subcommand`, this is the second
+  --> $DIR/multiple_external_subcommand.rs:14:12
+   |
+14 |     #[clap(external_subcommand)]
+   |            ^^^^^^^^^^^^^^^^^^^
diff --git a/clap_derive/tests/ui/skip_flatten.stderr b/clap_derive/tests/ui/skip_flatten.stderr
index e5230c0d..328dc9c3 100644
--- a/clap_derive/tests/ui/skip_flatten.stderr
+++ b/clap_derive/tests/ui/skip_flatten.stderr
@@ -1,4 +1,4 @@
-error: subcommand, flatten and skip cannot be used together
+error: `subcommand`, `flatten`, `external_subcommand` and `skip` cannot be used together
   --> $DIR/skip_flatten.rs:17:18
    |
 17 |     #[clap(skip, flatten)]
diff --git a/clap_derive/tests/ui/skip_subcommand.stderr b/clap_derive/tests/ui/skip_subcommand.stderr
index fff8796a..aeea1029 100644
--- a/clap_derive/tests/ui/skip_subcommand.stderr
+++ b/clap_derive/tests/ui/skip_subcommand.stderr
@@ -1,4 +1,4 @@
-error: subcommand, flatten and skip cannot be used together
+error: `subcommand`, `flatten`, `external_subcommand` and `skip` cannot be used together
   --> $DIR/skip_subcommand.rs:17:24
    |
 17 |     #[clap(subcommand, skip)]
diff --git a/clap_derive/tests/ui/subcommand_and_flatten.stderr b/clap_derive/tests/ui/subcommand_and_flatten.stderr
index b509b737..c9fcc431 100644
--- a/clap_derive/tests/ui/subcommand_and_flatten.stderr
+++ b/clap_derive/tests/ui/subcommand_and_flatten.stderr
@@ -1,4 +1,4 @@
-error: subcommand, flatten and skip cannot be used together
+error: `subcommand`, `flatten`, `external_subcommand` and `skip` cannot be used together
   --> $DIR/subcommand_and_flatten.rs:16:24
    |
 16 |     #[clap(subcommand, flatten)]
diff --git a/tests/validators.rs b/tests/validators.rs
deleted file mode 100644
index 5abedf6c..00000000
--- a/tests/validators.rs
+++ /dev/null
@@ -1,19 +0,0 @@
-use clap::{App, Arg};
-
-#[cfg(debug_assertions)]
-#[test]
-#[should_panic = "Argument 'test' has both `validator` and `validator_os` set which is not allowed"]
-fn both_validator_and_validator_os() {
-    let _ = App::new("test")
-        .arg(
-            Arg::with_name("test")
-                .validator(|val| val.parse::<u32>().map_err(|e| e.to_string()))
-                .validator_os(|val| {
-                    val.to_str()
-                        .unwrap()
-                        .parse::<u32>()
-                        .map_err(|e| e.to_string())
-                }),
-        )
-        .try_get_matches_from(&["app", "1"]);
-}