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

UnknownArgumentValueParser can only be used with string value argument #5081

Open
2 tasks done
weihanglo opened this issue Aug 19, 2023 · 2 comments
Open
2 tasks done
Labels
A-builder Area: Builder API C-bug Category: Updating dependencies M-breaking-change Meta: Implementing or merging this will introduce a breaking change.
Milestone

Comments

@weihanglo
Copy link

weihanglo commented Aug 19, 2023

Please complete the following tasks

Rust Version

rustc 1.72.0-beta.11 (7a3a43a3b 2023-08-18)

Clap Version

4.3.23

Minimal reproducible code

#!/usr/bin/env -S cargo +nightly -Zscript

//! ```cargo
//! [dependencies]
//! clap = "=4.3.23"
//! ```

fn main() {
    let args = clap::Command::new("test")
        .args([clap::Arg::new("libtest-ignore")
            .long("ignored")
            .action(clap::ArgAction::SetTrue)
            .value_parser(
                clap::builder::UnknownArgumentValueParser::suggest_arg("-- --ignored")
                    .and_suggest("not much else to say"),
            )
            .hide(true)])
        .get_matches();

    assert!(matches!(
        args.try_get_one::<bool>("libtest-ignore"),
        Err(clap::parser::MatchesError::Downcast { .. })
    ));

    args.try_get_one::<bool>("libtest-ignore").unwrap();
    // thread 'main' panicked at run.rs:25:48:
    // called `Result::unwrap()` on an `Err` value: Downcast { actual: alloc::string::String, expected: bool }
    // note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
}

Steps to reproduce the bug with the above code

Run the script above.

Actual Behaviour

I am not sure if this is expected. It turns out that the unknown argument must be defined as --flag <string>. Might be the result of StringValueParser in use.

Here is the current cargo integration that failed rust-lang/cargo#12529.

keep-going is always evaluated when constructing BuildConfig even it is not a valid argument. Cargo provides a default value for unknown argument, which contradicts the definition of bool --keep-going.

Expected Behaviour

UnknownArgumentValueParser can use with any type of argument.

Additional Context

cc #5080

Debug Output

No response

@epage
Copy link
Member

epage commented Aug 21, 2023

I had considered making the it parametrized but thought that might be too complex.

Maybe with a defaulted generic argument it won't be too bad and likely won't be a breaking change.

Alternatively, cargo could ignore the "invalid type" error since its pattern of reuse is a bit weird in of itself.

@epage
Copy link
Member

epage commented Aug 21, 2023

Even though in some circles, a new defaulted generic parameter is not a breaking change, for how UnknownArgumentValueParser is used, it is a breaking change so we can't resolve this until clap v5.

@epage epage added M-breaking-change Meta: Implementing or merging this will introduce a breaking change. A-builder Area: Builder API labels Aug 21, 2023
@epage epage added this to the 5.0 milestone Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-builder Area: Builder API C-bug Category: Updating dependencies M-breaking-change Meta: Implementing or merging this will introduce a breaking change.
Projects
None yet
Development

No branches or pull requests

2 participants