Feature: add Arg::required_unless_all

[gbear605]

Rust Version

rustc 1.24.0-nightly (7eb64b86c 2017-12-20)

Affected Version of clap

clap 2.29.0

Expected Behavior Summary

With the given sample app, I should be able to have the arguments:
-a -c -e
-a -c -f
-a -d -e
-a -d -f
-b -c -e
-b -c -f
-b -d -e
-b -d -f

The overall goal of the program is that I should have to call -x or I should have to call one from each of (-a, -b), (-c, -d), and (-e, -f). I should also never have both arguments from each argument group.

Actual Behavior Summary

For all of those combinations above, it says:
error: The following required arguments were not provided: -x

Steps to Reproduce the issue

Create a new binary Cargo project with Clap and the provided code as main.rs, run cargo run -- -a -c -e (or similar with any of the above combinations)

Sample Code or Link to Sample Code

extern crate clap;
use clap::{App, Arg};

fn main() {
    App::new("Sample App")
        .arg(Arg::with_name("a").conflicts_with("b").short("a"))
        .arg(Arg::with_name("b").short("b"))
        .arg(Arg::with_name("c").conflicts_with("d").short("c"))
        .arg(Arg::with_name("d").short("d"))
        .arg(Arg::with_name("e").conflicts_with("f").short("e"))
        .arg(Arg::with_name("f").short("f"))
        .arg(
            Arg::with_name("x")
                .required_unless_one(&["a", "b"])
                .required_unless_one(&["c", "d"])
                .required_unless_one(&["e", "f"])
                .short("x"),
        )
        .get_matches();
}

Debug output

