Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#[command(flatten)] on optional field makes it required #5092

Open
2 tasks done
smessmer opened this issue Aug 27, 2023 · 3 comments
Open
2 tasks done

#[command(flatten)] on optional field makes it required #5092

smessmer opened this issue Aug 27, 2023 · 3 comments
Labels
A-validators Area: ArgMatches validation logi C-bug Category: Updating dependencies M-breaking-change Meta: Implementing or merging this will introduce a breaking change. S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing

Comments

@smessmer
Copy link

Please complete the following tasks

Rust Version

1.72

Clap Version

4.3.4

Minimal reproducible code

use clap::{Args, Parser};

fn main() {
    MainArgs::parse();
}

#[derive(Parser, Debug)]
pub struct MainArgs {
    #[arg(short = 'V', long)]
    pub version: bool,

    #[command(flatten)]
    pub specific_args: Option<SpecificArgs>,
}

#[derive(Args, Debug)]
pub struct SpecificArgs {
    pub positional: String,
}

Steps to reproduce the bug with the above code

cargo r -- --version

Actual Behaviour

error: the following required arguments were not provided:
  <POSITIONAL>

Usage: test-executable --version <POSITIONAL>

For more information, try '--help'.

Expected Behaviour

No error because the SpecificArgs is optional.

Additional Context

I want to use this to have a top level Parser with a few global args (e.g. --version but also some others), and I want to reuse this across different CLI tools, each with their own ConcreteArgs. The ConcreteArgs are usually written as mandatory in their corresponding #[derive(Args)] parser, but they should only be mandatory if none of the global flags are present. To do this, I made SpecificArgs optional in the #[command(flatten)] but that doesn't seem to have the intended effect.

Similar discussion: #4954

Debug Output

No response

@smessmer smessmer added the C-bug Category: Updating dependencies label Aug 27, 2023
@epage epage added M-breaking-change Meta: Implementing or merging this will introduce a breaking change. A-validators Area: ArgMatches validation logi S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing labels Aug 28, 2023
@epage
Copy link
Member

epage commented Aug 28, 2023

We have a discrepancy in how we treat Option between Arg (required, no required) and ArgGroup (no-op, track whether present).

A lot of this depends on how people are using an ArgGroup

  • Composing structs: lack of Option shouldn't mean anything
  • Require or don't require an ArgGroup
  • Override required Args within an ArgGroup (what this issue wants

Can we pick a one-size-fits all option? If not, how do we allow variability of this?

@jannschu
Copy link

jannschu commented Sep 2, 2023

Is there a workaround?

@epage
Copy link
Member

epage commented Sep 3, 2023

The way to workaround this heavily depends on your use case. If you want one argument to be present when the group is, you can use ArgGroup::requires.

As an example, let's take the reproduction case and change it to use that:

use clap::{Args, Parser};

fn main() {
    MainArgs::parse();
}

#[derive(Parser, Debug)]
pub struct MainArgs {
    #[arg(short = 'V', long)]
    pub version: bool,

    #[command(flatten)]
    pub specific_args: Option<SpecificArgs>,
}

#[derive(Args, Debug)]
#[group(requires = "positional")]
pub struct SpecificArgs {
    #[arg(required = false)]
    pub positional: String,
}

SpecificArgs will only be instantiated if the group is present and if the group is present then positional must be present as well, so you can skip using Option for it and just override the default required = true with required = false.

MarkusPettersson98 added a commit to mullvad/mullvadvpn-app that referenced this issue Oct 26, 2023
…guration

SOCKS5 optionally supports username+password authentication, which has
been implemented previously. This commit addresses a bug in the argument
parsing, which made username+password required arguments when adding a
remote SOCKS5 api access method using `mullvad api-access add socks5
remote`.

Apparently, this is a known pitfall with `clap`: clap-rs/clap#5092
MarkusPettersson98 added a commit to mullvad/mullvadvpn-app that referenced this issue Oct 26, 2023
…guration

SOCKS5 optionally supports username+password authentication, which has
been implemented previously. This commit addresses a bug in the argument
parsing, which made username+password required arguments when adding a
remote SOCKS5 api access method using `mullvad api-access add socks5
remote`.

Apparently, this is a known pitfall with `clap`: clap-rs/clap#5092
MarkusPettersson98 added a commit to mullvad/mullvadvpn-app that referenced this issue Oct 27, 2023
…guration

SOCKS5 optionally supports username+password authentication, which has
been implemented previously. This commit addresses a bug in the argument
parsing, which made username+password required arguments when adding a
remote SOCKS5 api access method using `mullvad api-access add socks5
remote`.

Apparently, this is a known pitfall with `clap`: clap-rs/clap#5092
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-validators Area: ArgMatches validation logi C-bug Category: Updating dependencies M-breaking-change Meta: Implementing or merging this will introduce a breaking change. S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing
Projects
None yet
Development

No branches or pull requests

3 participants