fix(vm): combinations on lazy iterators skipped tail combinations 🔢 - #144
Merged
Conversation
CombinationsIter's pivot algorithm assumed `self.buffer.len()` reflects the full pool, but for lazy sources the buffer is filled on demand. When `indices[k-1]` reached the current buffer end the algorithm would declare the rightmost index "at max" against the not-yet-extended pool and advance an earlier index instead — yielding combinations out of lex order and skipping ones whose rightmost index lived in the unpulled tail. Concretely, `[1, 2, 3, 4].enumerate().combinations(2)` yielded five combinations instead of six, missing `((0, 1), (3, 4))`. Eager sources (`[...].combinations(...)`) were unaffected. Fix: pull `indices[k-1] + 1` before computing pivot. The rightmost index then pivots whenever the source still has elements, matching eager ordering. When pulls fail the buffer stays as-is and the pivot logic correctly falls back to advancing an earlier index. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
timfennis
added a commit
that referenced
this pull request
May 24, 2026
After #144 landed, master had two `bug0021_*` files — `bug0021_chained_vectorized_tuple_arith.ndc` (from the original #140 work, in master longer) and `bug0021_combinations_lazy_source.ndc` (just merged via #144). Renumber the combinations test to bug0022 since chained-vectorized got there first, and bump this branch's incompat-Dynamic test to bug0023. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
CombinationsIter's pivot algorithm gave incorrect results for lazy sources (any iterator that buffers on demand, e.g. the output of.enumerate(),.map(),.filter(), etc.).Reproducer
Eager sources (a list directly) were unaffected. Lazy sources missed combinations whose rightmost index landed in the not-yet-pulled tail.
Cause
CombinationsIterkeeps a growing buffer of pulled elements and a list ofindicesinto that buffer. To produce the next combination it picks the rightmost index that can still advance within the current buffer:For an eager source,
pool_lenis the final size and the check is correct. For a lazy source,pool_lenis "what's been pulled so far". Whenindices[k-1]reaches the end of the current buffer, the algorithm decides "no room to advance the rightmost index" and falls back to advancing an earlier index — even though one more pull would have unlocked it.The existing code did try to pull more, but only in the fallback
pivot = Nonebranch, which only fires when every index is at its current-pool maximum. In our trace above the algorithm picks pivot = 0 before reaching that fallback.Fix
Pull
indices[k - 1] + 1worth of buffer before computing the pivot. If the source still has elements,pool_lengrows and the rightmost index pivots as expected. If the source is exhausted the buffer stays put and the existing pivot logic correctly falls back to an earlier index.Tests
tests/functional/programs/900_bugs/bug0021_combinations_lazy_source.ndccovering the original 4-element case, the larger k = 3 case, and the eager-source baseline.🤖 Generated with Claude Code