clap-rs / clap

A full featured, fast Command Line Argument Parser for Rust
docs.rs/clap
Apache License 2.0
14.31k stars 1.04k forks source link

default_value_if depends on order in struct #4713

Open drmikehenry opened 1 year ago

drmikehenry commented 1 year ago

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 commented 1 year ago

For reference, in Cargo.toml is:

[dependencies]
clap = { version = "4.1.6", features = ["debug", "derive"] }
mattmadeofpasta commented 1 year ago

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 commented 1 year ago

Thanks for the explanation; your suggestion works perfectly.

epage commented 1 year ago

@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 commented 1 year ago

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.

epage commented 1 year ago

Past changes like this were treated as non-breaking