`NoUnsafeCell` errors proliferate even when using `AssertRecoverSafe` or manually implementing `RecoverSafe`

[abonander]

Test Case 1: Playground

use std::cell::Cell;

use std::panic::{self, AssertRecoverSafe};

fn main() {
    let should_work = AssertRecoverSafe(Cell::new(0));

    panic::recover(|| should_work.set(0));
}

Test Case 2: Playground

use std::cell::Cell;

use std::panic::{self, RecoverSafe};

struct MyWrapper(Cell<u32>);

impl RecoverSafe for MyWrapper {}

fn main() {
    let should_work = MyWrapper(Cell::new(0));

    panic::recover(|| should_work.0.set(0));
}

Both produce errors like this:

<anon>:8:5: 8:19 error: the trait `std::panic::NoUnsafeCell` is not implemented for the type `core::cell::UnsafeCell<i32>` [E0277]
<anon>:8     panic::recover(|| should_work.set(0));
             ^~~~~~~~~~~~~~
<anon>:8:5: 8:19 help: see the detailed explanation for E0277
<anon>:8:5: 8:19 note: the type core::cell::UnsafeCell<i32> contains interior mutability and a reference may not be safely transferrable across a recover boundary
<anon>:8:5: 8:19 note: required because of the requirements on the impl of `std::panic::NoUnsafeCell` for `core::cell::Cell<i32>`
<anon>:8:5: 8:19 note: required because of the requirements on the impl of `std::panic::NoUnsafeCell` for `std::panic::AssertRecoverSafe<core::cell::Cell<i32>>`
<anon>:8:5: 8:19 note: required because of the requirements on the impl of `std::panic::RecoverSafe` for `[closure@<anon>:8:20: 8:41 should_work:&std::panic::AssertRecoverSafe<core::cell::Cell<i32>>]`
<anon>:8:5: 8:19 note: required by `std::panic::recover`
error: aborting due to previous error
playpen: application terminated with error code 101

CC @alexcrichton

[alexcrichton]

Hm, so the first case here should work (e.g. AssertRecoverSafe should subvert basically everything), although the second case is correct in yielding an error (albeit not a very good one)

[abonander]

Is the second case not allowed then? RecoverSafe's documentation suggests that manually implementing it is okay to do:

Note, however, that this is not an unsafe trait, so there is not a succinct contract that this trait is providing. Instead it is intended as more of a "speed bump" to alert users of recover that broken invariants may be witnessed and may need to be accounted for.

I figured AssertRecoverSafe only existed to wrap types which you couldn't impl RecoverSafe for yourself, but for which you can ensure their invariants are upheld across the recover() boundary.

And what about moving values into the closure? AssertRecoverSafe can't be unwrapped, at least not currently.

[alexcrichton]

Oh wait sorry I misread your second example, I failed to notice the manual impl!

This is basically a case where closure captures are coming into play. The closures aren't annotated with move, so outer variables are captured by-reference which means that the actual types being tested against the RecoverSafe trait here are &AssertRecoverSafe<Cell<u32>> and &MyWrapper. In the first case that should be handled by #30513, but the second is legitimately indicating that it's not a recover-safe type (b/c the reference is different than the by-value).

You should be able to get around this via either move plus AssertRecoverSafe::new(&cell) or via struct MyWrapper<'a>(&'a Cell<u32>).

Certainly an interesting interaction going on here though. One would likely expect that if T isn't recover safe that impl RecoverSafe for T would fix the problem but it's likely what you really need to do is impl RecoverSafe for &T due to the way closures largely work.

[abonander]

There's impl<'a, T: NoUnsafeCell + ?Sized> RecoverSafe for &'a T, why not impl<'a, T: RecoverSafe + ?Sized> for &'a T? Coherence?

[alexcrichton]

Nah unfortunately that'd defeat the purpose of the RefCell<T> distinction. The type RefCell<T> is itself recover safe, it's just &RefCell<T> that's not (so the distinction need to be there)

[abonander]

It's unfortunate that impl RecoverSafe for &T fails due to coherence or something like that:

<anon>:11:1: 11:42 error: cross-crate traits with a default impl, like `std::panic::RecoverSafe`, can only be implemented for a struct/enum type, not `&'a MyWrapper` [E0321]
<anon>:11 impl<'a> RecoverSafe for &'a MyWrapper {}
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<anon>:11:1: 11:42 help: see the detailed explanation for E0321
<anon>:11:1: 11:42 error: conflicting implementations of trait `std::panic::RecoverSafe` for type `&MyWrapper`: [E0119]
<anon>:11 impl<'a> RecoverSafe for &'a MyWrapper {}
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<anon>:11:1: 11:42 help: see the detailed explanation for E0119
note: conflicting implementation in crate `std`
error: aborting due to 2 previous errors
playpen: application terminated with error code 101

[alexcrichton]

Hm interesting, I guess yeah you'll be forced to move the reference inside the wrapper rather than having a reference to the wrapper

[abonander]

Is it possible to implement NoUnsafeCell then? My use case can contain RefCell and friends but I'm guaranteeing that they're unobservable in user code after recovery because I immediately re-panic after escaping the FFI stack.

[abonander]

I guess I can just use AssertRecoverSafe once it's fixed 😛. I see your PR has been accepted so I'll look forward to trying it on tonight's nightly. When do the buildbots fire?

[alexcrichton]

Yeah currently you can implement NoUnsafeCell (being renamed to RefRecoverSafe) I believe (although I can't say I've tried).

For now though yeah I'd recommend just using AssertRecoverSafe plus move closures. The PR is in the queue but'll take a few hours at least to land.

[abonander]

@alexcrichton the problem with moving at this point in time is that I call recover in different branches of a match but I pass the same closed-over value to it; I can't do the branch in the closure because I need to do some FFI cleanup afterwards regardless of whether a panic happened or not (which also requires the closed-over value).

It would help if AssertRecoverSafe derived Copy and/or Clone so I could reuse it without constructing a new value for each match arm, but that would probably require some discussion because I don't know if it's safe or not.

I'll probably just implement NoUnsafeCell and fix the name after your PR drops. A quick -Z no-trans check confirms that it works just fine.

Thanks anyways!

[alexcrichton]

Oh yeah we could totally add more traits like Copy and Clone to AssertRecoverSafe, there shouldn't be any safety concerns there.