External subcommands that alias built-in subccommands cause panics

[epage]

Please complete the following tasks

• I have searched the discussions
• I have searched the existing issues

Rust Version

rustc 1.57.0 (f1edd0429 2021-11-29)

Clap Version

master

Minimal reproducible code

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

$ 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:

$ 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:

$ 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 checks 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

[epage]

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.

[epage]

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.