Skip to content
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.

Sign up for GitHub

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

Implement indexing slices with pairs of core::ops::Bound<usize> #77704

Merged
merged 2 commits into from
Apr 22, 2021
Merged

Implement indexing slices with pairs of core::ops::Bound<usize> #77704

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7efba4f
Implement indexing slices with pairs of ops::Bound<usize>
AnthonyMikh Oct 8, 2020
6763a40
Bump slice_index_with_ops_bound_pair to 1.53.0
m-ou-se Apr 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 112 additions & 0 deletions library/core/src/slice/index.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ mod private_slice_index {
impl Sealed for ops::RangeInclusive<usize> {}
#[stable(feature = "slice_get_slice", since = "1.28.0")]
impl Sealed for ops::RangeToInclusive<usize> {}
#[stable(feature = "slice_index_with_ops_bound_pair", since = "1.53.0")]
impl Sealed for (ops::Bound<usize>, ops::Bound<usize>) {}
}

/// A helper trait used for indexing operations.
Expand Down Expand Up @@ -546,3 +548,113 @@ where

ops::Range { start, end }
}

/// Convert pair of `ops::Bound`s into `ops::Range` without performing any bounds checking and (in debug) overflow checking
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhere in the user-facing documentation of the unchecked indexing methods, it should say that the user is also responsible for avoiding overflow when using an Excluded lower-bound or an Included upper-bound. Where does it say that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like those could possibly go on the impl itself. There are other invariants documented on other impl SliceIndexes already.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that basically undiscoverable?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh, it’s not as discoverable as directly on the methods themselves.

But it looks like this is actually also a problem for RangeInclusive::get_unchecked, which doesn’t appear to document the requirement that self.end != usize::MAX in the public API docs.

Maybe we should flesh out the docs for [T]::get_unchecked{_mut} a bit to mention the possibility of overflowing bounds if they’re converted between inclusive/exclusive?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree this is an existing problem.

Maybe we should flesh out the docs for [T]::get_unchecked{_mut} a bit to mention the possibility of overflowing bounds if they’re converted between inclusive/exclusive?

Sounds reasonable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation right now says:

Calling this method with an out-of-bounds index or a dangling slice pointer is undefined behavior even if the resulting reference is not used.

Since usize::MAX would always be out of bounds for anything taking up space, we're only talking about overflows in [T]::get_unchecked for ZSTs here, right? Or am I missing something?

Copy link
Member

@m-ou-se m-ou-se Jan 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And even for ZSTs, slices can not be longer than usize::MAX. So an Included(usize::MAX) range end already counts as out-of-bounds, permitting UB.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So an Included(usize::MAX) range end already counts as out-of-bounds, permitting UB.

Hm, I can agree with that interpretation, but I feel it would be worth calling out explicitly that if there is an overflow while computing the bounds, that also counts as out-of-bounds.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the very least it would be a great inline comment here and in RangeInclusive.

fn into_range_unchecked(
len: usize,
(start, end): (ops::Bound<usize>, ops::Bound<usize>),
) -> ops::Range<usize> {
use ops::Bound;
let start = match start {
Bound::Included(i) => i,
Bound::Excluded(i) => i + 1,
Bound::Unbounded => 0,
};
let end = match end {
Bound::Included(i) => i + 1,
Bound::Excluded(i) => i,
Bound::Unbounded => len,
};
start..end
}

/// Convert pair of `ops::Bound`s into `ops::Range`.
/// Returns `None` on overflowing indices.
fn into_range(
len: usize,
(start, end): (ops::Bound<usize>, ops::Bound<usize>),
) -> Option<ops::Range<usize>> {
use ops::Bound;
let start = match start {
Bound::Included(start) => start,
Bound::Excluded(start) => start.checked_add(1)?,
Bound::Unbounded => 0,
};

let end = match end {
Bound::Included(end) => end.checked_add(1)?,
Bound::Excluded(end) => end,
Bound::Unbounded => len,
};

// Don't bother with checking `start < end` and `end <= len`
// since these checks are handled by `Range` impls

Some(start..end)
}

/// Convert pair of `ops::Bound`s into `ops::Range`.
/// Panics on overflowing indices.
fn into_slice_range(
len: usize,
(start, end): (ops::Bound<usize>, ops::Bound<usize>),
) -> ops::Range<usize> {
use ops::Bound;
let start = match start {
Bound::Included(start) => start,
Bound::Excluded(start) => {
start.checked_add(1).unwrap_or_else(|| slice_start_index_overflow_fail())
}
Bound::Unbounded => 0,
};

let end = match end {
Bound::Included(end) => {
end.checked_add(1).unwrap_or_else(|| slice_end_index_overflow_fail())
}
Bound::Excluded(end) => end,
Bound::Unbounded => len,
};

// Don't bother with checking `start < end` and `end <= len`
// since these checks are handled by `Range` impls

start..end
}

