Add Iterator::collect_array method

[matklad]

With const-generics, we can now collect exactly N elements out of
the iterator.

Very similar API existed in the itertools crate for quite some time:

https://docs.rs/itertools/0.9.0/itertools/trait.Itertools.html#method.collect_tuple

This needs more thorough tests before merging (esp around Drop & panic), I'll add them if this API as a whole feels like a good idea.

[rust-highfive]

r? @sfackler

(rust-highfive has picked a reviewer for you, use r? to override)

[matklad]

Do we have this type somewhere already?

[MikailBag]

I guess there is one in [T; N]::map() method

[scottmcm]

Looks like there are more copies in other PRs, like #79451.

Maybe it'd be worth waiting for #75644? Then this might essentially just be [T; N]::try_generate(|_| it.next())...

[matklad]

Seems like we need core::collections::array_vec already :D

[scottmcm]

There's an item for the next libs meeting to talk about that https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/PRs.20need.20a.20decision.20on.20.60try_generate.60.20or.20.60ArrayVec.60.20or.20.2E.2E.2E/near/218779392 :)

[WaffleLapkin]

I'm currently writing Iterator::array_chunks and I have +- the same code :)

(though it's a bit more convoluted since DoubleEndedItetator impl requires an ability to push_back)

[KodrAus]

Unfortunately we rarely get to our needs decision items 🙃

If we keep tabs on these unwind-safe-partially-initialized-array types we're introducing I'd be happy not to block any of these PRs individually on them and run a pass over later to replace them with something more general.

[lcnr]

is there a reason to prefer this over a more general FromIterator impl for arrays?

Do you intend to keep this as unstable until we are comfortable adding the impl as that is impl stable, similar to array::IntoIter::new?

[matklad]

is there a reason to prefer this over a more general FromIterator impl for arrays?

FromIterator can't file, this method returns an Option (and there's also a can-of-worms question of whether it should return a Result).

Do you intend to keep this as unstable until we are comfortable adding the impl as that is impl stable, similar to array::IntoIter::new?

I think I don't understand the question, could you rephrase it? Shooting in the dark, yes, I expect this to be unstable for quite some time, both because it uses const-generics, and because I think the API itself needs to bake in for some times. However, long term we absolutely should have something like this on stable, in one form or another.

[lcnr]

We can add FromIterator for Result<[T; N]; Err> or FromIterator for Option<[T; N]> just fine and have a PR open which implements this: #69985

I think I don't understand the question, could you rephrase it?

My second question was based on my understanding that FromIterator for Option<[T; N]> would be preferable to a separate method. If we do want to add a FromIterator impl here I do not see the use of adding collect_array as well but I still think it would be useful to add an unstable method until FromIterator can be landed, because implementing that trait would be instantly stable.

[matklad]

Thanks for clarifying, I indeed misunderstood the question. I'll continue discussion in that PR to not split it over many threads.

[matklad]

I still think it would be useful to add an unstable method until FromIterator can be landed, because implementing that trait would be instantly stable.

I don't have a string opinion here, but it indeed might help with nailing down the fiddly Error type, for example!

[sfackler]

I don't think there's any prior art for this kind of fallible collection (even the result propagation impls only fail due to an inner Err rather than a size mismatch). The "collect-to-result" impl is already a bit undiscoverable, and I'd worry that a similar impl would be even more awkward to work with since you'd potentially need to explicitly name the error type chosen.

[scottmcm]

pondering: given that this will potentially collect only a prefix, should it be &mut self? The Option is enough to know whether the iterator was exhausted in the call, so one could use .collect_array::<4>() in a loop...

[matklad]

Ah, I think I misunderstood initially... You want to have "exact N" semantics, but you want &mut self at the same time?

This is interesting!

The problem I am seeing here is that, to get exactly N, you'll need to fetch N + 1 elements, so one would be dropped on the floor or stuffed into the Err variant somehow.

[scottmcm]

I was thinking that it.collect_array::<3>() would be equivalent to try { [it.next()?, it.next()?, it.next()?] }. So you'd get None if the length is shorter than the number requested, but if there's more you'd be able to call again to get more of them.

(Maybe with a rename like sfackler mentions -- this is reminding me of as_chunks and array_chunks on slices, so maybe something like that?)

[WaffleLapkin]

@scottmcm for iterators, self and &mut self is the same thing - you can always use .by_ref()

[scottmcm]

@WaffleLapkin Conceptually yes. But in implementation not exactly -- see all these benchmarks for example.

And if this doesn't exhaust the iterator, then it should be &mut self for consistency with other iterator methods that don't necessarily exhaust the iterator (like all and any, which are also &mut self).

[sfackler]

One option to avoid dealing with the "fallible collect" question could be to instead treat this as fn next_n<const N: usize>(&mut self) -> Option<[Self::Item; N]>, to make it more like a variant of next than a variant of collect.

[matklad]

Answering both #79659 (comment) and #79659 (comment): I think we should do what itertool does.

It has (modernizing API) .next_array<const N: usize>(&mut self) -> Option<[T; N]> and .collect_array<const N: usize>(self) -> Option<[T; N]>, where next gets the first N element and fails only for short iterators, while collect checks that there are exactly N elements.

It just seems wrong for .collect to drop excess elements on the floor, it should ensure that the number of elements matches exactly, just like TryFrom<Vec<T>> for [T; N].

To be clear, I am still not sure whether .collect_array or generic .collect is best for collect API, I am talking only about exact N semantcis here.

[matklad]

Urgh, the previous comment actually referes to #69985, as I am confusing the two (didn't have my morning pu'er yet :(). To clarify:

• this PR, Add Iterator::collect_array method #79659, implements "exactly N" semantics
• the Add FromIterator impl for [T; N] #69985 implements "at least N" semantics (unless I am misreading code), and I think this would be confusing.

[CryZe]

Maybe it might make sense to return some kind of Result that lets you decide how you want to deal with the overflowing case, so Result<[T; N], [T; N]> or so with maybe some convenience methods for unwrapping either Ok or Err (maybe as a general method on Result<T, T>, seemingly people want such a method anyway: https://twitter.com/djco/status/1334535545905672192 )

[matklad]

Attempting to summarize discussion here:

• we need both "take the first n elements" and "check that there are exactly n elements" methods.

• the first one should clearly be a fn next_n<const: N: usize>(&mut self) -> ??? dedicated method.

• for the latter, we have to choices:

  • a dedicated collect_array method
  • a collect::<Result<[T; N], _>>() API

  collect would be more composable (Iterator<Item=io::Result<String>> -> io::Result<Result<[String; 2], CollectArrayError>>), but it would also be absolutely undiscoverable.

Implementation wise, we def should wait for core::collections::ArrayVec. We might also need to wait for that for stabilization, as the perfect signature for next_n would be

fn next_n<const N: usize>(&mut self) -> Result<[T; N], ArrayVec<T, N - 1>>;

I suggest the following plan for this PR:

• repurpose it from "exactly N collect" to "at least N next_n"
• land with -> Option<[T; N]> signature with the understanding that we'd revise the return type once ArrayVec work lands
• postpone (close) until ArrayVec or equivalent is avaiable

[scottmcm]

FWIW, I just had a situation where I wanted exactly next_n in a loop -- trying to do a simd version of Sum<f32> following bluss's idea. I tried at least 4 different approaches, and they all optimized poorly even on a copied slice iter -- even a .size_hint().0 check couldn't make multiple .next()s compile without a check per call.

EDIT: for clarity, I did need partial results for this use, so unlike my other comment I wouldn't have been able to use -> Option<[T; N]>.

A thought if we don't want to wait for ArrayVec -- array::IntoIter would work for this.

EDIT2: That version as

fn next_n<const N: usize>(&mut self) -> Result<[T; N], ArrayVec<T, N - 1>>;

sounds amazing, though -- would be exactly right for the simd-sum.

[the8472]

Due to all the similar PRs floating around #81615 should help to centralize design discussion.

[bors]

☔ The latest upstream changes (presumably #81732) made this pull request unmergeable. Please resolve the merge conflicts.

[bstrie]

I suspect that this PR wants to become a thin wrapper around the internal API that landed in #82098 , yes?

[crlf0710]

Triage: what's next steps here?

[matklad]

Urgh, seems like I dropped the ball on this one. I feel that the plan outlined in #79659 (comment) is still valid, but I doubt that I'll pursue it to completion soon. So optimistically closing!

[ebenali]

and... ?

all the little features that I always end up needing and assuming rust supports have closed down PRs...