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

infer_subcommands should not fail if the ambiguity is over aliases #5021

Closed
2 tasks done
Xophmeister opened this issue Jul 19, 2023 · 3 comments · Fixed by #5025
Closed
2 tasks done

infer_subcommands should not fail if the ambiguity is over aliases #5021

Xophmeister opened this issue Jul 19, 2023 · 3 comments · Fixed by #5025
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-bug Category: Updating dependencies E-easy Call for participation: Experience needed to fix: Easy / not much

Comments

@Xophmeister
Copy link

Xophmeister commented Jul 19, 2023

Please complete the following tasks

Rust Version

rustc 1.70.0 (90c541806 2023-05-31) (built from a source tarball)

Clap Version

4.3.12

Minimal reproducible code

use clap::{Parser, Subcommand};

#[derive(Parser, Debug)]
#[command(infer_subcommands = true)]
struct TestCli {
    #[command(subcommand)]
    command: Commands,
}

#[derive(Debug, Subcommand)]
enum Commands {
    #[command(alias = "config")]
    Cfg,
}

fn main() {
    let cli = TestCli::parse();
    println!("{:?}", cli);
}

Steps to reproduce the bug with the above code

cargo run -- c

Actual Behaviour

It recognises the ambiguity between config and cfg:

error: unrecognized subcommand 'c'

  tip: some similar subcommands exist: 'config', 'cfg'
  tip: to pass 'c' as a value, use 'example -- c'

Usage: example <COMMAND>

For more information, try '--help'.

Expected Behaviour

When an ambiguity only spans a single subcommand and its aliases, then it should resolve to that subcommand:

TestCli { command: Cfg }

Additional Context

infer_long_args may also have the same behaviour; I haven't checked.

Debug Output

[clap_builder::builder::command]Command::_do_parse
[clap_builder::builder::command]Command::_build: name="example"
[clap_builder::builder::command]Command::_propagate:example
[clap_builder::builder::command]Command::_check_help_and_version:example 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:example
[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::get_matches_with
[clap_builder::parser::parser]Parser::get_matches_with: Begin parsing '"c"'
[clap_builder::parser::parser]Parser::possible_subcommand: arg=Ok("c")
[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::create_help_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::get_args: incls=[]
[ clap_builder::output::usage]Usage::get_args: unrolled_reqs=[]
[ clap_builder::output::usage]Usage::get_args: ret_val=[]
[ clap_builder::output::usage]Usage::create_help_usage: usage=example <COMMAND>
[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
@Xophmeister Xophmeister added the C-bug Category: Updating dependencies label Jul 19, 2023
Xophmeister added a commit to tweag/topiary that referenced this issue Jul 19, 2023
@epage
Copy link
Member

epage commented Jul 19, 2023

#4815 has some more information on this though that was covering multiple issues with inferring, so I've closed it out for one specific one, pointing here.

@epage epage added the A-parsing Area: Parser's logic and needs it changed somehow. label Jul 19, 2023
@SUPERCILEX
Copy link
Contributor

Won't be able to do research anytime soon, but if it's decided that this issue should be fixed, I can put up my PR.

@epage
Copy link
Member

epage commented Jul 19, 2023

When there is ambiguity between a subcommand and its alias, it would be an error today. Turning error cases into success is not a breaking change.

We have three different inference checks

  • subcommand names: doesn't handle aliases
  • subcommand longs: handles aliases
  • longs: handles aliases

(based on code inspection and experimentation in #4815)

So resolving this would resolve an inconsistency.

Deciding on the consistency to error (prefer subcommand names approach) would likely be considered a breaking change. It is also an overall lesser user experience. The only reason we may consider it is if we felt it would offer enough binary size savings that we weigh that out over the impact of people affected by this behavior. I have a hard time seeing how this would make a significant difference in binary size; many similar size changes go in without being measured.

So from that, I'd be willing to merge a fix for this. I would ask that the fix ensure that tests exist for subcommand longs and longs, adding them if they don't. A nice to have is to have the first commit add the test, verifying the existing behavior (error) and the second commit fixes the bug and updates the test. This helps show confirm to reviewers (1) the test is testing the right thing and (2) the change has the intended afect.

@epage epage added the E-easy Call for participation: Experience needed to fix: Easy / not much label Jul 19, 2023
Xophmeister added a commit to tweag/topiary that referenced this issue Aug 14, 2023
* Add env feature to clap and cargo update

* Dead Code: WIP CLI changes

* Dead Code: CLI feature parity

Although see clap-rs/clap#5020

* Note about not (yet) using infer_subcommands

See clap-rs/clap#5021

* Use clap/wrap_help

Although see clap-rs/clap#5022

* WIP: Strip out previous CLI argument parser setup

[skip ci]

* WIP: Normalise arguments for caller

[skip ci]

* Don't check for file-ness in the CLI argument parser

* File and directory canonicalisation for the argument parser

* Expose CLI types so they can be used downstream
Xophmeister added a commit to tweag/topiary that referenced this issue Aug 14, 2023
* Add env feature to clap and cargo update

* Dead Code: WIP CLI changes

* Dead Code: CLI feature parity

Although see clap-rs/clap#5020

* Note about not (yet) using infer_subcommands

See clap-rs/clap#5021

* Use clap/wrap_help

Although see clap-rs/clap#5022

* WIP: Strip out previous CLI argument parser setup

[skip ci]

* WIP: Normalise arguments for caller

[skip ci]

* Don't check for file-ness in the CLI argument parser

* File and directory canonicalisation for the argument parser

* Expose CLI types so they can be used downstream
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 E-easy Call for participation: Experience needed to fix: Easy / not much
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants