From eeb687f20e86f2e2cf61ef89139c102cb92abfcb Mon Sep 17 00:00:00 2001 From: Josh Stone Date: Tue, 21 Apr 2020 14:56:59 -0700 Subject: [PATCH] Don't fuse Chain in its second iterator Only the "first" iterator is actually set `None` when exhausted, depending on whether you iterate forward or backward. This restores behavior similar to the former `ChainState`, where it would transition from `Both` to `Front`/`Back` and only continue from that side. However, if you mix directions, then this may still set both sides to `None`, totally fusing the iterator. --- src/libcore/iter/adapters/chain.rs | 32 +++++++++---- src/libcore/tests/iter.rs | 76 ++++++++++++++++++------------ src/libcore/tests/lib.rs | 1 + 3 files changed, 68 insertions(+), 41 deletions(-) diff --git a/src/libcore/iter/adapters/chain.rs b/src/libcore/iter/adapters/chain.rs index 2dd405ced20..6700ef017bd 100644 --- a/src/libcore/iter/adapters/chain.rs +++ b/src/libcore/iter/adapters/chain.rs @@ -18,6 +18,9 @@ pub struct Chain { // adapter because its specialization for `FusedIterator` unconditionally descends into the // iterator, and that could be expensive to keep revisiting stuff like nested chains. It also // hurts compiler performance to add more iterator layers to `Chain`. + // + // Only the "first" iterator is actually set `None` when exhausted, depending on whether you + // iterate forward or backward. If you mix directions, then both sides may be `None`. a: Option, b: Option, } @@ -43,6 +46,17 @@ macro_rules! fuse { }; } +/// Try an iterator method without fusing, +/// like an inline `.as_mut().and_then(...)` +macro_rules! maybe { + ($self:ident . $iter:ident . $($call:tt)+) => { + match $self.$iter { + Some(ref mut iter) => iter.$($call)+, + None => None, + } + }; +} + #[stable(feature = "rust1", since = "1.0.0")] impl Iterator for Chain where @@ -54,7 +68,7 @@ where #[inline] fn next(&mut self) -> Option { match fuse!(self.a.next()) { - None => fuse!(self.b.next()), + None => maybe!(self.b.next()), item => item, } } @@ -85,7 +99,7 @@ where } if let Some(ref mut b) = self.b { acc = b.try_fold(acc, f)?; - self.b = None; + // we don't fuse the second iterator } Try::from_ok(acc) } @@ -114,7 +128,7 @@ where } self.a = None; } - fuse!(self.b.nth(n)) + maybe!(self.b.nth(n)) } #[inline] @@ -123,7 +137,7 @@ where P: FnMut(&Self::Item) -> bool, { match fuse!(self.a.find(&mut predicate)) { - None => fuse!(self.b.find(predicate)), + None => maybe!(self.b.find(predicate)), item => item, } } @@ -174,7 +188,7 @@ where #[inline] fn next_back(&mut self) -> Option { match fuse!(self.b.next_back()) { - None => fuse!(self.a.next_back()), + None => maybe!(self.a.next_back()), item => item, } } @@ -190,7 +204,7 @@ where } self.b = None; } - fuse!(self.a.nth_back(n)) + maybe!(self.a.nth_back(n)) } #[inline] @@ -199,7 +213,7 @@ where P: FnMut(&Self::Item) -> bool, { match fuse!(self.b.rfind(&mut predicate)) { - None => fuse!(self.a.rfind(predicate)), + None => maybe!(self.a.rfind(predicate)), item => item, } } @@ -216,7 +230,7 @@ where } if let Some(ref mut a) = self.a { acc = a.try_rfold(acc, f)?; - self.a = None; + // we don't fuse the second iterator } Try::from_ok(acc) } @@ -236,8 +250,6 @@ where } // Note: *both* must be fused to handle double-ended iterators. -// Now that we "fuse" both sides, we *could* implement this unconditionally, -// but we should be cautious about committing to that in the public API. #[stable(feature = "fused", since = "1.26.0")] impl FusedIterator for Chain where diff --git a/src/libcore/tests/iter.rs b/src/libcore/tests/iter.rs index 1a1dbcd7b87..7da02b11676 100644 --- a/src/libcore/tests/iter.rs +++ b/src/libcore/tests/iter.rs @@ -207,50 +207,64 @@ fn test_iterator_chain_find() { assert_eq!(iter.next(), None); } +struct Toggle { + is_empty: bool, +} + +impl Iterator for Toggle { + type Item = (); + + // alternates between `None` and `Some(())` + fn next(&mut self) -> Option { + if self.is_empty { + self.is_empty = false; + None + } else { + self.is_empty = true; + Some(()) + } + } + + fn size_hint(&self) -> (usize, Option) { + if self.is_empty { (0, Some(0)) } else { (1, Some(1)) } + } +} + +impl DoubleEndedIterator for Toggle { + fn next_back(&mut self) -> Option { + self.next() + } +} + #[test] fn test_iterator_chain_size_hint() { - struct Iter { - is_empty: bool, - } - - impl Iterator for Iter { - type Item = (); - - // alternates between `None` and `Some(())` - fn next(&mut self) -> Option { - if self.is_empty { - self.is_empty = false; - None - } else { - self.is_empty = true; - Some(()) - } - } - - fn size_hint(&self) -> (usize, Option) { - if self.is_empty { (0, Some(0)) } else { (1, Some(1)) } - } - } - - impl DoubleEndedIterator for Iter { - fn next_back(&mut self) -> Option { - self.next() - } - } - // this chains an iterator of length 0 with an iterator of length 1, // so after calling `.next()` once, the iterator is empty and the // state is `ChainState::Back`. `.size_hint()` should now disregard // the size hint of the left iterator - let mut iter = Iter { is_empty: true }.chain(once(())); + let mut iter = Toggle { is_empty: true }.chain(once(())); assert_eq!(iter.next(), Some(())); assert_eq!(iter.size_hint(), (0, Some(0))); - let mut iter = once(()).chain(Iter { is_empty: true }); + let mut iter = once(()).chain(Toggle { is_empty: true }); assert_eq!(iter.next_back(), Some(())); assert_eq!(iter.size_hint(), (0, Some(0))); } +#[test] +fn test_iterator_chain_unfused() { + // Chain shouldn't be fused in its second iterator, depending on direction + let mut iter = NonFused::new(empty()).chain(Toggle { is_empty: true }); + iter.next().unwrap_none(); + iter.next().unwrap(); + iter.next().unwrap_none(); + + let mut iter = Toggle { is_empty: true }.chain(NonFused::new(empty())); + iter.next_back().unwrap_none(); + iter.next_back().unwrap(); + iter.next_back().unwrap_none(); +} + #[test] fn test_zip_nth() { let xs = [0, 1, 2, 4, 5]; diff --git a/src/libcore/tests/lib.rs b/src/libcore/tests/lib.rs index 05f958cbe81..e7d36d327cd 100644 --- a/src/libcore/tests/lib.rs +++ b/src/libcore/tests/lib.rs @@ -42,6 +42,7 @@ #![feature(unwrap_infallible)] #![feature(leading_trailing_ones)] #![feature(const_forget)] +#![feature(option_unwrap_none)] extern crate test;