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

Tracking Issue for array_methods #76118

Closed
5 tasks
Tracked by #7
LukasKalbertodt opened this issue Aug 30, 2020 · 25 comments · Fixed by #103522
Closed
5 tasks
Tracked by #7

Tracking Issue for array_methods #76118

LukasKalbertodt opened this issue Aug 30, 2020 · 25 comments · Fixed by #103522
Labels
A-array Area: `[T; N]` A-slice Area: `[T]` B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@LukasKalbertodt
Copy link
Member

LukasKalbertodt commented Aug 30, 2020

This is a tracking issue for a couple of basic methods on arrays. The feature gate for the issue is #![feature(array_methods)]. Currently, this includes the following methods:

Note: these methods were all collected under the same array_methods feature name as I suspect many (often small) methods to be added to array in the near future. Creating a new feature and tracking issue for each seems overkill. We can still split off some methods into dedicated features later, if required.

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also uses as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Unresolved Questions

@LukasKalbertodt LukasKalbertodt added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Aug 30, 2020
@LukasKalbertodt
Copy link
Member Author

LukasKalbertodt commented Aug 30, 2020

Just for the protocol, I plan to add the following methods in the near future:

(discussions about the usefulness of those method should be had in the dedicated PR threads and not here)

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 3, 2020
…-to-array, r=Mark-Simulacrum

Add `[T; N]::as_[mut_]slice`

Part of me trying to populate arrays with a couple of basic useful methods, like slices already have. The ability to add methods to arrays were added in rust-lang#75212.  Tracking issue: rust-lang#76118

This adds:

```rust
impl<T, const N: usize> [T; N] {
    pub fn as_slice(&self) -> &[T];
    pub fn as_mut_slice(&mut self) -> &mut [T];
}
```

These methods are like the ones on `std::array::FixedSizeArray` and in the crate `arraytools`.
@KodrAus KodrAus added the Libs-Tracked Libs issues that are tracked on the team's project board. label Sep 10, 2020
@KodrAus KodrAus added the A-slice Area: `[T]` label Sep 22, 2020
@usbalbin
Copy link
Contributor

@LukasKalbertodt I have opened PR #79451 for zip, saw your comment just now.

bors added a commit to rust-lang-ci/rust that referenced this issue Jan 11, 2021
…ds, r=dtolnay

Add `[T; N]::each_ref` and `[T; N]::each_mut`

This PR adds the methods `each_ref` and `each_mut` to `[T; N]`. The ability to add methods to arrays was added in rust-lang#75212. These two methods are particularly useful with `map` which was also added in that PR. Tracking issue: rust-lang#76118

```rust
impl<T, const N: usize> [T; N] {
    pub fn each_ref(&self) -> [&T; N];
    pub fn each_mut(&mut self) -> [&mut T; N];
}
```
@rossmacarthur
Copy link
Contributor

With regards to stabilization I think as_slice, as_mut_slice could be stabilized first.

I think they are much more useful than the others in this tracking issue. These are already common methods for existing data types so it's not a new API.

I sometimes find myself running into cases where Deref coercion doesn't work and I need this. Currently I'm using &arr[..] which doesn't really communicate well why it is needed.

For example

// io::Read does something like this
trait Trait {}
impl Trait for &[u8] {}
impl<T: Trait + ?Sized> Trait for &mut T {}

fn takes_trait(t: impl Trait) {}

fn main() {
    let mut arr = [0, 1, 2, 3];
    
    // currently
    takes_trait(&mut &arr[..]);
    
    // with `.as_slice()` its nice and clear
    takes_trait(&mut arr.as_slice());
    
    // can't use deref coercion in this case
    takes_trait(&mut &arr);
    //          ^^^^^^^^^ the trait `Trait` is not implemented for `&[u8; 4]`
}

@LukasKalbertodt
Copy link
Member Author

I agree with @rossmacarthur and would like to propose stabilizing as_slice and as_mut_slice. These methods exist on many other types throughout std (e.g. Vec) and I can't think of a reason to wait with stabilization. I can't see how the name or semantics would still change. And I'm pretty sure we want those methods.

@jhpratt
Copy link
Member

jhpratt commented Aug 26, 2021

Partial stabilization is up: #88353

Manishearth added a commit to Manishearth/rust that referenced this issue Oct 4, 2021
…oshtriplett

Partially stabilize `array_methods`

This stabilizes `<[T; N]>::as_slice` and `<[T; N]>::as_mut_slice`, which is forms part of the `array_methods` feature: rust-lang#76118.

This also makes `<[T; N]>::as_slice` const due to its trivial nature.
Manishearth added a commit to Manishearth/rust that referenced this issue Oct 4, 2021
…oshtriplett

Partially stabilize `array_methods`

This stabilizes `<[T; N]>::as_slice` and `<[T; N]>::as_mut_slice`, which is forms part of the `array_methods` feature: rust-lang#76118.

This also makes `<[T; N]>::as_slice` const due to its trivial nature.
@workingjubilee workingjubilee added the A-array Area: `[T; N]` label Oct 6, 2021
@jplatte
Copy link
Contributor

jplatte commented Mar 22, 2022

Hm, I didn't think too much about it when posting the last reply. I guess you're right about Iterator::as_ref and the longer names are.. Longer. The current solution definitely isn't bad!

@Aaron1011
Copy link
Member

Aaron1011 commented Sep 21, 2022

Are there any further objections over the current names, or are these ready to stabilize?

@Zenithsiz
Copy link

Can these be made const fns? Or are we still missing features to allow these to become const fns?

@KamilaBorowska
Copy link
Contributor

KamilaBorowska commented Jan 4, 2023

Can these be made const fns? Or are we still missing features to allow these to become const fns?

