clap icon indicating copy to clipboard operation
clap copied to clipboard

Backwards incompatibility for ArgMatches and UnwindSafe

Open doivosevic opened this issue 2 years ago • 6 comments

Please complete the following tasks

Rust Version

1.60

Clap Version

between 3.1.18 and 3.2.6

Minimal reproducible code



fn a(a: ArgMatches) {
    b(a)
}

fn b<T: UnwindSafe>(b: T) {
    
}

Steps to reproduce the bug with the above code

cargo check

Actual Behaviour

Compile should pass

Expected Behaviour

Compile fails

Additional Context

This looks like a backwards incompatibility

Debug Output


error[E0277]: the type `(dyn std::any::Any + Sync + std::marker::Send + 'static)` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
  --> index/src/utils.rs:31:7
   |
31 |     b(a)
   |     - ^ `(dyn std::any::Any + Sync + std::marker::Send + 'static)` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
   |     |
   |     required by a bound introduced by this call
   |
   = help: the trait `RefUnwindSafe` is not implemented for `(dyn std::any::Any + Sync + std::marker::Send + 'static)`
   = note: required because of the requirements on the impl of `UnwindSafe` for `Arc<(dyn std::any::Any + Sync + std::marker::Send + 'static)>`
   = note: required because it appears within the type `parser::matches::any_value::AnyValue`
   = note: required because of the requirements on the impl of `UnwindSafe` for `Unique<parser::matches::any_value::AnyValue>`
   = note: required because it appears within the type `alloc::raw_vec::RawVec<parser::matches::any_value::AnyValue>`
   = note: required because it appears within the type `Vec<parser::matches::any_value::AnyValue>`
   = note: required because of the requirements on the impl of `UnwindSafe` for `Unique<Vec<parser::matches::any_value::AnyValue>>`
   = note: required because it appears within the type `alloc::raw_vec::RawVec<Vec<parser::matches::any_value::AnyValue>>`
   = note: required because it appears within the type `Vec<Vec<parser::matches::any_value::AnyValue>>`
   = note: required because it appears within the type `parser::matches::matched_arg::MatchedArg`
   = note: required because it appears within the type `indexmap::Bucket<clap::util::id::Id, parser::matches::matched_arg::MatchedArg>`
   = note: required because of the requirements on the impl of `UnwindSafe` for `Unique<indexmap::Bucket<clap::util::id::Id, parser::matches::matched_arg::MatchedArg>>`
   = note: required because it appears within the type `alloc::raw_vec::RawVec<indexmap::Bucket<clap::util::id::Id, parser::matches::matched_arg::MatchedArg>>`
   = note: required because it appears within the type `Vec<indexmap::Bucket<clap::util::id::Id, parser::matches::matched_arg::MatchedArg>>`
   = note: required because it appears within the type `indexmap::map::core::IndexMapCore<clap::util::id::Id, parser::matches::matched_arg::MatchedArg>`
   = note: required because it appears within the type `indexmap::map::IndexMap<clap::util::id::Id, parser::matches::matched_arg::MatchedArg>`
   = note: required because it appears within the type `ArgMatches`
note: required by a bound in `b`
  --> index/src/utils.rs:34:9
   |
34 | fn b<T: UnwindSafe>(b: T) {
   |         ^^^^^^^^^^ required by this bound in `b`

doivosevic avatar Jun 27 '22 17:06 doivosevic

I'm curious, how did you find this?

epage avatar Jun 27 '22 19:06 epage

I had code which was working which is let's say a wrapper for executing binaries on worker machines and when someone from my org bumped clap in develop my code broke after rebasing

doivosevic avatar Jun 27 '22 19:06 doivosevic

Note: I'm assuming we'd need to add another trait bound. It gets a little awkward making a breaking change to fix a breaking change but this would be unlikely to break someone.

epage avatar Jun 27 '22 19:06 epage

Some thoughts in working on a fix (#3878)

  • Auto-traits are a weird lot for ensuring compatibility on. Ideally, we could say what auto-traits are assumed and which aren't in a user-facing way. Instead we have to add compile-time tests for what auto traits we implement and don't. The big concern is accidentally implementing an auto-trait that we don't provide a guarantee for.
  • Its hard to decide whether we should intend a guarantee on this long term
  • I seem to be stuck in the implementation with parts dealing with Arc<dyn>. If we can't implement a fix for this, that puts us in an awkward space of either pulling the entire 3.2 release and breaking people or leaving it and letting this breakage continue (if we consider it a breakage).

epage avatar Jun 28 '22 02:06 epage

Can we then get an interface to get all the args from ArgMatches instead so that we can pass the args in some other format across this boundary?

doivosevic avatar Jun 28 '22 08:06 doivosevic

That most likely is blocked on breaking changes, see https://github.com/clap-rs/clap/issues/1206

epage avatar Jul 16 '22 02:07 epage

As clap v4 is now out, I'm going ahead and closing this.

epage avatar Oct 04 '22 15:10 epage