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

Backwards incompatibility for ArgMatches and UnwindSafe #3876

Closed
2 tasks done
doivosevic opened this issue Jun 27, 2022 · 7 comments
Closed
2 tasks done

Backwards incompatibility for ArgMatches and UnwindSafe #3876

doivosevic opened this issue Jun 27, 2022 · 7 comments
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-bug Category: Updating dependencies S-waiting-on-decision Status: Waiting on a go/no-go before implementing

Comments

@doivosevic
Copy link

doivosevic commented Jun 27, 2022

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 doivosevic added the C-bug Category: Updating dependencies label Jun 27, 2022
@epage
Copy link
Member

epage commented Jun 27, 2022

I'm curious, how did you find this?

@epage epage added A-parsing Area: Parser's logic and needs it changed somehow. S-waiting-on-decision Status: Waiting on a go/no-go before implementing labels Jun 27, 2022
@doivosevic
Copy link
Author

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

@epage
Copy link
Member

epage commented Jun 27, 2022

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 added a commit to epage/clap that referenced this issue Jun 28, 2022
By checking these types, we'll get some other types for free, like
`Command` verifying `Arg`.

This was inspired by clap-rs#3876
epage added a commit to epage/clap that referenced this issue Jun 28, 2022
By checking these types, we'll get some other types for free, like
`Command` verifying `Arg`.

This was inspired by clap-rs#3876
epage added a commit to epage/clap that referenced this issue Jun 28, 2022
@epage
Copy link
Member

epage commented Jun 28, 2022

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

@doivosevic
Copy link
Author

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?

@epage
Copy link
Member

epage commented Jul 16, 2022

That most likely is blocked on breaking changes, see #1206

@epage
Copy link
Member

epage commented Oct 4, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-bug Category: Updating dependencies S-waiting-on-decision Status: Waiting on a go/no-go before implementing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants