UnwindSafe docs are unclear

[gnzlbg]

After reading the current docs of panic::UnwindSafe I am left with the feeling that the explanation of the concept of "panic safety" and its auto trait has only been handwaved. The docs point to the RFC, which does a better job, but I still do not feel like I fully understood what "panic safety" is, what does UnwindSafe convey, and when is it ok to use AssertUnwindSafe or not.

I feel like the docs could do a much better job at explaining all of this, and that some examples in the RFC would help, but I don't think that any documentation we currently have about this is enough.

This is a complex topic and I have no experience teaching it to others, but if somebody has done that, their feedback should probably be taken into account.

I think the API docs are very important to get right because they are currently the only teaching materials that we have about this aspect of the language. The Rust book does not cover this either, and the nomicon Exception safety has very few words to say about this.

cc @steveklabnik @rust-lang/docs

[steveklabnik]

I haven't actually looked at these docs, so I can't agree or disagree :) Thanks for opening the issue. I wish that there was something more... actionable, about it, but this is a good first step 👍

[gnzlbg]

Thanks for opening the issue. I wish that there was something more... actionable, about it, but this is a good first step

Yes, me too. I'm not sure how to word what I got from reading these docs. I think my first intention is to clarify whether "it is just me" and I should re-read the docs again until it makes sense or whether other people have trouble understanding these.

The docs aren't short, there are a lot of words in there, and a lot of things are explained. By "unclear" I I think I more precisely mean that, after reading the docs, I cannot answer basic questions about what I think the docs attempted to explain:

• What is panic safety? (I'm missing a concise definition of panic safety - I find the definitions here and in the RFC "vague". That's not very actionable either, but I am not able to tell with the definitions provided whether some code is "unwind safe" or not, or whether AssertUnwindSafe is used correctly somewhere or not - the RFC talks about preventing logic bugs - so one thing that confuses me is that "panic unsafety" is not "unsafe" per se I guess)
• Why is panic safety important? (IIUC, a program that's not "panic safe" can still be "safe" as long as it does not contain unsafe code, so... "panic un-safety" is not "unsafe" per se, but it could cause some unsafe code that makes some assumptions about panic safety to be unsound)
• What problem does "panic safety", AssertUnwindSafe, UnwindSafe, etc. solve ?
• When should I implement / not implement the UnwindSafe trait for a type ?
• Which types in the standard library are not UnwindSafe and why? (I think examples of already existing types would have helped me understand this better, e.g., by not only reading the definitions but also seeing them being used in the wild - the RFC contains some more examples and these helped)
• When is it ok to use AssertUnwindSafe ? (IIUC if some type is not panic safe, but that does not pose a problem in the context it is used, then one can use this escape hatch, but see UnwindSafe is unergonomic #40628 )

There is also the issue that one needs to click through a lot of pieces to obtain the whole picture (e.g. the UnwindSafe trait docs, the AssertUnwindSafe docs, the RFC, the nomicon, etc.). I think it might be better to cover panic safety holistically in the core::panic top-level module docs, and explain there what it is, and which problems do the different pieces solve.

Another reason why the documentation feels unclear is because there is actually a lot of content in there, but reading it doesn't feel "definitive". E.g. it feels as if it isn't clear at all what "panic safety" is, nor what problems do these utilities solve, if any, and / or whether they are good solutions to these problems. I'm not sure how else to explain it, but I run into this docs once a year, and I've never felt I truly understood what the point of these features are. #40628 mentions

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.

and... this is what I ended up doing last time, and what I ended up doing today. After reading the docs, I'm not even able to write a comment saying whether doing that is "ok". In January or so I remember running into this again here:

rust/src/libtest/lib.rs

Line 570 in d6e4028

let result = catch_unwind(AssertUnwindSafe(testfn));

and I was like, "is this code correct?". Hard to tell, since there is no comment about why AssertUnwindSafe is correct there. I tried to write a comment, but if you take a look the function that gets passed to catch_unwind is a generic closure, so... it can be anything. How can one know that it is definitely going to be unwind safe ?

[jakoschiko]

@gnzlbg @BurntSushi @AltSysrq

If

• libcore is doing catch_unwind(AssertUnwindSafe(testfn)) and
• quickcheck is doing panic::catch_unwind(panic::AssertUnwindSafe(fun)) and
• proptest is doing panic::catch_unwind(AssertUnwindSafe(|| test(case))),

then it must be correct, right?

[gnzlbg]

@jakoschiko I think all of those libraries use AssertUnwindSafe incorrectly. Consider:

lazy_static! { static ref V: Mutex<Vec<u8>> = Mutex::new(vec![]); }

#[should_panic] #[test] fn foo() { unsafe {
    { let mut v = V.lock().unwrap(); v.reserve(42); v.set_len(42); }
    panic!();
}}
#[test] fn bar() {
    let v = V.lock().unwrap(); let v: &Vec<u8> = &*v;
    for i in v {    dbg!(*i); }
}

