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

Option to disable runtime-validation in debug release? #4405

Closed
2 tasks done
fanzeyi opened this issue Oct 19, 2022 · 4 comments
Closed
2 tasks done

Option to disable runtime-validation in debug release? #4405

fanzeyi opened this issue Oct 19, 2022 · 4 comments
Labels
A-builder Area: Builder API C-bug Category: Updating dependencies S-wont-fix Status: Closed as there is no plan to fix this

Comments

@fanzeyi
Copy link

fanzeyi commented Oct 19, 2022

Please complete the following tasks

Rust Version

rust playground 1.64.0

Clap Version

4.0.6

Minimal reproducible code

use clap::Parser;
use std::path::PathBuf;
use std::io::Result;

fn foobar(input: &str) -> Result<PathBuf> {
    println!("got input: {input}");
    if input.is_empty() {
        Ok(PathBuf::from("/"))
        // Err(std::io::Error::last_os_error())
    } else {
        Ok(PathBuf::from("/"))
    }
}

#[derive(Parser, Debug)]
#[clap(about = "Test")]
struct Args {
    #[clap(value_parser = foobar, default_value = "")]
    path: PathBuf,
}

fn main() {
    let args = Args::parse_from(vec!["binary", "/etc"]);
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=42894c861ebd752bb5d6f8624cdb88ca

Steps to reproduce the bug with the above code

Click Run button in playground.

Actual Behaviour

got input got called twice, once with the default empty string and once with supplied string.

Expected Behaviour

When the user supplies any value, the default value should not be used and parsed.

It looks like this happens in debug mode for validating supplied default values. Specifically this call here:

clap/src/builder/command.rs

Lines 3905 to 3906 in 6328c14

#[cfg(debug_assertions)]
assert_app(self);

Would it be possible to consider at least making this configurable? We run tests in both debug and release mode and this discrepancy is hard to work around. I can either write code in my parse function to cheat in debug mode (which is kinda awkward) or to avoid using default_value entirely. :(

Additional Context

No response

Debug Output

No response

@fanzeyi fanzeyi added the C-bug Category: Updating dependencies label Oct 19, 2022
@epage
Copy link
Member

epage commented Oct 19, 2022

Could you expand on why parsing the default twice in debug builds is a problem?

Would it be possible to consider at least making this configurable?

One way of doing this is feature flags but they have problems and should be limited in when we use them.

We do have help_expected and could add another flag like that but we need to make sure this feature pulls its weight for adding more noise to the API (something we are working on reducing).

The original issue is #3202 and PR is #3423. The big problem we are trying to address is that users can only catch some invalid cases by exhaustive testing of their CLI. Doing this in debug asserts lets users detect this sooner.

@fanzeyi
Copy link
Author

fanzeyi commented Oct 19, 2022

Could you expand on why parsing the default twice in debug builds is a problem?

So we are currently using default_value in combination with try_parse_str to compute a default_value if the user has not supplied it. It depends on where the user's current working directory is and as a result it may produce an error depending on where the user runs it.

I guess our setup isn't really using clap APIs "correctly". The X problem here is that there isn't an API to supply a function to calculate the default value without implementing Display or being OsString (I'm guessing this is for including the default value in help messages?). We want something like that since computing the default is nontrivial so we want to defer it as late as possible.

The big problem we are trying to address is that users can only catch some invalid cases by exhaustive testing of their CLI. Doing this in debug asserts lets users detect this sooner.

I understand the motivation of having this validation and I agree that having clap to validate it before it making to production is very helpful. I'm wondering if we can do the validation separately if we were to have an option disabling this runtime check.

@epage
Copy link
Member

epage commented Oct 20, 2022

So we are currently using default_value in combination with try_parse_str to compute a default_value if the user has not supplied it. It depends on where the user's current working directory is and as a result it may produce an error depending on where the user runs it.

If there isn't a default value to show in help, why give the default to clap instead of making the value optional and calculating it after parsing?

btw we are looking at generalizing how defaulting works. We've started brainstorming it at #3476

@epage epage added S-triage Status: New; needs maintainer attention. A-builder Area: Builder API labels Nov 8, 2022
@epage
Copy link
Member

epage commented Nov 8, 2022

As the original request is about providing clap with a sentinel default value that could just as well be handled by not providing a default, I'm inclined to close this. As listed above, we also are looking at allowing defaults to be handled in a different way. If people have input on that, see #3476.

@epage epage closed this as not planned Won't fix, can't repro, duplicate, stale Nov 8, 2022
@epage epage added S-wont-fix Status: Closed as there is no plan to fix this and removed S-triage Status: New; needs maintainer attention. labels Nov 8, 2022
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 S-wont-fix Status: Closed as there is no plan to fix this
Projects
None yet
Development

No branches or pull requests

2 participants