Tracking Issue for impl UnwindSafe/RefUnwindSafe for Condvar

[ecton]

The feature gate for the issue is #![feature(unwindsafe_condvar)].

This is a tracking issue for implementing core::panic::UnwindSafe and core::panic::RefUnwindSafe for std::sync::Condvar.

Currently, UnwindSafe and RefUnwindSafe are only implemented for some targets (See #118009). It was suggested a PR could be submitted to fix this inconsistency. However, in looking at how Mutex and RwLock get their unwind safety, I decided the best course of action would be to add the impl right next to those similar implementations:

rust/library/std/src/panic.rs

Lines 66 to 74 in 9afdb8d

	#[stable(feature = "catch_unwind", since = "1.9.0")]
	impl<T: ?Sized> UnwindSafe for Mutex<T> {}
	#[stable(feature = "catch_unwind", since = "1.9.0")]
	impl<T: ?Sized> UnwindSafe for RwLock<T> {}
	#[stable(feature = "unwind_safe_lock_refs", since = "1.12.0")]
	impl<T: ?Sized> RefUnwindSafe for Mutex<T> {}
	#[stable(feature = "unwind_safe_lock_refs", since = "1.12.0")]
	impl<T: ?Sized> RefUnwindSafe for RwLock<T> {}

By adding the implementation this way, my understanding is that the first step is submitting this tracking issue because impls are insta-stable.

Steps

• Add the two implementations
• Final comment period (FCP)
• Stabilization PR

Unresolved Questions

• Are the implementations on all targets unwind safe? I'm the original reporter of the issue, and was encouraged to submit a PR, but I don't actually know if all of the implementations are actually unwind safe.

[Noratrieb]

We don't use tracking issues for impls exactly because they are insta stable. Tracking issues are for tracking the state of merged unstable features.

[ecton]

We don't use tracking issues for impls exactly because they are insta stable. Tracking issues are for tracking the state of merged unstable features.

So how do I resolve this error:

 error: an `#[unstable]` annotation here has no effect
  --> library/std/src/panic.rs:70:1
   |
70 | #[unstable(feature = "unwindsafe_condvar", issue = "118009")]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: see issue #55436 <https://github.com/rust-lang/rust/issues/55436> for more information
   = note: `#[deny(ineffective_unstable_trait_impl)]` on by default

In other words: how do I submit a PR that isn't broken if I don't have a tracking feature name for this impl block? Or do you disagree that this is the way it should be implemented?

[ecton]

In case it wasn't clear how I was suggesting the change, it's simply to add the two impls side-by-side with the other synchronization primitives:

index 3728d5b64b8..6b4105bd91d 100644
--- a/library/std/src/panic.rs
+++ b/library/std/src/panic.rs
@@ -6,7 +6,7 @@
 use crate::collections;
 use crate::panicking;
 use crate::sync::atomic::{AtomicU8, Ordering};
-use crate::sync::{Mutex, RwLock};
+use crate::sync::{Condvar, Mutex, RwLock};
 use crate::thread::Result;
 
 #[doc(hidden)]
@@ -67,11 +67,15 @@ pub fn panic_any<M: 'static + Any + Send>(msg: M) -> ! {
 impl<T: ?Sized> UnwindSafe for Mutex<T> {}
 #[stable(feature = "catch_unwind", since = "1.9.0")]
 impl<T: ?Sized> UnwindSafe for RwLock<T> {}
+#[unstable(feature = "unwindsafe_condvar", issue = "118009")]
+impl UnwindSafe for Condvar {}
 
 #[stable(feature = "unwind_safe_lock_refs", since = "1.12.0")]
 impl<T: ?Sized> RefUnwindSafe for Mutex<T> {}
 #[stable(feature = "unwind_safe_lock_refs", since = "1.12.0")]
 impl<T: ?Sized> RefUnwindSafe for RwLock<T> {}
+#[unstable(feature = "unwindsafe_condvar", issue = "118009")]
+impl RefUnwindSafe for Condvar {}
 
 // https://github.com/rust-lang/rust/issues/62301
 #[stable(feature = "hashbrown", since = "1.36.0")]

Without the #[unstable] attribute, I get this error when building:

error: implementation has missing stability attribute
  --> library/std/src/panic.rs:71:1
   |
71 | impl UnwindSafe for Condvar {}
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Noratrieb]

You cannot use unstable on impls traits, it needs to be stable.

[ecton]

I understand that. What details do I put in the #[stable] attribute without a tracking issue? It seems like the only allowed format require linking to an issue and a name?

[Noratrieb]

Stable only requires a name and version (which I'd just copy from Mutex and RwLock)

[ecton]

Last question: Wouldn't that cause the documentation to say that Condvar implemented the traits since a specific Rust older version? If so, wouldn't it be a concern that the docs would be misleading given that Linux is one of the only platforms it currently is unwind safe on?

[Noratrieb]

🤷 I don't think it really matters, I would call it a bug before and this is fixing the bug of it always being supposed to inpelem the traits