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

clap4 requires that options implement Send and Sync #4347

Open
2 tasks done
alexcrichton opened this issue Oct 4, 2022 · 1 comment
Open
2 tasks done

clap4 requires that options implement Send and Sync #4347

alexcrichton opened this issue Oct 4, 2022 · 1 comment
Labels
A-derive Area: #[derive]` macro API C-bug Category: Updating dependencies S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing

Comments

@alexcrichton
Copy link
Contributor

alexcrichton commented Oct 4, 2022

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)

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

epage commented Oct 4, 2022

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>).

@epage epage added A-derive Area: #[derive]` macro API S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing labels Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-derive Area: #[derive]` macro API C-bug Category: Updating dependencies S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing
Projects
None yet
Development

No branches or pull requests

2 participants