`OneSidedRange` should be an `unsafe trait`

[lopopolo]

I tried this code:

#![feature(one_sided_range)]
#![feature(slice_take)]

use core::ops::{OneSidedRange, RangeBounds, Bound};

struct X(usize, usize);

impl RangeBounds<usize> for X {
    fn start_bound(&self) -> Bound<&usize> {
        Bound::Included(&self.0)
    }
    
    fn end_bound(&self) -> Bound<&usize> {
        Bound::Excluded(&self.1)
    }
}

impl OneSidedRange<usize> for X {}

fn main() {
    let mut slice: &[_] = &['a', 'b', 'c', 'd'];
    assert_eq!(None, slice.take(X(5, usize::MAX)));
    assert_eq!(None, slice.take(X(0, 5)));
    assert_eq!(None, slice.take(X(0, 4)));
    let expected: &[char] = &['a', 'b', 'c', 'd'];
    assert_eq!(Some(expected), slice.take(X(0, 4)));
}

I expected to see this happen: Compiler error because this code did not acknowledge the invariants of OneSidedRange without an unsafe keyword.

Instead, this happened:

Compiling playground v0.0.1 (/playground)
    Finished dev [unoptimized + debuginfo] target(s) in 2.23s
     Running `target/debug/playground`
thread 'main' panicked at 'internal error: entered unreachable code', /rustc/0f4bcadb46006bc484dad85616b484f93879ca4e/library/core/src/slice/mod.rs:104:14
note: [run with `RUST_BACKTRACE=1` environment variable to display a backtrace](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021#)

Meta

Nightly channel (playground)
Build using the Nightly version: 1.64.0-nightly

(2022-07-30 0f4bcad)

Backtrace

The backtrace starts from here:

rust/library/core/src/slice/mod.rs

Line 104 in ed793d8

_ => unreachable!(),

[Mark-Simulacrum]

In general, traits that have some expected behavior aren't unsafe -- and being able to cause asserts as a result is also fine. For example, BTreeMap will do something similar with a bad implementation of Ord, if it happens to detect it.

So I think this isn't a bug unless there's unsafe code relying on the correct implementation for soundness.

[lopopolo]

This seems to at least be a bit of a footgun in that the call to unreachable! has a load bearing panic and can't be replaced with std::hint::unreachable_unchecked. If the panic is meant to be part of the implementation, I'd expect to see panic! instead of unreachable!.

[Mark-Simulacrum]

The call to unreachable!() can be replaced with a more informative panic message, that would be a good change to make. However, in general, it's not the case that you can replace unreachable!() with UB -- it's not that uncommon for it to mean "this should be unreachable if all is well", and a bad trait impl that's not satisfying its contract is a relatively reasonable thing to not consider when calling unreachable!().