`from_iter` implementation may run indefinitely

[ThomasdenH]

Creating a vector or a matrix with the from_iter method may run indefinitely. I don't think this is the expected behaviour. I.e.

#[test]
fn test_nalgebra_finishes() {
    use nalgebra::DVector; // 0.32.5
    use std::iter;
    // Runs indefinitely
    let _vector = DVector::from_iterator(70, iter::from_fn(|| Some(1)));
}

will never finish. (Playground)

I think it would make sense to either:

1. Limit the iterator automatically and ignore the remaining entries.
2. Panic if the iterator has elements remaining if the capacity has been reached.

[Andlon]

Good catch. We should probably make it panic, since it's likely a mistake by the user if it doesn't fit, and if it's not the user can simply call .take(n) on their iterator before providing it to nalgebra.

[Andlon]

After looking at the PR (which looks great btw!) I'm a little bit uncertain about my previous statement. It might break some user's code (though should be easy to fix ...), and I'm unsure of how e.g. the from_iterator methods are used in the wild. I'm still leaning towards panicking, but I think it would be good to have @sebcrozet's input on it before making a final decision on the desired behavior.

[ThomasdenH]

No problem!

FWIW, simply cutting off the iterator also seems like a reasonable option. This would enable things like

DVector::from_iterator(70, 0..);

Notably, the current behaviour is inconsistent at the moment: for static matrices, additional entries will be ignored, while for dynamic matrices the code will either panic or run indefinitely.

[Andlon]

Yeah, in any case we should fix the problem of iterators running indefinitely, and make the behavior consistent :+1:

[sebcrozet]

Tough call. I didn’t find any occurrence of from_iter in the standard library that would help us pick an "idiomatic" solution here. There is the (slightly different) case of Option::from_iter which does cut off the iterator with Item = Option<T> as soon as it yields a None value.

In a way I prefer the option where we cut off the iterator instead of panicking, since it would avoid breaking the existing behavior for dynamic matrices (so we won’t start panicking existing code that relied on it), and it allows a couple of nice patterns like:

DVector::from_iterator(70, 0..);

as suggested by @ThomasdenH. Or even:

let mut my_iterator = ...;
let v1 = Vector3::from_iterator(&mut my_iterator); // Will get elements [0, 3) of `my_iterator`
let v2 = Vector3::from_iterator(&mut my_iterator); // Will get elements [3, 6)
let v3 = Vector3::from_iterator(&mut my_iterator); // And elements [6, 9)

[Andlon]

Thanks for your input @sebcrozet. I agree with that for the vector case, but the matrix case is what's troubling me. I can't imagine a single case where I'd legitimately have an iterator with a different number of elements than the number of entries in a matrix. This may be a fault of my imagination, however.

In any case, I'm fine with keeping the current behavior and just cut the iterator off.

[ThomasdenH]

What about from_vec and from_slice? These currently have the same behaviour. I think supplying a vector that is too large here is less likely to be intentional. That being said, this test uses a slice that is too long.

[ThomasdenH]

In a way I prefer the option where we cut off the iterator instead of panicking, since it would avoid breaking the existing behavior for dynamic matrices [...]

This is not quite true, the current behaviour is

• Vec, Iterator or slice to Const storage:
  • If too small, panic
  • If too large, ignore remaining entries
• Vec, Iterator or slice to Dyn storage:
  • If too small, panic
  • If too large and finite, panic
  • If too large and infinite, loop indefinitely