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

Open
Takashiidobe wants to merge 5 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 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
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
72 changes: 56 additions & 16 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 @@ -13,14 +12,16 @@ pub struct PadUsing<I, F> {
iter: Fuse<I>,
min: usize,
pos: usize,
back: usize,
total_len: 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, min, pos, back, total_len);
}

/// Create a new `PadUsing` iterator.
Expand All @@ -33,6 +34,8 @@ where
iter: iter.fuse(),
min,
pos: 0,
back: 0,
total_len: min,
filler,
}
}
Expand All @@ -48,7 +51,7 @@ where
fn next(&mut self) -> Option<Self::Item> {
match self.iter.next() {
None => {
if self.pos < self.min {
if self.pos + self.back < self.total_len {
let e = Some((self.filler)(self.pos));
self.pos += 1;
e
Expand All @@ -64,8 +67,18 @@ where
}

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 (iter_lower, iter_upper) = self.iter.size_hint();
let consumed = self.pos.saturating_add(self.back);

let total_lower = iter_lower.saturating_add(self.pos).max(self.min);
let lower_bound = total_lower.saturating_sub(consumed);

let upper_bound = iter_upper.map(|iter_upper| {
let total_upper = iter_upper.saturating_add(self.pos).max(self.min);
total_upper.saturating_sub(consumed)
});

(lower_bound, upper_bound)
}

fn fold<B, G>(self, mut init: B, mut f: G) -> B
Expand All @@ -87,25 +100,52 @@ 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()
let current_iter_len = self.iter.len();
let original_iter_len = current_iter_len.saturating_add(self.pos);
if self.total_len < original_iter_len {
self.total_len = original_iter_len;
}

if self.pos + self.back >= self.total_len {
return None;
}

let padding_count = self.total_len.saturating_sub(current_iter_len + self.pos);

if self.back < padding_count {
let idx = self.total_len - self.back - 1;
self.back += 1;
Some((self.filler)(idx))
Copy link
Member

@phimuemue phimuemue Dec 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'd expect next_back to look sth like this:

if elements_from_next + elements_from_next_back < elements_required { // Note: Same condition as in `fn next`
 let elements_remaining = elements_required - (elements_from_next + elements_from_next_back);
 elements_from_next_back += 1;
 if iter.len() < elements_remaining {
  Some((self.filler)(elements_required - elements_from_next_back)) // TODO Is the index computed correctly?
 } else {
  let e = iter.next_back();
  assert!(e.is_some); // If this triggers, incoming ExactSizeIterator is implemented incorrectly. I'd 
  e
 }
} else {
 iter.next_back()
}

This seems simpler to me.

Copy link
Contributor Author

@Takashiidobe Takashiidobe Dec 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is close but it still fails tests cause it doesn't compute the index properly.

I fixed it up and tested it like this

if self.elements_from_next + self.elements_from_next_back < self.elements_required {
    // Note: Same condition as in `fn next`
    let elements_remaining =
        self.elements_required - (self.elements_from_next + self.elements_from_next_back);
    self.elements_from_next_back += 1;
    if self.iter.len() < elements_remaining {
        Some((self.filler)(
            self.elements_required - self.elements_from_next_back,
        ))
    } else {
        let e = self.iter.next_back();
        assert!(e.is_some()); // If this triggers, incoming ExactSizeIterator is implemented incorrectly. I'd
        e
    }
} else {
    None
}

I rewrote your code like this, but it still does have the saturating sub. Tests will fail without it so I'm not so sure what to do there.

let total_consumed = self.elements_from_next + self.elements_from_next_back;

if self.iter.len() == 0 && total_consumed >= self.elements_required {
    return None;
}

let elements_remaining = self.elements_required.saturating_sub(total_consumed);
self.elements_from_next_back += 1;

if self.iter.len() < elements_remaining {
    Some((self.filler)(
        self.elements_required - self.elements_from_next_back,
    ))
} else {
    let e = self.iter.next_back();
    assert!(e.is_some());
    e
}

Copy link
Contributor Author

@Takashiidobe Takashiidobe Dec 5, 2025

Choose a reason for hiding this comment

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

One thing I'd also like to mention: there are sources of saturating_* in the codebase outside of pad_tail.

I think it would also be an improvement to have tests that can trigger cases that check for saturating ops saturating when they shouldn't if it's a concern for code reviews, since it would help the dev cycle.

Trying to write this pad_using properly, there's no way to do this without really good tests, this is really difficult (at least for me).

$ rg "saturating_"
tests/test_std.rs
501:        let mut v: Vec<_> = (n..n.saturating_add(m as _)).collect();
537:        let mut v: Vec<_> = (n..n.saturating_add(m as _)).collect();
1241:            let len = binomial(usize::saturating_sub(n + k, 1), k);

tests/quick.rs
63:            org_lower.saturating_sub(self.underestimate),
1985:        let result = &v[v.len().saturating_sub(n)..];

src/pad_tail.rs
77:            .saturating_add(self.elements_from_next_back);
80:            .saturating_add(self.elements_from_next)
82:        let lower_bound = total_lower.saturating_sub(consumed);
86:                .saturating_add(self.elements_from_next)
88:            total_upper.saturating_sub(consumed)
119:        let elements_remaining = self.elements_required.saturating_sub(total_consumed);
145:        let original_iter_len = iter_len.saturating_add(elements_from_next);

src/size_hint.rs
11:    let min = a.0.saturating_add(b.0);
24:    low = low.saturating_add(x);
33:    low = low.saturating_sub(x);
34:    hi = hi.map(|elt| elt.saturating_sub(x));
41:    let low = a.0.saturating_mul(b.0);
54:    low = low.saturating_mul(x);

src/iter_index.rs
59:            (self.end() + 1).saturating_sub(*self.start())

src/peek_nth.rs
72:        let unbuffered_items = (n + 1).saturating_sub(self.buf.len());
113:        let unbuffered_items = (n + 1).saturating_sub(self.buf.len());

src/sources.rs
25:///     let next = x1.saturating_add(*x2);

src/lib.rs
3673:                let mut iter = self.fuse().skip(low.saturating_sub(n));

src/combinations_with_replacement.rs
177:            k.saturating_sub(1)

Copy link
Member

@phimuemue phimuemue Dec 5, 2025

Choose a reason for hiding this comment

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

One thing I'd also like to mention: there are sources of saturating_* in the codebase outside of pad_tail.

True, but we should strive to avoid them if not strictly necessary.

Copy link
Member

@phimuemue phimuemue Dec 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Trying to write this pad_using properly [...], this is really difficult (at least for me).

For me, too. But we must not patch the code here and there until all of our tests succeed.

Instead, we should look for a simple explanation how pad_using works. My explanation looks like this:

  • iter.pad_using(n, ...) will generate at least n elements.
  • Iff iter itself generates n or more elements, filler will not supply elements.
    => If our iterator already generated more than n elements, we know that we must not call filler anymore (because more than n elements means that all of them come from iter).
  • As you correctly observed in your original PR, counting nexted and next_backed elements makes things easier.

I tried to execute this idea in code, and fwiw, fn next_back (as in my edited comment) successfully passes the tests on my machine:

        if self.elements_from_next + self.elements_from_next_back < self.elements_required {
            // Note: Same condition as in `fn next`
            let elements_remaining =
                self.elements_required - (self.elements_from_next + self.elements_from_next_back);
            self.elements_from_next_back += 1;
            if self.iter.len() < elements_remaining {
                Some((self.filler)(
                        self.elements_required - self.elements_from_next_back,
                ))
            } else {
                let e = self.iter.next_back();
                assert!(e.is_some()); // If this triggers, incoming ExactSizeIterator is implemented incorrectly. I'd
                e
            }
        } else {
            self.iter.next_back()
        }

Of course I'm biased, but to me my next_back looks easier than your rewritten function. 12

Thinking of it, we may even apply similar logic (i.e. check if we so far generated less than elements_required elements and behave accordingly) for fn next.

Footnotes

  1. Mine has no saturating_sub. saturating_sub begs the question when exactly the operation will actually saturate and when not. ↩

  2. Why exactly does yours test self.iter.len() == 0 && total_consumed >= self.elements_required? In particular, is len==0 redundant (because in this case just calling next_back would return None anyway)? ↩

Copy link
Member

@phimuemue phimuemue Dec 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Speaking of which: When we establish the elements_from_next + elements_from_next_back < elements_required thing, couldn't we also use it to straightforwardly implement other operations?

fn size_hint(self) {
 if (elements_from_next + elements_from_next_back < elements_required) {
  let elements_remaining = ...;
  let (lo, hi) = iter.size_hint();
  if (lo >= elements_remaining) {
   // iterator itself will supply all remaining elements
   (lo, hi)
  } else {
   // both iterator and filler may supply remaining elements
   ... // TODO
  }
 } else {
  // won't touch filler anymore
  iter.size_hint()
 }
}

I know that explicitly listing relevant cases may look ugly, but this way you can keep the simple cases really simple, and constrain the complexity to the other cases. I consider this better than relying on several calls to saturating_[add|sub] that still have case distinctions - but implicit ones.

Copy link
Contributor Author

@Takashiidobe Takashiidobe Dec 5, 2025

Choose a reason for hiding this comment

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

Ah I see the edit, the else branch was supposed to be next_back instead of None. That makes sense. With that, my approach looks like this:

fn next_back(&mut self) -> Option<Self::Item> {
    let total_consumed = self.elements_from_next + self.elements_from_next_back;

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

    let elements_remaining = self.elements_required - total_consumed;
    self.elements_from_next_back += 1;

    if self.iter.len() < elements_remaining {
        Some((self.filler)(
            self.elements_required - self.elements_from_next_back,
        ))
    } else {
        self.iter.next_back()
    }
}

It's the same implementation except I prefer guard clauses at the top, so I put the else branch at the top to short circuit, and there are no more saturating ops.

For next, I couldn't do exactly the same pattern cause len is only for ExactSizedIterator so it's slightly different but the same idea.

fn next(&mut self) -> Option<Self::Item> {
    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)
    }
}

For size_hint, I swapped the saturating ops with max. No saturation required because there's no risk of overflow.

fn size_hint(&self) -> (usize, Option<usize>) {
    let total_consumed = self.elements_from_next + self.elements_from_next_back;

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

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

    let lower_bound = low.max(elements_remaining);
    let upper_bound = high.map(|h| h.max(elements_remaining));

    (lower_bound, upper_bound)
}

} else {
self.min -= 1;
Some((self.filler)(self.min))
self.back += 1;
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.)

}
}

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 PadUsing {
iter,
min: _,
pos,
back,
mut total_len,
filler,
} = self;
let iter_len = iter.len();
let original_iter_len = iter_len.saturating_add(pos);
if total_len < original_iter_len {
total_len = original_iter_len;
}

let start_idx = iter_len + pos;
let end_idx = total_len - back;

init = (start_idx..end_idx).rev().map(filler).fold(init, &mut f);

iter.rfold(init, f)
}
}

Expand Down
19 changes: 19 additions & 0 deletions tests/specializations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,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();

for _ in fwd.by_ref() {}
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.

Plase make this while fwd.next_back.is_some (just as with bwd).


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