clap-rs / clap

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

Subcommand Conflicts with Argument (`derive` feature) #5513

Open lubomirkurcak opened 5 months ago

lubomirkurcak commented 5 months ago

Please complete the following tasks

Rust Version

rustc 1.78.0 (9b00956e5 2024-04-29)

Clap Version

clap = { version = "4.5.4", features = ["derive"] }

Minimal reproducible code

use clap::{Parser, Subcommand};

#[derive(Debug, Parser)]
struct Cli {
    name: String,
    #[command(subcommand)]
    command: Commands,
}

#[derive(Debug, Subcommand)]
enum Commands {
    Test,
}

fn main() {
    let args = Cli::parse();

    dbg!(&args);
}

Steps to reproduce the bug with the above code

OK

Positional name argument is hello, subcommand is test:

cargo run -- hello test

BAD

Positional name argument is test, subcommand is test:

cargo run -- test test

Actual Behaviour

OK

cargo run -- hello test
[src\main.rs:18:5] &args = Cli {
    name: "hello",
    command: Test,
}

BAD

cargo run -- test test
error: unexpected argument 'test' found

Usage: testing.exe <NAME> test

For more information, try '--help'.

Expected Behaviour

Clap should parse the arguments positionally:

cargo run -- test test
[src\main.rs:18:5] &args = Cli {
    name: "test",
    command: Test,
}

Additional Context

No response

Debug Output

cargo run -- test test [clap_builder::builder::command]Command::_do_parse [clap_builder::builder::command]Command::_build: name="testing" [clap_builder::builder::command]Command::_propagate:testing [clap_builder::builder::command]Command::_check_help_and_version:testing expand_help_tree=false [clap_builder::builder::command]Command::long_help_exists [clap_builder::builder::command]Command::_check_help_and_version: Building default --help [clap_builder::builder::command]Command::_check_help_and_version: Building help subcommand [clap_builder::builder::command]Command::_propagate_global_args:testing [clap_builder::builder::debug_asserts]Command::_debug_asserts [clap_builder::builder::debug_asserts]Arg::_debug_asserts:name [clap_builder::builder::debug_asserts]Arg::_debug_asserts:help [clap_builder::builder::debug_asserts]Command::_verify_positionals [clap_builder::parser::parser]Parser::get_matches_with [clap_builder::parser::parser]Parser::get_matches_with: Begin parsing '"test"' [clap_builder::parser::parser]Parser::possible_subcommand: arg=Ok("test") [clap_builder::parser::parser]Parser::get_matches_with: sc=Some("test") [clap_builder::parser::parser]Parser::parse_subcommand [ clap_builder::output::usage]Usage::get_required_usage_from: incls=[], matcher=false, incl_last=true [ clap_builder::output::usage]Usage::get_required_usage_from: unrolled_reqs=["name"] [ clap_builder::output::usage]Usage::get_required_usage_from:iter:"name" arg is_present=false [ clap_builder::output::usage]Usage::get_required_usage_from: ret_val=[StyledStr("")] [clap_builder::builder::command]Command::_build_subcommand Setting bin_name of test to "testing.exe test" [clap_builder::builder::command]Command::_build_subcommand Setting display_name of test to "testing-test" [clap_builder::builder::command]Command::_build: name="test" [clap_builder::builder::command]Command::_propagate:test [clap_builder::builder::command]Command::_check_help_and_version:test expand_help_tree=false [clap_builder::builder::command]Command::long_help_exists [clap_builder::builder::command]Command::_check_help_and_version: Building default --help [clap_builder::builder::command]Command::_propagate_global_args:test [clap_builder::builder::debug_asserts]Command::_debug_asserts [clap_builder::builder::debug_asserts]Arg::_debug_asserts:help [clap_builder::builder::debug_asserts]Command::_verify_positionals [clap_builder::parser::parser]Parser::parse_subcommand: About to parse sc=test [clap_builder::parser::parser]Parser::get_matches_with [clap_builder::parser::parser]Parser::get_matches_with: Begin parsing '"test"' [clap_builder::parser::parser]Parser::possible_subcommand: arg=Ok("test") [clap_builder::parser::parser]Parser::get_matches_with: sc=None [clap_builder::parser::parser]Parser::get_matches_with: Positional counter...1 [clap_builder::parser::parser]Parser::get_matches_with: Low index multiples...false [ clap_builder::output::usage]Usage::create_usage_with_title [ clap_builder::output::usage]Usage::create_usage_no_title [ clap_builder::output::usage]Usage::write_help_usage [ clap_builder::output::usage]Usage::write_arg_usage; incl_reqs=true [ clap_builder::output::usage]Usage::needs_options_tag [ clap_builder::output::usage]Usage::needs_options_tag:iter: f=help [ clap_builder::output::usage]Usage::needs_options_tag:iter Option is built-in [ clap_builder::output::usage]Usage::needs_options_tag: [OPTIONS] not required [ clap_builder::output::usage]Usage::write_args: incls=[] [ clap_builder::output::usage]Usage::get_args: unrolled_reqs=[] [ clap_builder::output::usage]Usage::write_subcommand_usage [ clap_builder::output::usage]Usage::create_usage_with_title: usage=Usage: testing.exe test [clap_builder::builder::command]Command::color: Color setting... [clap_builder::builder::command]Auto [clap_builder::builder::command]Command::color: Color setting... [clap_builder::builder::command]Auto error: unexpected argument 'test' found

Usage: testing.exe test

For more information, try '--help'.

epage commented 5 months ago

We have subcommand_precedence_over_arg but that only applies to options; subcommands always have precedence over positionals today (the name makes it sounds more general).

Some potential routes to go

Do nothing, discouraging mixing of positionals being additive with subcommands. maybe we could add some debug asserts to help with this though that can get complicated with various settings.

We break compatibility and default to positionals having higher precedence, controlled by subcommand_precedence_over_arg.

The parser could detect there are unsatisfied requireds and ensure give them precedene. Like with the debug asserts for the "do nothing" solution, this get complicated due to the various states we can be in

Add a new setting to toggle control over this. We've generally been working to shrinking the API, rather than growing it. The more features we add to the API, the less value we get out of them because they become harder to discover and be used.

ZakharEl commented 5 months ago

I see no reason to do this. derive should not incur any runtime costs for unused features, only compile time. I agree with Lubomirkurcak. This makes your library unuseable for many use cases.

ZakharEl commented 5 months ago

I would agree with Epage's case if the arg was an optional arg though.