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

Jump to bottom

[crater experiment] do the opposite of what zip side-effect documentation currently guarantees #91031

Closed
wants to merge 1 commit into from
Closed

[crater experiment] do the opposite of what zip side-effect documentation currently guarantees #91031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion library/alloc/src/collections/linked_list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1925,7 +1925,8 @@ impl<T: Clone> Clone for LinkedList<T> {
if self.len() > other.len() {
self.split_off(other.len());
}
for (elem, elem_other) in self.iter_mut().zip(&mut iter_other) {
let len = self.len();
for (elem, elem_other) in self.iter_mut().zip((&mut iter_other).take(len)) {
elem.clone_from(elem_other);
}
if !iter_other.is_empty() {
Expand Down
225 changes: 19 additions & 206 deletions library/core/src/iter/adapters/zip.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,6 @@ use crate::iter::{InPlaceIterable, SourceIter, TrustedLen};
pub struct Zip<A, B> {
a: A,
b: B,
// index, len and a_len are only used by the specialized version of zip
index: usize,
len: usize,
a_len: usize,
}
impl<A: Iterator, B: Iterator> Zip<A, B> {
pub(in crate::iter) fn new(a: A, b: B) -> Zip<A, B> {
Expand Down Expand Up @@ -88,14 +84,21 @@ where
}

#[inline]
#[doc(hidden)]
unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item
where
Self: TrustedRandomAccessNoCoerce,
{
// SAFETY: `ZipImpl::__iterator_get_unchecked` has same safety
// requirements as `Iterator::__iterator_get_unchecked`.
unsafe { ZipImpl::get_unchecked(self, idx) }
fn advance_by(&mut self, n: usize) -> Result<(), usize> {
let ((lower_a, _), (lower_b, _)) = (self.a.size_hint(), self.b.size_hint());
let lower = n.min(lower_a).min(lower_b);
let batched = match (self.b.advance_by(lower), self.a.advance_by(lower)) {
(Ok(()), Ok(())) => lower,
_ => panic!("size_hint contract violation"),
};

for i in batched..n {
if let (_, None) | (None, _) = (self.b.next(), self.a.next()) {
return Err(i);
}
}

Ok(())
}
}

Expand Down Expand Up @@ -123,30 +126,20 @@ trait ZipImpl<A, B> {
where
A: DoubleEndedIterator + ExactSizeIterator,
B: DoubleEndedIterator + ExactSizeIterator;
// 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 + 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
}
Zip { a, b }
}

#[inline]
default fn next(&mut self) -> Option<(A::Item, B::Item)> {
let x = self.a.next()?;
let y = self.b.next()?;
let x = self.a.next()?;
Some((x, y))
}

Expand Down Expand Up @@ -179,8 +172,8 @@ macro_rules! zip_impl_general_defaults {
}
}
}
match (self.a.next_back(), self.b.next_back()) {
(Some(x), Some(y)) => Some((x, y)),
match (self.b.next_back(), self.a.next_back()) {
(Some(y), Some(x)) => Some((x, y)),
(None, None) => None,
_ => unreachable!(),
}
Expand Down Expand Up @@ -215,157 +208,6 @@ where

(lower, upper)
}

