Skip to content

[lib] Prefer core::hint::unreachable_unchecked over core::unreachable macro #88606

Closed
Closed
[lib] Prefer core::hint::unreachable_unchecked over core::unreachable macro#88606

Description

@BastiDood
BastiDood
opened on Sep 3, 2021

At the moment, many of the core assertions in the standard library make use of the unreachable macro, which is simply an indirection for panic. This is totally fine, but I would like to begin an initiative on upgrading some of these invocations as core::hint::unreachable_unchecked so that we may (at least) give the compiler a chance to optimize the code better.

Below is a non-exhaustive list of possible candidates. I will be editing the issue as more candidates are introduced and as certain examples are marked 100% safe or not.

Merged

None so far.

Definitely Safe

This is the perfect candidate for core::hint::unreachable_unchecked since it is apparent that self is now the Cow::Owned variant.

rust/library/alloc/src/borrow.rs

Lines 275 to 287 in 97f2698

#[stable(feature = "rust1", since = "1.0.0")]
pub fn to_mut(&mut self) -> &mut <B as ToOwned>::Owned {
match *self {
Borrowed(borrowed) => {
*self = Owned(borrowed.to_owned());
match *self {
Borrowed(..) => unreachable!(),
Owned(ref mut owned) => owned,
}
}
Owned(ref mut owned) => owned,
}
}

Possible Candidates

rust/library/core/src/lazy.rs

Lines 52 to 64 in 673d0db

#[unstable(feature = "once_cell", issue = "74465")]
impl<T: Clone> Clone for OnceCell<T> {
fn clone(&self) -> OnceCell<T> {
let res = OnceCell::new();
if let Some(value) = self.get() {
match res.set(value.clone()) {
Ok(()) => (),
Err(_) => unreachable!(),
}
}
res
}
}

rust/library/std/src/lazy.rs

Lines 121 to 130 in adf1688

fn clone(&self) -> SyncOnceCell<T> {
let cell = Self::new();
if let Some(value) = self.get() {
match cell.set(value.clone()) {
Ok(()) => (),
Err(_) => unreachable!(),
}
}
cell
}

rust/library/std/src/lazy.rs

Lines 152 to 158 in adf1688

fn from(value: T) -> Self {
let cell = Self::new();
match cell.set(value) {
Ok(()) => cell,
Err(_) => unreachable!(),
}
}

rust/library/std/src/sys/sgx/abi/mod.rs

Lines 51 to 52 in 17f30e5

// Initialization is done.
Err(DONE) => {}

rust/library/std/src/sync/mpsc/oneshot.rs

Lines 105 to 106 in 673d0db

// Not possible, these are one-use channels
DATA => unreachable!(),

rust/library/std/src/sync/mpsc/oneshot.rs

Lines 170 to 173 in 673d0db

match (&mut *self.data.get()).take() {
Some(data) => Ok(data),
None => unreachable!(),
}

rust/library/std/src/sync/mpsc/oneshot.rs

Lines 188 to 190 in 673d0db

// We are the sole receiver; there cannot be a blocking
// receiver already.
_ => unreachable!(),

rust/library/std/src/sync/mpsc/oneshot.rs

Lines 251 to 252 in 673d0db

// We're the only ones that can block on this port
_ => unreachable!(),

rust/library/std/src/sync/mpsc/oneshot.rs

Lines 278 to 281 in 673d0db

// Now that we've got ownership of our state, figure out what to do
// about it.
match state {
EMPTY => unreachable!(),

rust/library/std/src/sys/sgx/mutex.rs

Lines 113 to 118 in a28109a

// There was a thread waiting, just pass the lock
if let NotifiedTcs::Single(tcs) = guard.notified_tcs() {
guard.lock_var_mut().owner = Some(tcs)
} else {
unreachable!() // called notify_one
}

rust/library/core/src/iter/traits/iterator.rs

Lines 3444 to 3449 in 3ed6c1d

unsafe fn __iterator_get_unchecked(&mut self, _idx: usize) -> Self::Item
where
Self: TrustedRandomAccessNoCoerce,
{
unreachable!("Always specialized");
}

rust/library/std/src/sys/sgx/rwlock.rs

Lines 134 to 139 in c5fbcd3

// One or more readers were waiting, pass the lock to them
if let NotifiedTcs::All { count } = rguard.notified_tcs() {
*rguard.lock_var_mut() = Some(count)
} else {
unreachable!() // called notify_all
}

rust/library/core/src/iter/adapters/zip.rs

Lines 182 to 186 in a49e38e

match (self.a.next_back(), self.b.next_back()) {
(Some(x), Some(y)) => Some((x, y)),
(None, None) => None,
_ => unreachable!(),
}

rust/library/core/src/iter/adapters/zip.rs

Lines 219 to 224 in a49e38e

default unsafe fn get_unchecked(&mut self, _idx: usize) -> <Self as Iterator>::Item
where
Self: TrustedRandomAccessNoCoerce,
{
unreachable!("Always specialized");
}

rust/library/std/src/sync/mpsc/stream.rs

Lines 429 to 444 in fe1c942

// Now that we've determined that this queue "has data", we peek at the
// queue to see if the data is an upgrade or not. If it's an upgrade,
// then we need to destroy this port and abort selection on the
// upgraded port.
if has_data {
match self.queue.peek() {
Some(&mut GoUp(..)) => match self.queue.pop() {
Some(GoUp(port)) => Err(port),
_ => unreachable!(),
},
_ => Ok(true),
}
} else {
Ok(false)
}
}

