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

assert_defaults assumes parsers are stateless (e.g. don't check file system for a files existence) #4643

Closed
2 tasks done
kmanc opened this issue Jan 15, 2023 · 13 comments · Fixed by #5017
Closed
2 tasks done
Labels
C-bug Category: Updating dependencies

Comments

@kmanc
Copy link

kmanc commented Jan 15, 2023

Please complete the following tasks

Rust Version

1.66.0

Clap Version

4.1.1

Minimal reproducible code

use clap::{self, Arg, Command, ValueHint};
use std::path::Path;

fn main() -> () {
    let app = cli();
    let matches = app.get_matches();
    let wordlist = matches.get_one::<String>("wordlist").unwrap().to_string();
}

fn cli() -> Command {
    let app = clap::Command::new(clap::crate_name!())
        .version(clap::crate_version!())
        .author(clap::crate_authors!())
        .about(clap::crate_description!());

    app.arg(
        Arg::new("wordlist")
            .short('w')
            .value_name("WORDLIST")
            .num_args(1)
            .value_hint(ValueHint::FilePath)
            .default_value("/usr/share/wordlists/seclists/raft-medium-directories.txt")
            .value_parser(clap::builder::ValueParser::new(wordlist_parse_wrap))
            .help("Wordlist for web discovery"),
    )
}

fn wordlist_parse_wrap(wordlist: &str) -> Result<String, clap::Error> {
    if !Path::new(&wordlist).exists() {
        return Err(clap::Error::raw(
            clap::error::ErrorKind::ValueValidation,
            "File does not exist",
        ));
    }
    Ok(wordlist.to_string())
}

Steps to reproduce the bug with the above code

cargo run

Actual Behaviour

thread 'main' panicked at 'Argument wordlist's default_value="/usr/share/wordlists/seclists/raft-medium-directories.txt" failed validation: error: invalid value '/usr/share/wordlists/seclists/raft-medium-directories.txt' for '-w ': error: File does not exist

Expected Behaviour

error: invalid value '/usr/share/wordlists/seclists/raft-medium-directories.txt' for '-w ': error: File does not exist

Additional Context

I know the documentation states

Most error states are handled as asserts under the assumption they are programming mistake and not something to handle at runtime

But in this case it isn't a programmer mistake. The default value may or may not be valid in this case, but programmer mistakes should be catchable at compile time rather than runtime. Obviously the validity of the arg isn't knowable at compile time and can't be checked, but as a result a normal failure is more appropriate than a panic.

The two potential solutions that come to mind are:

  1. Completely remove the assert_default function
  2. Create a new flag that can disable its use

Option 1 seems like less work, but there may be valid uses of the function (I haven't come across one but I'm sure there's good reason for its existence). Option 2 would probably be more work, but it is more explicit and allows for the programmer to opt in to the unvalidated defaults.

Debug Output

No response

@kmanc kmanc added the C-bug Category: Updating dependencies label Jan 15, 2023
@epage epage changed the title assert_defaults panic instead of graceful failure assert_defaults assumes parsers are stateless (e.g. don't check file system for a files existence) Jan 16, 2023
@epage
Copy link
Member

epage commented Jan 16, 2023

I've clarified the title to what I see as the root issue.

The old title implied a third solution of having Command::build return an error which would not help in your situation as it still prevents you from parsing.

but programmer mistakes should be catchable at compile time rather than runtime. Obviously the validity of the arg isn't knowable at compile time and can't be checked, but as a result a normal failure is more appropriate than a panic.

Not all programmer mistakes can be. We also regularly encourage people to add a test to help catch these asserts so they are caught at test-time rather than in-prod.

However, seems irrelevant to the actual problem at hand. Your parser is dependent on the state of the system and so asserting on the status of the parser can fail.

there may be valid uses of the function (I haven't come across one but I'm sure there's good reason for its existence).

When a user sets a raw default (ie a string) it might not be valid. To catch this, the user needs to exhaustively test their application. By adding an assert, we guaranteed that people will catch these bugs so long as they activate the debug asserts in a test. See #3202.

Create a new flag that can disable its use

Flags come with API complexity and extra build time, both things that we are working on reducing.

#1695 is another route to resolving this.

Another option is for the application's file existence check to not be done within clap but to be done afterwards.

In deciding what route to go, something we should keep in mind is that this will also affect #4074.

@kmanc
Copy link
Author

kmanc commented Jan 16, 2023

That all makes sense, thanks for the explanations!

Specific to the following, I admit I hadn't considered the obvious, but as I think of it, surely I'm not the only one who has done something like this before. How do others usually tackle the problem of accepting a file as an argument? If there's a best practice around that I certainly don't know it but would like to!

Another option is for the application's file existence check to not be done within clap but to be done afterwards.

@epage
Copy link
Member

epage commented Jan 20, 2023

How do others usually tackle the problem of accepting a file as an argument? If there's a best practice around that I certainly don't know it but would like to!

In my applications I pass the Patrh around and let the failure happen naturally on access. For better error messages, some people use crates like fs-err

@kmanc
Copy link
Author

kmanc commented Jan 21, 2023

It feels a little weird to be able to validate some but not all arguments though doesn't it?

@epage
Copy link
Member

epage commented Jan 24, 2023

I doubt clap will ever support validation of every case. For example, an argument might depend on the value of another argument.

@kmanc
Copy link
Author

kmanc commented Feb 12, 2023

Reviewing this issue, I think it may be miscategorized. The real issue here is that a default argument that doesn't pass validation is panicking when it should more gracefully exit through https://github.com/clap-rs/clap/blob/master/src/error/format.rs#L313.

The valueparser shouldn't be responsible for managing state as you've mentioned, but it also shouldn't cause a panic when the mechanism for explaining that a value doesn't pass validation already exists

@epage
Copy link
Member

epage commented Feb 13, 2023

This is categorized correctly

We were previously deferring all default parse errors to parse-time which made error reporting dependent on a specific command line and for the developer/user to be able to distinguish this as a development-time error. We found people were mistyping their defaults and wanted to move this from "test the right combination of command lines" to a part of the process that will always catch this, our debug asserts. This is what #3202 is about which I linked earlier.

Maybe you meant that programmer errors should pass up as errors, rather than panic.

  • This won't help in your case because it will prevent parsing the command line
  • This would require having a failable builder. Currently, for large trees of subcommands, we build only what is needed. With a failable builder, we could force building all, slowing down those applications and hurting ergonomics by requiring an explicit build step, or we could mix it into the user error when doing a parse. The derive API does this in some cases and is a frequent source of support issues.

@kmanc
Copy link
Author

kmanc commented Feb 13, 2023

Maybe you meant that programmer errors should pass up as errors, rather than panic.

That is correct, and what I said in the original post. If that's not a change you intend to make that's fine, but the intuitiveness of clap suffers greatly as a result

@epage
Copy link
Member

epage commented Feb 13, 2023

From experience in supporting clap users, we've had to support people more from problems with reporting developer errors through Result than where we do it through panic.

Even this issue, so far I've seen nothing that would have been helped by reporting this through Result.

@kmanc
Copy link
Author

kmanc commented Feb 14, 2023

The difference is the user experience of a panic vs a graceful exit as if they had typed in any other non-valid value. If the default is a file they don't have on their machine because they prefer to use another they shouldn't get a panic without the opportunity to provide their own value

@epage
Copy link
Member

epage commented Feb 14, 2023

The difference is the user experience of a panic vs a graceful exit as if they had typed in any other non-valid value.

From my experience in my own apps and supporting other people's apps, our panics are a better user experience for development-time bugs.

If the default is a file they don't have on their machine because they prefer to use another they shouldn't get a panic without the opportunity to provide their own value

Yes, I recognize that our current approach does not support parsers that are dependent on state, like the filesystem. That is why this issue is still open. I was not objecting to that but to trying to re-categegorize this.

@aj-bagwell
Copy link

I ran into this with clio where I was trying to set the default directory to './' which was panicing when the current working directory did not exist.

After I worked out that the asserts are only included in debug builds so won't affect real users, I decided that it was not that big an issue for me.

@epage
Copy link
Member

epage commented Jul 18, 2023

But it means users of clio wouldn't be able to run / test debug builds.

In weighing this vs #3202, I'm considering reverting the fix for #3202 and re-opening it. We offer higher-level ways of doing most things that prevent bugs like that happening (default_value_t, #[derive(ValueEum)])

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Updating dependencies
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants