Rollup merge of #153880 - asder8215:intersperse_nonfused, r=Mark-Simulacrum

Lifted intersperse and intersperse_with Fused transformation and updated documentation + tests

This PR once again builds on top of #152855. From the discussion in #152855, libs team came to the conclusion that `intersperse`/`intersperse_with` shouldn't transform the given iterator to a fused iterator always and a separator should be emitted between `Some(_)` items for non fused iterators (particularly just right before the subsequent `Some(_)`).

On top of the change Zakarumych added in the PR, I lifted the `FusedIterator` trait and transformation of the inner `iter` for `Intersperse`/`IntersperseWith`, so that we can display this behavior. I've adjusted the documentation and tests accordingly to reflect this change as well.

r? @jhpratt

Author: JonathanBrouwer

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

@@ -1,5 +1,4 @@
 use crate::fmt;
-use crate::iter::{Fuse, FusedIterator};
 
 /// An iterator adapter that places a separator between all elements.
 ///
@@ -14,23 +13,15 @@ where
     started: bool,
     separator: I::Item,
     next_item: Option<I::Item>,
-    iter: Fuse<I>,
-}
-
-#[unstable(feature = "iter_intersperse", issue = "79524")]
-impl<I> FusedIterator for Intersperse<I>
-where
-    I: FusedIterator,
-    I::Item: Clone,
-{
+    iter: I,
 }
 
 impl<I: Iterator> Intersperse<I>
 where
     I::Item: Clone,
 {
     pub(in crate::iter) fn new(iter: I, separator: I::Item) -> Self {
-        Self { started: false, separator, next_item: None, iter: iter.fuse() }
+        Self { started: false, separator, next_item: None, iter }
     }
 }
 
@@ -57,8 +48,9 @@ where
                 }
             }
         } else {
-            self.started = true;
-            self.iter.next()
+            let item = self.iter.next();
+            self.started = item.is_some();
+            item
         }
     }
 
@@ -95,15 +87,7 @@ where
     started: bool,
     separator: G,
     next_item: Option<I::Item>,
-    iter: Fuse<I>,
-}
-
-#[unstable(feature = "iter_intersperse", issue = "79524")]
-impl<I, G> FusedIterator for IntersperseWith<I, G>
-where
-    I: FusedIterator,
-    G: FnMut() -> I::Item,
-{
+    iter: I,
 }
 
 #[unstable(feature = "iter_intersperse", issue = "79524")]
@@ -146,7 +130,7 @@ where
     G: FnMut() -> I::Item,
 {
     pub(in crate::iter) fn new(iter: I, separator: G) -> Self {
-        Self { started: false, separator, next_item: None, iter: iter.fuse() }
+        Self { started: false, separator, next_item: None, iter }
     }
 }
 
@@ -173,8 +157,9 @@ where
                 }
             }
         } else {
-            self.started = true;
-            self.iter.next()
+            let item = self.iter.next();
+            self.started = item.is_some();
+            item
         }
     }
 

library/core/src/iter/traits/iterator.rs

