-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Remove unsound TrustedRandomAccess implementations #85874
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
Changes from all commits
a0d8a32
1c7f27f
69dd992
bbc6b26
f9c982c
9ff421d
89583e9
6d9c0a1
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 |
---|---|---|
|
@@ -91,7 +91,7 @@ where | |
#[doc(hidden)] | ||
unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item | ||
where | ||
Self: TrustedRandomAccess, | ||
Self: TrustedRandomAccessNoCoerce, | ||
{ | ||
// SAFETY: `ZipImpl::__iterator_get_unchecked` has same safety | ||
// requirements as `Iterator::__iterator_get_unchecked`. | ||
|
@@ -126,7 +126,66 @@ trait ZipImpl<A, B> { | |
// This has the same safety requirements as `Iterator::__iterator_get_unchecked` | ||
unsafe fn get_unchecked(&mut self, idx: usize) -> <Self as Iterator>::Item | ||
where | ||
Self: Iterator + TrustedRandomAccess; | ||
Self: Iterator + TrustedRandomAccessNoCoerce; | ||
} | ||
|
||
// Work around limitations of specialization, requiring `default` impls to be repeated | ||
// in intermediary impls. | ||
macro_rules! zip_impl_general_defaults { | ||
() => { | ||
default fn new(a: A, b: B) -> Self { | ||
Zip { | ||
a, | ||
b, | ||
index: 0, // unused | ||
len: 0, // unused | ||
a_len: 0, // unused | ||
} | ||
} | ||
|
||
#[inline] | ||
default fn next(&mut self) -> Option<(A::Item, B::Item)> { | ||
let x = self.a.next()?; | ||
let y = self.b.next()?; | ||
Some((x, y)) | ||
} | ||
|
||
#[inline] | ||
default fn nth(&mut self, n: usize) -> Option<Self::Item> { | ||
self.super_nth(n) | ||
} | ||
|
||
#[inline] | ||
default fn next_back(&mut self) -> Option<(A::Item, B::Item)> | ||
where | ||
A: DoubleEndedIterator + ExactSizeIterator, | ||
B: DoubleEndedIterator + ExactSizeIterator, | ||
{ | ||
// The function body below only uses `self.a/b.len()` and `self.a/b.next_back()` | ||
// and doesn’t call `next_back` too often, so this implementation is safe in | ||
// the `TrustedRandomAccessNoCoerce` specialization | ||
|
||
let a_sz = self.a.len(); | ||
let b_sz = self.b.len(); | ||
if a_sz != b_sz { | ||
// Adjust a, b to equal length | ||
if a_sz > b_sz { | ||
for _ in 0..a_sz - b_sz { | ||
self.a.next_back(); | ||
} | ||
} else { | ||
for _ in 0..b_sz - a_sz { | ||
self.b.next_back(); | ||
} | ||
} | ||
} | ||
match (self.a.next_back(), self.b.next_back()) { | ||
(Some(x), Some(y)) => Some((x, y)), | ||
(None, None) => None, | ||
_ => unreachable!(), | ||
} | ||
} | ||
}; | ||
} | ||
|
||
// General Zip impl | ||
|
@@ -137,54 +196,8 @@ where | |
B: Iterator, | ||
{ | ||
type Item = (A::Item, B::Item); | ||
default fn new(a: A, b: B) -> Self { | ||
Zip { | ||
a, | ||
b, | ||
index: 0, // unused | ||
len: 0, // unused | ||
a_len: 0, // unused | ||
} | ||
} | ||
|
||
#[inline] | ||
default fn next(&mut self) -> Option<(A::Item, B::Item)> { | ||
let x = self.a.next()?; | ||
let y = self.b.next()?; | ||
Some((x, y)) | ||
} | ||
|
||
#[inline] | ||
default fn nth(&mut self, n: usize) -> Option<Self::Item> { | ||
self.super_nth(n) | ||
} | ||
|
||
#[inline] | ||
default fn next_back(&mut self) -> Option<(A::Item, B::Item)> | ||
where | ||
A: DoubleEndedIterator + ExactSizeIterator, | ||
B: DoubleEndedIterator + ExactSizeIterator, | ||
{ | ||
let a_sz = self.a.len(); | ||
let b_sz = self.b.len(); | ||
if a_sz != b_sz { | ||
// Adjust a, b to equal length | ||
if a_sz > b_sz { | ||
for _ in 0..a_sz - b_sz { | ||
self.a.next_back(); | ||
} | ||
} else { | ||
for _ in 0..b_sz - a_sz { | ||
self.b.next_back(); | ||
} | ||
} | ||
} | ||
match (self.a.next_back(), self.b.next_back()) { | ||
(Some(x), Some(y)) => Some((x, y)), | ||
(None, None) => None, | ||
_ => unreachable!(), | ||
} | ||
} | ||
zip_impl_general_defaults! {} | ||
|
||
#[inline] | ||
default fn size_hint(&self) -> (usize, Option<usize>) { | ||
|
@@ -205,12 +218,35 @@ where | |
|
||
default unsafe fn get_unchecked(&mut self, _idx: usize) -> <Self as Iterator>::Item | ||
where | ||
Self: TrustedRandomAccess, | ||
Self: TrustedRandomAccessNoCoerce, | ||
{ | ||
unreachable!("Always specialized"); | ||
} | ||
} | ||
|
||
#[doc(hidden)] | ||
impl<A, B> ZipImpl<A, B> for Zip<A, B> | ||
where | ||
A: TrustedRandomAccessNoCoerce + Iterator, | ||
B: TrustedRandomAccessNoCoerce + Iterator, | ||
{ | ||
zip_impl_general_defaults! {} | ||
|
||
#[inline] | ||
default fn size_hint(&self) -> (usize, Option<usize>) { | ||
let size = cmp::min(self.a.size(), self.b.size()); | ||
(size, Some(size)) | ||
} | ||
|
||
#[inline] | ||
unsafe fn get_unchecked(&mut self, idx: usize) -> <Self as Iterator>::Item { | ||
let idx = self.index + idx; | ||
// SAFETY: the caller must uphold the contract for | ||
// `Iterator::__iterator_get_unchecked`. | ||
unsafe { (self.a.__iterator_get_unchecked(idx), self.b.__iterator_get_unchecked(idx)) } | ||
} | ||
} | ||
|
||
#[doc(hidden)] | ||
impl<A, B> ZipImpl<A, B> for Zip<A, B> | ||
where | ||
|
@@ -330,14 +366,6 @@ where | |
None | ||
} | ||
} | ||
|
||
#[inline] | ||
unsafe fn get_unchecked(&mut self, idx: usize) -> <Self as Iterator>::Item { | ||
let idx = self.index + idx; | ||
// SAFETY: the caller must uphold the contract for | ||
// `Iterator::__iterator_get_unchecked`. | ||
unsafe { (self.a.__iterator_get_unchecked(idx), self.b.__iterator_get_unchecked(idx)) } | ||
} | ||
} | ||
|
||
#[stable(feature = "rust1", since = "1.0.0")] | ||
|
@@ -354,6 +382,15 @@ unsafe impl<A, B> TrustedRandomAccess for Zip<A, B> | |
where | ||
A: TrustedRandomAccess, | ||
B: TrustedRandomAccess, | ||
{ | ||
} | ||
|
||
#[doc(hidden)] | ||
#[unstable(feature = "trusted_random_access", issue = "none")] | ||
unsafe impl<A, B> TrustedRandomAccessNoCoerce for Zip<A, B> | ||
where | ||
A: TrustedRandomAccessNoCoerce, | ||
B: TrustedRandomAccessNoCoerce, | ||
{ | ||
const MAY_HAVE_SIDE_EFFECT: bool = A::MAY_HAVE_SIDE_EFFECT || B::MAY_HAVE_SIDE_EFFECT; | ||
} | ||
|
@@ -417,7 +454,9 @@ impl<A: Debug, B: Debug> ZipFmt<A, B> for Zip<A, B> { | |
} | ||
} | ||
|
||
impl<A: Debug + TrustedRandomAccess, B: Debug + TrustedRandomAccess> ZipFmt<A, B> for Zip<A, B> { | ||
impl<A: Debug + TrustedRandomAccessNoCoerce, B: Debug + TrustedRandomAccessNoCoerce> ZipFmt<A, B> | ||
for Zip<A, B> | ||
{ | ||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
// It's *not safe* to call fmt on the contained iterators, since once | ||
// we start iterating they're in strange, potentially unsafe, states. | ||
|
@@ -431,34 +470,70 @@ impl<A: Debug + TrustedRandomAccess, B: Debug + TrustedRandomAccess> ZipFmt<A, B | |
/// | ||
/// The iterator's `size_hint` must be exact and cheap to call. | ||
/// | ||
/// `size` may not be overridden. | ||
/// `TrustedRandomAccessNoCoerce::size` may not be overridden. | ||
/// | ||
/// All subtypes and all supertypes of `Self` must also implement `TrustedRandomAccess`. | ||
/// In particular, this means that types with non-invariant parameters usually can not have | ||
/// an impl for `TrustedRandomAccess` that depends on any trait bounds on such parameters, except | ||
/// for bounds that come from the respective struct/enum definition itself, or bounds involving | ||
/// traits that themselves come with a guarantee similar to this one. | ||
/// | ||
/// If `Self: ExactSizeIterator` then `self.len()` must always produce results consistent | ||
/// with `self.size()`. | ||
/// | ||
/// `<Self as Iterator>::__iterator_get_unchecked` must be safe to call | ||
/// provided the following conditions are met. | ||
/// If `Self: Iterator`, then `<Self as Iterator>::__iterator_get_unchecked(&mut self, idx)` | ||
/// must be safe to call provided the following conditions are met. | ||
/// | ||
/// 1. `0 <= idx` and `idx < self.size()`. | ||
/// 2. If `self: !Clone`, then `get_unchecked` is never called with the same | ||
/// 2. If `Self: !Clone`, then `self.__iterator_get_unchecked(idx)` is never called with the same | ||
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 have never quite understood the way this is phrased. I mean of course an index will be accessed multiple times if an iterator gets cloned, but that's happens across the different clones. |
||
/// index on `self` more than once. | ||
/// 3. After `self.get_unchecked(idx)` has been called then `next_back` will | ||
/// only be called at most `self.size() - idx - 1` times. | ||
/// 4. After `get_unchecked` is called, then only the following methods will be | ||
/// called on `self`: | ||
/// * `std::clone::Clone::clone()` | ||
/// * `std::iter::Iterator::size_hint()` | ||
/// * `std::iter::DoubleEndedIterator::next_back()` | ||
/// * `std::iter::Iterator::__iterator_get_unchecked()` | ||
/// * `std::iter::TrustedRandomAccess::size()` | ||
/// 3. After `self.__iterator_get_unchecked(idx)` has been called, then `self.next_back()` will | ||
/// only be called at most `self.size() - idx - 1` times. If `Self: Clone` and `self` is cloned, | ||
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. Doesn't this assume that Note that the trait is called TrusedRandomAccess, so theoretically some future new iterator method could take some shortcut by directly indexing into an iterator in a non-linear fashion. 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. Well, first of all note that I didn’t write the original text, I just clarified it w.r.t.
so…
There you go, now you’re officially no longer allowed to call 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 guess for all of this to make just a little bit more sense, there should also be the requirement (currently undocumented) that calling |
||
/// then this number is calculated for `self` and its clone individually, | ||
/// but `self.next_back()` calls that happened before the cloning count for both `self` and the clone. | ||
/// 4. After `self.__iterator_get_unchecked(idx)` has been called, then only the following methods | ||
/// will be called on `self` or on any new clones of `self`: | ||
/// * `std::clone::Clone::clone` | ||
/// * `std::iter::Iterator::size_hint` | ||
/// * `std::iter::DoubleEndedIterator::next_back` | ||
/// * `std::iter::ExactSizeIterator::len` | ||
/// * `std::iter::Iterator::__iterator_get_unchecked` | ||
/// * `std::iter::TrustedRandomAccessNoCoerce::size` | ||
/// 5. If `T` is a subtype of `Self`, then `self` is allowed to be coerced | ||
/// to `T`. If `self` is coerced to `T` after `self.__iterator_get_unchecked(idx)` has already | ||
/// been called, then no methods except for the ones listed under 4. are allowed to be called | ||
/// on the resulting value of type `T`, either. Multiple such coercion steps are allowed. | ||
/// Regarding 2. and 3., the number of times `__iterator_get_unchecked(idx)` or `next_back()` is | ||
/// called on `self` and the resulting value of type `T` (and on further coercion results with | ||
/// sub-subtypes) are added together and their sums must not exceed the specified bounds. | ||
/// | ||
/// Further, given that these conditions are met, it must guarantee that: | ||
/// | ||
/// * It does not change the value returned from `size_hint` | ||
/// * It must be safe to call the methods listed above on `self` after calling | ||
/// `get_unchecked`, assuming that the required traits are implemented. | ||
/// * It must also be safe to drop `self` after calling `get_unchecked`. | ||
/// `self.__iterator_get_unchecked(idx)`, assuming that the required traits are implemented. | ||
/// * It must also be safe to drop `self` after calling `self.__iterator_get_unchecked(idx)`. | ||
/// * If `T` is a subtype of `Self`, then it must be safe to coerce `self` to `T`. | ||
// | ||
// FIXME: Clarify interaction with SourceIter/InPlaceIterable. Calling `SouceIter::as_inner` | ||
// after `__iterator_get_unchecked` is supposed to be allowed. | ||
#[doc(hidden)] | ||
#[unstable(feature = "trusted_random_access", issue = "none")] | ||
#[rustc_specialization_trait] | ||
pub unsafe trait TrustedRandomAccess: TrustedRandomAccessNoCoerce {} | ||
|
||
/// Like [`TrustedRandomAccess`] but without any of the requirements / guarantees around | ||
/// coercions to subtypes after `__iterator_get_unchecked` (they aren’t allowed here!), and | ||
/// without the requirement that subtypes / supertypes implement `TrustedRandomAccessNoCoerce`. | ||
/// | ||
/// This trait was created in PR #85874 to fix soundness issue #85873 without performance regressions. | ||
/// It is subject to change as we might want to build a more generally useful (for performance | ||
/// optimizations) and more sophisticated trait or trait hierarchy that replaces or extends | ||
/// [`TrustedRandomAccess`] and `TrustedRandomAccessNoCoerce`. | ||
#[doc(hidden)] | ||
#[unstable(feature = "trusted_random_access", issue = "none")] | ||
#[rustc_specialization_trait] | ||
pub unsafe trait TrustedRandomAccess: Sized { | ||
pub unsafe trait TrustedRandomAccessNoCoerce: Sized { | ||
// Convenience method. | ||
fn size(&self) -> usize | ||
where | ||
|
@@ -499,7 +574,7 @@ unsafe impl<I: Iterator> SpecTrustedRandomAccess for I { | |
} | ||
} | ||
|
||
unsafe impl<I: Iterator + TrustedRandomAccess> SpecTrustedRandomAccess for I { | ||
unsafe impl<I: Iterator + TrustedRandomAccessNoCoerce> SpecTrustedRandomAccess for I { | ||
unsafe fn try_get_unchecked(&mut self, index: usize) -> Self::Item { | ||
// SAFETY: the caller must uphold the contract for | ||
// `Iterator::__iterator_get_unchecked`. | ||
|
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.
Disclaimer: I'm still unfamiliar with
__iterator_get_unchecked
and the related specializations so I'm going to be asking some probably silly questions:Is this comment still relevant?
Also, why doesn't this need a
where Self: TrustedRandomAccessNoCoerce
bound?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.
It's unsafe code so it should have a
SAFETY
comment. And it's still correct since this method should only be called by specializing impls relying onTrustedRandomAccess
.TrustedRandomAccess
is implemented unconditionally for this iterator so the bound would be always true.But maybe it would still make sense for consistency or in case the trait impls are removed at some point. But I don't think it's needed for safety here.
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.
Aah, I see. I'm assuming that means that all the other
__iterator_get_unchecked
impls that leave of that bound also have it similarly implied? If that's the case I'd like to see those all have the bound added explicitly though that doesn't need to happen in this PR, but them being left off in some cases and not others seems somewhat confusing and could add to the maintenance burden.Uh oh!
There was an error while loading. Please reload this page.
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.
Actually consistency is the reason I removed the bound. Initially I had left the
Self: TrustedRandomAccess
bounds in place and relied on type errors to point out all the places that needed changing (toSelf: TrustedRandomAccessNoCoerce
). Afterwards, grepping for and going through all the remainingSelf: TrustedRandomAccess
occurrences (just to be sure) I came acoss this case (and another one, too IIRC) that didn’t produce an error message: Surprised me as well, I eventually figured out that the bound was entirely useless anyways. Looking at other iterators instd
, it seems to have be commonly done without anySelf: TrustedRandomAccess
bound before when those weren’t necessary. On all kinds of iterators, e.g. all the ones on slices (e.g. also the ones from.chunks(…)
and similar). IIRC, with the changes of this PR all the superfluous bounds are consistently gone.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.
So as far as I know, after this PR only those implementations that need the bound will have it. Don’t quote me on that, I’d need to double check if this is indeed true. This very PR demonstrated that leaving the bounds off can actually help maintainance in some cases, like it helped me being able to use error messages to find all the places that needed to change, like I described above. The redundant
Self: TrustedRandomAccess
binding didn’t generate any error messages though, so their existence made the refactor a bit harder (since I needed togrep
for things in order to fix those redundant bindings (for now, by removing them instead of changing them toSelf: TrustedRandomAccesNoCoerce
)).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.
Here’s a “grep” example. Looks like there are about 15 Iterators without the bound on the method (on the branch of this PR, so before this PR there were 13 Iterators without the bound)