Add FromIterator impl for [T; N]

[lperlaki]

This PR adds this impl:

impl<T, const N: usize> FromIterator for [T; N]
where
    [T; N]: LengthAtMost32

I think this in conjunction with #65819 would be really useful when working with arrays.

There was a previews (#29693) PR which got closed due to concerns which I feel can be avoided by using new language features like const generics and MaybeUninit.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[Centril]

On panic, you have a potential for a memory leak here. An OnDrop functionality should be used to drop the hitherto accumulated array to avoid the leak. (Please also add tests to ensure there is no leak, and more tests in general.)

[rust-highfive]

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2020-03-14T13:46:41.2288453Z ========================== Starting Command Output ===========================
2020-03-14T13:46:41.2305149Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/06eab72d-e15f-479c-a549-42fa7565a004.sh
2020-03-14T13:46:41.4384723Z 
2020-03-14T13:46:41.4451449Z ##[section]Finishing: Disable git automatic line ending conversion
2020-03-14T13:46:41.4485845Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/69985/merge to s
2020-03-14T13:46:41.4497525Z Task         : Get sources
2020-03-14T13:46:41.4498092Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-03-14T13:46:41.4498640Z Version      : 1.0.0
2020-03-14T13:46:41.4499060Z Author       : Microsoft
---
2020-03-14T13:46:43.6597986Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-03-14T13:46:43.6758697Z ##[command]git config gc.auto 0
2020-03-14T13:46:43.6803715Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-03-14T13:46:43.6829189Z ##[command]git config --get-all http.proxy
2020-03-14T13:46:43.6930517Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/69985/merge:refs/remotes/pull/69985/merge

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

[lcnr]

Bringing up discussion in #29693 once again, I am strongly in favor of implementing it for
Result<[T; n], ArrayFillError> instead.

pub enum ArrayFillError {
    /// It might be worth it to return an `[MaybeUnit<T>], usize` instead
    TooFew(usize),
    /// there were too many elements, with `T` being the `n+1`th element
    TooMany(T)
}

A imo better approach can be found in this comment: #69985 (comment)

[sfackler]

I don't feel great about introducing FromIterator implementations that can panic.

[lperlaki]

I like the idea of collecting a Result.

With an Error type where non data gets lost.

enum ArrayFillError<T, I: Iterator<Item=T>, const N: usize> {

    TooFew([MaybeUninit<T>; N], usize),

    TooMany([T; N], I)
}

This could maybe implement Iterator/IntoIterator to make it easy to reuse the data.

[lcnr]

I like the idea of collecting a Result.

With an Error type where non data gets lost.

enum ArrayFillError<T, I: Iterator<Item=T>, const N: usize> {

    TooFew([MaybeUninit<T>; N], usize),

    TooMany([T; N], I)
}

This could maybe implement Iterator/IntoIterator to make it easy to reuse the data.

Afaik this would not work, as knowing that there is an n+1th element requires calling next.

So TooMany would have to be TooMany([T; N], T, I)

[Centril]

If we make this fallible, I think Result<[T; N], TooFew> would be good. We can then use TooFew (bikeshed) but avoid directly exposing its fields. We would however allow you to extract some info, e.g. maybe how much we were able to collect successfully and what those elements were (with an API that is safer and less raw than [MaybeUninit<T>; N]).

I don't think it's a good idea to expose TooMany. In fact, it would be counter-productive to .by_ref() use cases where you might want to collect some elements into one container and collect subsequent elements to another.

[Centril]

@lperlaki Run ./x.py fmt locally to make CI happy (I recommend setting up VSCode with format-on-save + rust-analyzer).

[lcnr]

@Centril did you think of something like this: https://gist.github.com/lcnr/5208cc249333c771ae2770dfa351f8f4

I actually like this approach a lot more than my initial idea, even if it ends up being fairly complex (might require an RFC imo).

[Centril]

@lcnr Seems like a clean, flexible, and forward compatible approach. We'll need to keep that error type unstable though to avoid leaking const generics into stable.

[Centril]

@lperlaki Have you had a chance to catch up to the recent discussions?

[bors]

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[amosonn]

What about having the impl return Result<([T; n], I), TooFew>? This avoids calling next one additional time, and might make some usage patterns easier:

let x: [u64; 17], _ = (0..2).cycle().into()?

Edit: or even

let it = (0..5).cycle();
let x: [u64; 17], it = it.into()?;
let y: [u64; 13], _ = it.into()?;

[matklad]

Hey, I've accidently submitted an alternative in #79659, but let's centralize the discussion here. The curx of #79659 is adding a .collect_array::<{SIZE}> dedicated method, rather than leveraging FromIter.

I have my doubts where #69985 or #79659 PR are better solutions long term... I feel that #69985 might be more awkward to use in practice, as .collect is more generic than .collect_array, and so you might need more type information to make type inference see what you mean. Specifically, I think something like let [a, b, c] = iter.collect().unwrap() wouldn't work without ::<>, while let [a, b, c] = iter.collect_array().unwrap() works.

There's also discoverability concerns here, as collect_array can have dedicated doc page.

But I guess there are additional compositional benefits to the collect-style API? Is it correct that, if you have an Iterator<Item = io::Result<String>>, you can .collect::<io::Result<Result<[String; 3], _>>>() ?

[jyn514]

@matklad it would collect to io::Result<[String; 3]> I think.

[lcnr]

you could use either collect::<Result<[io::Result<String>; 3], FillErr>>() or collect::<io::Result<Result<[String; 3; FillErr]>>>

The second one using the impl for `Result<V, E> https://doc.rust-lang.org/nightly/std/iter/trait.FromIterator.html#impl-FromIterator%3CResult%3CA%2C%20E%3E%3E

[the8472]

Is there a meta-issue for all the different const-generic methods being implemented on arrays where the different alternative approaches could be discussed? I have looked around but couldn't find any.

[crlf0710]

Triage: What's the current status here?

[bstrie]

For anyone following this topic, I've filed #81615 to centralize design discussion.

[bstrie]

An internal helper function for converting iterators into arrays has been added in #82098, which I believe could greatly simplify the code here.

[crlf0710]

@bstrie Ping from triage! #82098 is now merged, what's next steps here?

[bstrie]

@crlf0710 The various PRs in this area are blocked on API design. See #81615 for the metabug.

[afetisov]

I don't feel good about a panic-happy FromIterator implementation. I suggest closing this issue, since the nightly array_map feature allows to to the same thing, but safely (even if slightly less ergonomically):

#![feature(array_map)]

fn f<T, It: Iterator<Item=T>, const N: usize>(mut it: It) -> [T; N] {
    [(); N].map(|_| it.next().unwrap())
}

fn main() {
    for v in [
        vec![true, false, true],
        vec![true, false, true, false],
        vec![true,],
    ] {
        println!("{:?}", f::<_, _, 3>(v.into_iter()));
    }
}

Output:

[true, false, true]
[true, false, true]

thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src/main.rs:4:31
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[leonardo-m]

(even if slightly less ergonomically):

Note that ergonomy what the whole point here.