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

Multi valued flags break collapsing short flags #5361

Closed
2 tasks done
BenWiederhake opened this issue Feb 17, 2024 · 2 comments
Closed
2 tasks done

Multi valued flags break collapsing short flags #5361

BenWiederhake opened this issue Feb 17, 2024 · 2 comments
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-bug Category: Updating dependencies S-waiting-on-decision Status: Waiting on a go/no-go before implementing

Comments

@BenWiederhake
Copy link
Contributor

BenWiederhake commented Feb 17, 2024

Please complete the following tasks

Rust Version

rustc 1.76.0 (07dca489a 2024-02-04)

Clap Version

4.5.1

Minimal reproducible code

use clap::{Arg, ArgAction, Command};
pub fn make_command() -> Command {
    Command::new("ouch")
        .arg(Arg::new("echo").short('e').num_args(0..))
        .arg(Arg::new("zero-terminated").short('z').action(ArgAction::SetTrue))
}
fn main() {
    let testcases = [
        // -e intentionally takes zero..many arguments:
        ("ouch -e", (vec![], false)),
        ("ouch -e asdf", (vec!["asdf"], false)),
        ("ouch -e foo bar", (vec!["foo", "bar"], false)),
        // When -e and -z are passed separately, it works well:
        ("ouch -e -z", (vec![], true)),
        ("ouch -e asdf -z", (vec!["asdf"], true)),
        ("ouch -e foo bar -z", (vec!["foo", "bar"], true)),
        ("ouch -z -e", (vec![], true)),
        ("ouch -z -e asdf", (vec!["asdf"], true)),
        ("ouch -z -e foo bar", (vec!["foo", "bar"], true)),
        // When -z goes first, there's no issue either:
        ("ouch -ze", (vec![], true)),
        ("ouch -ze asdf", (vec!["asdf"], true)),
        ("ouch -ze foo bar", (vec!["foo", "bar"], true)),
        // When they are passed together, all hell breaks loose:
        ("ouch -ez", (vec![], true)),
        ("ouch -ez asdf", (vec!["asdf"], true)),
        ("ouch -ez foo bar", (vec!["foo", "bar"], true)),
    ];
    for case in testcases {
        let (inputline, expected_result) = case;
        println!("input: {inputline}");
        match make_command().try_get_matches_from(inputline.split_whitespace()) {
            Ok(result) => {
                let echo_args: Vec<String> = result.get_many::<String>("echo").unwrap().map(String::from).collect();
                let actual_result = (echo_args, result.get_flag("zero-terminated"));
                println!("  expected: {expected_result:?}");
                println!("  actual:   {actual_result:?}");
                if expected_result.0 != actual_result.0 || expected_result.1 != actual_result.1 {
                    println!("  FAIL! <========");
                } else {
                    println!("  GOOD");
                }
            }
            Err(err) => {
                println!("Parsing failed?! {err:?}");
            }
        }
    }
}

Steps to reproduce the bug with the above code

The above code demonstrates several test cases. Among them is the problematic case -ez foo bar, where z is supposed to be interpreted as the noarg flag --zero-terminated. Note that behavior like this is sometimes desirable, e.g. when emulating GNU shuf:

$ gnushuf -ez foo bar baz | hd
00000000  62 61 72 00 66 6f 6f 00  62 61 7a 00              |bar.foo.baz.|
0000000c

Actual Behaviour

Clap interprets the z in -ez foo bar as the sole argument to -e, as if there was a call to value_delimiter. clap then errors, and refuses to understand the foo bar part of the command.

Expected Behaviour

Clap should (provide a switch to enable a mode in order to) prefer short-arg expansion over argument-taking. In other words, I want -ez foo bar to be interpreted as the two separate concepts --echo foo bar and --zero-terminated.

Additional Context

I understand that this might be considered a breaking change. I can see at least two ways to go about this:

  1. Make it opt-in on the Command-level, i.e. Command::single_dash_is_always_train_of_short_args(true) or something like that, so that all agglomerations of short flags are switched here.
  2. Make it opt-in on the Arg-level, i.e. Arg::permit_direct_short_arg(false) or something like that, so that -eARG is not even considered.

I admit that this may not be a "bug" in the "crash" sense, but it's definitely an incompatibility of existing features.

Debug Output

No response

Similar but different issues:

@BenWiederhake BenWiederhake added the C-bug Category: Updating dependencies label Feb 17, 2024
@epage epage added A-parsing Area: Parser's logic and needs it changed somehow. S-waiting-on-decision Status: Waiting on a go/no-go before implementing labels Feb 19, 2024
@epage
Copy link
Member

epage commented Feb 19, 2024

-ez foo bar takes the z as the argument and assumes that an attached value and unattached values are not mixed, and stops parsing -e.

Without other context, this feels niche enough that it wouldn't make sense to move forward on. As a user, I would be confused to see -ez foo bar and for that to be parsed the same as -ze foo bar. This would be complicated to implement, deferring the evaluation of short arguments while processing other arguments, which comes at the cost of binary size and compile times for all users who don't want this behavior. This would also has a cost in contributing to API bloat. Every new knob we add makes it harder to find every other knob that exists, including what we just added. The more we add, the less likely people will use any of it.

@BenWiederhake
Copy link
Contributor Author

Turns out, I wanted a boolean flag all along, and clap easily supports it. Sorry for the noise, and thanks for the quick response!

@BenWiederhake BenWiederhake closed this as not planned Won't fix, can't repro, duplicate, stale Feb 20, 2024
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 S-waiting-on-decision Status: Waiting on a go/no-go before implementing
Projects
None yet
Development

No branches or pull requests

2 participants