Add 'core::array::from_fn' and 'core::array::try_from_fn' by c410-f3r · Pull Req...

Copy link

Contributor

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

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

#[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 commented on Aug 17, 2020

r? @withoutboats

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

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

lcnr commented on 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 commented on 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 commented on 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 commented on 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 commented on Aug 24, 2020

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 commented on Aug 26, 2020 •

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

bors commented on Sep 5, 2020

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

camelid commented on Oct 7, 2020

@c410-f3r Could you resolve the merge conflicts?

bors commented 16 days ago

Commit 91ad91e has been approved by yaahc

workingjubilee commented 16 days ago

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-

Manishearth commented 16 days ago

[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 commented 16 days ago

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 commented 15 days ago

lets try this again...

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

bors commented 15 days ago

Commit 85c4a52 has been approved by yaahc

leonardo-m commented 13 days ago •

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 commented 13 days ago

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

leonardo-m commented 13 days ago •

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 commented 13 days ago

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 commented 13 days ago

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