Skip to content

Commit 8523788

Browse files
borsbors
committed
Auto merge of rust-lang#85874 - steffahn:fix_unsound_zip_optimization, r=yaahc
Remove unsound TrustedRandomAccess implementations Removes the implementations that depend on the user-definable trait `Copy`. Fixes rust-lang#85873 in the most straightforward way. <hr> _Edit:_ This PR now contains additional trait infrastructure to avoid performance regressions around in-place collect, see the discussion in this thread starting from the codegen test failure at rust-lang#85874 (comment). With this PR, `TrustedRandomAccess` gains additional documentation that specifically allows for and specifies the safety conditions around subtype coercions – those coercions can happen in safe Rust code with the `Zip` API’s usage of `TrustedRandomAccess`. This PR introduces a new supertrait of `TrustedRandomAccess`(currently named `TrustedRandomAccessNoCoerce`) that _doesn’t allow_ such coercions, which means it can be still be useful for optimizing cases such as in-place collect where no iterator is handed out to a user (who could do coercions) after a `get_unchecked` call; the benefit of the supertrait is that it doesn’t come with the additional safety conditions around supertraits either, so it can be implemented for more types than `TrustedRandomAccess`. The `TrustedRandomAccess` implementations for `vec::IntoIter`, `vec_deque::IntoIter`, and `array::IntoIter` are removed as they don’t conform with the newly documented safety conditions, this way unsoundness is removed. But this PR in turn (re-)adds a `TrustedRandomAccessNoCoerce` implementation for `vec::IntoIter` to avoid performance regressions from stable in a case of in-place collecting of `Vec`s [the above-mentioned codegen test failure]. Re-introducing the (currently nightly+beta-only) impls for `VecDeque`’s and `[T; N]`’s iterators is technically possible, but goes beyond the scope of this PR (i.e. it can happen in a future PR).
2 parents 4533be9 + 6d9c0a1 commit 8523788

File tree

16 files changed

+330
-135
lines changed

16 files changed

+330
-135
lines changed

‎library/alloc/src/collections/vec_deque/iter.rs

+8-7
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use core::fmt;
2-
use core::iter::{FusedIterator, TrustedLen, TrustedRandomAccess};
2+
use core::iter::{FusedIterator, TrustedLen, TrustedRandomAccess, TrustedRandomAccessNoCoerce};
33
use core::ops::Try;
44

55
use super::{count, wrap_index, RingSlices};
@@ -104,11 +104,8 @@ impl<'a, T> Iterator for Iter<'a, T> {
104104

105105
#[inline]
106106
#[doc(hidden)]
107-
unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item
108-
where
109-
Self: TrustedRandomAccess,
110-
{
111-
// Safety: The TrustedRandomAccess contract requires that callers only pass an index
107+
unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item {
108+
// Safety: The TrustedRandomAccess contract requires that callers only pass an index
112109
// that is in bounds.
113110
unsafe {
114111
let idx = wrap_index(self.tail.wrapping_add(idx), self.ring.len());
@@ -177,6 +174,10 @@ unsafe impl<T> TrustedLen for Iter<'_, T> {}
177174

178175
#[doc(hidden)]
179176
#[unstable(feature = "trusted_random_access", issue = "none")]
180-
unsafe impl<T> TrustedRandomAccess for Iter<'_, T> {
177+
unsafe impl<T> TrustedRandomAccess for Iter<'_, T> {}
178+
179+
#[doc(hidden)]
180+
#[unstable(feature = "trusted_random_access", issue = "none")]
181+
unsafe impl<T> TrustedRandomAccessNoCoerce for Iter<'_, T> {
181182
const MAY_HAVE_SIDE_EFFECT: bool = false;
182183
}

‎library/alloc/src/collections/vec_deque/iter_mut.rs

+8-7
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use core::fmt;
2-
use core::iter::{FusedIterator, TrustedLen, TrustedRandomAccess};
2+
use core::iter::{FusedIterator, TrustedLen, TrustedRandomAccess, TrustedRandomAccessNoCoerce};
33
use core::marker::PhantomData;
44

55
use super::{count, wrap_index, RingSlices};
@@ -90,11 +90,8 @@ impl<'a, T> Iterator for IterMut<'a, T> {
9090

9191
#[inline]
9292
#[doc(hidden)]
93-
unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item
94-
where
95-
Self: TrustedRandomAccess,
96-
{
97-
// Safety: The TrustedRandomAccess contract requires that callers only pass an index
93+
unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item {
94+
// Safety: The TrustedRandomAccess contract requires that callers only pass an index
9895
// that is in bounds.
9996
unsafe {
10097
let idx = wrap_index(self.tail.wrapping_add(idx), self.ring.len());
@@ -146,6 +143,10 @@ unsafe impl<T> TrustedLen for IterMut<'_, T> {}
146143

147144
#[doc(hidden)]
148145
#[unstable(feature = "trusted_random_access", issue = "none")]
149-
unsafe impl<T> TrustedRandomAccess for IterMut<'_, T> {
146+
unsafe impl<T> TrustedRandomAccess for IterMut<'_, T> {}
147+
148+
#[doc(hidden)]
149+
#[unstable(feature = "trusted_random_access", issue = "none")]
150+
unsafe impl<T> TrustedRandomAccessNoCoerce for IterMut<'_, T> {
150151
const MAY_HAVE_SIDE_EFFECT: bool = false;
151152
}

‎library/alloc/src/vec/into_iter.rs

+8-3
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,9 @@ use crate::alloc::{Allocator, Global};
22
use crate::raw_vec::RawVec;
33
use core::fmt;
44
use core::intrinsics::arith_offset;
5-
use core::iter::{FusedIterator, InPlaceIterable, SourceIter, TrustedLen, TrustedRandomAccess};
5+
use core::iter::{
6+
FusedIterator, InPlaceIterable, SourceIter, TrustedLen, TrustedRandomAccessNoCoerce,
7+
};
68
use core::marker::PhantomData;
79
use core::mem::{self};
810
use core::ptr::{self, NonNull};
@@ -166,7 +168,7 @@ impl<T, A: Allocator> Iterator for IntoIter<T, A> {
166168
#[doc(hidden)]
167169
unsafe fn __iterator_get_unchecked(&mut self, i: usize) -> Self::Item
168170
where
169-
Self: TrustedRandomAccess,
171+
Self: TrustedRandomAccessNoCoerce,
170172
{
171173
// SAFETY: the caller must guarantee that `i` is in bounds of the
172174
// `Vec<T>`, so `i` cannot overflow an `isize`, and the `self.ptr.add(i)`
@@ -219,7 +221,10 @@ unsafe impl<T, A: Allocator> TrustedLen for IntoIter<T, A> {}
219221
#[unstable(issue = "none", feature = "std_internals")]
220222
// T: Copy as approximation for !Drop since get_unchecked does not advance self.ptr
221223
// and thus we can't implement drop-handling
222-
unsafe impl<T, A: Allocator> TrustedRandomAccess for IntoIter<T, A>
224+
//
225+
// TrustedRandomAccess (without NoCoerce) must not be implemented because
226+
// subtypes/supertypes of `T` might not be `Copy`
227+
unsafe impl<T, A: Allocator> TrustedRandomAccessNoCoerce for IntoIter<T, A>
223228
where
224229
T: Copy,
225230
{

‎library/alloc/src/vec/source_iter_marker.rs

+19-2
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use core::iter::{InPlaceIterable, SourceIter, TrustedRandomAccess};
1+
use core::iter::{InPlaceIterable, SourceIter, TrustedRandomAccessNoCoerce};
22
use core::mem::{self, ManuallyDrop};
33
use core::ptr::{self};
44

@@ -71,6 +71,18 @@ where
7171
// drop any remaining values at the tail of the source
7272
// but prevent drop of the allocation itself once IntoIter goes out of scope
7373
// if the drop panics then we also leak any elements collected into dst_buf
74+
//
75+
// FIXME: Since `SpecInPlaceCollect::collect_in_place` above might use
76+
// `__iterator_get_unchecked` internally, this call might be operating on
77+
// a `vec::IntoIter` with incorrect internal state regarding which elements
78+
// have already been “consumed”. However, the `TrustedRandomIteratorNoCoerce`
79+
// implementation of `vec::IntoIter` is only present if the `Vec` elements
80+
// don’t have a destructor, so it doesn’t matter if elements are “dropped multiple times”
81+
// in this case.
82+
// This argument technically currently lacks justification from the `# Safety` docs for
83+
// `SourceIter`/`InPlaceIterable` and/or `TrustedRandomAccess`, so it might be possible that
84+
// someone could inadvertently create new library unsoundness
85+
// involving this `.forget_allocation_drop_remaining()` call.
7486
src.forget_allocation_drop_remaining();
7587

7688
let vec = unsafe { Vec::from_raw_parts(dst_buf, len, cap) };
@@ -101,6 +113,11 @@ fn write_in_place_with_drop<T>(
101113
trait SpecInPlaceCollect<T, I>: Iterator<Item = T> {
102114
/// Collects an iterator (`self`) into the destination buffer (`dst`) and returns the number of items
103115
/// collected. `end` is the last writable element of the allocation and used for bounds checks.
116+
///
117+
/// This method is specialized and one of its implementations makes use of
118+
/// `Iterator::__iterator_get_unchecked` calls with a `TrustedRandomAccessNoCoerce` bound
119+
/// on `I` which means the caller of this method must take the safety conditions
120+
/// of that trait into consideration.
104121
fn collect_in_place(&mut self, dst: *mut T, end: *const T) -> usize;
105122
}
106123

@@ -124,7 +141,7 @@ where
124141

125142
impl<T, I> SpecInPlaceCollect<T, I> for I
126143
where
127-
I: Iterator<Item = T> + TrustedRandomAccess,
144+
I: Iterator<Item = T> + TrustedRandomAccessNoCoerce,
128145
{
129146
#[inline]
130147
fn collect_in_place(&mut self, dst_buf: *mut T, end: *const T) -> usize {

‎library/core/src/iter/adapters/cloned.rs

+10-4
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
1-
use crate::iter::adapters::{zip::try_get_unchecked, TrustedRandomAccess};
1+
use crate::iter::adapters::{
2+
zip::try_get_unchecked, TrustedRandomAccess, TrustedRandomAccessNoCoerce,
3+
};
24
use crate::iter::{FusedIterator, TrustedLen};
35
use crate::ops::Try;
46

@@ -61,7 +63,7 @@ where
6163
#[doc(hidden)]
6264
unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> T
6365
where
64-
Self: TrustedRandomAccess,
66+
Self: TrustedRandomAccessNoCoerce,
6567
{
6668
// SAFETY: the caller must uphold the contract for
6769
// `Iterator::__iterator_get_unchecked`.
@@ -121,9 +123,13 @@ where
121123

122124
#[doc(hidden)]
123125
#[unstable(feature = "trusted_random_access", issue = "none")]
124-
unsafe impl<I> TrustedRandomAccess for Cloned<I>
126+
unsafe impl<I> TrustedRandomAccess for Cloned<I> where I: TrustedRandomAccess {}
127+
128+
#[doc(hidden)]
129+
#[unstable(feature = "trusted_random_access", issue = "none")]
130+
unsafe impl<I> TrustedRandomAccessNoCoerce for Cloned<I>
125131
where
126-
I: TrustedRandomAccess,
132+
I: TrustedRandomAccessNoCoerce,
127133
{
128134
const MAY_HAVE_SIDE_EFFECT: bool = true;
129135
}

‎library/core/src/iter/adapters/copied.rs

+10-4
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
1-
use crate::iter::adapters::{zip::try_get_unchecked, TrustedRandomAccess};
1+
use crate::iter::adapters::{
2+
zip::try_get_unchecked, TrustedRandomAccess, TrustedRandomAccessNoCoerce,
3+
};
24
use crate::iter::{FusedIterator, TrustedLen};
35
use crate::ops::Try;
46

@@ -77,7 +79,7 @@ where
7779
#[doc(hidden)]
7880
unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> T
7981
where
80-
Self: TrustedRandomAccess,
82+
Self: TrustedRandomAccessNoCoerce,
8183
{
8284
// SAFETY: the caller must uphold the contract for
8385
// `Iterator::__iterator_get_unchecked`.
@@ -137,9 +139,13 @@ where
137139

138140
#[doc(hidden)]
139141
#[unstable(feature = "trusted_random_access", issue = "none")]
140-
unsafe impl<I> TrustedRandomAccess for Copied<I>
142+
unsafe impl<I> TrustedRandomAccess for Copied<I> where I: TrustedRandomAccess {}
143+
144+
#[doc(hidden)]
145+
#[unstable(feature = "trusted_random_access", issue = "none")]
146+
unsafe impl<I> TrustedRandomAccessNoCoerce for Copied<I>
141147
where
142-
I: TrustedRandomAccess,
148+
I: TrustedRandomAccessNoCoerce,
143149
{
144150
const MAY_HAVE_SIDE_EFFECT: bool = I::MAY_HAVE_SIDE_EFFECT;
145151
}

‎library/core/src/iter/adapters/enumerate.rs

+10-4
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
1-
use crate::iter::adapters::{zip::try_get_unchecked, SourceIter, TrustedRandomAccess};
1+
use crate::iter::adapters::{
2+
zip::try_get_unchecked, SourceIter, TrustedRandomAccess, TrustedRandomAccessNoCoerce,
3+
};
24
use crate::iter::{FusedIterator, InPlaceIterable, TrustedLen};
35
use crate::ops::Try;
46

@@ -114,7 +116,7 @@ where
114116
#[doc(hidden)]
115117
unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> <Self as Iterator>::Item
116118
where
117-
Self: TrustedRandomAccess,
119+
Self: TrustedRandomAccessNoCoerce,
118120
{
119121
// SAFETY: the caller must uphold the contract for
120122
// `Iterator::__iterator_get_unchecked`.
@@ -207,9 +209,13 @@ where
207209

208210
#[doc(hidden)]
209211
#[unstable(feature = "trusted_random_access", issue = "none")]
210-
unsafe impl<I> TrustedRandomAccess for Enumerate<I>
212+
unsafe impl<I> TrustedRandomAccess for Enumerate<I> where I: TrustedRandomAccess {}
213+
214+
#[doc(hidden)]
215+
#[unstable(feature = "trusted_random_access", issue = "none")]
216+
unsafe impl<I> TrustedRandomAccessNoCoerce for Enumerate<I>
211217
where
212-
I: TrustedRandomAccess,
218+
I: TrustedRandomAccessNoCoerce,
213219
{
214220
const MAY_HAVE_SIDE_EFFECT: bool = I::MAY_HAVE_SIDE_EFFECT;
215221
}

‎library/core/src/iter/adapters/fuse.rs

+8-3
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ use crate::intrinsics;
22
use crate::iter::adapters::zip::try_get_unchecked;
33
use crate::iter::{
44
DoubleEndedIterator, ExactSizeIterator, FusedIterator, TrustedLen, TrustedRandomAccess,
5+
TrustedRandomAccessNoCoerce,
56
};
67
use crate::ops::Try;
78

@@ -131,7 +132,7 @@ where
131132
#[doc(hidden)]
132133
unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item
133134
where
134-
Self: TrustedRandomAccess,
135+
Self: TrustedRandomAccessNoCoerce,
135136
{
136137
match self.iter {
137138
// SAFETY: the caller must uphold the contract for
@@ -221,9 +222,13 @@ unsafe impl<I> TrustedLen for Fuse<I> where I: TrustedLen {}
221222
//
222223
// This is safe to implement as `Fuse` is just forwarding these to the wrapped iterator `I`, which
223224
// preserves these properties.
224-
unsafe impl<I> TrustedRandomAccess for Fuse<I>
225+
unsafe impl<I> TrustedRandomAccess for Fuse<I> where I: TrustedRandomAccess {}
226+
227+
#[doc(hidden)]
228+
#[unstable(feature = "trusted_random_access", issue = "none")]
229+
unsafe impl<I> TrustedRandomAccessNoCoerce for Fuse<I>
225230
where
226-
I: TrustedRandomAccess,
231+
I: TrustedRandomAccessNoCoerce,
227232
{
228233
const MAY_HAVE_SIDE_EFFECT: bool = I::MAY_HAVE_SIDE_EFFECT;
229234
}

‎library/core/src/iter/adapters/map.rs

+10-4
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
use crate::fmt;
2-
use crate::iter::adapters::{zip::try_get_unchecked, SourceIter, TrustedRandomAccess};
2+
use crate::iter::adapters::{
3+
zip::try_get_unchecked, SourceIter, TrustedRandomAccess, TrustedRandomAccessNoCoerce,
4+
};
35
use crate::iter::{FusedIterator, InPlaceIterable, TrustedLen};
46
use crate::ops::Try;
57

@@ -125,7 +127,7 @@ where
125127
#[doc(hidden)]
126128
unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> B
127129
where
128-
Self: TrustedRandomAccess,
130+
Self: TrustedRandomAccessNoCoerce,
129131
{
130132
// SAFETY: the caller must uphold the contract for
131133
// `Iterator::__iterator_get_unchecked`.
@@ -187,9 +189,13 @@ where
187189

188190
#[doc(hidden)]
189191
#[unstable(feature = "trusted_random_access", issue = "none")]
190-
unsafe impl<I, F> TrustedRandomAccess for Map<I, F>
192+
unsafe impl<I, F> TrustedRandomAccess for Map<I, F> where I: TrustedRandomAccess {}
193+
194+
#[doc(hidden)]
195+
#[unstable(feature = "trusted_random_access", issue = "none")]
196+
unsafe impl<I, F> TrustedRandomAccessNoCoerce for Map<I, F>
191197
where
192-
I: TrustedRandomAccess,
198+
I: TrustedRandomAccessNoCoerce,
193199
{
194200
const MAY_HAVE_SIDE_EFFECT: bool = true;
195201
}

‎library/core/src/iter/adapters/mod.rs

+3
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,9 @@ pub use self::map_while::MapWhile;
5151
#[unstable(feature = "trusted_random_access", issue = "none")]
5252
pub use self::zip::TrustedRandomAccess;
5353

54+
#[unstable(feature = "trusted_random_access", issue = "none")]
55+
pub use self::zip::TrustedRandomAccessNoCoerce;
56+
5457
#[unstable(feature = "iter_zip", issue = "83574")]
5558
pub use self::zip::zip;
5659

0 commit comments

Comments
 (0)