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

default_value_if depends on order in struct #4713

Open
2 tasks done
drmikehenry opened this issue Feb 18, 2023 · 6 comments
Open
2 tasks done

default_value_if depends on order in struct #4713

drmikehenry opened this issue Feb 18, 2023 · 6 comments
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-bug Category: Updating dependencies M-breaking-change Meta: Implementing or merging this will introduce a breaking change.
Milestone

Comments

@drmikehenry
Copy link

Please complete the following tasks

Rust Version

rustc 1.65.0 (897e37553 2022-11-02)

Clap Version

4.1.6

Minimal reproducible code

use clap::{builder::ArgPredicate, Parser};

#[derive(Parser, Debug)]
struct Cli {
    #[arg(
        long,
        default_value_if("imply_both", ArgPredicate::IsPresent, Some("true"))
    )]
    flag1: bool,

    /// imply both --flag1 and --flag2
    #[arg(long)]
    imply_both: bool,

    #[arg(
        long,
        default_value_if("imply_both", ArgPredicate::IsPresent, Some("true"))
    )]
    flag2: bool,
}

fn main() {
    let cli = Cli::parse();
    println!("{:#?}", cli);
}

Steps to reproduce the bug with the above code

cargo run -q --

Actual Behaviour

This output is generated when no options are given:

Cli {
    flag1: false,
    imply_both: false,
    flag2: true,
}

Note that flag2 is true.

Expected Behaviour

I expected that flag1, flag2, and imply_both would all be false.

Note that when I move the imply_both argument to the end of the Cli struct, it works as I expect:

use clap::{builder::ArgPredicate, Parser};

#[derive(Parser, Debug)]
struct Cli {
    #[arg(
        long,
        default_value_if("imply_both", ArgPredicate::IsPresent, Some("true"))
    )]
    flag1: bool,

    #[arg(
        long,
        default_value_if("imply_both", ArgPredicate::IsPresent, Some("true"))
    )]
    flag2: bool,

    /// imply both --flag1 and --flag2
    #[arg(long)]
    imply_both: bool,
}

fn main() {
    let cli = Cli::parse();
    println!("{:#?}", cli);
}
Cli {
    flag1: false,
    flag2: false,
    imply_both: false,
}

But with this ordering, the automatically generated help is not in my preferred order.

Additional Context

No response

Debug Output

[      clap::builder::command] 	Command::_do_parse
[      clap::builder::command] 	Command::_build: name="clapdefault"
[      clap::builder::command] 	Command::_propagate:clapdefault
[      clap::builder::command] 	Command::_check_help_and_version:clapdefault expand_help_tree=false
[      clap::builder::command] 	Command::long_help_exists
[      clap::builder::command] 	Command::_check_help_and_version: Building default --help
[      clap::builder::command] 	Command::_propagate_global_args:clapdefault
[clap::builder::debug_asserts] 	Command::_debug_asserts
[clap::builder::debug_asserts] 	Arg::_debug_asserts:flag1
[clap::builder::debug_asserts] 	Arg::_debug_asserts:imply_both
[clap::builder::debug_asserts] 	Arg::_debug_asserts:flag2
[clap::builder::debug_asserts] 	Arg::_debug_asserts:help
[clap::builder::debug_asserts] 	Command::_verify_positionals
[        clap::parser::parser] 	Parser::get_matches_with
[        clap::parser::parser] 	Parser::add_defaults
[        clap::parser::parser] 	Parser::add_defaults:iter:flag1:
[        clap::parser::parser] 	Parser::add_default_value: has conditional defaults
[        clap::parser::parser] 	Parser::add_default_value:iter:flag1: has default vals
[        clap::parser::parser] 	Parser::add_default_value:iter:flag1: wasn't used
[        clap::parser::parser] 	Parser::react action=SetTrue, identifier=None, source=DefaultValue
[   clap::parser::arg_matcher] 	ArgMatcher::start_custom_arg: id="flag1", source=DefaultValue
[        clap::parser::parser] 	Parser::push_arg_values: ["false"]
[        clap::parser::parser] 	Parser::add_single_val_to_arg: cur_idx:=1
[        clap::parser::parser] 	Parser::add_defaults:iter:imply_both:
[        clap::parser::parser] 	Parser::add_default_value: doesn't have conditional defaults
[        clap::parser::parser] 	Parser::add_default_value:iter:imply_both: has default vals
[        clap::parser::parser] 	Parser::add_default_value:iter:imply_both: wasn't used
[        clap::parser::parser] 	Parser::react action=SetTrue, identifier=None, source=DefaultValue
[   clap::parser::arg_matcher] 	ArgMatcher::start_custom_arg: id="imply_both", source=DefaultValue
[        clap::parser::parser] 	Parser::push_arg_values: ["false"]
[        clap::parser::parser] 	Parser::add_single_val_to_arg: cur_idx:=2
[        clap::parser::parser] 	Parser::add_defaults:iter:flag2:
[        clap::parser::parser] 	Parser::add_default_value: has conditional defaults
[        clap::parser::parser] 	Parser::react action=SetTrue, identifier=None, source=DefaultValue
[   clap::parser::arg_matcher] 	ArgMatcher::start_custom_arg: id="flag2", source=DefaultValue
[        clap::parser::parser] 	Parser::push_arg_values: ["true"]
[        clap::parser::parser] 	Parser::add_single_val_to_arg: cur_idx:=3
[        clap::parser::parser] 	Parser::add_defaults:iter:help:
[        clap::parser::parser] 	Parser::add_default_value: doesn't have conditional defaults
[        clap::parser::parser] 	Parser::add_default_value:iter:help: doesn't have default vals
[     clap::parser::validator] 	Validator::validate
[     clap::parser::validator] 	Validator::validate_conflicts
[     clap::parser::validator] 	Validator::validate_exclusive
[     clap::parser::validator] 	Validator::validate_required: required=ChildGraph([])
[     clap::parser::validator] 	Validator::gather_requires
[     clap::parser::validator] 	Validator::validate_required: is_exclusive_present=false
[   clap::parser::arg_matcher] 	ArgMatcher::get_global_values: global_arg_vec=[]
Cli {
    flag1: false,
    imply_both: false,
    flag2: true,
}
@drmikehenry drmikehenry added the C-bug Category: Updating dependencies label Feb 18, 2023
@drmikehenry
Copy link
Author