@@ -635,11 +635,24 @@ pub const trait Iterator {
     /// of the original iterator.
     ///
     /// Specifically on fused iterators, it is guaranteed that the new iterator
-    /// places a copy of `separator` between adjacent `Some(_)` items. However,
-    /// for non-fused iterators, [`intersperse`] will create a new iterator that
-    /// is a fused version of the original iterator and place a copy of `separator`
-    /// between adjacent `Some(_)` items. This behavior for non-fused iterators
-    /// is subject to change.
+    /// places a copy of `separator` between *adjacent* `Some(_)` items. For non-fused iterators,
+    /// it is guaranteed that [`intersperse`] will create a new iterator that places a copy
+    /// of `separator` between `Some(_)` items, particularly just right before the subsequent
+    /// `Some(_)` item.
+    ///
+    /// For example, consider the following non-fused iterator:
+    ///
+    /// ```text
+    /// Some(1) -> Some(2) -> None -> Some(3) -> Some(4) -> ...
+    /// ```
+    ///
+    /// If this non-fused iterator were to be interspersed with `0`,
+    /// then the interspersed iterator will produce:
+    ///
+    /// ```text
+    /// Some(1) -> Some(0) -> Some(2) -> None -> Some(0) -> Some(3) -> Some(0) ->
+    /// Some(4) -> ...
+    /// ```
     ///
     /// In case `separator` does not implement [`Clone`] or needs to be
     /// computed every time, use [`intersperse_with`].
@@ -688,10 +701,23 @@ pub const trait Iterator {
     ///
     /// Specifically on fused iterators, it is guaranteed that the new iterator
     /// places an item generated by `separator` between adjacent `Some(_)` items.
-    /// However, for non-fused iterators, [`intersperse_with`] will create a new
-    /// iterator that is a fused version of the original iterator and place an item
-    /// generated by `separator` between adjacent `Some(_)` items. This
-    /// behavior for non-fused iterators is subject to change.
+    /// For non-fused iterators, it is guaranteed that [`intersperse_with`] will
+    /// create a new iterator that places an item generated by `separator` between `Some(_)`
+    /// items, particularly just right before the subsequent `Some(_)` item.
+    ///
+    /// For example, consider the following non-fused iterator:
+    ///
+    /// ```text
+    /// Some(1) -> Some(2) -> None -> Some(3) -> Some(4) -> ...
+    /// ```
+    ///
+    /// If this non-fused iterator were to be interspersed with a `separator` closure
+    /// that returns `0` repeatedly, the interspersed iterator will produce:
+    ///
+    /// ```text
+    /// Some(1) -> Some(0) -> Some(2) -> None -> Some(0) -> Some(3) -> Some(0) ->
+    /// Some(4) -> ...
+    /// ```
     ///
     /// The `separator` closure will be called exactly once each time an item
     /// is placed between two adjacent items from the underlying iterator;

library/coretests/tests/iter/adapters/intersperse.rs

@@ -153,10 +153,6 @@ fn test_try_fold_specialization_intersperse_err() {
     assert_eq!(iter.next(), None);
 }
 
-// FIXME(iter_intersperse): `intersperse` current behavior may change for
-// non-fused iterators, so this test will likely have to
-// be adjusted; see PR #152855 and issue #79524
-// if `intersperse` doesn't change, remove this FIXME.
 #[test]
 fn test_non_fused_iterator_intersperse() {
     #[derive(Debug)]
@@ -183,24 +179,26 @@ fn test_non_fused_iterator_intersperse() {
     }
 
     let counter = 0;
-    // places a 2 between `Some(_)` items
+    // places a 1 between `Some(_)` items
     let non_fused_iter = TestCounter { counter };
-    let mut intersperse_iter = non_fused_iter.intersperse(2);
-    // Since `intersperse` currently transforms the original
-    // iterator into a fused iterator, this intersperse_iter
-    // should always have `None`
-    for _ in 0..counter + 6 {
-        assert_eq!(intersperse_iter.next(), None);
-    }
+    let mut intersperse_iter = non_fused_iter.intersperse(1);
+    // Interspersed iter produces:
+    // `None` -> `Some(2)` -> `None` -> `Some(1)` -> Some(4)` -> `None` ->  `Some(1)` ->
+    // `Some(6)` -> and then `None` endlessly
+    assert_eq!(intersperse_iter.next(), None);
+    assert_eq!(intersperse_iter.next(), Some(2));
+    assert_eq!(intersperse_iter.next(), None);
+    assert_eq!(intersperse_iter.next(), Some(1));
+    assert_eq!(intersperse_iter.next(), Some(4));
+    assert_eq!(intersperse_iter.next(), None);
+    assert_eq!(intersperse_iter.next(), Some(1));
+    assert_eq!(intersperse_iter.next(), Some(6));
+    assert_eq!(intersperse_iter.next(), None);
 
-    // Extra check to make sure it is `None` after processing 6 items
+    // Extra check to make sure it is `None` after processing all items
     assert_eq!(intersperse_iter.next(), None);
 }
 
-// FIXME(iter_intersperse): `intersperse` current behavior may change for
-// non-fused iterators, so this test will likely have to
-// be adjusted; see PR #152855 and issue #79524
-// if `intersperse` doesn't change, remove this FIXME.
 #[test]
 fn test_non_fused_iterator_intersperse_2() {
     #[derive(Debug)]
@@ -228,35 +226,26 @@ fn test_non_fused_iterator_intersperse_2() {
     }
 
     let counter = 0;
-    // places a 2 between `Some(_)` items
+    // places a 100 between `Some(_)` items
     let non_fused_iter = TestCounter { counter };
-    let mut intersperse_iter = non_fused_iter.intersperse(2);
-    // Since `intersperse` currently transforms the original
-    // iterator into a fused iterator, this interspersed iter
-    // will be `Some(1)` -> `Some(2)` -> `Some(2)` -> and then
-    // `None` endlessly
-    let mut items_processed = 0;
-    for num in 0..counter + 6 {
-        if num < 3 {
-            if num % 2 != 0 {
-                assert_eq!(intersperse_iter.next(), Some(2));
-            } else {
-                items_processed += 1;
-                assert_eq!(intersperse_iter.next(), Some(items_processed));
-            }
-        } else {
-            assert_eq!(intersperse_iter.next(), None);
-        }
-    }
+    let mut intersperse_iter = non_fused_iter.intersperse(100);
+    // Interspersed iter produces:
+    // `Some(1)` -> `Some(100)` -> `Some(2)` -> `None` -> `Some(100)`
+    //  -> `Some(4)` -> `Some(100)` -> `Some(5)` -> `None` endlessly
+    assert_eq!(intersperse_iter.next(), Some(1));
+    assert_eq!(intersperse_iter.next(), Some(100));
+    assert_eq!(intersperse_iter.next(), Some(2));
+    assert_eq!(intersperse_iter.next(), None);
+    assert_eq!(intersperse_iter.next(), Some(100));
+    assert_eq!(intersperse_iter.next(), Some(4));
+    assert_eq!(intersperse_iter.next(), Some(100));
+    assert_eq!(intersperse_iter.next(), Some(5));
+    assert_eq!(intersperse_iter.next(), None);
 
     // Extra check to make sure it is `None` after processing 6 items
     assert_eq!(intersperse_iter.next(), None);
 }
 
-// FIXME(iter_intersperse): `intersperse_with` current behavior may change for
-// non-fused iterators, so this test will likely have to
-// be adjusted; see PR #152855 and issue #79524
-// if `intersperse_with` doesn't change, remove this FIXME.
 #[test]
 fn test_non_fused_iterator_intersperse_with() {
     #[derive(Debug)]
@@ -285,22 +274,24 @@ fn test_non_fused_iterator_intersperse_with() {
     let counter = 0;
     let non_fused_iter = TestCounter { counter };
     // places a 2 between `Some(_)` items
-    let mut intersperse_iter = non_fused_iter.intersperse_with(|| 2);
-    // Since `intersperse` currently transforms the original
-    // iterator into a fused iterator, this intersperse_iter
-    // should always have `None`
-    for _ in 0..counter + 6 {
-        assert_eq!(intersperse_iter.next(), None);
-    }
+    let mut intersperse_iter = non_fused_iter.intersperse_with(|| 1);
+    // Interspersed iter produces:
+    // `None` -> `Some(2)` -> `None` -> `Some(1)` -> Some(4)` -> `None` ->  `Some(1)` ->
+    // `Some(6)` -> and then `None` endlessly
+    assert_eq!(intersperse_iter.next(), None);
+    assert_eq!(intersperse_iter.next(), Some(2));
+    assert_eq!(intersperse_iter.next(), None);
+    assert_eq!(intersperse_iter.next(), Some(1));
+    assert_eq!(intersperse_iter.next(), Some(4));
+    assert_eq!(intersperse_iter.next(), None);
+    assert_eq!(intersperse_iter.next(), Some(1));
+    assert_eq!(intersperse_iter.next(), Some(6));
+    assert_eq!(intersperse_iter.next(), None);
 
     // Extra check to make sure it is `None` after processing 6 items
     assert_eq!(intersperse_iter.next(), None);
 }
 
-// FIXME(iter_intersperse): `intersperse_with` current behavior may change for
-// non-fused iterators, so this test will likely have to
-// be adjusted; see PR #152855 and issue #79524
-// if `intersperse_with` doesn't change, remove this FIXME.
 #[test]
 fn test_non_fused_iterator_intersperse_with_2() {
     #[derive(Debug)]
@@ -328,26 +319,21 @@ fn test_non_fused_iterator_intersperse_with_2() {
     }
 
     let counter = 0;
-    // places a 2 between `Some(_)` items
+    // places a 100 between `Some(_)` items
     let non_fused_iter = TestCounter { counter };
-    let mut intersperse_iter = non_fused_iter.intersperse(2);
-    // Since `intersperse` currently transforms the original
-    // iterator into a fused iterator, this interspersed iter
-    // will be `Some(1)` -> `Some(2)` -> `Some(2)` -> and then
-    // `None` endlessly
-    let mut items_processed = 0;
-    for num in 0..counter + 6 {
-        if num < 3 {
-            if num % 2 != 0 {
-                assert_eq!(intersperse_iter.next(), Some(2));
-            } else {
-                items_processed += 1;
-                assert_eq!(intersperse_iter.next(), Some(items_processed));
-            }
-        } else {
-            assert_eq!(intersperse_iter.next(), None);
-        }
-    }
+    let mut intersperse_iter = non_fused_iter.intersperse_with(|| 100);
+    // Interspersed iter produces:
+    // `Some(1)` -> `Some(100)` -> `Some(2)` -> `None` -> `Some(100)`
+    //  -> `Some(4)` -> `Some(100)` -> `Some(5)` -> `None` endlessly
+    assert_eq!(intersperse_iter.next(), Some(1));
+    assert_eq!(intersperse_iter.next(), Some(100));
+    assert_eq!(intersperse_iter.next(), Some(2));
+    assert_eq!(intersperse_iter.next(), None);
+    assert_eq!(intersperse_iter.next(), Some(100));
+    assert_eq!(intersperse_iter.next(), Some(4));
+    assert_eq!(intersperse_iter.next(), Some(100));
+    assert_eq!(intersperse_iter.next(), Some(5));
+    assert_eq!(intersperse_iter.next(), None);
 
     // Extra check to make sure it is `None` after processing 6 items
     assert_eq!(intersperse_iter.next(), None);