Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
9 changes: 3 additions & 6 deletions ndc_stdlib/src/sequence.rs
Original file line number Diff line number Diff line change
Expand Up @@ -560,8 +560,7 @@ mod inner {

/// Returns the `k` sized combinations of the given sequence `seq` as a lazy iterator of tuples.
#[function(return_type = Iterator<Value>)]
pub fn combinations(seq: SeqValue, k: i64) -> anyhow::Result<Value> {
let k = k as usize;
pub fn combinations(seq: SeqValue, k: usize) -> anyhow::Result<Value> {
let iter = CombinationsIter::new(seq, k)
.ok_or_else(|| anyhow!("combinations requires a sequence"))?;
Ok(Value::iterator(iter.into_shared()))
Expand All @@ -576,8 +575,7 @@ mod inner {

/// Returns the `k` sized permutations of the given sequence `seq` as a list of tuples.
#[function(return_type = Vec<_>)]
pub fn permutations(seq: SeqValue, k: i64) -> anyhow::Result<Value> {
let k = k as usize;
pub fn permutations(seq: SeqValue, k: usize) -> anyhow::Result<Value> {
Ok(Value::list(
seq.try_into_iter()
.ok_or_else(|| anyhow!("permutations requires a sequence"))?
Expand Down Expand Up @@ -738,8 +736,7 @@ mod inner {

/// Return a list that represents the powerset of the elements of `seq` that are exactly `length` long.
#[function(name = "subsequences", return_type = Vec<_>)]
pub fn subsequences_len(seq: SeqValue, length: i64) -> anyhow::Result<Value> {
let length = length as usize;
pub fn subsequences_len(seq: SeqValue, length: usize) -> anyhow::Result<Value> {
Ok(Value::list(
seq.try_into_iter()
.ok_or_else(|| anyhow!("subsequences requires a sequence"))?
Expand Down
15 changes: 13 additions & 2 deletions ndc_vm/src/iterator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,11 @@ pub struct CombinationsIter {
buffer: Vec<Value>,
/// `Some` only for lazy (Shared) sources; set to `None` once exhausted.
source: Option<ValueIter>,
/// Combination size. The `indices` vector is only allocated once the
/// buffer is confirmed to hold at least `k` elements, so an absurdly
/// large `k` (e.g. `i64::MAX`) over a short source yields nothing
/// instead of overflowing the allocator with `(0..k).collect()`.
k: usize,
indices: Vec<usize>,
first: bool,
}
Expand All @@ -413,7 +418,8 @@ impl CombinationsIter {
Some(Self {
buffer,
source,
indices: (0..k).collect(),
k,
indices: Vec::new(),
first: true,
})
}
Expand Down Expand Up @@ -445,7 +451,7 @@ impl CombinationsIter {

impl VmIterator for CombinationsIter {
fn next(&mut self) -> Option<Value> {
let k = self.indices.len();
let k = self.k;

if k == 0 {
return if self.first {
Expand All @@ -461,6 +467,10 @@ impl VmIterator for CombinationsIter {
if !self.ensure_index(k - 1) {
return None;
Comment thread
timfennis marked this conversation as resolved.
}
// The buffer now holds at least `k` elements, so this allocation
// is bounded by data we actually materialised — no capacity
// overflow even when `k` is enormous.
self.indices = (0..k).collect();
} else {
// For lazy sources: pull one more element before choosing the
// pivot. Without this, when `indices[k-1]` reaches the end of
Expand Down Expand Up @@ -510,6 +520,7 @@ impl VmIterator for CombinationsIter {
Some(Rc::new(RefCell::new(Self {
buffer: self.buffer.clone(),
source: source_copy,
k: self.k,
indices: self.indices.clone(),
first: self.first,
})))
Expand Down
13 changes: 13 additions & 0 deletions tests/functional/programs/900_bugs/bug0027_combinations_huge_k.ndc
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// `combinations(seq, k)` built its index vector eagerly with
// `(0..k).collect()`. A huge `k` (e.g. `i64::MAX`) over a short source
// asked the allocator for an `i64::MAX`-element `Vec<usize>` and aborted
// with "capacity overflow". The index vector is now only allocated once
// the buffer is confirmed to hold at least `k` elements, so an
// over-large `k` simply yields no combinations instead of panicking.
assert_eq(combinations([1, 2, 3], 9223372036854775807).list(), []);

// `k` larger than the source length yields nothing (boundary just above n).
assert_eq(combinations([1, 2, 3], 4).list(), []);

// Valid combinations are still produced correctly.
assert_eq([1, 2, 3, 4].combinations(2).list(), [(1, 2), (1, 3), (1, 4), (2, 3), (2, 4), (3, 4)]);
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// `combinations(seq, k)` took `k` as an `i64` and cast it with `k as usize`,
// turning a negative `k` into `usize::MAX` and aborting with "capacity
// overflow" while building the index vector. `k` is now a `usize` argument,
// so a negative value is rejected by argument conversion with a graceful
// error — matching the sibling `windows`/`chunks`/`take` functions.
// expect-error: expected a non-negative integer, but the value was negative
combinations([1, 2, 3], -1);
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// `permutations(seq, k)` cast `k as usize`, silently turning a negative `k`
// into `usize::MAX` and returning an empty list. `k` is now a `usize`
// argument, so a negative value is rejected with a graceful error instead.
// expect-error: expected a non-negative integer, but the value was negative
permutations([1, 2, 3], -1);
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// `subsequences(seq, length)` cast `length as usize`, silently turning a
// negative `length` into `usize::MAX` and returning an empty list. `length`
// is now a `usize` argument, so a negative value is rejected with a graceful
// error instead.
// expect-error: expected a non-negative integer, but the value was negative
subsequences([1, 2, 3], -1);
Loading