kevin@beefcake: ~/Projects/fake 
➜ ./target/debug/fake -a -c -e                                                                        ✓ [git: master]
DEBUG:clap:Parser::propagate_settings: self=Sample App, g_settings=AppFlags(
    (empty)
)
DEBUG:clap:Parser::get_matches_with;
DEBUG:clap:Parser::create_help_and_version;
DEBUG:clap:Parser::create_help_and_version: Building --help
DEBUG:clap:Parser::create_help_and_version: Building --version
DEBUG:clap:Parser::get_matches_with: Begin parsing '"-a"' ([45, 97])
DEBUG:clap:Parser::is_new_arg:"-a":NotFound
DEBUG:clap:Parser::is_new_arg: arg_allows_tac=false
DEBUG:clap:Parser::is_new_arg: - found
DEBUG:clap:Parser::is_new_arg: starts_new_arg=true
DEBUG:clap:Parser::possible_subcommand: arg="-a"
DEBUG:clap:Parser::get_matches_with: possible_sc=false, sc=None
DEBUG:clap:ArgMatcher::process_arg_overrides:None;
DEBUG:clap:Parser::parse_short_arg: full_arg="-a"
DEBUG:clap:Parser::parse_short_arg:iter:a
DEBUG:clap:Parser::parse_short_arg:iter:a: Found valid flag
DEBUG:clap:Parser::check_for_help_and_version_char;
DEBUG:clap:Parser::check_for_help_and_version_char: Checking if -a is help or version...Neither
DEBUG:clap:Parser::parse_flag;
DEBUG:clap:ArgMatcher::inc_occurrence_of: arg=a
DEBUG:clap:ArgMatcher::inc_occurrence_of: first instance
DEBUG:clap:Parser::groups_for_arg: name=a
DEBUG:clap:Parser::groups_for_arg: No groups defined
DEBUG:clap:Parser:get_matches_with: After parse_short_arg Flag
DEBUG:clap:Parser::get_matches_with: Begin parsing '"-c"' ([45, 99])
DEBUG:clap:Parser::is_new_arg:"-c":Flag
DEBUG:clap:Parser::is_new_arg: arg_allows_tac=false
DEBUG:clap:Parser::is_new_arg: - found
DEBUG:clap:Parser::is_new_arg: starts_new_arg=true
DEBUG:clap:Parser::possible_subcommand: arg="-c"
DEBUG:clap:Parser::get_matches_with: possible_sc=false, sc=None
DEBUG:clap:ArgMatcher::process_arg_overrides:Some("a");
DEBUG:clap:Parser::parse_short_arg: full_arg="-c"
DEBUG:clap:Parser::parse_short_arg:iter:c
DEBUG:clap:Parser::parse_short_arg:iter:c: Found valid flag
DEBUG:clap:Parser::check_for_help_and_version_char;
DEBUG:clap:Parser::check_for_help_and_version_char: Checking if -c is help or version...Neither
DEBUG:clap:Parser::parse_flag;
DEBUG:clap:ArgMatcher::inc_occurrence_of: arg=c
DEBUG:clap:ArgMatcher::inc_occurrence_of: first instance
DEBUG:clap:Parser::groups_for_arg: name=c
DEBUG:clap:Parser::groups_for_arg: No groups defined
DEBUG:clap:Parser:get_matches_with: After parse_short_arg Flag
DEBUG:clap:Parser::get_matches_with: Begin parsing '"-e"' ([45, 101])
DEBUG:clap:Parser::is_new_arg:"-e":Flag
DEBUG:clap:Parser::is_new_arg: arg_allows_tac=false
DEBUG:clap:Parser::is_new_arg: - found
DEBUG:clap:Parser::is_new_arg: starts_new_arg=true
DEBUG:clap:Parser::possible_subcommand: arg="-e"
DEBUG:clap:Parser::get_matches_with: possible_sc=false, sc=None
DEBUG:clap:ArgMatcher::process_arg_overrides:Some("c");
DEBUG:clap:Parser::parse_short_arg: full_arg="-e"
DEBUG:clap:Parser::parse_short_arg:iter:e
DEBUG:clap:Parser::parse_short_arg:iter:e: Found valid flag
DEBUG:clap:Parser::check_for_help_and_version_char;
DEBUG:clap:Parser::check_for_help_and_version_char: Checking if -e is help or version...Neither
DEBUG:clap:Parser::parse_flag;
DEBUG:clap:ArgMatcher::inc_occurrence_of: arg=e
DEBUG:clap:ArgMatcher::inc_occurrence_of: first instance
DEBUG:clap:Parser::groups_for_arg: name=e
DEBUG:clap:Parser::groups_for_arg: No groups defined
DEBUG:clap:Parser:get_matches_with: After parse_short_arg Flag
DEBUG:clap:ArgMatcher::process_arg_overrides:Some("e");
DEBUG:clap:Parser::remove_overrides:[];
DEBUG:clap:Validator::validate;
DEBUG:clap:Parser::add_defaults;
DEBUG:clap:Validator::validate_blacklist;
DEBUG:clap:Validator::validate_blacklist:iter:c;
DEBUG:clap:Parser::groups_for_arg: name=c
DEBUG:clap:Parser::groups_for_arg: No groups defined
DEBUG:clap:Validator::validate_blacklist:iter:e;
DEBUG:clap:Parser::groups_for_arg: name=e
DEBUG:clap:Parser::groups_for_arg: No groups defined
DEBUG:clap:Validator::validate_blacklist:iter:a;
DEBUG:clap:Parser::groups_for_arg: name=a
DEBUG:clap:Parser::groups_for_arg: No groups defined
DEBUG:clap:Validator::validate_required: required=["x"];
DEBUG:clap:Validator::validate_required:iter:x:
DEBUG:clap:Validator::is_missing_required_ok: a=x
DEBUG:clap:Validator::validate_arg_conflicts: a="x";
DEBUG:clap:Validator::validate_required_unless: a="x";
DEBUG:clap:Validator::missing_required_error: extra=None
DEBUG:clap:Parser::color;
DEBUG:clap:Parser::color: Color setting...Auto
DEBUG:clap:is_a_tty: stderr=true
DEBUG:clap:Validator::missing_required_error: reqs=[
    "x"
]
DEBUG:clap:usage::get_required_usage_from: reqs=["x"], extra=None
DEBUG:clap:usage::get_required_usage_from: after init desc_reqs=[]
DEBUG:clap:usage::get_required_usage_from: no more children
DEBUG:clap:usage::get_required_usage_from: final desc_reqs=["x"]
DEBUG:clap:usage::get_required_usage_from: args_in_groups=[]
DEBUG:clap:usage::get_required_usage_from:iter:x:
DEBUG:clap:Colorizer::error;
DEBUG:clap:Validator::missing_required_error: req_args="\n    \u{1b}[1;31m-x\u{1b}[0m"
DEBUG:clap:usage::create_usage_with_title;
DEBUG:clap:usage::create_usage_no_title;
DEBUG:clap:usage::smart_usage;
DEBUG:clap:usage::get_required_usage_from: reqs=["x", "c", "e", "a"], extra=None
DEBUG:clap:usage::get_required_usage_from: after init desc_reqs=[]
DEBUG:clap:usage::get_required_usage_from: no more children
DEBUG:clap:usage::get_required_usage_from: final desc_reqs=["a", "c", "e", "x"]
DEBUG:clap:usage::get_required_usage_from: args_in_groups=[]
DEBUG:clap:usage::get_required_usage_from:iter:a:
DEBUG:clap:usage::get_required_usage_from:iter:c:
DEBUG:clap:usage::get_required_usage_from:iter:e:
DEBUG:clap:usage::get_required_usage_from:iter:x:
DEBUG:clap:Parser::color;
DEBUG:clap:Parser::color: Color setting...Auto
DEBUG:clap:is_a_tty: stderr=true
DEBUG:clap:Colorizer::error;
DEBUG:clap:Colorizer::good;
error: The following required arguments were not provided:
    -x

