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

default_value may not be included in possible_values and only checked in a runtime #3202

Open
2 tasks done
Gordon01 opened this issue Dec 21, 2021 · 4 comments · Fixed by #3423
Open
2 tasks done

default_value may not be included in possible_values and only checked in a runtime #3202

Gordon01 opened this issue Dec 21, 2021 · 4 comments · Fixed by #3423
Labels
A-builder Area: Builder API C-enhancement Category: Raise on the bar on expectations E-easy Call for participation: Experience needed to fix: Easy / not much
Milestone

Comments

@Gordon01
Copy link

Please complete the following tasks

  • I have searched the discussions
  • I have searched the existing issues

Clap Version

3.0.0-rc.7

Describe your use case

A configuration when a default_value is not listed in possible_values is valid and i only get error in runtime.

example
#[clap(short, long, default_value_t = 64, possible_values = ["16", "32"])]
this also works:
#[clap(short, long, default_value = "64", possible_values = ["16", "32"])]

Describe the solution you'd like

I want to get error at compile time.
Maybe implementing a #3201 may automatically resolve this issue.

#[clap(short, long, default_value_t = 128, possible_values = ["12", "16", "32"])]
fat: u8,

error: default value is not in range of possible values

Alternatives, if applicable

No response

Additional Context

No response

@Gordon01 Gordon01 added the C-enhancement Category: Raise on the bar on expectations label Dec 21, 2021
@epage
Copy link
Member

epage commented Dec 21, 2021

It would help if you included a fully working code sample and the current runtime error output. I have some guesses as to which errors you are seeing but I'd have to recreate a working sample to verify. This is needed to triage this for how important it is for resolving.

@epage epage added the S-triage Status: New; needs maintainer attention. label Dec 21, 2021
@Gordon01
Copy link
Author

use clap::Parser;

#[derive(Parser)]
#[clap(author, version, about)]
struct Cli {
    #[clap(default_value = "4", possible_values = ["1", "3", "5"])]
    count: u8,
}

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

    println!("Count: {}", cli.count);
}

cargo run -q

error: "4" isn't a valid value for '<COUNT>'
[possible values: 1, 3, 5]

So yeah, this is just an ergonomic issue, but it would be great if this error could be checked at compile time.

@epage
Copy link
Member

epage commented Dec 22, 2021

Thanks! i'm surprised we leave this to being an error from the validators.

The first priority would be to add a debug assert for this. That will catch it in all of our APIs. In our migration guide, we've recommended making tests out of the debug asserts. We should also add that to the regular documentation.

#2740 proposed porting our debug asserts to run at compile time. Not all of them thoroughly can (#3133) so even if we ignored the builder API, we'd still need the debug asserts in some cases. This is one that wouldn't run into that problem. When considering the builder API, we also have to deal with code duplication and code drift. That makes it unlikely for us to duplicate the debug asserts at compile time.

If we haven't, we should probably also have a debug assert that validates the default value against the validator function, if present.

@epage epage added A-builder Area: Builder API E-easy Call for participation: Experience needed to fix: Easy / not much and removed S-triage Status: New; needs maintainer attention. labels Dec 22, 2021
@epage epage added this to the 3.1 milestone Feb 2, 2022
epage added a commit to epage/clap that referenced this issue Feb 8, 2022
This can help people catch them via `App::debug_assert` rather than
waiting until the default is used and validated.

Fixes clap-rs#3202
epage added a commit to epage/clap that referenced this issue Jul 18, 2023
We'll need to re-evaluate how to solve clap-rs#3202.

Fixes clap-rs#4643
@epage
Copy link
Member

epage commented Jul 18, 2023

Because of #4643, #5017 reverted the fix for this

@epage epage reopened this Jul 18, 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-enhancement Category: Raise on the bar on expectations 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.

2 participants