From bed379aa85a8dbf1befd3f527403786cadcbaf52 Mon Sep 17 00:00:00 2001 From: The8472 Date: Fri, 2 Apr 2021 17:57:09 +0200 Subject: [PATCH] break as many Zip side-effects as we can --- library/core/src/iter/adapters/zip.rs | 47 ----- library/core/src/iter/traits/double_ended.rs | 77 +++++++- library/core/src/iter/traits/iterator.rs | 78 +++++++- library/core/tests/iter/adapters/cloned.rs | 17 -- library/core/tests/iter/adapters/mod.rs | 26 --- library/core/tests/iter/adapters/zip.rs | 195 ------------------- src/test/ui/issues/issue-3044.stderr | 2 +- 7 files changed, 143 insertions(+), 299 deletions(-) diff --git a/library/core/src/iter/adapters/zip.rs b/library/core/src/iter/adapters/zip.rs index 2f8f504d8fcaa..6162549336eb4 100644 --- a/library/core/src/iter/adapters/zip.rs +++ b/library/core/src/iter/adapters/zip.rs @@ -231,16 +231,6 @@ where 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; - 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 } @@ -257,22 +247,7 @@ where let delta = cmp::min(n, self.len - self.index); let end = self.index + delta; while self.index < end { - let i = self.index; 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) @@ -284,28 +259,6 @@ 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 { - self.a.next_back(); - } - 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 { self.len -= 1; self.a_len -= 1; diff --git a/library/core/src/iter/traits/double_ended.rs b/library/core/src/iter/traits/double_ended.rs index 6f8cb6b5a65b6..0888f202175e3 100644 --- a/library/core/src/iter/traits/double_ended.rs +++ b/library/core/src/iter/traits/double_ended.rs @@ -1,3 +1,4 @@ +use super::super::{TrustedLen, TrustedRandomAccess}; use crate::ops::{ControlFlow, Try}; /// An iterator able to yield elements from both ends. @@ -278,16 +279,12 @@ pub trait DoubleEndedIterator: Iterator { /// ``` #[inline] #[stable(feature = "iter_rfold", since = "1.27.0")] - fn rfold(mut self, init: B, mut f: F) -> B + fn rfold(self, init: B, f: F) -> B where Self: Sized, F: FnMut(B, Self::Item) -> B, { - let mut accum = init; - while let Some(x) = self.next_back() { - accum = f(accum, x); - } - accum + self.rfold_spec(init, f) } /// Searches for an element of an iterator from the back that satisfies a predicate. @@ -361,3 +358,71 @@ impl<'a, I: DoubleEndedIterator + ?Sized> DoubleEndedIterator for &'a mut I { (**self).nth_back(n) } } + +trait SpecIteratorDefaultImpls: DoubleEndedIterator { + fn rfold_spec(self, init: B, f: F) -> B + where + Self: Sized, + F: FnMut(B, Self::Item) -> B; +} + +impl SpecIteratorDefaultImpls for T +where + T: DoubleEndedIterator, +{ + #[inline] + default fn rfold_spec(mut self, init: B, mut f: F) -> B + where + Self: Sized, + F: FnMut(B, Self::Item) -> B, + { + let mut accum = init; + while let Some(x) = self.next_back() { + accum = f(accum, x); + } + accum + } +} + +impl SpecIteratorDefaultImpls for T +where + T: DoubleEndedIterator + Sized + TrustedLen, +{ + #[inline] + default fn rfold_spec(mut self, init: B, mut f: F) -> B + where + Self: Sized, + F: FnMut(B, Self::Item) -> B, + { + let mut accum = init; + while self.size_hint().1.is_none() { + accum = f(accum, self.next_back().unwrap()) + } + for _ in 0..self.size_hint().1.unwrap() { + accum = f(accum, self.next_back().unwrap()); + } + accum + } +} + +impl SpecIteratorDefaultImpls for T +where + T: DoubleEndedIterator + Sized + TrustedLen + TrustedRandomAccess, +{ + #[inline] + fn rfold_spec(mut self, init: B, mut f: F) -> B + where + Self: Sized, + F: FnMut(B, Self::Item) -> B, + { + let mut accum = init; + // SAFETY: every element is only read once, as required by the + // TrustedRandomAccess contract + unsafe { + for i in (0..self.size()).rev() { + accum = f(accum, self.__iterator_get_unchecked(i)) + } + } + accum + } +} diff --git a/library/core/src/iter/traits/iterator.rs b/library/core/src/iter/traits/iterator.rs index d5d0c287992c5..bcad58f3ab0f1 100644 --- a/library/core/src/iter/traits/iterator.rs +++ b/library/core/src/iter/traits/iterator.rs @@ -5,13 +5,13 @@ use crate::cmp::{self, Ordering}; use crate::ops::{ControlFlow, Try}; -use super::super::TrustedRandomAccess; use super::super::{Chain, Cloned, Copied, Cycle, Enumerate, Filter, FilterMap, Fuse}; use super::super::{FlatMap, Flatten}; use super::super::{FromIterator, Intersperse, IntersperseWith, Product, Sum, Zip}; use super::super::{ Inspect, Map, MapWhile, Peekable, Rev, Scan, Skip, SkipWhile, StepBy, Take, TakeWhile, }; +use super::super::{TrustedLen, TrustedRandomAccess}; fn _assert_is_object_safe(_: &dyn Iterator) {} @@ -2125,16 +2125,12 @@ pub trait Iterator { #[doc(alias = "inject")] #[inline] #[stable(feature = "rust1", since = "1.0.0")] - fn fold(mut self, init: B, mut f: F) -> B + fn fold(self, init: B, f: F) -> B where Self: Sized, F: FnMut(B, Self::Item) -> B, { - let mut accum = init; - while let Some(x) = self.next() { - accum = f(accum, x); - } - accum + self.fold_spec(init, f) } /// Reduces the elements to a single one, by repeatedly applying a reducing @@ -3420,3 +3416,71 @@ impl Iterator for &mut I { (**self).nth(n) } } + +trait SpecIteratorDefaultImpls: Iterator { + fn fold_spec(self, init: B, f: F) -> B + where + Self: Sized, + F: FnMut(B, Self::Item) -> B; +} + +impl SpecIteratorDefaultImpls for T +where + T: Iterator, +{ + #[inline] + default fn fold_spec(mut self, init: B, mut f: F) -> B + where + Self: Sized, + F: FnMut(B, Self::Item) -> B, + { + let mut accum = init; + while let Some(x) = self.next() { + accum = f(accum, x); + } + accum + } +} + +impl SpecIteratorDefaultImpls for T +where + T: Iterator + Sized + TrustedLen, +{ + #[inline] + default fn fold_spec(mut self, init: B, mut f: F) -> B + where + Self: Sized, + F: FnMut(B, Self::Item) -> B, + { + let mut accum = init; + while self.size_hint().1.is_none() { + accum = f(accum, self.next().unwrap()); + } + for _ in 0..self.size_hint().1.unwrap() { + accum = f(accum, self.next().unwrap()) + } + accum + } +} + +impl SpecIteratorDefaultImpls for T +where + T: Iterator + Sized + TrustedLen + TrustedRandomAccess, +{ + #[inline] + fn fold_spec(mut self, init: B, mut f: F) -> B + where + Self: Sized, + F: FnMut(B, Self::Item) -> B, + { + let mut accum = init; + // SAFETY: every element is only read once, as required by the + // TrustedRandomAccess contract + unsafe { + for i in 0..self.size() { + accum = f(accum, self.__iterator_get_unchecked(i)) + } + } + accum + } +} diff --git a/library/core/tests/iter/adapters/cloned.rs b/library/core/tests/iter/adapters/cloned.rs index 78babb7feab18..f3f0cd9cb4bd8 100644 --- a/library/core/tests/iter/adapters/cloned.rs +++ b/library/core/tests/iter/adapters/cloned.rs @@ -17,23 +17,6 @@ fn test_cloned() { assert_eq!(it.next_back(), None); } -#[test] -fn test_cloned_side_effects() { - let mut count = 0; - { - let iter = [1, 2, 3] - .iter() - .map(|x| { - count += 1; - x - }) - .cloned() - .zip(&[1]); - for _ in iter {} - } - assert_eq!(count, 2); -} - #[test] fn test_cloned_try_folds() { let a = [1, 2, 3, 4, 5, 6, 7, 8, 9]; diff --git a/library/core/tests/iter/adapters/mod.rs b/library/core/tests/iter/adapters/mod.rs index 96a53be1eaa50..05cce37474e73 100644 --- a/library/core/tests/iter/adapters/mod.rs +++ b/library/core/tests/iter/adapters/mod.rs @@ -20,8 +20,6 @@ mod take; mod take_while; mod zip; -use core::cell::Cell; - /// An iterator that panics whenever `next` or next_back` is called /// after `None` has already been returned. This does not violate /// `Iterator`'s contract. Used to test that iterator adaptors don't @@ -159,27 +157,3 @@ impl<'a, T> Iterator for CycleIter<'a, T> { elt } } - -#[derive(Debug)] -struct CountClone(Cell); - -impl CountClone { - pub fn new() -> Self { - Self(Cell::new(0)) - } -} - -impl PartialEq for CountClone { - fn eq(&self, rhs: &i32) -> bool { - self.0.get() == *rhs - } -} - -impl Clone for CountClone { - fn clone(&self) -> Self { - let ret = CountClone(self.0.clone()); - let n = self.0.get(); - self.0.set(n + 1); - ret - } -} diff --git a/library/core/tests/iter/adapters/zip.rs b/library/core/tests/iter/adapters/zip.rs index 000c15f72c886..e3f0d2f151294 100644 --- a/library/core/tests/iter/adapters/zip.rs +++ b/library/core/tests/iter/adapters/zip.rs @@ -1,4 +1,3 @@ -use super::*; use core::iter::*; #[test] @@ -18,200 +17,6 @@ fn test_zip_nth() { assert_eq!(it.nth(3), None); } -#[test] -fn test_zip_nth_side_effects() { - let mut a = Vec::new(); - let mut b = Vec::new(); - let value = [1, 2, 3, 4, 5, 6] - .iter() - .cloned() - .map(|n| { - a.push(n); - n * 10 - }) - .zip([2, 3, 4, 5, 6, 7, 8].iter().cloned().map(|n| { - b.push(n * 100); - n * 1000 - })) - .skip(1) - .nth(3); - assert_eq!(value, Some((50, 6000))); - assert_eq!(a, vec![1, 2, 3, 4, 5]); - assert_eq!(b, vec![200, 300, 400, 500, 600]); -} - -#[test] -fn test_zip_next_back_side_effects() { - let mut a = Vec::new(); - let mut b = Vec::new(); - let mut iter = [1, 2, 3, 4, 5, 6] - .iter() - .cloned() - .map(|n| { - a.push(n); - n * 10 - }) - .zip([2, 3, 4, 5, 6, 7, 8].iter().cloned().map(|n| { - b.push(n * 100); - n * 1000 - })); - - // The second iterator is one item longer, so `next_back` is called on it - // one more time. - assert_eq!(iter.next_back(), Some((60, 7000))); - assert_eq!(iter.next_back(), Some((50, 6000))); - assert_eq!(iter.next_back(), Some((40, 5000))); - assert_eq!(iter.next_back(), Some((30, 4000))); - assert_eq!(a, vec![6, 5, 4, 3]); - assert_eq!(b, vec![800, 700, 600, 500, 400]); -} - -#[test] -fn test_zip_nth_back_side_effects() { - let mut a = Vec::new(); - let mut b = Vec::new(); - let value = [1, 2, 3, 4, 5, 6] - .iter() - .cloned() - .map(|n| { - a.push(n); - n * 10 - }) - .zip([2, 3, 4, 5, 6, 7, 8].iter().cloned().map(|n| { - b.push(n * 100); - n * 1000 - })) - .nth_back(3); - assert_eq!(value, Some((30, 4000))); - assert_eq!(a, vec![6, 5, 4, 3]); - assert_eq!(b, vec![800, 700, 600, 500, 400]); -} - -#[test] -fn test_zip_next_back_side_effects_exhausted() { - let mut a = Vec::new(); - let mut b = Vec::new(); - let mut iter = [1, 2, 3, 4, 5, 6] - .iter() - .cloned() - .map(|n| { - a.push(n); - n * 10 - }) - .zip([2, 3, 4].iter().cloned().map(|n| { - b.push(n * 100); - n * 1000 - })); - - iter.next(); - iter.next(); - iter.next(); - iter.next(); - assert_eq!(iter.next_back(), None); - assert_eq!(a, vec![1, 2, 3, 4, 6, 5]); - assert_eq!(b, vec![200, 300, 400]); -} - -#[test] -fn test_zip_cloned_sideffectful() { - let xs = [CountClone::new(), CountClone::new(), CountClone::new(), CountClone::new()]; - let ys = [CountClone::new(), CountClone::new()]; - - for _ in xs.iter().cloned().zip(ys.iter().cloned()) {} - - assert_eq!(&xs, &[1, 1, 1, 0][..]); - assert_eq!(&ys, &[1, 1][..]); - - let xs = [CountClone::new(), CountClone::new()]; - let ys = [CountClone::new(), CountClone::new(), CountClone::new(), CountClone::new()]; - - for _ in xs.iter().cloned().zip(ys.iter().cloned()) {} - - assert_eq!(&xs, &[1, 1][..]); - assert_eq!(&ys, &[1, 1, 0, 0][..]); -} - -#[test] -fn test_zip_map_sideffectful() { - let mut xs = [0; 6]; - let mut ys = [0; 4]; - - 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!(&ys, &[1, 1, 1, 1]); - - let mut xs = [0; 4]; - let mut ys = [0; 6]; - - 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]); -} - -#[test] -fn test_zip_map_rev_sideffectful() { - let mut xs = [0; 6]; - let mut ys = [0; 4]; - - { - let mut it = xs.iter_mut().map(|x| *x += 1).zip(ys.iter_mut().map(|y| *y += 1)); - it.next_back(); - } - assert_eq!(&xs, &[0, 0, 0, 1, 1, 1]); - assert_eq!(&ys, &[0, 0, 0, 1]); - - let mut xs = [0; 6]; - let mut ys = [0; 4]; - - { - let mut it = xs.iter_mut().map(|x| *x += 1).zip(ys.iter_mut().map(|y| *y += 1)); - (&mut it).take(5).count(); - it.next_back(); - } - assert_eq!(&xs, &[1, 1, 1, 1, 1, 1]); - assert_eq!(&ys, &[1, 1, 1, 1]); -} - -#[test] -fn test_zip_nested_sideffectful() { - let mut xs = [0; 6]; - let ys = [0; 4]; - - { - // test that it has the side effect nested inside enumerate - let it = xs.iter_mut().map(|x| *x = 1).enumerate().zip(&ys); - it.count(); - } - assert_eq!(&xs, &[1, 1, 1, 1, 1, 0]); -} - -#[test] -fn test_zip_nth_back_side_effects_exhausted() { - let mut a = Vec::new(); - let mut b = Vec::new(); - let mut iter = [1, 2, 3, 4, 5, 6] - .iter() - .cloned() - .map(|n| { - a.push(n); - n * 10 - }) - .zip([2, 3, 4].iter().cloned().map(|n| { - b.push(n * 100); - n * 1000 - })); - - iter.next(); - iter.next(); - iter.next(); - iter.next(); - assert_eq!(iter.nth_back(0), None); - assert_eq!(a, vec![1, 2, 3, 4, 6, 5]); - assert_eq!(b, vec![200, 300, 400]); -} - #[test] fn test_zip_trusted_random_access_composition() { let a = [0, 1, 2, 3, 4]; diff --git a/src/test/ui/issues/issue-3044.stderr b/src/test/ui/issues/issue-3044.stderr index b93aeade95e42..b8300d5071ccd 100644 --- a/src/test/ui/issues/issue-3044.stderr +++ b/src/test/ui/issues/issue-3044.stderr @@ -11,7 +11,7 @@ LL | | }); note: associated function defined here --> $SRC_DIR/core/src/iter/traits/iterator.rs:LL:COL | -LL | fn fold(mut self, init: B, mut f: F) -> B +LL | fn fold(self, init: B, f: F) -> B | ^^^^ error: aborting due to previous error