-
Notifications
You must be signed in to change notification settings - Fork 13k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add slice::check_range
#75207
Add slice::check_range
#75207
Changes from 4 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,8 +66,7 @@ use core::intrinsics::{arith_offset, assume}; | |
use core::iter::{FromIterator, FusedIterator, TrustedLen}; | ||
use core::marker::PhantomData; | ||
use core::mem::{self, ManuallyDrop, MaybeUninit}; | ||
use core::ops::Bound::{Excluded, Included, Unbounded}; | ||
use core::ops::{self, Index, IndexMut, RangeBounds}; | ||
use core::ops::{self, Index, IndexMut, Range, RangeBounds}; | ||
use core::ptr::{self, NonNull}; | ||
use core::slice::{self, SliceIndex}; | ||
|
||
|
@@ -1311,35 +1310,7 @@ impl<T> Vec<T> { | |
// the hole, and the vector length is restored to the new length. | ||
// | ||
let len = self.len(); | ||
let start = match range.start_bound() { | ||
Included(&n) => n, | ||
Excluded(&n) => n + 1, | ||
Unbounded => 0, | ||
}; | ||
let end = match range.end_bound() { | ||
Included(&n) => n + 1, | ||
Excluded(&n) => n, | ||
Unbounded => len, | ||
}; | ||
|
||
#[cold] | ||
#[inline(never)] | ||
fn start_assert_failed(start: usize, end: usize) -> ! { | ||
panic!("start drain index (is {}) should be <= end drain index (is {})", start, end); | ||
} | ||
|
||
#[cold] | ||
#[inline(never)] | ||
fn end_assert_failed(end: usize, len: usize) -> ! { | ||
panic!("end drain index (is {}) should be <= len (is {})", end, len); | ||
} | ||
|
||
if start > end { | ||
start_assert_failed(start, end); | ||
} | ||
if end > len { | ||
end_assert_failed(end, len); | ||
} | ||
let Range { start, end } = self.check_range(range); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like this method was pretty finely tuned, do we have any existing benchmarks we can check to see whether there's any impact? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think However, I wouldn't expect a regression. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there aren't any benches then I'd consider it a bit of a speculative implementation anyways, so am happy for this to be made simpler 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This call here is the problem: this implicitly creates a slice covering the entire vector, which is a problem because that slice aliases with other pointers that existed before and that should remain valid. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I proposed a fix in #76662. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting. Wouldn't this use the safe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It prevents aliasing from safe clients, but There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See here for the testcase that shows how unsafe code can rely on this property. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wow, I had no idea There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Many of these guarantees are documented here. I might have gone overboard in the test and check more than is guaranteed, but then it doesn't seem unlikely that people will take this guarantee as far as they can -- so I added a test basically for every operation that I could imagine preserving the buffer. Someone else double-checking this certainly would not hurt. :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, I read that passage but not thoroughly enough to notice these subtleties.
IIUC, there is still room for some of those calls to be removed. From the guarantees:
Thus, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah... maybe we should add a comment though that this test does not constitute a guarantee. I'll prepare a PR. |
||
|
||
unsafe { | ||
// set self.vec length's to start, to be safe in case Drain is leaked | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,7 +30,7 @@ use crate::intrinsics::{assume, exact_div, is_aligned_and_not_null, unchecked_su | |
use crate::iter::*; | ||
use crate::marker::{self, Copy, Send, Sized, Sync}; | ||
use crate::mem; | ||
use crate::ops::{self, FnMut, Range}; | ||
use crate::ops::{self, Bound, FnMut, Range, RangeBounds}; | ||
use crate::option::Option; | ||
use crate::option::Option::{None, Some}; | ||
use crate::ptr::{self, NonNull}; | ||
|
@@ -350,6 +350,79 @@ impl<T> [T] { | |
unsafe { &mut *index.get_unchecked_mut(self) } | ||
} | ||
|
||
/// Converts a range over this slice to [`Range`]. | ||
/// | ||
/// The returned range is safe to pass to [`get_unchecked`] and [`get_unchecked_mut`]. | ||
/// | ||
/// [`get_unchecked`]: #method.get_unchecked | ||
/// [`get_unchecked_mut`]: #method.get_unchecked_mut | ||
/// | ||
/// # Panics | ||
/// | ||
/// Panics if the range is out of bounds. | ||
/// | ||
/// # Examples | ||
/// | ||
/// ``` | ||
/// #![feature(slice_check_range)] | ||
/// | ||
/// let v = [10, 40, 30]; | ||
/// assert_eq!(1..2, v.check_range(1..2)); | ||
/// assert_eq!(0..2, v.check_range(..2)); | ||
/// assert_eq!(1..3, v.check_range(1..)); | ||
/// ``` | ||
/// | ||
/// Panics when [`Index::index`] would panic: | ||
/// | ||
/// ```should_panic | ||
/// #![feature(slice_check_range)] | ||
/// | ||
/// [10, 40, 30].check_range(2..1); | ||
/// ``` | ||
/// | ||
/// ```should_panic | ||
/// #![feature(slice_check_range)] | ||
/// | ||
/// [10, 40, 30].check_range(1..4); | ||
/// ``` | ||
/// | ||
/// ```should_panic | ||
/// #![feature(slice_check_range)] | ||
/// | ||
/// [10, 40, 30].check_range(1..=usize::MAX); | ||
/// ``` | ||
/// | ||
/// [`Index::index`]: ops::Index::index | ||
#[track_caller] | ||
#[unstable(feature = "slice_check_range", issue = "none")] | ||
pub fn check_range<R: RangeBounds<usize>>(&self, range: R) -> Range<usize> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can't unfortunately return a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain more? I don't see how this would be useless, since it converts It could return a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your code is useful. I meant to say that returning a CheckedRange is not so useful. As you say it could be done with the reference to the slice, but it could make it over engineered, so all is OK. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, that makes sense. Thanks for the clarification. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that we only need the length of the slice, I don't think this method should take a reference to the full slice. References are very strong types, they assert the aliasing rules and they assert that the memory they point to is initialized. As my other messages here show, that is a problem for this method. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For future readers, this conversation was continued here. |
||
let start = match range.start_bound() { | ||
Bound::Included(&start) => start, | ||
Bound::Excluded(start) => { | ||
start.checked_add(1).unwrap_or_else(|| slice_start_index_overflow_fail()) | ||
} | ||
Bound::Unbounded => 0, | ||
}; | ||
|
||
let len = self.len(); | ||
let end = match range.end_bound() { | ||
Bound::Included(end) => { | ||
end.checked_add(1).unwrap_or_else(|| slice_end_index_overflow_fail()) | ||
} | ||
Bound::Excluded(&end) => end, | ||
Bound::Unbounded => len, | ||
}; | ||
|
||
if start > end { | ||
slice_index_order_fail(start, end); | ||
} | ||
if end > len { | ||
slice_end_index_len_fail(end, len); | ||
} | ||
|
||
Range { start, end } | ||
} | ||
|
||
/// Returns a raw pointer to the slice's buffer. | ||
/// | ||
/// The caller must ensure that the slice outlives the pointer this | ||
|
@@ -2438,26 +2511,11 @@ impl<T> [T] { | |
/// ``` | ||
#[stable(feature = "copy_within", since = "1.37.0")] | ||
#[track_caller] | ||
pub fn copy_within<R: ops::RangeBounds<usize>>(&mut self, src: R, dest: usize) | ||
pub fn copy_within<R: RangeBounds<usize>>(&mut self, src: R, dest: usize) | ||
where | ||
T: Copy, | ||
{ | ||
let src_start = match src.start_bound() { | ||
ops::Bound::Included(&n) => n, | ||
ops::Bound::Excluded(&n) => { | ||
n.checked_add(1).unwrap_or_else(|| slice_index_overflow_fail()) | ||
} | ||
ops::Bound::Unbounded => 0, | ||
}; | ||
let src_end = match src.end_bound() { | ||
ops::Bound::Included(&n) => { | ||
n.checked_add(1).unwrap_or_else(|| slice_index_overflow_fail()) | ||
} | ||
ops::Bound::Excluded(&n) => n, | ||
ops::Bound::Unbounded => self.len(), | ||
}; | ||
assert!(src_start <= src_end, "src end is before src start"); | ||
assert!(src_end <= self.len(), "src is out of bounds"); | ||
let Range { start: src_start, end: src_end } = self.check_range(src); | ||
let count = src_end - src_start; | ||
assert!(dest <= self.len() - count, "dest is out of bounds"); | ||
unsafe { | ||
|
@@ -3034,7 +3092,14 @@ fn slice_index_order_fail(index: usize, end: usize) -> ! { | |
#[inline(never)] | ||
#[cold] | ||
#[track_caller] | ||
fn slice_index_overflow_fail() -> ! { | ||
fn slice_start_index_overflow_fail() -> ! { | ||
panic!("attempted to index slice from after maximum usize"); | ||
} | ||
|
||
#[inline(never)] | ||
#[cold] | ||
#[track_caller] | ||
fn slice_end_index_overflow_fail() -> ! { | ||
panic!("attempted to index slice up to maximum usize"); | ||
} | ||
|
||
|
@@ -3370,15 +3435,15 @@ unsafe impl<T> SliceIndex<[T]> for ops::RangeInclusive<usize> { | |
#[inline] | ||
fn index(self, slice: &[T]) -> &[T] { | ||
if *self.end() == usize::MAX { | ||
slice_index_overflow_fail(); | ||
slice_end_index_overflow_fail(); | ||
} | ||
(*self.start()..self.end() + 1).index(slice) | ||
} | ||
|
||
#[inline] | ||
fn index_mut(self, slice: &mut [T]) -> &mut [T] { | ||
if *self.end() == usize::MAX { | ||
slice_index_overflow_fail(); | ||
slice_end_index_overflow_fail(); | ||
} | ||
(*self.start()..self.end() + 1).index_mut(slice) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a hack, but it's not too bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this use
buffer_as_slice
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used that method at first, but I decided to use
from_raw_parts
instead, since it's a hack either way. The issue is that I need a contiguous slice forcheck_range
, so I use the firstself.len()
elements even if they're not initialized.I could change it to
unsafe { self.buffer_as_slice()[..self.len()] }
, but that adds an unnecessary bounds check and looks more valid than it is.I also added more of this explanation to the safety comment.