default unsafe fn get_unchecked(&mut self, _idx: usize) -> <Self as Iterator>::Item
where
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
A: TrustedRandomAccess + Iterator,
B: TrustedRandomAccess + Iterator,
{
fn new(a: A, b: B) -> Self {
let a_len = a.size();
let len = cmp::min(a_len, b.size());
Zip { a, b, index: 0, len, a_len }
}

#[inline]
fn next(&mut self) -> Option<(A::Item, B::Item)> {
if self.index < self.len {
let i = self.index;
// since get_unchecked executes code which can panic we increment the counters beforehand
// so that the same index won't be accessed twice, as required by TrustedRandomAccess
self.index += 1;
// SAFETY: `i` is smaller than `self.len`, thus smaller than `self.a.len()` and `self.b.len()`
unsafe {
Some((self.a.__iterator_get_unchecked(i), self.b.__iterator_get_unchecked(i)))
}
} else if A::MAY_HAVE_SIDE_EFFECT && self.index < self.a_len {
let i = self.index;
// as above, increment before executing code that may panic
self.index += 1;
self.len += 1;
// match the base implementation's potential side effects
// SAFETY: we just checked that `i` < `self.a.len()`
unsafe {
self.a.__iterator_get_unchecked(i);
}
None
} else {
None
}
}

#[inline]
fn size_hint(&self) -> (usize, Option<usize>) {
let len = self.len - self.index;
(len, Some(len))
}

#[inline]
fn nth(&mut self, n: usize) -> Option<Self::Item> {
let delta = cmp::min(n, self.len - self.index);
let end = self.index + delta;
while self.index < end {
let i = self.index;
// since get_unchecked executes code which can panic we increment the counters beforehand
// so that the same index won't be accessed twice, as required by TrustedRandomAccess
self.index += 1;
if A::MAY_HAVE_SIDE_EFFECT {
// SAFETY: the usage of `cmp::min` to calculate `delta`
// ensures that `end` is smaller than or equal to `self.len`,
// so `i` is also smaller than `self.len`.
unsafe {
self.a.__iterator_get_unchecked(i);
}
}
if B::MAY_HAVE_SIDE_EFFECT {
// SAFETY: same as above.
unsafe {
self.b.__iterator_get_unchecked(i);
}
}
}

self.super_nth(n - delta)
}

#[inline]
fn next_back(&mut self) -> Option<(A::Item, B::Item)>
where
A: DoubleEndedIterator + ExactSizeIterator,
B: DoubleEndedIterator + ExactSizeIterator,
{
if A::MAY_HAVE_SIDE_EFFECT || B::MAY_HAVE_SIDE_EFFECT {
let sz_a = self.a.size();
let sz_b = self.b.size();
// Adjust a, b to equal length, make sure that only the first call
// of `next_back` does this, otherwise we will break the restriction
// on calls to `self.next_back()` after calling `get_unchecked()`.
if sz_a != sz_b {
let sz_a = self.a.size();
if A::MAY_HAVE_SIDE_EFFECT && sz_a > self.len {
for _ in 0..sz_a - self.len {
// since next_back() may panic we increment the counters beforehand
// to keep Zip's state in sync with the underlying iterator source
self.a_len -= 1;
self.a.next_back();
}
debug_assert_eq!(self.a_len, self.len);
}
let sz_b = self.b.size();
if B::MAY_HAVE_SIDE_EFFECT && sz_b > self.len {
for _ in 0..sz_b - self.len {
self.b.next_back();
}
}
}
}
if self.index < self.len {
// since get_unchecked executes code which can panic we increment the counters beforehand
// so that the same index won't be accessed twice, as required by TrustedRandomAccess
self.len -= 1;
self.a_len -= 1;
let i = self.len;
// SAFETY: `i` is smaller than the previous value of `self.len`,
// which is also smaller than or equal to `self.a.len()` and `self.b.len()`
unsafe {
Some((self.a.__iterator_get_unchecked(i), self.b.__iterator_get_unchecked(i)))
}
} else {
None
}
}
}

#[stable(feature = "rust1", since = "1.0.0")]
Expand All @@ -376,25 +218,6 @@ where
{
}

#[doc(hidden)]
#[unstable(feature = "trusted_random_access", issue = "none")]
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;
}

#[stable(feature = "fused", since = "1.26.0")]
impl<A, B> FusedIterator for Zip<A, B>
where
Expand Down Expand Up @@ -448,16 +271,6 @@ impl<A: Debug, B: Debug> 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.
f.debug_struct("Zip").finish()
}
}

/// An iterator whose items are random-accessible efficiently
///
/// # Safety
Expand Down
2 changes: 1 addition & 1 deletion library/core/tests/iter/adapters/cloned.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ fn test_cloned_side_effects() {
.zip(&[1]);
for _ in iter {}
}
assert_eq!(count, 2);
assert_eq!(count, 1);
}

