Skip to content

Fix pad_using bug producing items when it shouldn't #1079

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

Closed
Takashiidobe wants to merge 6 commits into from
+77 −52
Closed

Fix pad_using bug producing items when it shouldn't #1079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
625c2b9
add double ended iterator tests that make sure using next back after …
Takashiidobe Dec 5, 2025
979d7a7
code review feedback: try to remove saturating_ ops from next_back
Takashiidobe Dec 5, 2025
8e8b98a
refactor most implementations of pad_tail to remove saturating ops + …
Takashiidobe Dec 5, 2025
ee3022a
fix pad using's fold to keep track of elements_from_next_back like rfold
Takashiidobe Dec 6, 2025
5524abc
add test for fold as well as rfold to check for double ended iterator…
Takashiidobe Dec 6, 2025
537d094
code review feedback + remove fold + rfold implementations cause we s…
Takashiidobe Dec 13, 2025
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
101 changes: 49 additions & 52 deletions src/pad_tail.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use crate::size_hint;
use std::iter::{Fuse, FusedIterator};

/// An iterator adaptor that pads a sequence to a minimum length by filling
Expand All @@ -11,28 +10,36 @@ use std::iter::{Fuse, FusedIterator};
#[must_use = "iterator adaptors are lazy and do nothing unless consumed"]
pub struct PadUsing<I, F> {
iter: Fuse<I>,
min: usize,
pos: usize,
elements_from_next: usize,
elements_from_next_back: usize,
elements_required: usize,
filler: F,
}

impl<I, F> std::fmt::Debug for PadUsing<I, F>
where
I: std::fmt::Debug,
{
debug_fmt_fields!(PadUsing, iter, min, pos);
debug_fmt_fields!(
PadUsing,
iter,
elements_from_next,
elements_from_next_back,
elements_required
);
}

