Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add FromIterator impl for [T; N] #69985

Closed

Conversation

lperlaki
Copy link

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
Copy link
Collaborator

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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 13, 2020
@jonas-schievink jonas-schievink added F-const_generics `#![feature(const_generics)]` needs-fcp This change is insta-stable, so needs a completed FCP to proceed. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Mar 13, 2020
@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@lperlaki lperlaki force-pushed the add-from-iterator-for-arrays branch from 0a92e53 to 74cd4c4 Compare March 13, 2020 19:26
@rust-highfive

This comment has been minimized.

@lperlaki lperlaki force-pushed the add-from-iterator-for-arrays branch from 74cd4c4 to 1235f16 Compare March 13, 2020 20:35
@rust-highfive

This comment has been minimized.

@lperlaki lperlaki force-pushed the add-from-iterator-for-arrays branch from 1235f16 to eb0efb2 Compare March 13, 2020 20:47
@rust-highfive

This comment has been minimized.

@lperlaki lperlaki force-pushed the add-from-iterator-for-arrays branch from eb0efb2 to 70b3e7c Compare March 13, 2020 21:01
@rust-highfive

This comment has been minimized.

src/libcore/array/mod.rs Outdated Show resolved Hide resolved
let mut array: [MaybeUninit<T>; N] = MaybeUninit::uninit_array();

for p in array.iter_mut() {
p.write(iter.next().expect("Iterator is to short"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

src/libcore/array/mod.rs Outdated Show resolved Hide resolved
src/libcore/array/mod.rs Outdated Show resolved Hide resolved
@rust-highfive

This comment has been minimized.

@rust-highfive
Copy link
Collaborator

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
Copy link
Contributor

lcnr commented Mar 14, 2020

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
Copy link
Member

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

@lperlaki
Copy link
Author

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
Copy link
Contributor

lcnr commented Mar 14, 2020

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
Copy link
Contributor

Centril commented Mar 14, 2020

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
Copy link
Contributor

Centril commented Mar 14, 2020

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

src/libcore/array/mod.rs Outdated Show resolved Hide resolved
@lcnr
Copy link
Contributor

lcnr commented Mar 14, 2020

@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
Copy link
Contributor

Centril commented Mar 14, 2020

@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
Copy link
Contributor

Centril commented Mar 30, 2020

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

@bors
Copy link
Contributor

bors commented Nov 17, 2020

☔ 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
Copy link
Contributor

amosonn commented Nov 28, 2020

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
Copy link
Member

matklad commented Dec 3, 2020

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
Copy link
Member

jyn514 commented Dec 3, 2020

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

@lcnr
Copy link
Contributor

lcnr commented Dec 3, 2020

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
Copy link
Member

the8472 commented Dec 17, 2020

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
Copy link
Member

Triage: What's the current status here?

@bstrie
Copy link
Contributor

bstrie commented Feb 1, 2021

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

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 20, 2021
…ray, r=dtolnay

Add internal `collect_into_array[_unchecked]` to remove duplicate code

Unlike the similar PRs  rust-lang#69985, rust-lang#75644 and rust-lang#79659, this PR only adds private functions and does not propose any new public API. The change is just for the purpose of avoiding duplicate code.

Many array methods already contained the same kind of code and there are still many array related methods to come (e.g. `Iterator::{chunks, map_windows, next_n, ...}`, `[T; N]::{cloned, copied, ...}`, ...) which all basically need this functionality. Writing custom `unsafe` code for each of those doesn't seem like a good idea. I added two functions in this PR (and not just the `unsafe` version) because I already know that I need the `Option`-returning version for `Iterator::map_windows`.

This is closely related to rust-lang#81615. I think that all options listed in that issue can be implemented using the function added in this PR. The only instance where `collect_array_into` might not be general enough is when the caller want to handle incomplete arrays manually. Currently, if `iter` yields fewer than `N` items, `None` is returned and the already yielded items are dropped. But as this is just a private function, it can be made more general in future PRs.

And while this was not the goal, this seems to lead to better assembly for `array::map`: https://rust.godbolt.org/z/75qKTa (CC `@JulianKnodt)`

Let me know what you think :)

CC `@matklad` `@bstrie`
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Feb 22, 2021
…ray, r=dtolnay

Add internal `collect_into_array[_unchecked]` to remove duplicate code

Unlike the similar PRs  rust-lang#69985, rust-lang#75644 and rust-lang#79659, this PR only adds private functions and does not propose any new public API. The change is just for the purpose of avoiding duplicate code.

Many array methods already contained the same kind of code and there are still many array related methods to come (e.g. `Iterator::{chunks, map_windows, next_n, ...}`, `[T; N]::{cloned, copied, ...}`, ...) which all basically need this functionality. Writing custom `unsafe` code for each of those doesn't seem like a good idea. I added two functions in this PR (and not just the `unsafe` version) because I already know that I need the `Option`-returning version for `Iterator::map_windows`.

This is closely related to rust-lang#81615. I think that all options listed in that issue can be implemented using the function added in this PR. The only instance where `collect_array_into` might not be general enough is when the caller want to handle incomplete arrays manually. Currently, if `iter` yields fewer than `N` items, `None` is returned and the already yielded items are dropped. But as this is just a private function, it can be made more general in future PRs.

And while this was not the goal, this seems to lead to better assembly for `array::map`: https://rust.godbolt.org/z/75qKTa (CC ``@JulianKnodt)``

Let me know what you think :)

CC ``@matklad`` ``@bstrie``
@bstrie
Copy link
Contributor

bstrie commented Feb 22, 2021

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

@crlf0710
Copy link
Member

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

@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 12, 2021
@bstrie
Copy link
Contributor

bstrie commented Mar 15, 2021

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

@crlf0710 crlf0710 added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 2, 2021
@afetisov
Copy link

afetisov commented Jul 7, 2021

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

@lperlaki lperlaki closed this Aug 27, 2021
@leonardo-m
Copy link

(even if slightly less ergonomically):

Note that ergonomy what the whole point here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-const_generics `#![feature(const_generics)]` needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-blocked Status: Blocked on something else such as an RFC or other implementation work. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.