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).

Author: bors

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

@@ -1,5 +1,5 @@
 use core::fmt;
-use core::iter::{FusedIterator, TrustedLen, TrustedRandomAccess};
+use core::iter::{FusedIterator, TrustedLen, TrustedRandomAccess, TrustedRandomAccessNoCoerce};
 use core::ops::Try;
 
 use super::{count, wrap_index, RingSlices};
@@ -104,11 +104,8 @@ impl<'a, T> Iterator for Iter<'a, T> {
 
     #[inline]
     #[doc(hidden)]
-    unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item
-    where
-        Self: TrustedRandomAccess,
-    {
-        // Safety: The TrustedRandomAccess contract requires that callers only  pass an index
+    unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item {
+        // Safety: The TrustedRandomAccess contract requires that callers only pass an index
         // that is in bounds.
         unsafe {
             let idx = wrap_index(self.tail.wrapping_add(idx), self.ring.len());
@@ -177,6 +174,10 @@ unsafe impl<T> TrustedLen for Iter<'_, T> {}
 
 #[doc(hidden)]
 #[unstable(feature = "trusted_random_access", issue = "none")]
-unsafe impl<T> TrustedRandomAccess for Iter<'_, T> {
+unsafe impl<T> TrustedRandomAccess for Iter<'_, T> {}
+
+#[doc(hidden)]
+#[unstable(feature = "trusted_random_access", issue = "none")]
+unsafe impl<T> TrustedRandomAccessNoCoerce for Iter<'_, T> {
     const MAY_HAVE_SIDE_EFFECT: bool = false;
 }

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

@@ -1,5 +1,5 @@
 use core::fmt;
-use core::iter::{FusedIterator, TrustedLen, TrustedRandomAccess};
+use core::iter::{FusedIterator, TrustedLen, TrustedRandomAccess, TrustedRandomAccessNoCoerce};
 use core::marker::PhantomData;
 
 use super::{count, wrap_index, RingSlices};
@@ -90,11 +90,8 @@ impl<'a, T> Iterator for IterMut<'a, T> {
 
     #[inline]
     #[doc(hidden)]
-    unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item
-    where
-        Self: TrustedRandomAccess,
-    {
-        // Safety: The TrustedRandomAccess contract requires that callers only  pass an index
+    unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item {
+        // Safety: The TrustedRandomAccess contract requires that callers only pass an index
         // that is in bounds.
         unsafe {
             let idx = wrap_index(self.tail.wrapping_add(idx), self.ring.len());
@@ -146,6 +143,10 @@ unsafe impl<T> TrustedLen for IterMut<'_, T> {}
 
 #[doc(hidden)]
 #[unstable(feature = "trusted_random_access", issue = "none")]
-unsafe impl<T> TrustedRandomAccess for IterMut<'_, T> {
+unsafe impl<T> TrustedRandomAccess for IterMut<'_, T> {}
+
+#[doc(hidden)]
+#[unstable(feature = "trusted_random_access", issue = "none")]
+unsafe impl<T> TrustedRandomAccessNoCoerce for IterMut<'_, T> {
     const MAY_HAVE_SIDE_EFFECT: bool = false;
 }

library/alloc/src/vec/into_iter.rs

@@ -2,7 +2,9 @@ use crate::alloc::{Allocator, Global};
 use crate::raw_vec::RawVec;
 use core::fmt;
 use core::intrinsics::arith_offset;
-use core::iter::{FusedIterator, InPlaceIterable, SourceIter, TrustedLen, TrustedRandomAccess};
+use core::iter::{
+    FusedIterator, InPlaceIterable, SourceIter, TrustedLen, TrustedRandomAccessNoCoerce,
+};
 use core::marker::PhantomData;
 use core::mem::{self};
 use core::ptr::{self, NonNull};
@@ -166,7 +168,7 @@ impl<T, A: Allocator> Iterator for IntoIter<T, A> {
     #[doc(hidden)]
     unsafe fn __iterator_get_unchecked(&mut self, i: usize) -> Self::Item
     where
-        Self: TrustedRandomAccess,
+        Self: TrustedRandomAccessNoCoerce,
     {
         // SAFETY: the caller must guarantee that `i` is in bounds of the
         // `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> {}
 #[unstable(issue = "none", feature = "std_internals")]
 // T: Copy as approximation for !Drop since get_unchecked does not advance self.ptr
 // and thus we can't implement drop-handling
-unsafe impl<T, A: Allocator> TrustedRandomAccess for IntoIter<T, A>
+//
+// TrustedRandomAccess (without NoCoerce) must not be implemented because
+// subtypes/supertypes of `T` might not be `Copy`
+unsafe impl<T, A: Allocator> TrustedRandomAccessNoCoerce for IntoIter<T, A>
 where
     T: Copy,
 {

library/alloc/src/vec/source_iter_marker.rs

@@ -1,4 +1,4 @@
-use core::iter::{InPlaceIterable, SourceIter, TrustedRandomAccess};
+use core::iter::{InPlaceIterable, SourceIter, TrustedRandomAccessNoCoerce};
 use core::mem::{self, ManuallyDrop};
 use core::ptr::{self};
 
@@ -71,6 +71,18 @@ where
         // drop any remaining values at the tail of the source
         // but prevent drop of the allocation itself once IntoIter goes out of scope
         // if the drop panics then we also leak any elements collected into dst_buf
+        //
+        // FIXME: Since `SpecInPlaceCollect::collect_in_place` above might use
+        // `__iterator_get_unchecked` internally, this call might be operating on
+        // a `vec::IntoIter` with incorrect internal state regarding which elements
+        // have already been “consumed”. However, the `TrustedRandomIteratorNoCoerce`
+        // implementation of `vec::IntoIter` is only present if the `Vec` elements
+        // don’t have a destructor, so it doesn’t matter if elements are “dropped multiple times”
+        // in this case.
+        // This argument technically currently lacks justification from the `# Safety` docs for
+        // `SourceIter`/`InPlaceIterable` and/or `TrustedRandomAccess`, so it might be possible that
+        // someone could inadvertently create new library unsoundness
+        // involving this `.forget_allocation_drop_remaining()` call.
         src.forget_allocation_drop_remaining();
 
         let vec = unsafe { Vec::from_raw_parts(dst_buf, len, cap) };
@@ -101,6 +113,11 @@ fn write_in_place_with_drop<T>(
 trait SpecInPlaceCollect<T, I>: Iterator<Item = T> {
     /// Collects an iterator (`self`) into the destination buffer (`dst`) and returns the number of items
     /// collected. `end` is the last writable element of the allocation and used for bounds checks.
+    ///
+    /// This method is specialized and one of its implementations makes use of
+    /// `Iterator::__iterator_get_unchecked` calls with a `TrustedRandomAccessNoCoerce` bound
+    /// on `I` which means the caller of this method must take the safety conditions
+    /// of that trait into consideration.
     fn collect_in_place(&mut self, dst: *mut T, end: *const T) -> usize;
 }
 
@@ -124,7 +141,7 @@ where
 
 impl<T, I> SpecInPlaceCollect<T, I> for I
 where
-    I: Iterator<Item = T> + TrustedRandomAccess,
+    I: Iterator<Item = T> + TrustedRandomAccessNoCoerce,
 {
     #[inline]
     fn collect_in_place(&mut self, dst_buf: *mut T, end: *const T) -> usize {

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

@@ -1,4 +1,6 @@
-use crate::iter::adapters::{zip::try_get_unchecked, TrustedRandomAccess};
+use crate::iter::adapters::{
+    zip::try_get_unchecked, TrustedRandomAccess, TrustedRandomAccessNoCoerce,
+};
 use crate::iter::{FusedIterator, TrustedLen};
 use crate::ops::Try;
 
@@ -61,7 +63,7 @@ where
     #[doc(hidden)]
     unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> T
     where
-        Self: TrustedRandomAccess,
+        Self: TrustedRandomAccessNoCoerce,
     {
         // SAFETY: the caller must uphold the contract for
         // `Iterator::__iterator_get_unchecked`.
@@ -121,9 +123,13 @@ where
 
 #[doc(hidden)]
 #[unstable(feature = "trusted_random_access", issue = "none")]
-unsafe impl<I> TrustedRandomAccess for Cloned<I>
+unsafe impl<I> TrustedRandomAccess for Cloned<I> where I: TrustedRandomAccess {}
+
+#[doc(hidden)]
+#[unstable(feature = "trusted_random_access", issue = "none")]
+unsafe impl<I> TrustedRandomAccessNoCoerce for Cloned<I>
 where
-    I: TrustedRandomAccess,
+    I: TrustedRandomAccessNoCoerce,
 {
     const MAY_HAVE_SIDE_EFFECT: bool = true;
 }

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

@@ -1,4 +1,6 @@
-use crate::iter::adapters::{zip::try_get_unchecked, TrustedRandomAccess};
+use crate::iter::adapters::{
+    zip::try_get_unchecked, TrustedRandomAccess, TrustedRandomAccessNoCoerce,
+};
 use crate::iter::{FusedIterator, TrustedLen};
 use crate::ops::Try;
 
@@ -77,7 +79,7 @@ where
     #[doc(hidden)]
     unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> T
     where
-        Self: TrustedRandomAccess,
+        Self: TrustedRandomAccessNoCoerce,
     {
         // SAFETY: the caller must uphold the contract for
         // `Iterator::__iterator_get_unchecked`.
@@ -137,9 +139,13 @@ where
 
 #[doc(hidden)]
 #[unstable(feature = "trusted_random_access", issue = "none")]
-unsafe impl<I> TrustedRandomAccess for Copied<I>
+unsafe impl<I> TrustedRandomAccess for Copied<I> where I: TrustedRandomAccess {}
+
+#[doc(hidden)]
+#[unstable(feature = "trusted_random_access", issue = "none")]
+unsafe impl<I> TrustedRandomAccessNoCoerce for Copied<I>
 where
-    I: TrustedRandomAccess,
+    I: TrustedRandomAccessNoCoerce,
 {
     const MAY_HAVE_SIDE_EFFECT: bool = I::MAY_HAVE_SIDE_EFFECT;
 }

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

@@ -1,4 +1,6 @@
-use crate::iter::adapters::{zip::try_get_unchecked, SourceIter, TrustedRandomAccess};
+use crate::iter::adapters::{
+    zip::try_get_unchecked, SourceIter, TrustedRandomAccess, TrustedRandomAccessNoCoerce,
+};
 use crate::iter::{FusedIterator, InPlaceIterable, TrustedLen};
 use crate::ops::Try;
 
@@ -114,7 +116,7 @@ where
     #[doc(hidden)]
     unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> <Self as Iterator>::Item
     where
-        Self: TrustedRandomAccess,
+        Self: TrustedRandomAccessNoCoerce,
     {
         // SAFETY: the caller must uphold the contract for
         // `Iterator::__iterator_get_unchecked`.
@@ -207,9 +209,13 @@ where
 
 #[doc(hidden)]
 #[unstable(feature = "trusted_random_access", issue = "none")]
-unsafe impl<I> TrustedRandomAccess for Enumerate<I>
+unsafe impl<I> TrustedRandomAccess for Enumerate<I> where I: TrustedRandomAccess {}
+
+#[doc(hidden)]
+#[unstable(feature = "trusted_random_access", issue = "none")]
+unsafe impl<I> TrustedRandomAccessNoCoerce for Enumerate<I>
 where
-    I: TrustedRandomAccess,
+    I: TrustedRandomAccessNoCoerce,
 {
     const MAY_HAVE_SIDE_EFFECT: bool = I::MAY_HAVE_SIDE_EFFECT;
 }

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

@@ -2,6 +2,7 @@ use crate::intrinsics;
 use crate::iter::adapters::zip::try_get_unchecked;
 use crate::iter::{
     DoubleEndedIterator, ExactSizeIterator, FusedIterator, TrustedLen, TrustedRandomAccess,
+    TrustedRandomAccessNoCoerce,
 };
 use crate::ops::Try;
 
@@ -131,7 +132,7 @@ where
     #[doc(hidden)]
     unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item
     where
-        Self: TrustedRandomAccess,
+        Self: TrustedRandomAccessNoCoerce,
     {
         match self.iter {
             // SAFETY: the caller must uphold the contract for
@@ -221,9 +222,13 @@ unsafe impl<I> TrustedLen for Fuse<I> where I: TrustedLen {}
 //
 // This is safe to implement as `Fuse` is just forwarding these to the wrapped iterator `I`, which
 // preserves these properties.
-unsafe impl<I> TrustedRandomAccess for Fuse<I>
+unsafe impl<I> TrustedRandomAccess for Fuse<I> where I: TrustedRandomAccess {}
+
+#[doc(hidden)]
+#[unstable(feature = "trusted_random_access", issue = "none")]
+unsafe impl<I> TrustedRandomAccessNoCoerce for Fuse<I>
 where
-    I: TrustedRandomAccess,
+    I: TrustedRandomAccessNoCoerce,
 {
     const MAY_HAVE_SIDE_EFFECT: bool = I::MAY_HAVE_SIDE_EFFECT;
 }

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

@@ -1,5 +1,7 @@
 use crate::fmt;
-use crate::iter::adapters::{zip::try_get_unchecked, SourceIter, TrustedRandomAccess};
+use crate::iter::adapters::{
+    zip::try_get_unchecked, SourceIter, TrustedRandomAccess, TrustedRandomAccessNoCoerce,
+};
 use crate::iter::{FusedIterator, InPlaceIterable, TrustedLen};
 use crate::ops::Try;
 
@@ -125,7 +127,7 @@ where
     #[doc(hidden)]
     unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> B
     where
-        Self: TrustedRandomAccess,
+        Self: TrustedRandomAccessNoCoerce,
     {
         // SAFETY: the caller must uphold the contract for
         // `Iterator::__iterator_get_unchecked`.
@@ -187,9 +189,13 @@ where
 
 #[doc(hidden)]
 #[unstable(feature = "trusted_random_access", issue = "none")]
-unsafe impl<I, F> TrustedRandomAccess for Map<I, F>
+unsafe impl<I, F> TrustedRandomAccess for Map<I, F> where I: TrustedRandomAccess {}
+
+#[doc(hidden)]
+#[unstable(feature = "trusted_random_access", issue = "none")]
+unsafe impl<I, F> TrustedRandomAccessNoCoerce for Map<I, F>
 where
-    I: TrustedRandomAccess,
+    I: TrustedRandomAccessNoCoerce,
 {
     const MAY_HAVE_SIDE_EFFECT: bool = true;
 }

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

@@ -51,6 +51,9 @@ pub use self::map_while::MapWhile;
 #[unstable(feature = "trusted_random_access", issue = "none")]
 pub use self::zip::TrustedRandomAccess;
 
+#[unstable(feature = "trusted_random_access", issue = "none")]
+pub use self::zip::TrustedRandomAccessNoCoerce;
+
 #[unstable(feature = "iter_zip", issue = "83574")]
 pub use self::zip::zip;