/// Create a new `PadUsing` iterator.
pub fn pad_using<I, F>(iter: I, min: usize, filler: F) -> PadUsing<I, F>
pub fn pad_using<I, F>(iter: I, elements_required: usize, filler: F) -> PadUsing<I, F>
where
I: Iterator,
F: FnMut(usize) -> I::Item,
{
PadUsing {
iter: iter.fuse(),
min,
pos: 0,
elements_from_next: 0,
elements_from_next_back: 0,
elements_required,
filler,
}
}
Expand All @@ -44,40 +51,35 @@ where
{
type Item = I::Item;

#[inline]
fn next(&mut self) -> Option<Self::Item> {
match self.iter.next() {
None => {
if self.pos < self.min {
let e = Some((self.filler)(self.pos));
self.pos += 1;
e
} else {
None
}
}
e => {
self.pos += 1;
e
}
let total_consumed = self.elements_from_next + self.elements_from_next_back;

if total_consumed >= self.elements_required {
self.iter.next()
} else if let Some(e) = self.iter.next() {
self.elements_from_next += 1;
Some(e)
} else {
let e = (self.filler)(self.elements_from_next);
self.elements_from_next += 1;
Some(e)
Comment on lines +57 to +65
Copy link
Member

@phimuemue phimuemue Dec 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very nice imo. It tells

  • total_consumed does not overcount elements_required, which (presumably) avoids overflows.

}
}

fn size_hint(&self) -> (usize, Option<usize>) {
let tail = self.min.saturating_sub(self.pos);
size_hint::max(self.iter.size_hint(), (tail, Some(tail)))
}
let total_consumed = self.elements_from_next + self.elements_from_next_back;

if total_consumed >= self.elements_required {
return self.iter.size_hint();
}
Comment on lines +72 to +74
Copy link
Member

@phimuemue phimuemue Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice and simple. Don't overcount elements_required, similar to next.


let elements_remaining = self.elements_required - total_consumed;
let (low, high) = self.iter.size_hint();

fn fold<B, G>(self, mut init: B, mut f: G) -> B
where
G: FnMut(B, Self::Item) -> B,
{
let mut pos = self.pos;
init = self.iter.fold(init, |acc, item| {
pos += 1;
f(acc, item)
});
(pos..self.min).map(self.filler).fold(init, f)
let lower_bound = low.max(elements_remaining);
let upper_bound = high.map(|h| h.max(elements_remaining));

(lower_bound, upper_bound)
}
}

Expand All @@ -87,25 +89,20 @@ where
F: FnMut(usize) -> I::Item,
{
fn next_back(&mut self) -> Option<Self::Item> {
if self.min == 0 {
self.iter.next_back()
} else if self.iter.len() >= self.min {
self.min -= 1;
self.iter.next_back()
} else {
self.min -= 1;
Some((self.filler)(self.min))
let total_consumed = self.elements_from_next + self.elements_from_next_back;

if total_consumed >= self.elements_required {
return self.iter.next_back();
}
}

Comment on lines 94 to 97
Copy link
Member

@phimuemue phimuemue Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, nice. Similar to next.

fn rfold<B, G>(self, mut init: B, mut f: G) -> B
where
G: FnMut(B, Self::Item) -> B,
{
init = (self.iter.len()..self.min)
.map(self.filler)
.rfold(init, &mut f);
self.iter.rfold(init, f)
let index_from_back = self.elements_required - self.elements_from_next_back - 1;
self.elements_from_next_back += 1;

if index_from_back >= self.iter.len() {
Some((self.filler)(index_from_back))
Comment on lines 98 to 102
Copy link
Member

@phimuemue phimuemue Dec 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is incorrect. Comparing self.iter.len to index_from_back only woks if elements_next is zero.

Consider this scenario:

let mut it = vec![1,2].pad_using(3, |_| 0);
assert_eq!(it.next(), Some(1));
assert_eq!(it.next_back(), Some(0));
assert_eq!(it.next_back(), Some(2)); // <- This triggers.

Please re-instate the previous thing (comparing iter.len to elements_remaining).

If you want to add a test, look for fn pad_using in specializations.rs and change it to this (please do it in a separate commit):

    fn pad_using(v: Vec<u8>) -> () {
        {
            use std::convert::TryFrom;
            let it = v.iter().copied().pad_using(10, |i| u8::try_from(5 * i).unwrap_or(u8::MAX));
            test_specializations(&it);
            test_double_ended_specializations(&it);
        }
        {
            let only_oks = v.into_iter().map(Ok).collect::<Vec<_>>();
            for elements_required in 0..128 {
                let mut oks_then_errs = only_oks.clone();
                while oks_then_errs.len() < elements_required {
                    oks_then_errs.push(Err(oks_then_errs.len()));
                }
                assert_eq!(
                    only_oks.iter().copied().pad_using(elements_required, Err).collect::<Vec<_>>(),
                    oks_then_errs
                );
                // Check next, next_back. Unexpectedly tricky for pad_using
                let mut iter_check = oks_then_errs.into_iter();
                let mut iter_real = only_oks.iter().copied().pad_using(elements_required, Err);
                let mut rng = rand::thread_rng();
                loop {
                    let element = if rng.gen_bool(0.5) {
                        let element_next_check = iter_check.next();
                        let element_next_real = iter_real.next();
                        assert_eq!(element_next_check, element_next_real);
                        element_next_real
                    } else {
                        let element_next_back_check = iter_check.next_back();
                        let element_next_back_real = iter_real.next_back();
                        assert_eq!(element_next_back_check, element_next_back_real);
                        element_next_back_real
                    };
                    if element.is_none() {
                        break;
                    }
                }
            }
        }
    }

} else {
self.iter.next_back()
Copy link
Member

@phimuemue phimuemue Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please assert that the result of next_back is_some. Otherwise incrementing elements_from_next_back does not make sense and we'd possibly overcount elements_required.

You could even think about incrementing in each branch explicitly (just as you did in next). (Or adjust next to match next_back: Aligned logic in next and next_back might be easier to understand.)

Copy link
Member

@phimuemue phimuemue Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2. 'm confused about the need to assert, either in this version or the previous version of the code (since they both contain the same logic). self.iter.next_back() is always Some, so I don't know what the assert call does. incrementing self.elements_from_next_back doesn't impact the underlying iterator.

Please add debug_assert that the element obtained from next_back actually is_some. We could leave it out with the argument "garbage in, garbage out", but the debug_asserts catches the in-garbage instead of tunneling through wrong results.

}
}
}

Expand Down
28 changes: 28 additions & 0 deletions tests/specializations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,15 @@ where
}
}
}
check_specialized!(it, |i| {
let mut parameters_from_fold = vec![];
let fold_result = i.fold(vec![], |mut acc, v: I::Item| {
parameters_from_fold.push((acc.clone(), v.clone()));
acc.push(v);
acc
});
(parameters_from_fold, fold_result)
});
check_specialized!(it, |i| {
let mut parameters_from_rfold = vec![];
let rfold_result = i.rfold(vec![], |mut acc, v: I::Item| {
Expand All @@ -131,6 +140,25 @@ where
for n in 0..size + 2 {
check_specialized!(it, |mut i| i.nth_back(n));
}

let mut fwd = it.clone();
let mut bwd = it.clone();

while fwd.next().is_some() {}

assert_eq!(
fwd.next_back(),
None,
"iterator leaks elements after consuming forwards"
);

while bwd.next_back().is_some() {}

assert_eq!(
bwd.next(),
None,
"iterator leaks elements after consuming backwards"
);
}

quickcheck! {
Expand Down
Loading