Bug when grouping positional args with options

[nayeemrmn]

Rust Version

rustc 1.42.0 (b8cedc004 2020-03-09)

Code

use clap;
use clap::Arg;
use clap::ArgGroup;

fn main() {
  let app = clap::App::new("hello")
    .bin_name("deno")
    .arg(
      Arg::with_name("option1")
        .long("option1")
        .takes_value(false),
    )
    .arg(
      Arg::with_name("pos1")
        .takes_value(true),
    )
    .group(
      ArgGroup::with_name("arg1")
        .args(&["pos1", "option1"])
        .required(true),
    )
    .arg(
      Arg::with_name("pos2")
        .takes_value(true)
    );

    match app.get_matches_from_safe(std::env::args().collect::<Vec<String>>()) {
      Ok(_) => (),
      Err(err) => err.exit(),
    }
}

Steps to reproduce the issue

cargo run -- -h

Affected Version of clap*

2.33.0

Actual Behavior Summary

USAGE:
    deno <pos1|--option1> [pos1]

cargo run -- --option1 abcd binds abcd to pos1 and complains about conflict between option1 and pos1

Blocks https://github.com/denoland/deno/pull/4635, ref https://github.com/denoland/deno/pull/4635#issuecomment-610548655.

Expected Behavior Summary

USAGE:
    deno <pos1|--option1> [pos2]

cargo run -- --option1 abcd should bind abcd to pos2. In other words, it should detect --option1 having been given as an option and accordingly skip pos1 which --option1 is grouped with.

--

If this is actually intended I welcome suggestions as to how I can get the above behaviour. But what else should be the semantics of grouping positional args with options?

---

IssueHunt Summary

Backers (Total: $20.00)

• pksunkara ($20.00)

Become a backer now!

Or submit a pull request to get the deposits!

Tips

• Checkout the Issuehunt explorer to discover more funded issues.
• Need some help from other developers? Add your repositories on IssueHunt to raise funds.

[pksunkara]

Yeah, this definitely looks like a bug. cc @CreepySkeleton

[CreepySkeleton]

Yes. I would expect all of the following to be valid invocations:

app pos1 pos2
app --option1=val
app --option1 val pos2 <- THIS FAILS

Working on the fix, might take a while.

[nayeemrmn]

app --option1=val
app --option1 val pos2 <- THIS FAILS

Small correction that --option1 doesn't take a value in my example but that would make for a better one.

[ldm0]

Should be reopen. The usage output is still not correct.

[pksunkara]

@ldm0 You were saying that the original fix is wrong. Can you point out a test case why it is wrong?

[ldm0]

@pksunkara Check this:

let m = clap::App::new("hello")
    .bin_name("deno")
    .arg(Arg::new("pos1").takes_value(true))
    .arg(Arg::new("option2").long("option2").takes_value(false))
    .arg(Arg::new("pos2").takes_value(true))
    .group(
        ArgGroup::new("arg2")
            .args(&["pos2", "option2"])
            .required(true),
    )
    .try_get_matches();

dbg!(m);

with:

cargo run -- --option2 abcd

You will notice it complains about conflict between option2 and pos2. However the abcd should be placed in pos1.

[issuehunt-oss[bot]]

@pksunkara has funded $20.00 to this issue.

---

• Submit pull request via IssueHunt to receive this reward.
• Want to contribute? Chip in to this issue via IssueHunt.
• Checkout the IssueHunt Issue Explorer to see more funded issues.
• Need help from developers? Add your repository on IssueHunt to raise funds.

[dmaahs2017]

Is this issue solved?

[ldm0]

Is this issue solved?

Nope, currently.

[pksunkara]

@ldm0 Can we focus on finishing this first? Or is it blocked by other stuff?

[rami3l]

@pksunkara @ldm0 The example above now passes on master with AppSettings::AllowMissingPositional:

/// Test for https://github.com/clap-rs/clap/issues/1794.
#[test]
fn positional_args_with_options() {
    let app = clap::App::new("hello")
        .bin_name("deno")
        .setting(AppSettings::AllowMissingPositional)
        .arg(Arg::new("option1").long("option1").takes_value(false))
        .arg(Arg::new("pos1").takes_value(true))
        .group(
            ArgGroup::new("arg1")
                .args(&["pos1", "option1"])
                .required(true),
        )
        .arg(Arg::new("pos2").takes_value(true));

    let m = app.get_matches_from(&["deno", "--option1", "abcd"]);
    assert!(m.is_present("option1"));
    assert!(!m.is_present("pos1"));
    assert!(m.is_present("pos2"));
    assert_eq!(m.value_of("pos2"), Some("abcd"));
}

And the usage is correctly shown as:

USAGE:
    deno <pos1|--option1> [pos2]

This AppSettings option is required due to the logic here, which indicates that a pos_counter bump is only available under certain conditions (which is needed for the example to work):

https://github.com/clap-rs/clap/blob/8b6034e668de0933ab0f88ad14a60a8f1d79a172/src/parse/parser.rs#L483-L484

Is this intended?

• If so, I can make a PR to add this test and finally close this issue.
• If not, maybe we should change the API to allow this special case of skipping certain pos args?

[pksunkara]

@ldm0 what do you think?

[ldm0]

This works:

use clap;
use clap::Arg;
use clap::ArgGroup;
use clap::AppSettings;

fn main() {
    let app = clap::App::new("hello")
        .bin_name("deno")
        .setting(AppSettings::AllowMissingPositional)
        .arg(Arg::new("option1").long("option1").takes_value(false))
        .arg(Arg::new("pos1").takes_value(true))
        .arg(Arg::new("pos2").takes_value(true))
        .group(
            ArgGroup::new("arg1")
                .args(&["pos1", "option1"])
                .required(true),
        )
        .get_matches();
}

But this is not:

use clap;
use clap::Arg;
use clap::ArgGroup;
use clap::AppSettings;

fn main() {
    let app = clap::App::new("hello")
        .bin_name("deno")
        .setting(AppSettings::AllowMissingPositional)
        .arg(Arg::new("option1").long("option1").takes_value(false))
        .arg(Arg::new("pos1").takes_value(true))
        .arg(Arg::new("pos2").takes_value(true))
        .arg(Arg::new("pos3").takes_value(true))
        .group(
            ArgGroup::new("arg1")
                .args(&["pos1", "pos2", "option1"])
                .required(true),
        )
        .get_matches();
}

error: The argument '--option1' cannot be used with '<pos1>'

USAGE:
    deno [pos3] <pos1|pos2|--option1>

For more information try --help

I don't think this issue can be closed now.

[epage]

I'm torn on this. AllowMissingPositional is very limited solution. The right way of doing this is to check for conflicts for positionals and skip them. Right now, conflicts are only a validation concern and not a parse concern and I was hoping to keep it that way for our modularization work. However, being able to control when to skip positionals in this ways is also associated with our desire to make clap more flexible.

Going to put this back in the design/decision status and going to remove the milestone.