No argument validation on globals

[gabbifish]

Rust Version

rustc 1.36.0 (a53f9df32 2019-07-03)

Affected Version of clap

2.33.0

Bug or Feature Request Summary

I was hoping that I could make an argument required, and also use the global() function to ensure that a user of my CLI could specify my required option wherever they please.

Expected Behavior Summary

An Arg with both .global() and .required_unless() would be usable.

Actual Behavior Summary

I see

thread 'main' panicked at 'Global arguments cannot be required.

	'binding' is marked as global and required', /home/gabbi/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/clap-2.33.0/src/app/parser.rs:204:9

Steps to Reproduce the issue

Create a clap App with the arg below, and attempt to run it.

Sample Code or Link to Sample Code

                .arg(
                    Arg::with_name("binding")
                    .help("The binding of the namespace this action applies to")
                    .short("b")
                    .long("binding")
                    .value_name("BINDING NAME")
                    .takes_value(true)
                    .global(true)
                    .conflicts_with("namespace-id")
                )

[jamestwebber]

I'll +1 this issue, I would like this to work as well. My tool has a bunch of universal required arguments (e.g. input directory) and then different tasks have some specific options. I was hoping to have usage look like

cmd --input [dir] --cmd-specific-option [option]
cmd subcmd --input [dir] --subcmd-specific-option [option]

Where --input is always required but can be specified after subcmd for readability (I find that dropping a subcommand in the middle of the line is hard to use/read).

Maybe there's a better way to set this up?

[erayerdin]

I don't know why this is the case, but a workaround now would be defining a .default_value("whatever") and do whatever when you get whatever as value.

This is one way, however one should keep in mind that this default value also limits what can be done with value. In my case, I will use an arg as Telegram token value, which is a long string, so setting default to "INVALID" makes sense in my case because I'm fairly sure there won't be any bot token with value "INVALID". On the other hand, if I used that arg as a file path, that would eliminate a directory or file named "INVALID".

Wish this will be implemented soon.

[pksunkara]

I would welcome people to design how this should work and implement it. Unfortunately, as it is now, we have no design to implement this because the global arg gets propagated to all subcommands and we haven't yet decided on how to resolve the required situation when that happens.

[jplatte]

Until this is supported, maybe the derive macro could raise an error when #[clap(global = true)] is used on a required setting?

[erayerdin]

I have come to say that I have run across this use-case several times now. It's been a long time (since Sep 2019) and this should be considered.

[epage]

It's been a long time (since Sep 2019) and this should be considered.

I can understand the desire for this but it unfortunately isn't a priority. Before, the focus was on finishing the committed items for clap 3 by maintainers who had little time. We are now have some more liberal sponsorship (my employer) but I've completed the sponsored objective (clap3) and have to balance clap work against other sponsored objectives (cargo-edit atm). We have around 160 open issues which all exist because someone cares about them and we have to prioritize among them. Our current area of focus is in re-shaping clap to be more open ended with faster compile times and smaller binary sizes.

If you'd like to work on this, I would be willing to help mentor you on it.

[wildwestrom]

I'd like to start working on this. I'm just not sure where in the codebase I should be looking first.
I just got started with a debugger to see what's going on.
@pksunkara @epage

[epage]

The relevant calls are roughly
_do_parse

• validate via get_matches_with which is what checks for required args and sets defaults
• propagate_globals

Naively swapping the calls around would probably affect the way globals and defaults interact. Whether we are willing to change that is up to the details of the situation. Unsure what other concerns might lurk in this.

Note that master is now for v4. I would encourage doing development on a major feature there. If you want it earlier than v4's release (trying to keep it narrowly scoped to not extend more than a month or two) then once its merged, you can cherry pick it into the v3-master branch.

[wildwestrom]

OK, I've bitten off more than I can chew. I'm gonna need more help on this if anything. It's hard for me to track where things are going even stepping through with a debugger.

Here's a minimal example:

use clap::{Arg, Command};
fn main() {
    let subcmd1 = Command::new("sub1");
    let subcmd2 = Command::new("sub2")
        .arg(clap::Arg::new("optional_arg").help("Optional argument for subcommand"));

    let cli = Command::new("clap-fix")
        .arg(Arg::new("string_arg").required(true).global(true))
        .arg(Arg::new("num_arg").global(true).short('n').long("num-arg"))
        .subcommand(subcmd1)
        .subcommand(subcmd2);

    // This works but it's incorrect.
    let _ = cli.get_matches_from(["clap-fix", "string_arg_before", "sub1", "string_arg_after"]);
    // If this works then we're good.
    // let _ = cli.get_matches_from(["clap-fix", "sub1", "string_arg_after"]);
}

I also saw you guys were on opencollective. Can I open a bounty on this issue?

[epage]

I also saw you guys were on opencollective. Can I open a bounty on this issue?

Probably? Previous maintainers were the ones mostly dealing with that; I have little experience. I've not seen people take up bounties too much for issues.

[dvc94ch]

@epage was this fixed recently, seems to work with 0.4.29?

[dvc94ch]

ah sorry, the issue persists, just the error message is wrong now. should I open a separate issue?

app-store-connect device list --api-key ~/.analog/developer.key.json
error: The following required arguments were not provided:
  --api-key <API_KEY>