Here if foo is run before bar, V is left in an invalid state, and when bar runs, it will use a Vec with broken invariants, resulting in undefined behavior.

The whole point of UnwindSafe is preventing these programs from compiling. Apparently, "correct usage" of AssertUnwindSafe requires its user to prove that the closure is actually UnwindSafe. Those test libraries take arbitrary user tests and use AssertUnwindSafe without proving that these tests aren't actually UnwindSafe, and are therefore using AssertUnwindSafe incorrectly. What all those test libraries should do instead is require a + UnwindSafe bound on the user test code they execute. Ideally, that would prevent the example above from compiling, e.g., because foo captures a V: &mut Mutex<...Vec<...>> type that is not UnwindSafe, producing an error like:

foo is not a valid test because it is not UnwindSafe

or something like that. They could then offer a way for the test writer to specify that their test is UnwindSafe even if the compiler doesn't know, such that the code above might need to change to:

#[assert_unwind_safe]
#[should_panic] #[test] fn foo() { unsafe {
    { let mut v = V.lock().unwrap(); v.reserve(42); v.set_len(42); }
    panic!();
}}

or similar. For example, they could also require that functions with #[should_panic] must be unwind safe, but right now this is not documented anywhere (e.g. the Rust book does not mention anything about this).

[sfackler]

UnwindSafe is not a "real" safety barrier- AssertUnwindSafe does not require unsafe to use! The foo test in your example is the problem, not the use of catch_unwind. Unsafe code must be sound in the presence of panics regardless, period. Even if the test harness did not use catch_unwind at all, you'd still run into exactly the same problem as long as the harness was running more than one thread (even if foo was not tagged should_panic).

[gnzlbg]

@sfackler What you mention is covered in the UnwindSafe docs, which say that the goal of UnwindSafe is to prevent accidental misuses like this. The question here is whether libtest and other libraries are using AssertUnwindSafe correctly by stamping it on catch_unwind calls to unknown code.

These libraries don't know what that code does, so why are they asserting that such code is UnwindSafe ? Such code could invoke UB only when a panic is caught by the catch unwind.

Unsafe code must be sound in the presence of panics regardless, period.

True, and UnwindSafe does not claim to solve the safety problem, only the accidental misuse problem. The topic being discussed here is whether unconditional use of AssertUnwindSafe on all catch_unwinds is ok, libtest being a prime example of the standard library using AssertUnwindSafe on unknown code. If that's ok, then UnwindSafe does not solve the accidental misuse problem, and from the docs and its RFC I have no idea which problem would it solve.

Even if the test harness did not use catch_unwind at all, you'd still run into exactly the same problem as long as the harness was running more than one thread (even if foo was not tagged should_panic).

That's a different problem. Let's assume a single threaded runtime like the ones quickcheck, proptest, or libtest with RUST_TEST_THREADS=1 use.

[sfackler]

AssertUnwindSafe is also there to say "I don't care about unwind safety, just make this thing work".

[gnzlbg]

@sfackler if that's the case, then the uses in libstd (and by the people in #40628 that just stamp AssertUnwindSafe everywhere) are correct, and the AssertUnwindUnsafe docs are not only unclear, but also completely incorrect. Currently, the docs say that AssertUnwindSafe is there to assert something, e.g., in:

Wrapping the entire closure amounts to a blanket assertion that all captured variables are unwind safe.

which is a very different thing from expressing that one does not care about unwind safety. The wording that you propose makes more sense to me.