clap-rs / clap

A full featured, fast Command Line Argument Parser for Rust
docs.rs/clap
Apache License 2.0
13.66k stars 1.02k forks source link

Consider relaxing the bounds of Send + Sync on TypedValueParser #5347

Closed csmclaren closed 4 months ago

csmclaren commented 4 months ago

Please complete the following tasks

Clap Version

4.5.0

Describe your use case

  1. It's quite ergonomic that the value_parser attribute allows one to express the following in a single line, delegating option argument parsing to their library's existing parsing functionality without any extra code:

[arg(long, short, value_parser = |s: &str| MyObject::try_from(s))]

my_object: MyObject

  1. If an error is encountered during a call to Cli::parse(), Clap will print the first line of the error message, which includes the option name "--my-object" and option argument name "MY_OBJECT". This is elegant, as it decouples the option and option argument names from the user's library. Then, clap will print the Error returned by my TryFrom implementation which can contain more detailed information that only the user's library knows.

error: invalid value '123' for '--my-object ': <- from clap

detailed information <- from the error returned by TryFrom

Because TypedValueParser enforces Send + Sync, one can't compile any value_parser function which does not adhere to these bounds.

In my case, one component of the Error object returned by TryFrom contains an instance of RefCell. This is a requirement of the implementation of my particular library and non-trivial to work around. As such, it's impossible for me to use the above pattern.

Describe the solution you'd like

Please consider, if possible, relaxing the bounds on TypedValueParser.

Alternatives, if applicable

No response

Additional Context

No response

epage commented 4 months ago

I assume you mean Send + Sync on the error type?

Loosening that would be a breaking change because clap's error type would no longer be Send + Sync which can then affect using it with other parts of the Rust ecosystem.

There is a general assumption that errors are Send + Sync, e.g. anyhow requires it.

A workaround is to render the error before passing it back up to clap by using .to_string().

csmclaren commented 4 months ago

Yes, thank you, I did mean Send + Sync on the Error.

While I'm reticent to say that all errors should implement Send + Sync, I can see now that this is a general assumption in a number of libraries and why you might not wish to consider the change.

Furthermore, I think the workaround you suggested is ultimately the better option, in part because it's so short to implement.

For others who might encounter this issue, the workaround for my example above is as follows:

[arg(long, short, value_parser = |s: &str| MyObject::try_from(s).map_err(|e| e.to_string()))]

my_object: MyObject,

And here is a variation on the workaround that does not use a closure and is explicit about the error type:

[arg(long, short, value_parser = parse_my_object))]

my_object: MyObject,

fn parse_my_object(s: &str) -> Result<MyObject, Box<dyn std::error::Error + Send + Sync>> { MyObject::try_from(s).map_err(|e| e.to_string().into()) }