Did I miss any safety issues when calling `unreachable_unchecked`?

code review
1

The following is discussing a patch I created for doukutsu-rs:

Basically, our NPC list is an array of RefCell<NPC>'s. Every living NPC needs to be updated each frame. In many cases, the update method needs to temporarily unborrow the NPC, perform some manipulation on the NPC list, and then borrow the NPC again.

In order to facilitate this, I have crafted a "BorrowedNPC" object - it's basically a RefMut, except it has a method called "unborrow_then" to temporarily unborrow the RefMut and run a closure. (See the implementation "unborrow_then" in the file.)

Here is what a BorrowedNPC looks like:

pub enum BorrowedNPC<'a> {
    // TODO: split into mutably borrowed and immutably borrowed variants?
    Borrowed {
        ref_mut: RefMut<'a, NPC>,
        cell: &'a NPCCell,
        token: &'a mut NPCAccessToken,
    },
    /// SAFETY: Constructing the `Unborrowed` variant is **inherently unsafe**.
    /// Rust doesn't support marking enum variants as unsafe, but if it did, this
    /// one would be.
    /// 
    /// A `BorrowedNPC` in the `Unborrowed` state must **never** be dereferenced.
    /// Doing so will invoke `unreachable_unchecked`, leading to immediate Undefined
    /// Behavior.
    /// 
    /// Never expose a `BorrowedNPC` in the `Unborrowed` state outside this module.
    Unborrowed,
}

The reason for the SAFETY hazard will become apparent.

The method unborrow_then works by switching the BorrowedNPC to the Unborrowed state, calling a closure, then borrowing the RefCell again:

    fn unborrow_then<F, T>(&mut self, f: F) -> T
    where
        F: FnOnce(&mut NPCAccessToken) -> T
    {
        match self {
            BorrowedNPC::Borrowed { .. } => {
                let old = std::mem::replace(self, unsafe { BorrowedNPC::new_unborrowed() });
                let (old_ref_mut, token, cell) = match old {
                    BorrowedNPC::Borrowed { ref_mut, token, cell } => (ref_mut, token, cell),
                    _ => unsafe { core::hint::unreachable_unchecked() }
                };

                drop(old_ref_mut);

                let result = f(token);

                *self = cell.borrow_mut(token);

                result
            }
            _ => unsafe { core::hint::unreachable_unchecked() }
        }
    }

This method assumes that self will be in the Borrowed state at the beginning of the call - this is enforced by never allowing a BorrowedNPC::Unborrowed to escape the module. Hence the use of unreachable_unchecked. But that's not the big issue here...

A BorrowedNPC holds a reference to an NPC struct (when it's in the Borrowed state). When it's Borrowed, I need to be able to access the NPC's fields with the dot operator. This is achieved with a Deref implementation:


impl Deref for BorrowedNPC<'_> {
    type Target = NPC;

    #[inline]
    fn deref(&self) -> &Self::Target {
        match self {
            BorrowedNPC::Borrowed { ref_mut, .. } => &*ref_mut,
            // `unreachable_unchecked` tells the compiler to assume this branch
            // will never be reached, skipping any checks.
            // Verified via Godbolt: This significantly improves codegen when
            // accessing `BorrowedNPC` fields.
            _ => unsafe { core::hint::unreachable_unchecked() }
        }
    }
}

Here's the problem: When the unreachable! macro is used in this code, every NPC field access emits assembly instructions to check the Borrowed state. This seemed (arguably) inefficient, so this patch changes it to unreachable_unchecked. The upshot is that when accessing NPC fields, it no longer checks the Borrowed state - it is as cheap as a regular memory access. The downside is that a major safety hazard now exists - the BorrowedNPC cannot be Deref'd if it is in the Unborrowed state, or else it will trigger Undefined Behavior.

I was careful to avoid Deref'ing a BorrowedNPC::Unborrowed, and to never expose a BorrowedNPC::Unborrowed outside the module. But I would like feedback on this approach. Does this code seem somewhat reasonable? Have I missed any obvious safety failures?

Going back to unborrow_then, this line:

                *self = cell.borrow_mut(token);

does not trigger the Deref method, correct?

2

A panic will leave it as unborrowed.

3

You're right, I forgot to take panics into account. I thought of a few solutions.

  1. Mark "unborrow_then" as unsafe and note that callers should be cautious if catch_unwinding.
  2. Use the old unreachable! macro and hope the compiler gets better at optimizing away unnecessary checks in the future.
  3. I thought of changing unborrow_then to consume a BorrowedNPC instead of an &mut BorrowedNPC. You'd call it like:
let self = self.unborrow_then(...)

If I'm not mistaken this would prevent an Unborrowed from being exposed outside of unborrow_then even in the case of a panic. Unfortunately the language doesn't allow "let self =" statements? Correct me if I'm wrong.

4

This is not a sufficient safety condition. Drop::drop() is just as able to observe state after a panic as catch_unwind() is. (And “be careful” is not precise enough to be possible to obey.)

You can’t declare a new self, but you can assign to self.

fn something(mut self) {
    ...
    self = self.unborrow_then(...);
    ...
}