Skip to content

Fix IteratorRandom::sample and sample_fill consuming the iterator when amount == 0#1810

Open
teddytennant wants to merge 1 commit into
rust-random:masterfrom
teddytennant:iter-sample-amount-zero
Open

Fix IteratorRandom::sample and sample_fill consuming the iterator when amount == 0#1810
teddytennant wants to merge 1 commit into
rust-random:masterfrom
teddytennant:iter-sample-amount-zero

Conversation

@teddytennant

Copy link
Copy Markdown
Contributor
  • Added a CHANGELOG.md entry

Summary

Make IteratorRandom::sample and sample_fill return early when amount == 0 instead of consuming the iterator and advancing the RNG.

Motivation

With amount == 0, sample_fill skips the initial fill loop (len < amount is immediately false) and sample's take(0) reservoir is empty, so both fall through to the reservoir-sampling loop. That loop walks the entire remaining iterator and draws one random index per element, even though nothing can ever be selected. So the degenerate case is O(n) instead of O(1), exhausts an iterator passed by_ref, and perturbs the RNG state.

Details

Added an early return in both methods: sample returns Vec::new() and sample_fill returns 0 when the buffer is empty. No behavior change for amount > 0.

New test test_sample_amount_zero checks that neither method consumes any items (counted via inspect) and that the RNG state is untouched afterwards; the sample half is gated on feature = "alloc". Verified with cargo test --lib seq::iterator on default features and --no-default-features. Existing value-stability tests are unaffected since they use amount > 0.

… the iterator

Both methods fell through to the reservoir-sampling loop when amount == 0,
iterating the entire input and drawing one random index per element even
though nothing can be selected. Return early instead, so the degenerate
case is O(1), leaves the iterator untouched and no longer advances the
RNG state.
Copilot AI review requested due to automatic review settings July 11, 2026 17:43

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@dhardy dhardy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not keen on this approach since (a) it special-cases amount == 0, leaving the complexity at O(n) for all amount > 0 and (b) some people may rely on the iterator being exhausted and/or on the current permutations to rng (there's a reason choose_stable exists).

We could also consider optimisations (as seen in choose) for amount << n, but then some users may also want stable variants and the additional complexity is hard to justify.

Since the complexity is documented as O(n) the best solution is probably to keep the existing behaviour. (Callers can still special-case amount == 0 if desired.)

@dhardy dhardy added the B-value Breakage: changes output values label Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-value Breakage: changes output values

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants