`UnwindSafe` is unergonomic

[dpc]

As I expressed on IRC, I feel like, unless quickly taken care of, UnwindSafe is going to be a "failed feature". I've seen now quite a bit of people that told me that "they just wrap everything in AssertUnwindSafe" defeating the whole purpose of it. I've also seen comments that UnwindSafe is a PITA - sentiment that I'm beginning to share. I am tempted to ignore UnwindSafe completely as well, even though I'd like to do stuff properly. Please hear me out.

UnwindSafe and RefUnwindSafe are not too complicated, but they force people into writing a lot of boilerplate, and the worst part once used ... force that boilerplate on all other users, that might not even know what is it all about.

Static dispatch works OK, since similarly to Send and Sync any struct will get UnwindSafe and RefUnwindSafe if all fields of it satisfy it.

The problem is dynamic dispatch, trait object and trait bounds. Any time someone has to use eg. Box<TraitObject>, even if it's only due to lack of impl Trait on stable, that person most probably should have done Box<Trait + RefUnwindSafe>. Otherwise that Box<Trait> does not satisfy SafeUnwind, even though most probably that would be the intention. After all "This trait is namely not implemented by UnsafeCell, the root of all interior mutability".

And even if that person is aware of "unwind safety", after putting +RefUnwindSafe it won't work because:

only Send/Sync traits can be used as additional traits in a trait object 

To get this working a lot of boilerplate needs to be added. Example in my code which is just a PITA. And the worst part: after putting UnwindSafe bound in types used in open-traits (to be implemented by users of a library), now all users have to satisfy that bound. They will have to remember about it, add blanket implementations adding UnwindSafe for every type used as trait object, and get libraries they might want to use it, to do the same...

I don't know. Maybe I'm missing something here, but I feel like at least +UnwindSafe should work just like +Send and +Sync.

Also, I think if the default would be different, there would be almost no problem. Box<Trait> should mean Box<Trait + RefUnwindSafe>, and users should be able to opt-out of it with Box<Trait + !RefUnwindSafe>. This way, unaware uses would get their types UnwindSafe without knowing it.

[nagisa]

It is not recommended to use this function for a general try/catch mechanism.

documentation for catch_unwind

[eddyb]

We should remove that limitation on trait objects, as the current implementation allows any number of OIBITs (aka "auto traits"), but that doesn't mean you should be abusing panic catching machinery.

[SimonSapin]

@nagisa I don’t think your point makes this issue any less valid, unless you mean that catch_unwind should never be used at all. Isn’t it necessary for C->Rust FFI?

[eddyb]

@SimonSapin Hmpf, I would've assumed the typical inputs to FFI would be UnwindSafe and thus not pose any problem, but I could very well be wrong, and I believe it'd be much graver concern than trait objects.

[abonander]

@eddyb I wouldn't expect using trait objects across a catch_unwind() boundary to constitute abuse of panic catching machinery. FFI isn't the only legitimate use of the mechanism.

I don't think the concern with proliferation of catch_unwind() (which is what led to the introduction of UnwindSafe) was well founded to begin with. We don't need to make unsafe-esque APIs deliberately unergonomic to try to compensate for some perceived propensity for laziness; I think the community has done really well at reinforcing good paradigms and correcting bad ones, and I think they could be trusted to treat unwind-safety with care as well.

But in the very least, we could allow UnwindSafe and RefUnwindSafe to be concatenated into trait objects. That, and I think Sync should imply RefUnwindSafe and Send should imply UnwindSafe, because it doesn't make sense why they shouldn't.

[eddyb]

@abonander FWIW, if someone wants to write an RFC for extending the allowed "extra traits" from Send and Sync to any "auto traits", they have my support, and the implementation is just removing the one error.

[dpc]

The problem I described affects people who are not using catch_unwind and might not even know what is it about. As a library I'm trying to make my core type Logger to be as robust as possible - my types require thread-safety so conceptually adding UnwindSafe to Send+Sync seems natural, but the toll will have to be paid be everyone and 99.9% of users don't want to deal with UnwindSafe-related matters.

Also, remember that if it's all too cumbersome to do, then people will just get used to ignoring everything and adding AssertUwindSafe without thinking.

But in the very least, we could allow UnwindSafe and RefUnwindSafe to be concatenated into trait objects. That, and I think Sync should imply RefUnwindSafe and Send should imply UnwindSafe, because it doesn't make sense why they shouldn't.

I'd like that, yes. I still think that unwind-traits should be opt-out (or they should be negative traits in the first place: we have unsafe { ... } blocks because we expect 99.9% of the code to be safe, and as far as I understand we want all types possible to be unwind safe, so we should maybe have RefUnwindUnsafe and UwindUnsafe traits in the first place. But since I completely missed the issue, I might not know about something. Right now it seems to me (maybe I'm very wrong about it), the ergonomy was not a concern to prevent people from abusing catch_unwind and it might backfire by people assuming it's not worth the trouble to get right.

[durka]

@eddyb is it "any auto trait" or "any trait with no methods"? I mean, implementation-wise.

[eddyb]

Right now if you just remove the error, it's specifically "any number of auto traits".

[abonander]

It is not recommended to use this function for a general try/catch mechanism.

It was weird to call it catch_unwind() then, especially given that the concerns in the original stabilization discussion about conflating it with general exception handling seem to have been ignored. Maybe it came up in the triage meeting but that wasn't really elaborated on: #27719 (comment)

So the blocker on Send and Sync implying UnwindSafe and RefUnwindSafe appears to have been coherence, but I think it's about time we re-addressed this in the context of specialization. Could it be done with default impl since that (presumably) doesn't require any items and allows more-specific impls? Tangentally, a PR to implement default impl has been open since November with no movement: #37860