#[test]
Expand Down
23 changes: 11 additions & 12 deletions library/core/tests/iter/adapters/zip.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ fn test_zip_next_back_side_effects_exhausted() {
iter.next();
iter.next();
assert_eq!(iter.next_back(), None);
assert_eq!(a, vec![1, 2, 3, 4, 6, 5]);
assert_eq!(a, vec![1, 2, 3, 6, 5, 4]);
assert_eq!(b, vec![200, 300, 400]);
}

Expand All @@ -119,7 +119,7 @@ fn test_zip_cloned_sideffectful() {

for _ in xs.iter().cloned().zip(ys.iter().cloned()) {}

assert_eq!(&xs, &[1, 1, 1, 0][..]);
assert_eq!(&xs, &[1, 1, 0, 0][..]);
assert_eq!(&ys, &[1, 1][..]);

let xs = [CountClone::new(), CountClone::new()];
Expand All @@ -128,7 +128,7 @@ fn test_zip_cloned_sideffectful() {
for _ in xs.iter().cloned().zip(ys.iter().cloned()) {}

assert_eq!(&xs, &[1, 1][..]);
assert_eq!(&ys, &[1, 1, 0, 0][..]);
assert_eq!(&ys, &[1, 1, 1, 0][..]);
}

#[test]
Expand All @@ -138,7 +138,7 @@ fn test_zip_map_sideffectful() {

for _ in xs.iter_mut().map(|x| *x += 1).zip(ys.iter_mut().map(|y| *y += 1)) {}

assert_eq!(&xs, &[1, 1, 1, 1, 1, 0]);
assert_eq!(&xs, &[1, 1, 1, 1, 0, 0]);
assert_eq!(&ys, &[1, 1, 1, 1]);

let mut xs = [0; 4];
Expand All @@ -147,7 +147,7 @@ fn test_zip_map_sideffectful() {
for _ in xs.iter_mut().map(|x| *x += 1).zip(ys.iter_mut().map(|y| *y += 1)) {}

assert_eq!(&xs, &[1, 1, 1, 1]);
assert_eq!(&ys, &[1, 1, 1, 1, 0, 0]);
assert_eq!(&ys, &[1, 1, 1, 1, 1, 0]);
}

#[test]
Expand Down Expand Up @@ -176,15 +176,15 @@ fn test_zip_map_rev_sideffectful() {

#[test]
fn test_zip_nested_sideffectful() {
let mut xs = [0; 6];
let ys = [0; 4];
let xs = [0; 4];
let mut ys = [0; 6];

{
// test that it has the side effect nested inside enumerate
let it = xs.iter_mut().map(|x| *x = 1).enumerate().zip(&ys);
let it = xs.iter().enumerate().zip(ys.iter_mut().map(|x| *x = 1));
it.count();
}
assert_eq!(&xs, &[1, 1, 1, 1, 1, 0]);
assert_eq!(&ys, &[1, 1, 1, 1, 1, 0]);
}

#[test]
Expand All @@ -208,7 +208,7 @@ fn test_zip_nth_back_side_effects_exhausted() {
iter.next();
iter.next();
assert_eq!(iter.nth_back(0), None);
assert_eq!(a, vec![1, 2, 3, 4, 6, 5]);
assert_eq!(a, vec![1, 2, 3, 6, 5, 4]);
assert_eq!(b, vec![200, 300, 400]);
}

Expand All @@ -227,8 +227,7 @@ fn test_zip_trusted_random_access_composition() {
assert_eq!(z1.next().unwrap(), (0, 0));

let mut z2 = z1.zip(c);
fn assert_trusted_random_access<T: TrustedRandomAccess>(_a: &T) {}
assert_trusted_random_access(&z2);

assert_eq!(z2.next().unwrap(), ((1, 1), 1));
}

Expand Down