Repository: clap-rs/clap (language: rust)

Issue:
External subcommands that alias built-in subccommands cause panics
### Please complete the following tasks

- [X] I have searched the [discussions](https://github.com/clap-rs/clap/discussions)
- [X] I have searched the existing issues

### Rust Version

rustc 1.57.0 (f1edd0429 2021-11-29)

### Clap Version

master

### Minimal reproducible code

```rust
use clap::{app_from_crate, arg, App, AppSettings};

fn main() {
    let matches = app_from_crate!()
        .global_setting(AppSettings::PropagateVersion)
        .global_setting(AppSettings::UseLongFormatForHelpSubcommand)
        .setting(AppSettings::SubcommandRequiredElseHelp)
        .setting(AppSettings::AllowExternalSubcommands)
        .subcommand(
            App::new("add")
                .about("Adds files to myapp")
                .arg(arg!([NAME])),
        )
        .get_matches();

    match matches.subcommand() {
        Some(("add", sub_matches)) => println!(
            "'myapp add' was used, name is: {:?}",
            sub_matches.value_of("NAME")
        ),
        _ => unreachable!(),
    }
}

```


### Steps to reproduce the bug with the above code

See below

### Actual Behaviour

```console
$ cargo run -- add name
   Compiling test-clap v0.1.0 (/home/epage/src/personal/test-clap)
    Finished dev [unoptimized + debuginfo] target(s) in 0.80s
     Running `target/debug/test-clap add name`
'myapp add' was used, name is: Some("name")

$ cargo run -- -- add name
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `target/debug/test-clap -- add name`
thread 'main' panicked at '`NAME` is not a name of an argument or a group.
Make sure you're using the name of the argument itself and not the name of short or long flags.', /home/epage/src/personal/clap/src/parse/matches/arg_matches.rs:119:24
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```

This happens because `--` is escaping the command so it becomes an external subcommand that aliases the built-in.  A simple `match` does not catch this and we then try to access `"NAME"` in an `ArgMatches` meant for external subcommands (only `""` exists)

### Expected Behaviour

Its a bit unclear.  `cargo` just passes the `ArgMatches` along, warning about the contents, like:
```console
$ cargo run -- -- check --invalid_argument -some-other-argument
    Finished dev [unoptimized + debuginfo] target(s) in 0.05s
     Running `target/debug/cargo -- check --invalid_argument -some-other-argument`
warning: trailing arguments after built-in command `check` are ignored: `--invalid_argument -some-other-argument`
    Checking cargo v0.60.0 (/home/epage/src/personal/cargo)
^C  Building [=======================> ] 169/171: cargo                                                           
```

So with the above problem, that'd look like:
```console
$ cargo run -- add name
   Compiling test-clap v0.1.0 (/home/epage/src/personal/test-clap)
    Finished dev [unoptimized + debuginfo] target(s) in 0.80s
     Running `target/debug/test-clap add name`
'myapp add' was used, name is: Some("name")

$ cargo run -- -- add name
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `target/debug/test-clap -- add name`
'myapp add' was used, name is: None
```

### Additional Context

Ran into this when porting cargo to clap3 with a test failure from `cargo_alias_config::weird_check`.  `cargo` takes the approach of running the escaped command but ignoring the arguments.  This means we pass an external subcommand's `ArgMatches` into a function assuming its getting `check`s `ArgMatches` and panics when it accesses an "undefined" field.

Builder users can detect that its an external subcommand by checking for `values_of("")` but derive users can't do that.

You have to even know to do that.

Once you do that, the question is what is next.  Pass it through? Error?  Fallback to the built-in like cargo?  Seems like this should be the users choice.  The challenge is with the fallback.  They now need an `ArgMatches` that has the same shape (ie won't panic) when passing to that part of their program.

### Debug Output

_No response_


Discussion / hints:
So my thoughts
- Call this out in in the `AllowExternalSubcommands` documentation
- Add an extra check to the derive to ensure we actually populate external subcommands.

Unsure what I'll do for cargo to maintain behavior.
A big problem with detecting this is the fact that we only populate `""` in `ArgMatches` if there are arguments for the external subcommand.  The mere presence of `""` causes `cargo` to issue a warning so others, in theory, could be relying on that and it'd be breaking to start populating it.

Without a way to detect this until 4.0:
- We can't fix the derive until then
- We are limited in what we can say in the docs.

So the choice is go straight for 4.0 already or apply a surgical hack to unblock cargo until 4.0.

I'm leaning towards the surgical hack.

Fix patch:
diff --git a/clap_derive/src/derives/subcommand.rs b/clap_derive/src/derives/subcommand.rs
index f8aa52c860f..7d123479857 100644
--- a/clap_derive/src/derives/subcommand.rs
+++ b/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)
+                }
             }
         }
     });
diff --git a/src/build/command.rs b/src/build/command.rs
index 4ba7c844727..1ba2629109b 100644
--- a/src/build/command.rs
+++ b/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
diff --git a/src/parse/arg_matcher.rs b/src/parse/arg_matcher.rs
index 323fb5b9abc..31804a46fcc 100644
--- a/src/parse/arg_matcher.rs
+++ b/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);
diff --git a/src/parse/parser.rs b/src/parse/parser.rs
index 5ca9cfe8d0d..6262d18d887 100644
--- a/src/parse/parser.rs
+++ b/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 {


Test patch:
diff --git a/tests/builder/app_settings.rs b/tests/builder/app_settings.rs
index 3013406b816..20c5109b140 100644
--- a/tests/builder/app_settings.rs
+++ b/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
diff --git a/tests/derive/subcommands.rs b/tests/derive/subcommands.rs
index 0986df574cf..7dad15f210b 100644
--- a/tests/derive/subcommands.rs
+++ b/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 }
+    );
+}