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 'core::array::from_fn' and 'core::array::try_from_fn' #75644

Merged
merged 7 commits into from
Oct 9, 2021

Conversation

c410-f3r
Copy link
Contributor

@c410-f3r c410-f3r commented Aug 17, 2020

These auxiliary methods fill uninitialized arrays in a safe way and are particularly useful for elements that don't implement Clone, Copy or Default.

// Foo doesn't implement Default
struct Foo(usize);

let _array = core::array::from_fn::<_, _, 2>(|idx| Foo(idx));

Different from FromIterator, it is guaranteed that the array will be fully filled and no error regarding uninitialized state will be throw. In certain scenarios, however, the creation of an element can fail and that is why the try_from_fn function is also provided.

#[derive(Debug, PartialEq)]
enum SomeError {
    Foo,
}

let array = core::array::try_from_fn(|i| Ok::<_, SomeError>(i));
assert_eq!(array, Ok([0, 1, 2, 3, 4]));

let another_array = core::array::try_from_fn(|_| Err(SomeError::Foo));
assert_eq!(another_array, Err(SomeError::Foo));

@rust-highfive
Copy link
Collaborator

r? @withoutboats

(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 17, 2020
@leonardo-m
Copy link

I use similar functions since some months. I'm glad to see some love for arrays in stdlib, once we have some const generics :-)

@c410-f3r c410-f3r changed the title Add create_array and create_array_rslt Add build_array and try_build_array Aug 18, 2020
library/core/src/array/mod.rs Outdated Show resolved Hide resolved
library/core/src/array/mod.rs Show resolved Hide resolved
library/core/tests/array.rs Outdated Show resolved Hide resolved
@lcnr
Copy link
Contributor

lcnr commented Aug 20, 2020

I personally do not think that this method has enough advantages over FromIterator for Result<[T; N], _> to be included.

build_array will be identical to let arr: [T; N] = (0..).map(f).collect().unwrap() while try_build_array will be identical to let arr: [T; N] = (0..).map(f).collect()?.unwrap(), which might need type annotations.

Considering that build_array doesn't compose too well with other iterators I think it might end up as a lesser known alternative to using FromIterator. I would prefer FromIterator to be the recommended way to build arrays from iterators.

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Aug 20, 2020

These functions should not be seen as FromIterator competitors or substitutes because they solve a different type of problem: The problem of safely filling uninitialized arrays with accuracy without panicking at run-time.

Take, for example, let arr: [i32; 10] = (0..2).collect::<Result<_, _>>().unwrap(). It is easy to lint that this operation will fail and the user should adjust [i32; 10] to [i32; 2] but generally it isn't the case. The Iterator might come from a network data slice of variable length or the Iterator itself was badly implemented and isn't reliable.

With the above reasoning:

  1. If the Iterator has fewer elements, let arr: [i32; 10] = unknown_iterator_source.collect::<Result<_, _>>().unwrap() will panic at run-time.
  2. If the Iterator has more elements, let arr: [i32; 10] = unknown_iterator_source.collect::<Result<_, _>>().unwrap() won't panic but this situation will lead to inaccuracy because of the discarded elements.

It is also worth mentioning that the try_build_array error is a user provided element that isn't equal to FillError although one could map_err for convergence.

Clippy

Another use-case is the expect_used and unwrap_used lints. Even when it is known that an operation will succeed, #[forbid] won't permit expect or unwrap.

#![forbid(clippy::expect, clippy::unwrap)]

fn main() {
  let _arr: [i32; 10] = (0..).map(f).collect().unwrap(); // Error!
}

#[deny] could be used instead but it probably would lead to a titanic amount of #[allow] exceptions across the code-base.

Like I said, these functions should be seen as complementary companions to FromIterator instead of competitors. The rich Iterator API has its own advantages and solve a lot of problems but not the ones listed here.

@lcnr
Copy link
Contributor

lcnr commented Aug 20, 2020

Take, for example, let arr: [usize; 10] = (0..2).collect::<Result<_, _>>().unwrap(). It is easy to lint that this operation will fail and the user should adjust [usize; 10] to [usize; 2] but generally it isn't the case. The Iterator might come from a network data slice of variable length or the Iterator itself was badly implemented and isn't reliable.

But build_array doesn't help here either, does it?

It is also worth mentioning that the try_build_array error is a user provided element that isn't equal to FillError although one could map_err for convergence.

Yeah, this is why I used ?.unwrap() in my example, so the resulting type would be Result<Result<[T; N]; ?>, E> for a fallible iterator.

With the above reasoning: ...

This is true, but I do not see how build_array helps here as build array requires infinite iterators, so you can't use it in these cases.

Another use-case is the expect_used and unwrap_used lints. Even when it is known that an operation will succeed, #[forbid] won't permit expect or unwrap.

That one is a downside I did not consider. While I personally do think these lints are rarely appropriate, there are situations where not using any unwraps is a good thing.


In general build_array pretty much only works with a subset of iterators with infinite size so imo it isn't the right tool to fix the problems mentioned.

I can think of 2 solutions which I personally prefer:

  • use something like the following method if you can't deal with unwrap in your codebase, i.e. for embedded or safety stuff:
trait UnwrapExt {
    type Inner;
    fn guaranteed_unwrap(self) -> Self::Inner;
}

impl<T, E> UnwrapExt for Result<T, E> {
    type Inner = T;
    fn guaranteed_unwrap(self) -> T {
        extern "C" {
            fn failed_to_optimize_unwrap_branch_check();
        }
        // This causes a linker error if the unwrap isn't optimized away, should probably 
        // only be used if your crate does not have any dependencies.
        self.unwrap_or_else(|_| failed_to_optimize_unwrap_branch_check())
    }
}
  • add trait InfiniteIterator: Iterator { fn next(&mut self) -> <Self as Iterator>::Item } and use that to implement FromIterator<I> for [T; N]. This does allows for all infinite iterators, which should be a superset of what build_array allows.

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Aug 20, 2020

As previously commented, these functions deal with a different type of problem and shouldn't interact with the Iterator API because they are meant for safe state initialization with accuracy without panicking at run-time, thus the callbacks in the function signature.

If someone wants to use iterators to create arrays, then core::iter::FromIterator is indeed a great tool but it simply can't provide the already listed guarantees.

UnwrapExt::guaranteed_unwrap seems tricky and trait InfiniteIterator: Iterator looks promising. Not sure how to convince ppl which one should be preferable though

@scottmcm
Copy link
Member

A few thoughts here:

  • How do we pick between array::build(...) and [T; N]::build(...) here?
  • array::build_array(...) seems to go against the "don't repeat the module name" rule.
  • build says "builder pattern" to me, so I'm not sure it's the best name. Personally I called it generate.

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Aug 26, 2020

+1 for [T; N]::generate and/or [T; N]::try_generate

@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 T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 15, 2020
@camelid
Copy link
Member

camelid commented Oct 7, 2020

@c410-f3r Could you resolve the merge conflicts?

@c410-f3r c410-f3r changed the title Add build_array and try_build_array Add [T; N]::generate and [T; N]::try_generate Oct 7, 2020
@c410-f3r c410-f3r force-pushed the array branch 2 times, most recently from 6ff44b8 to 0acad64 Compare October 7, 2020 21:21
@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 8, 2021
@Manishearth
Copy link
Member

[RUSTC-TIMING] corebenches test:true 0.085
error: function is never used: `catch_unwind_silent`
   --> library/core/tests/array.rs:428:4
    |
428 | fn catch_unwind_silent<F, R>(f: F) -> std::thread::Result<R>
    |
    |
    = note: `-D dead-code` implied by `-D warnings`
[RUSTC-TIMING] coretests test:true 14.903
error: could not compile `core` due to previous error

@bors r-

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Oct 8, 2021

In the name of Hades! ...this commit breaks the build when checked against all targets. #89656

[RUSTC-TIMING] corebenches test:true 0.085
error: function is never used: `catch_unwind_silent`
   --> library/core/tests/array.rs:428:4
    |
428 | fn catch_unwind_silent<F, R>(f: F) -> std::thread::Result<R>
    |
    |
    = note: `-D dead-code` implied by `-D warnings`
[RUSTC-TIMING] coretests test:true 14.903
error: could not compile `core` due to previous error

Code used only in a #[cfg] must be omitted outside that #[cfg] condition to prevent the dead code warning, often by applying the #[cfg] to the function itself.

@bors r-

Thanks. An unfortunate oversight

@yaahc
Copy link
Member

yaahc commented Oct 8, 2021

lets try this again...

In the name of Hades I approve this commit!
@bors r+

@bors
Copy link
Contributor

bors commented Oct 8, 2021

📌 Commit 85c4a52 has been approved by yaahc

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 8, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Oct 9, 2021
Add 'core::array::from_fn' and 'core::array::try_from_fn'

These auxiliary methods fill uninitialized arrays in a safe way and are particularly useful for elements that don't implement `Default`.

```rust
// Foo doesn't implement Default
struct Foo(usize);

let _array = core::array::from_fn::<_, _, 2>(|idx| Foo(idx));
```

Different from `FromIterator`, it is guaranteed that the array will be fully filled and no error regarding uninitialized state will be throw. In certain scenarios, however, the creation of an **element** can fail and that is why the `try_from_fn` function is also provided.

```rust
#[derive(Debug, PartialEq)]
enum SomeError {
    Foo,
}

let array = core::array::try_from_fn(|i| Ok::<_, SomeError>(i));
assert_eq!(array, Ok([0, 1, 2, 3, 4]));

let another_array = core::array::try_from_fn(|_| Err(SomeError::Foo));
assert_eq!(another_array, Err(SomeError::Foo));
 ```
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 9, 2021
…laumeGomez

Rollup of 6 pull requests

Successful merges:

 - rust-lang#75644 (Add 'core::array::from_fn' and 'core::array::try_from_fn')
 - rust-lang#87528 (stack overflow handler specific openbsd change.)
 - rust-lang#88436 (std: Stabilize command_access)
 - rust-lang#89614 (Update to Unicode 14.0)
 - rust-lang#89664 (Add documentation to boxed conversions)
 - rust-lang#89700 (Fix invalid HTML generation for higher bounds)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 86bf3ce into rust-lang:master Oct 9, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 9, 2021
@leonardo-m
Copy link

leonardo-m commented Oct 10, 2021

I've tried to use this from_fn, but its usefulness is limited by the index that's always usize (and when possible I prefer to avoid type casts). Is it possible and a good idea to make the index type (input of F) generic (defaulting to usize)?

@jhpratt
Copy link
Member

jhpratt commented Oct 11, 2021

@leonardo-m Like it or not, the index type is always usize. Type casts are a fact of life when it comes to indexing.

@scottmcm
Copy link
Member

@leonardo-m Note that default type parameters on functions aren't a thing, so I don't think it could even work.

@leonardo-m
Copy link

leonardo-m commented Oct 11, 2021

jhpratt in my codebase I use various ways to reduce the frequency of casting for indexes. It isn't a fact of life like Death or Taxes.

I've seen that a from_fn like this is usable in more places in my codebase:

pub fn from_fn<Ti, To, F, const N: usize>(mut cb: F) -> [To; N]
where
    Ti: ZeroSucc + Copy,
    F: FnMut(Ti) -> To,
{
    let mut idx = Ti::ZERO;

    [(); N].map(|_| {
        let res = cb(idx);
        idx.succ();
        res
    })
}

The disadvantage is some kind of trait for Zero and successive, and in a subset of from_fn usages you need to specify the type: from_fn(|i: u8| ...). In my code I've seen that adding a type to the index in about 50% of the cases isn't too much of a hassle. And the need to write from_fn::<_, _, _, N>( is very uncommon (because in most cases you can assign it like this: let a: [_; N] = from_fn).

(Another factor that reduces the usefulness of from_fn is that it isn't const. This is because array::map isn't const, and I guess that boils down to as_mut_ptr() not being const).

@c410-f3r
Copy link
Contributor Author

Supposing that each iteration increases the counter, an u8 would overflow in a standard x86_64 PC where N > 255 and (try_)from_fn_(checked|saturating|wrapping) variants don't seem worth it.

@jplatte
Copy link
Contributor

jplatte commented Oct 11, 2021

I think a better place for discussing this than this merged PR would be the tracking issue: #89379

bors added a commit to rust-lang-ci/rust that referenced this pull request May 22, 2022
…ttmcm

Stabilize `array_from_fn`

## Overall

Stabilizes `core::array::from_fn` ~~and `core::array::try_from_fn`~~ to allow the creation of custom infallible ~~and fallible~~ arrays.

Signature proposed for stabilization here, tweaked as requested in the meeting:

```rust
// in core::array

pub fn from_fn<T, const N: usize, F>(_: F) -> [T; N];
```

Examples in https://doc.rust-lang.org/nightly/std/array/fn.from_fn.html

## History

* On 2020-08-17, implementation was [proposed](rust-lang#75644).
* On 2021-09-29, tracking issue was [created](rust-lang#89379).
* On 2021-10-09, the proposed implementation was [merged](bc8ad24).
* On 2021-12-03, the return type of `try_from_fn` was [changed](rust-lang#91286 (comment)).

## Considerations

* It is being assumed that indices are useful and shouldn't be removed from the callbacks
* The fact that `try_from_fn` returns an unstable type `R: Try` does not prevent stabilization. Although I'm honestly not sure about it.
* The addition or not of repeat-like variants is orthogonal to this PR.

These considerations are not ways of saying what is better or what is worse. In reality, they are an attempt to move things forward, anything really.

cc rust-lang#89379
workingjubilee pushed a commit to tcdi/postgrestd that referenced this pull request Sep 15, 2022
Stabilize `array_from_fn`

## Overall

Stabilizes `core::array::from_fn` ~~and `core::array::try_from_fn`~~ to allow the creation of custom infallible ~~and fallible~~ arrays.

Signature proposed for stabilization here, tweaked as requested in the meeting:

```rust
// in core::array

pub fn from_fn<T, const N: usize, F>(_: F) -> [T; N];
```

Examples in https://doc.rust-lang.org/nightly/std/array/fn.from_fn.html

## History

* On 2020-08-17, implementation was [proposed](rust-lang/rust#75644).
* On 2021-09-29, tracking issue was [created](rust-lang/rust#89379).
* On 2021-10-09, the proposed implementation was [merged](rust-lang-ci/rust@bc8ad24).
* On 2021-12-03, the return type of `try_from_fn` was [changed](rust-lang/rust#91286 (comment)).

## Considerations

* It is being assumed that indices are useful and shouldn't be removed from the callbacks
* The fact that `try_from_fn` returns an unstable type `R: Try` does not prevent stabilization. Although I'm honestly not sure about it.
* The addition or not of repeat-like variants is orthogonal to this PR.

These considerations are not ways of saying what is better or what is worse. In reality, they are an attempt to move things forward, anything really.

cc rust-lang/rust#89379
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.