USAGE:
    fake -a -c -e -x

For more information try --help

[willmurphyscode]

I believe this is an issue where short() conflicts with required_unless_one(). In the original code sample, removing short("x") will produce the expected behavior. Here's a PR with a failing unit test: #1136 @kbknapp Do you have any thoughts on how/where to address this issue?

[gbear605]

From testing, this also occurs with long().

[gbear605]

Also, having multiple required_unless_one() doesn't work as I hoped it would - it combines them so that it just requires one of a, b, c, d, e, f, when I wanted it to required one of a, b, one of c, d, and one of e, f. Is there any way to do this with clap?

[kbknapp]

Thanks for filing! I'll start to take a look. I've been away for the holidays and traveling. @willmurphyscode I saw your test cases in the PR. Do you want to continue with this?

[willmurphyscode]

@kbknapp I will start digging today. I'll ping you if I get stuck.

[willmurphyscode]

@kbknapp I've been experimenting with the tests, trying to discover what is different between the passing and failing test runs, and the differences begin at this line:
https://github.com/kbknapp/clap-rs/blob/b6f36f55a04f8ed963a524cf1b87c7e7869964c2/src/app/parser.rs#L295

When there are no short or long forms of the arg that is "required_unless_one," then the arg is added as a positional, but if there are short or long forms, the arg is added as a flag. I believe that the correct behavior is accidentally dependent on the required_unless_one arg being treated as a positional and not as a flag, but I haven't figured out why yet. Does it seem like I'm on the right track? Do you have any insight as to where I might keep looking?

[kbknapp]