Usage: app-store-connect --api-key <API_KEY> device --api-key <API_KEY> <COMMAND>

For more information try '--help'

[sondrelg]

Is it possible to work around this by manually producing the right error when a global argument isn't specified?

I tried handling the None case of my optional global argument by calling:

Foo::command()
  .error(
    MissingRequiredArgument, 
    "the following required arguments were not provided:\n  \x1b[32m--application <APP>\x1b[0m",
  )
  .exit()

And that seems to produce the right error:

error: the following required arguments were not provided:
  --application <APP>

USAGE:
    ok secret --application <APP> <COMMAND>

For more information, try '--help'.

EDIT: The original error message had an error

[epage]

If it isn't important to provide the custom usage, I'd just drop that and have Command::error generate it for you.

In theory, we've opened up all of the APIs so you can generate the error like clap:

• Error::new with ErrorKind::MissingRequiredArgument
• Error::with_cmd
• Error::insert with ContextKind::InvalidArg, ContextValue::Strings(required)
• Error::insert with ContextKind::Usage, ContextValue::StyledStr(usage)

See https://docs.rs/clap/latest/src/clap/error/mod.rs.html#487

The RichFormatter will then generate the appropriate message for you.

[hwittenborn]

Sorry if I'm missing something, but is there any technical reason this can't be done? Like if the check for when an Arg is both global and required was simply removed, would there be something that would start breaking?

[epage]

Sorry if I'm missing something, but is there any technical reason this can't be done? Like if the check for when an Arg is both global and required was simply removed, would there be something that would start breaking?

Currently, globals are propagated after all validation has occurred which would cause missing-required errors where they don't apply.

For details, see my earlier comment

[kingzcheung]

This is very strange and does not conform to the normal usage.

cmd subcmd --global-arg # not work
cmd --global-arg subcmd # work

[Kibouo]

Sometimes you want flags that are available for each subcommands. Replicating it in every subcommand's config is not great for maintainability.

…

________________________________ From: KK Cheung ***@***.***> Sent: Wednesday, November 29, 2023 11:55:26 AM To: clap-rs/clap ***@***.***> Cc: Kibouo ***@***.***>; Manual ***@***.***> Subject: Re: [clap-rs/clap] Why can't global args be required? (#1546) This is very strange and does not conform to the normal usage. cmd cubcmd --global-arg # not work cmd --global-arg cubcmd # work — Reply to this email directly, view it on GitHub<#1546 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AGESTOPXHBMVPTJIIHPCM5DYG4IB5AVCNFSM4IWKXEK2U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOBTGE3DMNJYGQ4A>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[Emilgardis]

While this isn't solved, I've managed to implement a rather (little) hacky way to still get a nice error message when using global and default_value

playground

use clap::{CommandFactory as _, FromArgMatches as _, Parser};

pub const DEFAULT_NOT_SET: &str = "\u{0}\u{beef}";

#[derive(Debug, Parser)]
pub struct Cli {
    #[arg(global = true, long, default_value = DEFAULT_NOT_SET)]
    pub required: String,
    #[command(subcommand)]
    pub subcommand: Option<Subcommand>,
}

#[derive(Debug, clap::Subcommand)]
pub enum Subcommand {
    Command {
        
    },
}

pub fn main() -> Result<(),Box<dyn std::error::Error>> {
    let _cli: Cli = {
        let mut cmd = Cli::command();
        let matches = Cli::command().get_matches_from(["playground", "command"]);
        let mut strings = vec![];

        let succ = std::iter::successors(Some((cmd.clone(), &matches)), |(cmd, matches)| {
            let (sub, matches) = matches.subcommand()?;
            Some((cmd.find_subcommand(sub)?.clone(), matches))
        });
        for (cmd, matches) in succ {
            let ids = cmd
                .get_arguments()
                .filter(|&arg| arg.is_global_set())
                .map(|arg| {
                    (
                        arg.clone()
                            // this is a hacky way to display, without the override here the stylized call will panic
                            .num_args(arg.get_num_args().unwrap_or_default())
                            .to_string(),
                        arg.get_id(),
                    )
                });

            for (display, id) in ids {
                if !matches!(
                    matches.value_source(id.as_str()),
                    Some(clap::parser::ValueSource::DefaultValue)
                ) {
                    continue;
                }
                let mut raw = matches.get_raw(id.as_str()).unwrap();
                if raw.len() != 1 {
                    continue;
                }
                let raw = raw.next().unwrap();
                if raw.to_string_lossy() != DEFAULT_NOT_SET {
                    continue;
                }
                strings.push(display);
            }
        }

        if strings.is_empty() {
            Cli::from_arg_matches(&matches)?
        } else {
            let mut err = clap::Error::new(clap::error::ErrorKind::MissingRequiredArgument);
            err.insert(
                clap::error::ContextKind::InvalidArg,
                clap::error::ContextValue::Strings(strings),
            );
            err.insert(
               clap::error::ContextKind::Usage,
               clap::error::ContextValue::StyledStr(cmd.render_usage()),
            );
            err.with_cmd(&cmd).exit();
        }
    };
    Ok(())
}

one thing missing here is usage, since there is no good way to render it the same way clap does internally.