rust/library/std/src/sync/mpsc/shared.rs

Lines 338 to 343 in 673d0db

match self.queue.pop() {
mpsc::Data(t) => Ok(t),
mpsc::Empty => Err(Disconnected),
// with no senders, an inconsistency is impossible.
mpsc::Inconsistent => unreachable!(),
}

rust/library/std/src/sys/unix/kernel_copy.rs

Lines 201 to 207 in dfd7b8d

match result {
CopyResult::Ended(bytes_copied) => return Ok(bytes_copied + written),
CopyResult::Error(e, _) => return Err(e),
CopyResult::Fallback(0) => { /* use the fallback below */ }
CopyResult::Fallback(_) => {
unreachable!("splice should not return > 0 bytes on the fallback path")
}

rust/library/std/src/path.rs

Line 912 in 2ad56d5

State::Done => unreachable!(),

rust/library/std/src/net/ip.rs

Lines 1679 to 1684 in a49e38e

match segments[5] {
// IPv4 Compatible address
0 => write!(f, "::{}", ipv4),
// IPv4 Mapped address
0xffff => write!(f, "::ffff:{}", ipv4),
_ => unreachable!(),

rust/library/alloc/src/collections/btree/split.rs

Lines 49 to 56 in 673d0db

match (split_edge.force(), right_node.force()) {
(Internal(edge), Internal(node)) => {
left_node = edge.descend();
right_node = node.first_edge().descend();
}
(Leaf(_), Leaf(_)) => break,
_ => unreachable!(),
}

rust/library/alloc/src/collections/btree/map.rs

Lines 184 to 187 in 3ed6c1d

let mut out_node = match root.borrow_mut().force() {
Leaf(leaf) => leaf,
Internal(_) => unreachable!(),
};

rust/library/alloc/src/collections/btree/node.rs

Line 1222 in 23461b2

Err(_) => unreachable!("empty internal node"),

rust/library/alloc/src/collections/btree/node.rs

Lines 1424 to 1439 in 23461b2

match (left_node.reborrow_mut().force(), right_node.reborrow_mut().force()) {
(ForceResult::Internal(mut left), ForceResult::Internal(mut right)) => {
// Make room for stolen edges.
slice_shr(right.edge_area_mut(..new_right_len + 1), count);
// Steal edges.
move_to_slice(
left.edge_area_mut(new_left_len + 1..old_left_len + 1),
right.edge_area_mut(..count),
);
right.correct_childrens_parent_links(0..new_right_len + 1);
}
(ForceResult::Leaf(_), ForceResult::Leaf(_)) => {}
_ => unreachable!(),
}

rust/library/alloc/src/collections/btree/node.rs

Lines 1487 to 1503 in 23461b2

match (left_node.reborrow_mut().force(), right_node.reborrow_mut().force()) {
(ForceResult::Internal(mut left), ForceResult::Internal(mut right)) => {
// Steal edges.
move_to_slice(
right.edge_area_mut(..count),
left.edge_area_mut(old_left_len + 1..new_left_len + 1),
);
// Fill gap where stolen edges used to be.
slice_shl(right.edge_area_mut(..old_right_len + 1), count);
left.correct_childrens_parent_links(old_left_len + 1..new_left_len + 1);
right.correct_childrens_parent_links(0..new_right_len + 1);
}
(ForceResult::Leaf(_), ForceResult::Leaf(_)) => {}
_ => unreachable!(),
}

rust/library/alloc/src/collections/btree/node.rs

Lines 1599 to 1609 in 23461b2

match (left_node.force(), right_node.force()) {
(ForceResult::Internal(mut left), ForceResult::Internal(mut right)) => {
move_to_slice(
left.edge_area_mut(new_left_len + 1..old_left_len + 1),
right.edge_area_mut(1..new_right_len + 1),
);
right.correct_childrens_parent_links(1..new_right_len + 1);
}
(ForceResult::Leaf(_), ForceResult::Leaf(_)) => {}
_ => unreachable!(),
}

rust/library/std/src/sync/mpsc/sync.rs

Lines 111 to 114 in 673d0db

match mem::replace(&mut guard.blocker, f(signal_token)) {
NoneBlocked => {}
_ => unreachable!(),
}

rust/library/std/src/sync/mpsc/mod.rs

Line 828 in 607d6b0

Flavor::Sync(..) => unreachable!(),

rust/library/std/src/sync/mpsc/sync.rs

Line 243 in 673d0db

BlockedSender(..) => unreachable!(),

rust/library/std/src/sync/mpsc/sync.rs

Lines 328 to 342 in 673d0db

// If this is a no-buffer channel (cap == 0), then if we didn't wait we
// need to ACK the sender. If we waited, then the sender waking us up
// was already the ACK.
let pending_sender2 = if guard.cap == 0 && !waited {
match mem::replace(&mut guard.blocker, NoneBlocked) {
NoneBlocked => None,
BlockedReceiver(..) => unreachable!(),
BlockedSender(token) => {
guard.canceled.take();
Some(token)
}
}
} else {
None
};

rust/library/std/src/sync/mpsc/sync.rs

Lines 378 to 382 in 673d0db

match mem::replace(&mut guard.blocker, NoneBlocked) {
NoneBlocked => {}
BlockedSender(..) => unreachable!(),
BlockedReceiver(token) => wakeup(token, guard),
}

rust/library/std/src/sync/mpsc/sync.rs

Lines 402 to 409 in 673d0db

let waiter = match mem::replace(&mut guard.blocker, NoneBlocked) {
NoneBlocked => None,
BlockedSender(token) => {
*guard.canceled.take().unwrap() = true;
Some(token)
}
BlockedReceiver(..) => unreachable!(),
};

rust/library/std/src/sync/mpsc/mod.rs

Lines 1157 to 1176 in 607d6b0

let new_port = match *unsafe { self.inner() } {
Flavor::Oneshot(ref p) => match p.recv(None) {
Ok(t) => return Ok(t),
Err(oneshot::Disconnected) => return Err(RecvError),
Err(oneshot::Upgraded(rx)) => rx,
Err(oneshot::Empty) => unreachable!(),
},
Flavor::Stream(ref p) => match p.recv(None) {
Ok(t) => return Ok(t),
Err(stream::Disconnected) => return Err(RecvError),
Err(stream::Upgraded(rx)) => rx,
Err(stream::Empty) => unreachable!(),
},
Flavor::Shared(ref p) => match p.recv(None) {
Ok(t) => return Ok(t),
Err(shared::Disconnected) => return Err(RecvError),
Err(shared::Empty) => unreachable!(),
},
Flavor::Sync(ref p) => return p.recv(None).map_err(|_| RecvError),
};

Must Remain unreachable

None so far.

Action

I would love to send in a PR that resolves this issue! However, I do need some help and guidance from the library maintainers to ensure that undefined behavior is 110% avoided. Feedback on which parts are safe regardless of platform/architecture/hardware is much appreciated.

As mentioned earlier, I will be editing this issue as more people chime in. Thanks!

Activity

  1. hellow554 commented on Sep 3, 2021

    @hellow554
    hellow554
    on Sep 3, 2021
    Contributor

    6 issues before this (#88600) is a very good reason to not do it and remain unreachable!(). In fact, a lot of ICEs will hit either a unreachable or an unwrap (or similar).

    If it is really unreachable, the compiler will figure it out and eleminate the branch, see https://rust.godbolt.org/z/5Kez3MPGW

    I don't think that this is reasonable but I'd like to hear others people opinion.

  2. the8472 commented on Sep 3, 2021

    @the8472
    the8472
    on Sep 3, 2021
    Member

    #86520 went in the opposite direction. Have you checked the performanee on any of these, e.g. by running the benches included in the library?

  3. BastiDood commented on Sep 4, 2021

    @BastiDood
    BastiDood
    on Sep 4, 2021
    ContributorAuthor

    Have you checked the performance on any of these?

    Honestly, this shouldn't be necessary anymore. I sense that there is already a strong consensus that the compiler is (more than) smart enough to detect truly unreachable branches. The panic infrastructure as a safety mechanism is hard to argue against, especially since the compiler already optimizes it away in most cases.

    This is great! Glad that I can certainly say that I can close this issue with much greater faith in the compiler's abilities. Thanks for the valuable input, everyone! I believe this issue does not need to linger any longer.

  4. BastiDood
    closed this as completedon Sep 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions