[Pre-RFC]: Deprecating UnwindSafe

[nico-abram]

In May, in the pull request where UnwindSafe was moved to core, it was mentioned the libs team may want to deprecate it Move UnwindSafe, RefUnwindSafe, AssertUnwindSafe to core by dtolnay · Pull Request #84662 · rust-lang/rust · GitHub

I found a comment in this issue mentioning deprecation as far back as 2019: `UnwindSafe` is unergonomic · Issue #40628 · rust-lang/rust · GitHub

• Feature Name: deprecate_unwind_safe
• Start Date: 2022-01-17
• RFC PR: (leave this empty)
• Rust Issue: (leave this empty)

Summary

Currently rust has the UnwindSafe and RefUnwindSafe marker traits. This RFC proposes to deprecate them, and remove the F: UnwindSafe bound on catch_unwind.

Motivation

Unwind safety is not actually related to safety. It acts as a lint. AssertUnwindSafe can be used to ignore it, and using it does not require unsafe. If using it results in undefined behaviour or unsoundness, the problem lies elsewhere. The existence of unwind safety makes it seem as if you can rely on it for soundness, which is not true (See discussion in UnwindSafe docs are unclear.)

It can also be problematic when a type does not implement the marker trait, but it could, notably with trait objects (See discussion in UnwindSafe is unergonomic). It can also be a pain point for library authors, who are not sure if they should add a bound on them for their generic types to guarantee their types are UnwindSafe, which would make their downstream users sometimes have to use AssertUnwindSafe despite not using catch_unwind just to satisfy the bounds.

Guide-level explanation

UnwindSafe and RefUnwindSafe are deprecated, and you never need to use them. If you can cause undefined behaviour with catch_unwind, something else is unsound.

The following now compiles:

    let x = std::cell::UnsafeCell::new(1u8);
    let result = std::panic::catch_unwind(|| {
        println!("{:p}", x.get());
        panic!()
    });

Which used to require AssertUnwindSafe:

    let x = std::panic::AssertUnwindSafe(std::cell::UnsafeCell::new(1u8));
    let result = std::panic::catch_unwind(|| {
        println!("{:p}", x.get());
        panic!()
    });

Reference-level explanation

UnwindSafe and RefUnwindSafe are now deprecated, and the UnwindSafe bound on the F generic parameter of catch_unwind is removed.

Drawbacks

We lose any value that UnwindSafe was actually providing as a lint.

Rationale and alternatives

• We could keep UnwindSafe without deprecating it.
• We could make using something !UnwindSafe through catch_unwind a warning via language magic instead of completely removing it

Prior art

In the pull request where UnwindSafe was moved to core, it was mentioned the libs team may want to deprecate it Move UnwindSafe, RefUnwindSafe, AssertUnwindSafe to core by dtolnay · Pull Request #84662 · rust-lang/rust · GitHub

I found a comment in this issue mentioning deprecation as far back as 2019: `UnwindSafe` is unergonomic · Issue #40628 · rust-lang/rust · GitHub

Unresolved questions

• How will this impact the ecosystem? How will libraries with an MSRV deal with this?

Future possibilities

[frank-king]

How about requires an unsafe {} block if UnwindSafe or RefUnwindSafe meets requirement?

As you said, it might lose an unsafe lint if these two marker traits are removed.

[CAD97]

@frank-king

How about requires an unsafe {} block

The point is that there's nothing unsafe about the lack of UnwindSafe. Changing the cover from AssertUnwindSafe to unsafe {} would make the situation worse, not better.

(Technically, now the lack of UnwindSafe could be used for soundness, but a) it currently can't, so this'd be a big change and very likely to be incorrectly suppressed, and b) it becomes more important to "correctly" handle UnwindSafe, which is already a very unclear question without it being one of soundness. Also, this'd mean making being in an unsafe context effect the type system somehow and I have no idea how.)

[georgebarwood]

