RFC: Amend `recover` with a `PanicSafe` bound

[alexcrichton]

Instead of a 'static bound on the function, instead add a new marker trait,
PanicSafe, to encapsulate the concept of exception safety as a trait bound
which can be used to serve as a speed bump for users of panic::recover.

[seanmonstar]

'An' should be 'A'.

[reem]

In principle I have nothing against this proposal, since it encourages more safety, but I am really concerned about the currently bad interaction of trait objects and OIBITS. I fear that this will make many types non-PanicSafe even when they probably should be. All new code will now have to consider this trait when defining any trait objects.

[alexcrichton]

@reem that is indeed a concern! It was explicitly talked about in the section about global complexity, so could you rephrase your concerns with respect to the wording there?

For example that section indicates that the only area this is expected to arise is in a "thread-pool-like" scenario with catch_panic in the bottom, but the abstraction would continue to propagate panics thereby not requiring the bound outside of the implementation. Do you disagree with this? Do you have use cases in mind where the PanicSafe bound would propagate outwards?

[seanmonstar]

@alexcrichton an example I think @reem means:

struct Stream {
    inner: Box<Read + Send>
}

// else where
let foo = SomethingHoldingAStream();
panic::recover(|| {
  foo.something();
});

Since the PanicSafe marker was not included for the trait object, the Stream is not PanicSafe, even though it might be.

[alexcrichton]

@seanmonstar

Yes it's trivial to create an example where one would have to think about an OIBIT, but the key quote from that section is:

Overall, the expected idiomatic usage of recover should mean that PanicSafe is rarely mentioned, if at all

The example you've shown isn't necessarily idiomatic usage as it's not expected to use recover for general error handling but rather only at FFI boundaries and at the core of abstractions which otherwise need to handle panics (e.g. some forms of pools).

[seanmonstar]

Sorry, I don't have much to say about the implementation at all, I don't deal with ffi boundaries myself. Just tossing in an example that @reem and I have run in to, thanks to OIBITs behaving the way they do.

[oli-obk]

*causes

[dgrunwald]

What happened to impl<T: 'static + Send> ExnSafe for T {} in @aturon's original proposal?

If we had such an impl, trait objects like Box<Read + Send> would automatically be panic-safe.

[RalfJung]

@dgrunwald The RFC actually answers this. That impl is currently prohibited by the coherence checks.

[RalfJung]

I am slightly confused by the explicit impl for &'a T. Won't it be the case that the default impl already covers all borrows? I had a look at the OIBIT RFC and couldn't find anything explicitly saying that the default does not cover borrows.

[nikomatsakis]

I'm adding T-lang just because I think this lies on the intersection of libs and lang. cc @rust-lang/lang

[wthrowe]

Given that the language team hasn't decided what this syntax means yet and is considering making it an error, I don't see how we can make useful comments on the implementation.