For reference, in Cargo.toml is:

[dependencies]
clap = { version = "4.1.6", features = ["debug", "derive"] }

@mattmadeofpasta
Copy link
Contributor

Using ArgPredicate::Equals("true".into()) instead of ArgPredicate::IsPresent does what you are trying to achieve.

use clap::{builder::ArgPredicate, Parser};

#[derive(Parser, Debug)]
struct Cli {
    #[arg(
        long,
        default_value_if("imply_both", ArgPredicate::Equals("true".into()), Some("true"))
    )]
    flag1: bool,

    /// imply both --flag1 and --flag2
    #[arg(long)]
    imply_both: bool,

    #[arg(
        long,
        default_value_if("imply_both", ArgPredicate::Equals("true".into()), Some("true"))
    )]
    flag2: bool,
}

fn main() {
    let cli = Cli::parse();
    println!("{:#?}", cli);
}
$ cargo run -q --  
Cli {
    flag1: false,
    imply_both: false,
    flag2: false,
}
$ cargo run -q --  --imply-both
Cli {
    flag1: true,
    imply_both: true,
    flag2: true,
}

I am not sure what is happening here with ArgPredicate::IsPresent. It looks like ArgPredicate::IsPresent also includes if the argument has already been defined and has a default value.

@drmikehenry
Copy link
Author

Thanks for the explanation; your suggestion works perfectly.

@epage
Copy link
Member

epage commented Feb 22, 2023

@mattmadeofpasta thanks for providing that answer!

I'm going to re-open though as generally the ArgPredicate::IsPresent means "it is explicitly present" which excludes defaults (but includes env variables) but default_value_if is not doing an explicit check.

@epage epage reopened this Feb 22, 2023
@epage epage added A-parsing Area: Parser's logic and needs it changed somehow. M-breaking-change Meta: Implementing or merging this will introduce a breaking change. labels Feb 22, 2023
@epage epage added this to the 5.0 milestone Feb 22, 2023
@epage
Copy link
Member

epage commented Feb 22, 2023

I went ahead and marked this as a breaking change and scheduled it for 5.0. This is one of those cases where the line between bug fix and breaking change is unclear, so I marked it as the more extreme case until a more thorough determination can be made.

drmikehenry added a commit to drmikehenry/git-ibundle that referenced this issue Feb 28, 2023
As shown in <clap-rs/clap#4713>, instead of
`ArgPredicate::IsPresent` to detect `--basis-current`, it should be
`ArgPredicate::Equals("true".into())`.

This allows arbitrary ordering of clap arguments so that help text can
be ordered as desired.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-bug Category: Updating dependencies M-breaking-change Meta: Implementing or merging this will introduce a breaking change.
Projects
None yet
Development

No branches or pull requests

3 participants