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 array::FillError similar to array::IntoIter #75717

Closed
wants to merge 4 commits into from

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Aug 19, 2020

Taken from #69985 without actually implementing FromIterator so we can merge this without having to worry about
stabilizing anything.

While implementing new methods for arrays, this is very often needed and we otherwise need to reimplement this in each new method.

A huge thanks to @lperlaki, who has done most of the work necessary for this PR as part of #69985.

To summarize, this PR adds struct array::FillError<T, const N: usize>, which is intended as part of the return type of FromIterator for arrays, once we implement this (requires at least stable min_const_generics). The current concept is to implement FromIterator for Result<[T; N], FillError<T, N>>, returning a FillError if the iterator had less than N elements.

Similar to IntoIter, we don't yet actually implement FromIterator but instead add a probably permanently unstable new method to FillError, meaning that one can now write FillError::new().fill(iter).unwrap() to collect into an array, which was previously not cleanly possible.

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 19, 2020
@ollie27
Copy link
Member

ollie27 commented Aug 19, 2020

FillError is essentially ArrayVec so it may as well be added with a full Vec like API so it can be used for more than just an error type. It can implement FromIterator directly (panicking if the are too many items) and then when you need an array you can use something like .into_inner().

@lcnr
Copy link
Contributor Author

lcnr commented Aug 19, 2020

Potentially, I am not sure if we want ArrayVec as part of std, it might require an RFC or something 🤔

I wouldn't panic if there aren't too many items but just silently ignore the rest. As Iterator is implemented for &mut impl Iterator this allows us to decide how we want to deal with the remaining elements.

Opened https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/.60ArrayVec.60 on zulip to talk about this

@ollie27
Copy link
Member

ollie27 commented Aug 19, 2020

I wouldn't panic if there aren't too many items but just silently ignore the rest.

I think that would be error prone. If you want to throw away items you can use .take(N) like you would with any other collection. If you want to do anything else you can use .by_ref().take(N). Panicking also has precedence, for example std::iter::repeat(()).collect::<Vec<()>>() will panic, not silently drop excess items.

@lcnr
Copy link
Contributor Author

lcnr commented Aug 20, 2020

std::iter::repeat(()).collect::<Vec<()>>() will panic because the Vec overflows which is most definitely a logic error of the user and IMO not really comparable. Not exhausting an iterator when collecting into an array is however fairly frequently used.

I am not sure how error prone this will be, but as we start by adding this without implementing FromIterator
we can change this later on if it ends up causing errors.

See #69985 (comment) where we previously discussed this.

@kennytm
Copy link
Member

kennytm commented Aug 21, 2020

r? @sfackler

@bors
Copy link
Contributor

bors commented Sep 5, 2020

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

@jyn514 jyn514 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 15, 2020
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-8 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.
Initialized empty Git repository in /home/runner/work/rust/rust/.git/
##[command]git remote add origin https://github.com/rust-lang/rust
##[command]git config gc.auto 0
##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/75717/merge:refs/remotes/pull/75717/merge
---
   Compiling libc v0.2.77
   Compiling std v0.0.0 (/checkout/library/std)
   Compiling compiler_builtins v0.1.35
   Compiling unwind v0.0.0 (/checkout/library/unwind)
error[E0599]: no function or associated item named `slice_get_ref` found for union `mem::maybe_uninit::MaybeUninit<_>` in the current scope
    |
    |
230 |         unsafe { MaybeUninit::slice_get_ref(&self.array[0..self.len]) }
    |                               ^^^^^^^^^^^^^ function or associated item not found in `mem::maybe_uninit::MaybeUninit<_>`
   ::: library/core/src/mem/maybe_uninit.rs:221:1
    |
221 | pub union MaybeUninit<T> {
221 | pub union MaybeUninit<T> {
    | ------------------------ function or associated item `slice_get_ref` not found for this

error[E0599]: no function or associated item named `slice_get_mut` found for union `mem::maybe_uninit::MaybeUninit<_>` in the current scope
    |
    |
236 |         unsafe { MaybeUninit::slice_get_mut(&mut self.array[0..self.len]) }
    |                               ^^^^^^^^^^^^^ function or associated item not found in `mem::maybe_uninit::MaybeUninit<_>`
   ::: library/core/src/mem/maybe_uninit.rs:221:1
    |
221 | pub union MaybeUninit<T> {
221 | pub union MaybeUninit<T> {
    | ------------------------ function or associated item `slice_get_mut` not found for this
error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0599`.
error: could not compile `core`.
---
== clock drift check ==
  local time: Thu Sep 24 08:32:23 UTC 2020
  network time: Thu, 24 Sep 2020 08:32:24 GMT
== end clock drift check ==
##[error]Process completed with exit code 1.
Terminate orphan process: pid (6543) (python)

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 lcnr changed the title add array::FillError similar to array::FromIterator add array::FillError similar to array::IntoIter Sep 24, 2020
@bors
Copy link
Contributor

bors commented Sep 25, 2020

☔ The latest upstream changes (presumably #77172) 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

@SimonSapin
Copy link
Contributor

I think a const-generics-based ArrayVec totally makes sense in the standard library, and I agree with @ollie27 this is pretty much it except for the name.

crates.io has multiple ArrayVec-like libraries that have explored the design space, so I’m not sure an RFC would help a lot here as I don’t expect new ideas to come out of more design discussion.

@c410-f3r
Copy link
Contributor

I made a pre-rfc about having pure stack-based and hybrid stack/heap arrays on std -> https://github.com/c410-f3r/rfcs/blob/mem_structs/text/2946-mem_structs.md.

If these structures are really desired, I can finish the RFC ASAP

@SimonSapin
Copy link
Contributor

For what it’s worth I think the design of SmallVec a.k.a. DynVec is much less obvious. (For example using an actual enum means having padding for the discriminant + field alignment, making the container larger than it really needs to be.) I’d rather not tie it together with ArrayVec.

@lcnr
Copy link
Contributor Author

lcnr commented Sep 26, 2020

At least in my personal view adding something like SmallVec to std is not something we want. In my experience SmallVec is incredibly overused without having the performance tests backing it up, so I don't think they fit into the standard library.

I wrote and saw a few changes where smallvecs made performance worse due to slower lookup.

So yeah, let's add something like ArrayVec instead here we can then recommend iter.collect().into_array()(naming pending) to collect into an array

@lcnr lcnr closed this Sep 26, 2020
@c410-f3r
Copy link
Contributor

c410-f3r commented Sep 26, 2020

@SimonSapin @lcnr Looks good! An ArrayVec-like structure will be!

I don't personally use SmallVec in my projects. The inclusion of such thing in the RFC was mostly due to the unstable untagged_unions feature and for being # 22 in the All-time Downloads section of crates.io.

@lcnr lcnr deleted the add-fill-error branch September 26, 2020 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.