(Basically the question is whether &Mutex<UnsafeCell<()>> implements IsPanicSafe. RFC #19 says yes, compiler says no.)

[alexcrichton]

@RalfJung yeah I was a little surprised myself to see it work, but in some local testing all the cases I tried appeared to work as intended.

[RalfJung]

That's probably part of the semantic mismatch mentioned by @wthrowe above. We should really get the semantics of OIBITS straight before relying more on them...

[bluss]

It seems inproportionate to add another OIBIT that doesn't govern any safety boundary at all, and just manages the panic recovery speed bump.

[alexcrichton]

@bluss That's certainly a point of contention! I think a key part of the discussion about this RFC will be whether the "extra safety" is worth the additional complexity of this new marker trait. I believe that this does not add as much complexity as other markers like Send and Sync, but I may have missed something!

On the other hand, however, there are also many opinions that some form of "speed bump" is necessary here (and lots of other various opinions about safety guarantees, etc). In other words, although this doesn't necessarily correspond to a memory safety boundary, there are certainly good arguments to be made for including it nonetheless!

[pythonesque]

I don't think it's inappropriate to add a safe OIBIT; what else is that for, but something like this? I really like this proposal.

[Gankro]

@dgrunwald OIBITS Structural Traits can be un-impl'd so inferring that Box<Send + Read> is ExnSafe is unsound.

---

I continue to be largely ambivalent about adding more of these traits. The bang-for-the-buck on thread-safety is huge, but exception-safety is a relatively obscure problem. That said, it seems like using these in any kind of generic context is "doing it wrong", so I guess it's fine?

[Gankro]

@pythonesque I should have qualified that: exception safety issues are relatively obscure in Rust. The argument, as far as I recall from the previous thread, is that recover potential subverts this.

[pythonesque]

@Gankro Yes, that's the argument. And as you said, the global section does a good job of laying out why the OIBIT doesn't really matter here: it shouldn't come up if you're using panic properly for the most part. Honestly, the main reason I want PanicSafe is so anything that plans to catch an exception with your type has to either (1) advertise that fact, or (2) the author has to actually think before doing it (I think Mutex and poisoning is a great example of why that's effective in practice; there's nothing stopping people from writing a function that grabs the poisoned value out of it if necessary, but there's just enough extra effort involved that people usually don't, which is exactly what we want).

[SimonSapin]

So are raw pointers panic-safe?

[pythonesque]

Yes.

[bluss]

@pythonesque The problem is that the PanicSafe doesn't give you actual safety. A type without the marker can be modified, and still be accessed after panic (during unwinding) in destructors. In safe rust code.

Second problem, the term "panic safe" is stolen, and we have to qualify if we mean PanicSafe or actually panic safe (= memory safe in the presence of unwinding / panic).

[alexcrichton]

@glaebhoerl

I think it's important to emphasize @sfackler's point that many of your questions are quite relevant regardless of whether recover exists or not. Exception safety is already an issue in Rust today and it's not going away.

While they are good questions to think about, it's important to acknowledge that the recover function is simply adding one more vector to view broken invariants along, and this RFC is making it even harder to do just that. Along those lines I would say that we're definitely thinking hard about making exception safety "something you don't have to worry about". I think @tomaka put it well in saying that we're not requiring transactional styles because it doesn't matter if you never actually witness the broken invariant.

[durka]

An easy question: why isn't PanicSafe an unsafe trait, like Send and Sync? It seems like you're making an unchecked assertion if you impl it. Or will users not be allowed to impl it at all (by compiler magic, I guess) and you'd use an AssertPanicSafe wrapper instead? I guess it's not clear to me why the wrapper is provided; you could just make your own newtype and [unsafe] impl PanicSafe the way you have to do if you want to assert Sendness or Syncness.

And a triviality: in the Unresolved Questions section it lists the question of keeping thread::catch_panic or moving it, but that seems like not a question anymore.

[RalfJung]

unsafe is for memory safety. If using a function / implementing a trait can never lead to memory unsafety, it should not be unsafe. Implementing PanicSafe can not lead to memory unsafety. It can lead to observation of violated invariants, but that cannot make safe code crash.

Essentially, think of PanicSafe as being its own kind of "unsafety", and AssertPanicSafe as the unsafe keyword for this "unsafety". It has sometimes been requested that Rust should not just offer unsafe to opt out of all safety, but a more fine-grained approach that maintains some properties, while leaving others unchecked. Well, the proposed panic safety is pretty much exactly that, coded up entirely within the language.

[wthrowe]

unsafe is used to prevent violation of invariants in the standard library. For example, I can't think of a way to violate memory safety by creating a CString with an interior null byte. Of course, observing the broken invariants protected in this case can already be done in safe code, so it is hard to argue that observing them in this particular way should be unsafe.

However, I also don't really see the point of providing AssertPanicSafe<T> as another way to spell impl PanicSafe for NewType(T). This is supposed to be a rarely used interface; having more convenience features than, say, Sync seems odd. (Of course, I'm also still not convinced that PanicSafe is a good idea, so I'm probably not the best one for justifying particular parts of the implementation.)

[huonw]

@wthrowe the invariants unsafe protects are things people/libraries can rely on to always be true, and hence can do things that would be memory unsafe if they were untrue. Without the unsafe, every user would have to be careful to handle interior nulls in a way that doesn't cause memory unsafety. Of course, it may be the case that std itself doesn't trigger any problems, with that particular invariant.

(In this instance, CString does store what it thinks the length of the string is internally, and the unsafe means people can rely on (in memory-unsafety terms) the length reported on the Rust side matching that reported by strlen on the C side.)

Of course, observing the broken invariants protected in this case can already be done in safe code, so it is hard to argue that observing them in this particular way should be unsafe.

What do you mean by this?

[wthrowe]

I can't think of any way that tricking C into believing it has less space allocated then it actually does could lead to memory unsafety, but perhaps there's something I'm not thinking of. In any case, that might be the same sort of guarantee we want to give with PanicSafe, in which case PanicSafe should be an unsafe trait even if it can't directly lead to memory unsafety.

For the second point I was just referring to the issue that has been mentioned several times above that all of this can already be observed in destructor calls during unwinding.

[huonw]

I can't think of any way that tricking C into believing it has less space allocated then it actually does could lead to memory unsafety, but perhaps there's something I'm not thinking of.

Hm, maybe. @rust-lang/libs should talk about it.

In any case, that might be the same sort of guarantee we want to give with PanicSafe, in which case PanicSafe should be an unsafe trait even if it can't directly lead to memory unsafety.

This isn't appropriate for the same reasons recover isn't unsafe, and is even more restrictive: we would have to make what it is trying to protect against a guarantee globally to be useful, which isn't possible due to destructors etc.

For the second point I was just referring to the issue that has been mentioned several times above that all of this can already be observed in destructor calls during unwinding.

Hm, I don't see how it is possible to observe an invalid CString without unsafe unless it panics internally in a bad place (which would a bug in the CString implementation). Baring bugs, the only way to get an invalid one is via from_vec_unchecked which requires unsafe to call.

[wthrowe]

Oh, sorry! When I said "observing the broken invariants protected in this case can already be done in safe code" I meant the case discussed in this RFC, not CString. Rereading my previous comment I see that that is completely unclear.

I think I agree with everything you are saying. Sorry for the confusion!

[alexcrichton]

The libs team discussed this during triage yesterday and the conclusion was to merge it at this time. We're still not 100% sure that this will make it all the way to stabilization, but the general consensus about this RFC seems to be either in approval or "ok, let's try it out". The libs team wants to definitely keep an eye out for the global complexity problem associated with adding a new marker trait, there's rationale here as to why it's mitigated but something new can always pop up!

To emphasize, this RFC is now entering what is basically the "implementation phase" where it will still be unstable in the standard library but we'll be able to play around with it on nightly and get feedback about the concrete API. There will be another round of discussion when we move to stabilize this.

Thanks again for all the discussion everyone!