@willmurphyscode You're exactly correct about that line of code, but I'm not sure if that's the issue or not. You're right, the tests don't use an arg with a short as the one that defines (required_unless_one), but that shouldn't be the issue (at least it's not supposed to be).

There was recently a rework to how clap validates, so I've updated the debug output in the OP to v2.29.2 (where the error still occurs).

The error seems to say Validator::validate_required_unless is the one to blame. I'd start there and work backwards to see where it's getting the info from.

[kbknapp]

@gbear605 can you test this against the master branch and see if it fixes it for you? @willmurphyscode has come up with a fix, and if it works I'll release the new version.

[gbear605]

My issue now is somewhat the opposite of before, although this may be intended behavior:

It now allows me to do all the arguments I want, but it also allows me to have just one or two of them. For instance, I can pass in "-a -c" or "-a" and it works, but I instead want it to require "-x" unless all three of the other argument pairs are provided.

That's what I was trying to achieve with the three calls to .required_unless_one(), but it seems to combine all three of them into the equivalent of a single required_unless_one call.

I would say that overall this is an improvement over before, as my issue now is not clearly a bug and the clear bug I was seeing before is now gone.

My debug output now with passing in just "-a":

DEBUG:clap:Parser::propagate_settings: self=Sample App, g_settings=AppFlags(
    (empty)
)
DEBUG:clap:Parser::get_matches_with;
DEBUG:clap:Parser::create_help_and_version;
DEBUG:clap:Parser::create_help_and_version: Building --help
DEBUG:clap:Parser::create_help_and_version: Building --version
DEBUG:clap:Parser::get_matches_with: Begin parsing '"-a"' ([45, 97])
DEBUG:clap:Parser::is_new_arg:"-a":NotFound
DEBUG:clap:Parser::is_new_arg: arg_allows_tac=false
DEBUG:clap:Parser::is_new_arg: - found
DEBUG:clap:Parser::is_new_arg: starts_new_arg=true
DEBUG:clap:Parser::possible_subcommand: arg="-a"
DEBUG:clap:Parser::get_matches_with: possible_sc=false, sc=None
DEBUG:clap:ArgMatcher::process_arg_overrides:None;
DEBUG:clap:Parser::parse_short_arg: full_arg="-a"
DEBUG:clap:Parser::parse_short_arg:iter:a
DEBUG:clap:Parser::parse_short_arg:iter:a: Found valid flag
DEBUG:clap:Parser::check_for_help_and_version_char;
DEBUG:clap:Parser::check_for_help_and_version_char: Checking if -a is help or version...Neither
DEBUG:clap:Parser::parse_flag;
DEBUG:clap:ArgMatcher::inc_occurrence_of: arg=a
DEBUG:clap:ArgMatcher::inc_occurrence_of: first instance
DEBUG:clap:Parser::groups_for_arg: name=a
DEBUG:clap:Parser::groups_for_arg: No groups defined
DEBUG:clap:Parser:get_matches_with: After parse_short_arg Flag
DEBUG:clap:ArgMatcher::process_arg_overrides:Some("a");
DEBUG:clap:Parser::remove_overrides:[];
DEBUG:clap:Validator::validate;
DEBUG:clap:Parser::add_defaults;
DEBUG:clap:Validator::validate_blacklist;
DEBUG:clap:Validator::validate_blacklist:iter:a;
DEBUG:clap:Parser::groups_for_arg: name=a
DEBUG:clap:Parser::groups_for_arg: No groups defined
DEBUG:clap:Validator::validate_required: required=["x"];
DEBUG:clap:Validator::validate_required:iter:x:
DEBUG:clap:Validator::is_missing_required_ok: a=x
DEBUG:clap:Validator::validate_arg_conflicts: a="x";
DEBUG:clap:Validator::validate_required_unless: a="x";
DEBUG:clap:Validator::validate_matched_args;
DEBUG:clap:Validator::validate_matched_args:iter:a: vals=[]
DEBUG:clap:Validator::validate_arg_requires:a;
DEBUG:clap:Validator::validate_arg_num_occurs: a=a;
DEBUG:clap:usage::create_usage_with_title;
DEBUG:clap:usage::create_usage_no_title;
DEBUG:clap:usage::get_required_usage_from: reqs=["x"], extra=None
DEBUG:clap:usage::get_required_usage_from: after init desc_reqs=[]
DEBUG:clap:usage::get_required_usage_from: no more children
DEBUG:clap:usage::get_required_usage_from: final desc_reqs=["x"]
DEBUG:clap:usage::get_required_usage_from: args_in_groups=[]
DEBUG:clap:usage::get_required_usage_from:iter:x:
DEBUG:clap:usage::needs_flags_tag;
DEBUG:clap:usage::needs_flags_tag:iter: f=a;
DEBUG:clap:Parser::groups_for_arg: name=a
DEBUG:clap:Parser::groups_for_arg: No groups defined
DEBUG:clap:usage::needs_flags_tag:iter: [FLAGS] required
DEBUG:clap:usage::create_help_usage: usage=test_project [FLAGS] -x
DEBUG:clap:ArgMatcher::get_global_values: global_arg_vec=[]

[willmurphyscode]

@gbear605 Let me see if I understand this issue. You basically want this behavior (in some pseudocode here):

if (x.passed?) || ((a.passed? || b.passed?) && (c.passed? || d.passed?) && (e.passed? || f.passed?)):
   valid
else:
   invalid

Is that correct? Right now, required_unless_one simply builds up an array of arguments and treats the argument it modifies as optional unless one of them is passed. This seems like a new feature, basically making required_unless_one able to build up multiple predicates if it is called more than once on the same arg.

[gbear605]

That's correct.

So, it seems that the actual bug involved here was fixed by @willmurphyscode's fix and now this is just a feature request for that behavior.

[kbknapp]

Thanks for the work @willmurphyscode ! I'm going to remove the bug and 2.x labels. However, I'll leave this feature request open for when I'm opening the feature request window for 3.x.

I have to keep blinders on or I'll never finish 3.x.