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::repeat for making an array from a non-Copy non-const value #310

Closed
scottmcm opened this issue Dec 5, 2023 · 10 comments
Closed
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@scottmcm
Copy link
Member

scottmcm commented Dec 5, 2023

Proposal

Problem statement

For a type that's copy, [x; N] works great for getting an array.
For a constructor that's const, [const { Foo::new() }; N] works great (or hopefully will soon) for getting an array.

However, if you have a general String value that you want to repeat, there's no great way right now.

We should add one.

Motivating examples or use cases

Like how vec![x; N] supports non-Copy values, we should have a way to do that for normal arrays too.

See zulip thread https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/syntactical.20parallelism.20for.20multiple.20clones/near/403589163

As an interesting bonus, this would also let array::repeat(x) work inferring the length in many cases, which could actually be a nice thing to use even with Copy types, until [x; _] (or however we spell that) ends up happening.

Solution sketch

This is trivial to implement in core with the existing internal methods:

// in core::array

pub fn repeat<T: Clone, const N: usize>(x: T) -> [T; N] {
    from_trusted_iterator(iter::repeat_n(x, N))
}

Alternatives

The simplest option is array::from_fn(|_| x.clone()), but that's sub-optimal in that it never re-uses the original value, likely losing capacity -- since the whole reason to not be using [x; N] in the first place is that the type isn't Copy, and thus plausibly has a non-trivial Drop.

Fixing that takes something like this (from Kevin Reid):

fn dup<T: Clone, const N: usize>(v: T) -> [T; N] {
    let mut buf = Some(v);
    core::array::from_fn(|i| if i == N - 1 { buf.take() } else { buf.clone() }.unwrap())
}

But that's far more complicated, and doesn't necessarily optimize well either. It could be done with unsafe, but once that's happening it's probably good to have it in core again to encapsulate the unsafe in a known-safe interface.

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

  • We think this problem seems worth solving, and the standard library might be the right place to solve it.
  • We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

  • We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
  • We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.
@scottmcm scottmcm added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Dec 5, 2023
@scottmcm
Copy link
Member Author

scottmcm commented Dec 5, 2023

As I was typing this out, @the8472 pointed out that some types -- notably Arc -- could do something faster here if they know it's happening.

So another alternative here might be something like

trait Clone {
    // ... existing methods ...
    fn multi_clone<const N: usize>(&self) -> [Self; N] where Self: Sized { array::from_fn(|_| self.clone()) }
    fn multi_clone_consuming<const N: usize>(self) -> [Self; N] where Self: Sized { array::from_trusted_iterator(iter::repeat_n(self, N)) }
}

Which could be overridden for the few types that care.

@ChayimFriedman2
Copy link

Perhaps instead of adding more and more iterator-like function to arrays, we should just have a collect_array() method on iterators?

@scottmcm
Copy link
Member Author

scottmcm commented Dec 6, 2023

Something very much like that exists in nightly already, @ChayimFriedman2 -- https://doc.rust-lang.org/nightly/std/iter/trait.Iterator.html#method.next_chunk

The problem is that even if that's stable, it's fallible because there's no type-based minimum length on iterators -- and there can't be because next exists -- other then "it's infinite". And while iter::repeat could be InfiniteIterator, repeat(x).next_chunk_infallible() doesn't give the "keep the capacity from x" guarantee.

So even if I could write iter::repeat_n(x, N).next_chunk::<_, N>().unwrap(), because everything in there was already stable, I'd still want array::repeat(x) because it's convenient.

@Amanieu
Copy link
Member

Amanieu commented Dec 19, 2023

It seems like there are 2 separate motivations here, which each have different optimal solutions.

If the primary concern is the syntax, then array::repeat is a good solution: it's simple an easy to use.

If the primary concern is performance due to multiple refcount increments, then a more general solution would be a way to increment an Arc multiple times at once and then return the new Arcs in an iterator:

impl Arc<T> {
    fn increment_multiple(self: Self, n: usize) -> impl Iterator<Item = Arc<T>>;
}

@scottmcm
Copy link
Member Author

My primary intent here is that there's no safe-and-efficient way to do [x; N] for non-Copy things today. I think we should have an obvious way to point to.

If it does other things too, that's great, but that wasn't my goal with this ACP.

@scottmcm
Copy link
Member Author

cc rust-lang/rust#119530 (comment), for another motivation example

@scottmcm
Copy link
Member Author

I was reminded of this again today in Discord: https://discord.com/channels/273534239310479360/1120175689124036669/1219021342003691520

image

Being able to say array::repeat(foo) as the answer would be great there, and if that doesn't address the Arc cases that's fine.

@the8472
Copy link
Member

the8472 commented Mar 17, 2024

Hrm, that reminds me of the java feature where the method-reference syntax can create instance-bound ~closures.

I.e. one would just write repeat(clonable) as from_fn(clonable::clone)

@scottmcm
Copy link
Member Author

scottmcm commented Jun 12, 2024

I responded to the above question (#310 (comment)), so nominating to get eyes on it again
@rustbot label +i-libs-api-nominated

@rustbot rustbot added the I-libs-api-nominated Indicates that an issue has been nominated for discussion during a team meeting. label Jun 12, 2024
@Amanieu
Copy link
Member

Amanieu commented Jun 18, 2024

We discussed this in the @rust-lang/libs-api meeting and are happy to accept it!

@Amanieu Amanieu closed this as completed Jun 18, 2024
@Amanieu Amanieu added ACP-accepted API Change Proposal is accepted (seconded with no objections) and removed I-libs-api-nominated Indicates that an issue has been nominated for discussion during a team meeting. labels Jun 18, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 19, 2024
Implement `array::repeat`

See rust-lang/libs-team#310.

I've decided to make the function use the input value as last element instead of cloning it to every position and dropping it, and to make this part of the API so that callers are not surprised by this behaviour.

TODO: open a tracking issue. I'll wait for the ACP to be accepted, first.

`@rustbot` label +T-libs-api +T-libs
r? libs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

5 participants