fix: Provide path to avoid UTF-8 panics

[epage]

Before, validating UTF-8 was all-or-nothing and would cause a panic if
someone used the right API with non-UTF-8 input.

Now, all arguments are validated for UTF-8, unless opted-out. This
ensures a non-panicing path forward at the cost of people using the
builder API that previously did value_of_os need to now set this flag.

Fixes #751

[pksunkara]

Can you please add ui tests for all the aborts you added?

[epage]

I just duplicated existing aborts, just moving them up earlier in the process. I at least confirmed the subcommand abort is already covered

[pksunkara]

Both the aborts are for different scenarios, so, I would still expect different tests here since this scenario is not covered.

[epage]

To be clear which aborts we are talking about, I'm assuming you are referring to

                        _ => abort!(
                            variant,
                            "The enum variant marked with `external_subcommand` must be \
                             a single-typed tuple, and the type must be either `Vec<String>` \
                             or `Vec<OsString>`."
                        ),

and

                        None => abort!(
                            ty.span(),
                            "The type must be either `Vec<String>` or `Vec<OsString>` \
                             to be used with `external_subcommand`."
                        ),

To boil these two aborts down, they are:

• Invalid enum variant for external subcommands
• Invalid type in Vec inside external subcommands

These checks were added to gen_augment but already exist in gen_from_arg_matches and are already covered by clap_derive/tests/ui/external_subcommand_wrong_type.rs

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();
}

[pksunkara]

Okay, so you said you duplicated 2 aborts in this PR from old code.

The first one is this which is duplicated from this. Since gen_from_arg_matches is called first before gen_augment is called, we don't need the new abort. We can guarantee that new abort never triggers which means we need an INTERNAL_ERR panic here.

The second one is this which is duplicated from this. But we are not handling Vec<String> here which means the phrasing here should be different. And we need to make sure both the phrasings are tested.

[pksunkara]

Updated the comment with edits to make it more clearer.

[epage]

Having one function abort on behalf of both functions creates a behavior dependency between them that is non-obvious and can easily break as code is refactored in the future.

[pksunkara]

I was thinking about asking to move this to be above both of these functions. But this is too much time/discussion for a simple thing. Forget about the first abort. Let's just resolve the second abort as described above.

[epage]

I went ahead though the other one has precedence so the change does not show up in error messages.

[pksunkara]

Both the aborts are for different scenarios, so, I would still expect different tests here since this scenario is not covered.