I personally found UnwindSafe somewhat confusing ( I guess I spent (wasted?) 10 minutes trying too understand what it was meant to be doing ), and I don't see how it could be useful to anyone. I think Rust would be better without it. On the other hand, perhaps I am missing something, but I don't know what.

[Nemo157]

@nico-abram

We lose any value that UnwindSafe was actually providing as a lint for unsafe code authors.

I don’t think this should mention unsafe, UnwindSafe is a lint for safe code only.

[nico-abram]

You're right. I edited it

[bjorn3]

As a datapoint the chalk engine doesn't implement RefUnwindSafe and it turned out that unwinding can indeed cause state corruption. Rust-analyzer used to use AssertUnwindSafe, but that resulted in occasional panics inside of chalk due to unwinds corrupting the internal state, so in correctly reset chalk state after a panic by matklad · Pull Request #1932 · rust-analyzer/rust-analyzer · GitHub a RefUnwindSafe impl had to be replaced with clearing the chalk cache on every panic.

[chrefr]

Perhaps we can keep the trait as a warning and not an error? This will require some magic, sure, but perhaps it worths it.

[nico-abram]

That's a very interesting idea! Added it as an alternative

[steffahn]

That’s my preferred solution as well. It has nice side-effects: for example it allows you to “fix” you types’ UnwindSafe bounds – if they’re off – without a breaking change. Currently, looking at lots of crates, they often don’t pay too much attention to unwind safety, but PRs that fix those would not make too much sense either, because it’s not a soundness-relevant thing, so breaking changes aren’t really warranted. (Unlike for example for the case of fixing incorrect Send/Sync implementations.)

If e.g. Sync should generally imply RefUnwindSafe, and similar things, this might even allow introducing a super-trait relation Sync: RefUnwindSafe and Send: UnwindSafe, which would also help in a lot of cases with trait objects. (Maybe that is a nonsensical idea for some reason; I haven’t thought about this too much yet, I just wanted to demonstrate that even some fairly stark otherwise-breaking re-designs of Rust in this regard would be possible. One more conservative idea would be to just require RefUnwindSafe for static variables [but of course, failing to meet the requirement would just be a warning].)

The downside is that it’s a fundamentally new feature of trait resolution, afaict. At least to me, turning missing trait implementations (for a marker trait) into a warning seems like a highly nontrivial thing to do.

[kornel]

I agree with the deprecation.

Usually when Rust throws errors, even if they're hard to understand, there's an underlying safety issue, so there's something I can investigate and know when I've fixed it. Unwind safety doesn't seem like that at all. Every time I encounter it I'm not sure if it's a real issue, or a false alarm due to its limitations and flaky design, so I don't really have an option other than slap AssertUnwindSafe and hope I didn't break anything. There's no way to distinguish between very common accidental omissions, and rare cases where objects were intentionally left as unwind-unsafe.

[Aloso]

If the main issue is that the name UnwindSafe is (incorrectly) associated with memory safety, how about renaming the traits and related types to something more descriptive/less confusing? The old names could still be re-exported for backwards compatibility and deprecated.

[Soni]

We don't agree with the deprecation, it's an useful lint. We do agree with the renaming, but to what?

We use UnwindSafe stuff in hexchat-plugin. Arguably we shouldn't care, but having the users think about unwinding means they should be better able to write code that remains (partially) working instead of breaking the whole plugin and requiring a hard restart. Panics/bugs happen, and UnwindSafe basically says "hey, please take poisoning/etc into account".

(At the same time, it's also useful to propagate panics from one callback to another. But anyway.)

[matklad]

To expand on this a bit: the thing which contained the !UnwindSafe type was a macro-generated super-generic, spawling several crates salsa database. Exactly the case where it’s hard to see the problem with a naked eye, and where auto-traits help.

The reason why we had AssertUnwindSafe is that we had to work around compiler bug at some point: basically, compiler sometimes forgot during incremental compilation that something was unwind safe: Assert that DB is unwind-safe, instead of proving · matklad/rust-analyzer@fcffa6b · GitHub.

If not for that compiler bug, UnwindSafe would’ve saved us big time.