From 4ee00b521238adac640817f72f316a8e74c5afdd Mon Sep 17 00:00:00 2001 From: Joshua Liebow-Feeser Date: Fri, 20 Sep 2024 10:03:11 -0700 Subject: [PATCH] [byte_slice] Clean up docs and methods (#1700) Makes progress on #1692 --- src/byte_slice.rs | 81 ++++++++++++++++++++--------------------------- src/ref.rs | 46 ++++++++++++--------------- 2 files changed, 54 insertions(+), 73 deletions(-) diff --git a/src/byte_slice.rs b/src/byte_slice.rs index 7514576f78..d3e970af77 100644 --- a/src/byte_slice.rs +++ b/src/byte_slice.rs @@ -65,10 +65,10 @@ impl ByteSliceMut for B {} /// # Safety /// /// If `B: CopyableByteSlice`, then the dereference stability properties -/// required by `ByteSlice` (see that trait's safety documentation) do not only -/// hold regarding two calls to `b.deref()` or `b.deref_mut()`, but also hold -/// regarding `c.deref()` or `c.deref_mut()`, where `c` is produced by copying -/// `b`. +/// required by [`ByteSlice`] (see that trait's safety documentation) do not +/// only hold regarding two calls to `b.deref()` or `b.deref_mut()`, but also +/// hold regarding `c.deref()` or `c.deref_mut()`, where `c` is produced by +/// copying `b`. pub unsafe trait CopyableByteSlice: ByteSlice + Copy + CloneableByteSlice {} /// A [`ByteSlice`] which can be cloned without violating dereference stability. @@ -76,9 +76,9 @@ pub unsafe trait CopyableByteSlice: ByteSlice + Copy + CloneableByteSlice {} /// # Safety /// /// If `B: CloneableByteSlice`, then the dereference stability properties -/// required by `ByteSlice` (see that trait's safety documentation) do not only -/// hold regarding two calls to `b.deref()` or `b.deref_mut()`, but also hold -/// regarding `c.deref()` or `c.deref_mut()`, where `c` is produced by +/// required by [`ByteSlice`] (see that trait's safety documentation) do not +/// only hold regarding two calls to `b.deref()` or `b.deref_mut()`, but also +/// hold regarding `c.deref()` or `c.deref_mut()`, where `c` is produced by /// `b.clone()`, `b.clone().clone()`, etc. pub unsafe trait CloneableByteSlice: ByteSlice + Clone {} @@ -98,61 +98,48 @@ pub unsafe trait CloneableByteSlice: ByteSlice + Clone {} /// - `first`'s address is `addr` and its length is `split` /// - `second`'s address is `addr + split` and its length is `len - split` pub unsafe trait SplitByteSlice: ByteSlice { - /// Splits the slice at the midpoint. + /// Attempts to split `self` at the midpoint. /// - /// `x.split_at(mid)` returns `x[..mid]` and `x[mid..]`. + /// `s.split_at(mid)` returns `Ok((s[..mid], s[mid..]))` if `mid <= + /// s.deref().len()` and otherwise returns `Err(s)`. /// - /// # Panics + /// # Safety /// - /// `x.split_at(mid)` panics if `mid > x.deref().len()`. - #[must_use] + /// Unsafe code may rely on this function correctly implementing the above + /// functionality. #[inline] - fn split_at(self, mid: usize) -> (Self, Self) { - if let Ok(splits) = try_split_at(self, mid) { - splits + fn split_at(self, mid: usize) -> Result<(Self, Self), Self> { + if mid <= self.deref().len() { + // SAFETY: Above, we ensure that `mid <= self.deref().len()`. By + // invariant on `ByteSlice`, a supertrait of `SplitByteSlice`, + // `.deref()` is guranteed to be "stable"; i.e., it will always + // dereference to a byte slice of the same address and length. Thus, + // we can be sure that the above precondition remains satisfied + // through the call to `split_at_unchecked`. + unsafe { Ok(self.split_at_unchecked(mid)) } } else { - panic!("mid > len") + Err(self) } } /// Splits the slice at the midpoint, possibly omitting bounds checks. /// - /// `x.split_at_unchecked(mid)` returns `x[..mid]` and `x[mid..]`. + /// `s.split_at_unchecked(mid)` returns `s[..mid]` and `s[mid..]`. /// /// # Safety /// - /// `mid` must not be greater than `x.deref().len()`. + /// `mid` must not be greater than `self.deref().len()`. + /// + /// # Panics + /// + /// Implementations of this method may choose to perform a bounds check and + /// panic if `mid > self.deref().len()`. They may also panic for any other + /// reason. Since it is optional, callers must not rely on this behavior for + /// soundness. #[must_use] unsafe fn split_at_unchecked(self, mid: usize) -> (Self, Self); } -/// Attempts to split the slice at the midpoint. -/// -/// `x.try_split_at(mid)` returns `Ok((x[..mid], x[mid..]))` if `mid <= -/// x.deref().len()` and otherwise returns `Err(x)`. -/// -/// # Safety -/// -/// Unsafe code may rely on this function correctly implementing the above -/// functionality. -#[inline] -pub(crate) fn try_split_at(slice: S, mid: usize) -> Result<(S, S), S> -where - S: SplitByteSlice, -{ - if mid <= slice.deref().len() { - // SAFETY: Above, we ensure that `mid <= self.deref().len()`. By - // invariant on `ByteSlice`, a supertrait of `SplitByteSlice`, - // `.deref()` is guranteed to be "stable"; i.e., it will always - // dereference to a byte slice of the same address and length. Thus, we - // can be sure that the above precondition remains satisfied through the - // call to `split_at_unchecked`. - unsafe { Ok(slice.split_at_unchecked(mid)) } - } else { - Err(slice) - } -} - /// A shorthand for [`SplitByteSlice`] and [`ByteSliceMut`]. pub trait SplitByteSliceMut: SplitByteSlice + ByteSliceMut {} impl SplitByteSliceMut for B {} @@ -174,7 +161,7 @@ pub unsafe trait IntoByteSlice<'a>: ByteSlice { /// # Safety /// /// The returned reference has the same address and length as `self.deref()` - /// or `self.deref_mut()`. + /// and `self.deref_mut()`. /// /// Note that, combined with the safety invariant on [`ByteSlice`], this /// safety invariant implies that the returned reference is "stable" in the @@ -199,7 +186,7 @@ pub unsafe trait IntoByteSliceMut<'a>: IntoByteSlice<'a> + ByteSliceMut { /// # Safety /// /// The returned reference has the same address and length as `self.deref()` - /// or `self.deref_mut()`. + /// and `self.deref_mut()`. /// /// Note that, combined with the safety invariant on [`ByteSlice`], this /// safety invariant implies that the returned reference is "stable" in the diff --git a/src/ref.rs b/src/ref.rs index 5eeefcbeb7..9813f1cacc 100644 --- a/src/ref.rs +++ b/src/ref.rs @@ -234,12 +234,12 @@ where return Err(AlignmentError::new(bytes).into()); } let (bytes, suffix) = - try_split_at(bytes, mem::size_of::()).map_err(|b| SizeError::new(b).into())?; + bytes.split_at(mem::size_of::()).map_err(|b| SizeError::new(b).into())?; // SAFETY: We just validated alignment and that `bytes` is at least as - // large as `T`. `try_split_at(bytes, mem::size_of::())?` ensures - // that the new `bytes` is exactly the size of `T`. By safety - // postcondition on `SplitByteSlice::try_split_at` we can rely on - // `try_split_at` to produce the correct `bytes` and `suffix`. + // large as `T`. `bytes.split_at(mem::size_of::())?` ensures that the + // new `bytes` is exactly the size of `T`. By safety postcondition on + // `SplitByteSlice::split_at` we can rely on `split_at` to produce the + // correct `bytes` and `suffix`. let r = unsafe { Ref::new_unchecked(bytes) }; Ok((r, suffix)) } @@ -252,17 +252,16 @@ where } else { return Err(SizeError::new(bytes).into()); }; - let (prefix, bytes) = - try_split_at(bytes, split_at).map_err(|b| SizeError::new(b).into())?; + let (prefix, bytes) = bytes.split_at(split_at).map_err(|b| SizeError::new(b).into())?; if !util::aligned_to::<_, T>(bytes.deref()) { return Err(AlignmentError::new(bytes).into()); } // SAFETY: Since `split_at` is defined as `bytes_len - size_of::()`, // the `bytes` which results from `let (prefix, bytes) = - // try_split_at(bytes, split_at)?` has length `size_of::()`. After + // bytes.split_at(split_at)?` has length `size_of::()`. After // constructing `bytes`, we validate that it has the proper alignment. - // By safety postcondition on `SplitByteSlice::try_split_at` we can rely - // on `try_split_at` to produce the correct `prefix` and `bytes`. + // By safety postcondition on `SplitByteSlice::split_at` we can rely on + // `split_at` to produce the correct `prefix` and `bytes`. let r = unsafe { Ref::new_unchecked(bytes) }; Ok((prefix, r)) } @@ -373,13 +372,11 @@ where // underflow. #[allow(unstable_name_collisions, clippy::incompatible_msrv)] let split_at = unsafe { source.len().unchecked_sub(remainder.len()) }; - let (bytes, suffix) = - try_split_at(source, split_at).map_err(|b| SizeError::new(b).into())?; + let (bytes, suffix) = source.split_at(split_at).map_err(|b| SizeError::new(b).into())?; // SAFETY: `try_cast_into` validates size and alignment, and returns a // `split_at` that indicates how many bytes of `source` correspond to a - // valid `T`. By safety postcondition on `SplitByteSlice::try_split_at` - // we can rely on `try_split_at` to produce the correct `source` and - // `suffix`. + // valid `T`. By safety postcondition on `SplitByteSlice::split_at` we + // can rely on `split_at` to produce the correct `source` and `suffix`. let r = unsafe { Ref::new_unchecked(bytes) }; Ok((r, suffix)) } @@ -431,13 +428,11 @@ where }; let split_at = remainder.len(); - let (prefix, bytes) = - try_split_at(source, split_at).map_err(|b| SizeError::new(b).into())?; + let (prefix, bytes) = source.split_at(split_at).map_err(|b| SizeError::new(b).into())?; // SAFETY: `try_cast_into` validates size and alignment, and returns a - // `try_split_at` that indicates how many bytes of `source` correspond to - // a valid `T`. By safety postcondition on - // `SplitByteSlice::try_split_at` we can rely on `try_split_at` to - // produce the correct `prefix` and `bytes`. + // `split_at` that indicates how many bytes of `source` correspond to a + // valid `T`. By safety postcondition on `SplitByteSlice::split_at` we + // can rely on `split_at` to produce the correct `prefix` and `bytes`. let r = unsafe { Ref::new_unchecked(bytes) }; Ok((prefix, r)) } @@ -532,10 +527,7 @@ where Some(len) => len, None => return Err(SizeError::new(source).into()), }; - if source.len() < expected_len { - return Err(SizeError::new(source).into()); - } - let (prefix, bytes) = source.split_at(expected_len); + let (prefix, bytes) = source.split_at(expected_len).map_err(SizeError::new)?; Self::from_bytes(prefix).map(move |l| (l, bytes)) } @@ -581,7 +573,9 @@ where } else { return Err(SizeError::new(source).into()); }; - let (bytes, suffix) = source.split_at(split_at); + // SAFETY: The preceeding `source.len().checked_sub(expected_len)` + // guarantees that `split_at` is in-bounds. + let (bytes, suffix) = unsafe { source.split_at_unchecked(split_at) }; Self::from_bytes(suffix).map(move |l| (bytes, l)) } }