#[stable(feature = "slice_index_with_ops_bound_pair", since = "1.53.0")]
unsafe impl<T> SliceIndex<[T]> for (ops::Bound<usize>, ops::Bound<usize>) {
type Output = [T];

#[inline]
fn get(self, slice: &[T]) -> Option<&Self::Output> {
into_range(slice.len(), self)?.get(slice)
}

#[inline]
fn get_mut(self, slice: &mut [T]) -> Option<&mut Self::Output> {
into_range(slice.len(), self)?.get_mut(slice)
}

#[inline]
unsafe fn get_unchecked(self, slice: *const [T]) -> *const Self::Output {
// SAFETY: the caller has to uphold the safety contract for `get_unchecked`.
unsafe { into_range_unchecked(slice.len(), self).get_unchecked(slice) }
}

#[inline]
unsafe fn get_unchecked_mut(self, slice: *mut [T]) -> *mut Self::Output {
// SAFETY: the caller has to uphold the safety contract for `get_unchecked_mut`.
unsafe { into_range_unchecked(slice.len(), self).get_unchecked_mut(slice) }
}

#[inline]
fn index(self, slice: &[T]) -> &Self::Output {
into_slice_range(slice.len(), self).index(slice)
}

#[inline]
fn index_mut(self, slice: &mut [T]) -> &mut Self::Output {
into_slice_range(slice.len(), self).index_mut(slice)
}
}
43 changes: 43 additions & 0 deletions library/core/tests/slice.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1280,6 +1280,9 @@ mod slice_index {
}
)*) => {$(
mod $case_name {
#[allow(unused_imports)]
use core::ops::Bound;

#[test]
fn pass() {
let mut v = $data;
Expand Down Expand Up @@ -1376,6 +1379,24 @@ mod slice_index {
bad: data[7..=6];
message: "out of range";
}

in mod boundpair_len {
data: [0, 1, 2, 3, 4, 5];

good: data[(Bound::Included(6), Bound::Unbounded)] == [];
good: data[(Bound::Unbounded, Bound::Included(5))] == [0, 1, 2, 3, 4, 5];
good: data[(Bound::Unbounded, Bound::Excluded(6))] == [0, 1, 2, 3, 4, 5];
good: data[(Bound::Included(0), Bound::Included(5))] == [0, 1, 2, 3, 4, 5];
good: data[(Bound::Included(0), Bound::Excluded(6))] == [0, 1, 2, 3, 4, 5];
good: data[(Bound::Included(2), Bound::Excluded(4))] == [2, 3];
good: data[(Bound::Excluded(1), Bound::Included(4))] == [2, 3, 4];
good: data[(Bound::Excluded(5), Bound::Excluded(6))] == [];
AnthonyMikh marked this conversation as resolved.
Show resolved Hide resolved
good: data[(Bound::Included(6), Bound::Excluded(6))] == [];
good: data[(Bound::Excluded(5), Bound::Included(5))] == [];
good: data[(Bound::Included(6), Bound::Included(5))] == [];
bad: data[(Bound::Unbounded, Bound::Included(6))];
message: "out of range";
}
}

panic_cases! {
Expand Down Expand Up @@ -1416,6 +1437,14 @@ mod slice_index {
bad: data[4..=2];
message: "but ends at";
}

in mod boundpair_neg_width {
data: [0, 1, 2, 3, 4, 5];

good: data[(Bound::Included(4), Bound::Excluded(4))] == [];
bad: data[(Bound::Included(4), Bound::Excluded(3))];
message: "but ends at";
}
}

panic_cases! {
Expand All @@ -1434,6 +1463,20 @@ mod slice_index {
bad: data[..= usize::MAX];
message: "maximum usize";
}

in mod boundpair_overflow_end {
data: [0; 1];

bad: data[(Bound::Unbounded, Bound::Included(usize::MAX))];
AnthonyMikh marked this conversation as resolved.
Show resolved Hide resolved
message: "maximum usize";
}

in mod boundpair_overflow_start {
data: [0; 1];

bad: data[(Bound::Excluded(usize::MAX), Bound::Unbounded)];
message: "maximum usize";
}
} // panic_cases!
}

Expand Down