clap-rs / clap

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

clap4 requires that options implement `Send` and `Sync` #4347

Open alexcrichton opened 2 years ago

alexcrichton commented 2 years ago

Maintainer's notes

Please complete the following tasks

Rust Version

rustc 1.64.0 (a55dd71d5 2022-09-19)

Clap Version

3.2.22 and 4.0.9

Minimal reproducible code

use std::cell::Cell;

fn main() {}

fn parse_option(_: &str) -> Result<Cell<u32>, Box<dyn std::error::Error + Send + Sync>> {
    loop {}
}

mod clap3 {
    use clap3 as clap;
    use std::cell::Cell;

    #[derive(clap::Parser)]
    struct MyApp {
        #[clap(long, parse(try_from_str = super::parse_option))]
        option: Cell<u32>,
    }
}

mod clap4 {
    use clap4 as clap;
    use std::cell::Cell;

    #[derive(clap::Parser)]
    struct MyApp {
        #[clap(long, value_parser = super::parse_option)]
        option: Cell<u32>,
    }
}

Steps to reproduce the bug with the above code

With this manifest:

[package]
name = "tmp"
version = "0.1.0"
edition = "2021"

[dependencies]
clap3 = { version = "3", features = ["derive"], package = 'clap' }
clap4 = { version = "4", features = ["derive"], package = 'clap' }

run cargo build

Actual Behaviour

The clap4 module fails to compile but the clap3 module succeeds.

Expected Behaviour

This application has no need to require that MyApp is either Send or Sync hence the usage off Cell, and ideally upgrading to clap 4 would preserve the ability to have this same type structure in options.

Additional Context

No response

Debug Output

No extra output is printed at this time. (this is a compile error not a runtime error)

epage commented 2 years ago

This is a byproduct of the value parser work we did in clap 3.2. We were wanting to overhaul how part of the derive worked, including parsing only once rather than once for validation and once to store in the struct. As part of this, we moved the relevant processing down into ArgMatches. As ArgMatches is Clone + Send + Sync, we had to force these traits onto the derived field types during clap 3.2. Since then, we have only received feedback on Clone which we are fairly limited on resolving.

If there is interest in relaxing Send + Sync, we can look into that for clap v5. The hard part will be understanding how important those traits are for builder users. I was already surprised when we accidentally removed an auto-trait I had never heard of and broke people

It'd be great if we could come up with a "choose your own adventure / constraints" API so the derive would require the minimal constraints while users can opt-in to greater constraints. As a feature flag, clone wouldn't work because adding it both adds constraints and removes constraints, so its breaking as either clone or no_clone (I also am wanting to keep feature flags to a minimum since they aren't always easy to work with).

Another option is if we can come up with a design for #3912, then users can workaround this by separating out what the parsed type is (u32) from the stored type (Cell<u32>).