Technically each_ref can be made a const fn (it can be implemented on stable), however it would require complicating the method noticeably, and that probably wouldn't be worth it at this point.

For instance, with array_macro crate (to show that it's possible, obviously standard library shouldn't be using it), it's possible to implement it like this:

const fn each_ref<T, const N: usize>(arr: &[T; N]) -> [&T; N] {
    array![i => &arr[i]; N]
}

@dead-claudia
Copy link

Curious why provide each_ref and each_mut methods, and not just providing by-ref equivalents to all 4 of the self-consuming array methods.

  • .map(self, f) -> .map_ref(&self, f), .map_mut(&mut self, f)
  • .try_map(self, f) -> .try_map_ref(&self, f), .try_map_mut(&mut self, f)
  • .zip(self, other) -> .zip_ref(&self, &other)/.zip_mut(&mut self, &mut other)
  • .transpose(self) -> .transpose_ref(&self)/.transpose_mut(&mut self)

This would avoid intermediate arrays of almost-always-pointless references, would save a method call in almost all cases, and likely be easier to optimize for. Also, .transpose_{ref,mut} has direct precedent elsewhere - all the assume_init methods for MaybeUninit also carry *_ref and *_mut variants.

@workingjubilee
Copy link
Member

Noteworthy: zip_array was also recalled for generating intermediate references.

@pierzchalski
Copy link
Contributor

@LukasKalbertodt Is there anything specific blocking stabilization of each_ref and each_mut? I notice we haven't gotten consensus on the names, but also no one seems to have a very strong opinion afaics?

@LukasKalbertodt
Copy link
Member Author

@pierzchalski There is an open PR that stabilizes these methods as is. It seems like the current names are generally accepted now.

In the PR there are no objections, but @dead-claudia raised an objection in this thread above. I would disagree though: having three versions of every method makes for a really bloated and awkward API. Yes, it's not uncommon in Rust to have these three versions of the same method, but usually only if there isn't a better way. In the MaybeUninit example, I'm not entirely sure why this specific API was chosen, but I don't think it's possible to add some foo_ref method to replace bar.assume_init_ref() with bar.foo_ref().assume_init(). And also: there, it's just one method with three versions.

Regarding optimization: yep, I heard that array methods tend to optimize non-optimally in some cases. That's unfortunate of course. But this is just a consequences of how the optimization pipeline works right now; the API does not inherently make proper optimizations impossible! Many people have proposed some kind of constant length iterator with lazy evaluation, but I have no idea if there is any progress on that. Thus, I personally don't think these are arguments against each_ref and each_mut. And I hope the linked PR will be merged soon.

@dtolnay
Copy link
Member

dtolnay commented Sep 17, 2023

@rust-lang/libs-api:
@rfcbot fcp merge

impl [T; N] {
    pub fn each_ref(&self) -> [&T; N];
    pub fn each_mut(&mut self) -> [&mut T; N];
}

I suggested this naming in #75490 (review) and provided further rationale for it in #75490 (review). I continue to think these are good names. The naming discussion above has reached a stopping point without any traction behind any better set of names, in my opinion.

Aside from naming, a point to consider was whether these functions are amenable enough to optimization. See #76118 (comment) with a response in #76118 (comment).

In #80094 (comment) we removed pub fn zip<U>(self, [U; N]) -> [(T, U); N] because [(T, U); N] is not the end product anyone wanted; it would always have been followed up with a map in practice, suggesting some kind of iterator-based zip and chunk/collect would be more suitable.

Is [&T; N] / [&mut T; N] a different situation? Currently the documentation identifies only map as the motivation.

let strings = ["Ferris".to_string(), "♥".to_string(), "Rust".to_string()];
let is_ascii = strings.each_ref().map(|s| s.is_ascii());
assert_eq!(is_ascii, [true, false, true]);

// We can still access the original array: it has not been moved.

But notice that &[&T] is a construct that is relatively more common in APIs than [(T, U); N], so at least each_ref might find a use there. Stuff like:

pub fn register_lints(&mut self, lints: &[&'static Lint]) {

args: &[&'ll Value],

pub fn const_array(&self, ty: &'ll Type, elts: &[&'ll Value]) -> &'ll Value {

Alternatives: something like strings.iter().map(|s| s.is_ascii()).array_chunks::<3>().next().unwrap().

@rfcbot
Copy link

rfcbot commented Sep 17, 2023

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 17, 2023
@pravic
Copy link
Contributor

pravic commented Oct 20, 2023

Any news about this?

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jan 16, 2024
@rfcbot
Copy link

rfcbot commented Jan 16, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jan 26, 2024
@rfcbot
Copy link

rfcbot commented Jan 26, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Jan 26, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 26, 2024
…, r=dtolnay

stabilise array methods

Closes rust-lang#76118

Stabilises the remaining array methods

FCP is yet to be carried out for this

There wasn't a clear consensus on the naming, but all the other alternatives had some flaws as discussed in the tracking issue and there was a silence on this issue for a year
@bors bors closed this as completed in c9ab37b Jan 27, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jan 27, 2024
Rollup merge of rust-lang#103522 - Dylan-DPC:76118/array-methods-stab, r=dtolnay

stabilise array methods

Closes rust-lang#76118

Stabilises the remaining array methods

FCP is yet to be carried out for this

There wasn't a clear consensus on the naming, but all the other alternatives had some flaws as discussed in the tracking issue and there was a silence on this issue for a year
jonhoo added a commit to jonhoo/haphazard that referenced this issue Jan 28, 2024
Now that `array::each_mut` has stabilized (rust-lang/rust#76118), we'll
be able to get rid of this unsafe block! We'll have to wait for Rust
1.77 to release this though (and maybe a bit longer if we want to
preserve MSRV).
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-array Area: `[T; N]` A-slice Area: `[T]` B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.