`get_unchecked` and friends should do bound checks in debug mode.

[ticki]

This can help catching a lot of safety issues.

[bluss]

It's a nice thought, but they cannot change to do this. It would be a breaking change since they are often used to go outside the formal length (i.e. of a vector), but still inside the capacity.

[ticki]

It's a nice thought, but they cannot change to do this. It would be a breaking change since they are often used to go outside the formal length (i.e. of a vector), but still inside the capacity.

Not true. offset is used for this.

[hanna-kruppe]

Plus, if get_unchecked were used for this purpose, it would probably be UB and should be changed since it returns &T, not *const T.

[ticki]

Yeah, &T holds the invariant that the inner value is initialized and valid, as opposed to *mut T or Unique<T>.

[bluss]

Current use of get_unchecked to go out of bounds of a slice:

• rust/src/libcollections/vec.rs

  Lines 553 to 555 in 26d8b6f

  	self.len -= 1;
  	let len = self.len;
  	ptr::drop_in_place(self.get_unchecked_mut(len));

• rust/src/libcollections/vec.rs

  Line 830 in 26d8b6f

  Some(ptr::read(self.get_unchecked(self.len())))

• rust/src/libcollections/vec.rs

  Line 856 in 26d8b6f

  ptr::copy_nonoverlapping(other.as_ptr(), self.get_unchecked_mut(len), other.len());

• rust/src/libcollections/vec.rs

  Line 1098 in 26d8b6f

  let ptr = self.get_unchecked_mut(len) as *mut T;

• rust/src/libcollections/vec.rs

  Line 1458 in 26d8b6f

  ptr::write(vector.get_unchecked_mut(0), element);

• rust/src/libcollections/vec.rs

  Line 1563 in 26d8b6f

  ptr::write(self.get_unchecked_mut(len), element);

It's ubiquitous in the implementation of Vec.

Non-libstd code uses the same pattern. It would for example break arrayvec

• https://github.com/bluss/arrayvec/blob/d43c959fa8afa912c497104b60a64892e1178f9d/src/lib.rs#L178

Break syntex

• https://github.com/serde-rs/syntex/blob/ed1a9ad18b66b9941d08735f5931f047104a85e9/syntex_syntax/src/util/move_map.rs#L32-L37

If it would be UB to access past the length but inside a capacity, why?

[ticki]

If it would be UB to access past the length but inside a capacity, why?

This code is definitely broken. &T asserts that the inner value is initialized, but it isn't past the length of the vector. It's very suprising that it does this instead of using offset.

[jonas-schievink]

Probably related to the fact that slice::get_unchecked has like no documentation, despite being unsafe.

[hanna-kruppe]

It's probably UB because it creates a &T to a location that is not initialized. The reference is immediately converted to a raw pointer, but simply existing is bad AFAIK. By analogy, in #36925 @arielb1 said that creating a &[T] (that is immediately converted to *const [T]) with a null pointer is insta-UB.

Granted, references are currently annotated with non-null metadata for LLVM, while they may not be an analogous attribute for "pointee memory is initialized". So it may not get miscompiled today. But it's still super dubious and should get phased out.

[bluss]

It's a question that the memory model has to answer, whether it is broken or not.

[bluss]

We can agree that this case is not broken, right, because it points at something that's initialized to a valid value?

    pub fn truncate(&mut self, len: usize) {
        unsafe {
            // drop any extra elements
            while len < self.len {
                // decrement len before the drop_in_place(), so a panic on Drop
                // doesn't re-drop the just-failed value.
                self.len -= 1;
                let len = self.len;
                ptr::drop_in_place(self.get_unchecked_mut(len));
            }
        }
    }

rust/src/libcollections/vec.rs

Lines 553 to 555 in 26d8b6f

	self.len -= 1;
	let len = self.len;
	ptr::drop_in_place(self.get_unchecked_mut(len));

[hanna-kruppe]

That use case is indeed not UB.

[arielb1]

It's probably UB because it creates a &T to a location that is not initialized. The reference is immediately converted to a raw pointer, but simply existing is bad AFAIK. By analogy, in #36925 @arielb1 said that creating a &[T] (that is immediately converted to *const [T]) with a null pointer is insta-UB.

That sounds like a good enough reason not to require references to have valid values behind them, as long as the memory behind them is valid. In any case, ATM we do not have any plans of requiring that.

[leonardo-m]

(Hello, this is my first comment around here).
I think the debug asserts in the get functions are important to catch some possible bugs. When I define custom matrices and the like I usually put debug_asserts to catch some bugs in debug builds. Debug builds are usually so slow compared to release builds, that adding the bound tests usually gives a quite small performance loss.

If adding bound checks in debug mode to get_unchecked/get_unchecked_mut is an unacceptable breaking change, then I suggest to add two new methods that do it. This makes the Vec/Slice API bigger, but I think it's worth it.

A problem is: how to name those two functions? get_debug_checked/get_debug_checked_mut could be clear, but they seems a bit too much long.