Skip to content

impl Default for array::IntoIter #141574

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fee1-dead
Copy link
Member

@fee1-dead fee1-dead commented May 26, 2025

cc #91583

my personal use of this feature comes from https://github.com/fee1-dead/w/blob/092db5df631ea515b688bae99c7f02eef12d7221/src/cont.rs#L154-L170

insta-stable, but I feel like this is small enough to not require an ACP (but a FCP per https://forge.rust-lang.org/libs/maintaining-std.html#when-theres-new-trait-impls)? feel free to correct me if I am wrong.

@fee1-dead fee1-dead added the needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. label May 26, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 26, 2025

r? @jhpratt

rustbot has assigned @jhpratt.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 26, 2025
@ChrisDenton
Copy link
Member

@rust-lang/libs-api

@jhpratt jhpratt added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label May 26, 2025
@jhpratt
Copy link
Member

jhpratt commented May 26, 2025

ACP is primarily to avoid unnecessary implementation effort, which is trivial here. I don't see any issue with going straight to FCP for this. Nominating for team discussion.

@jhpratt jhpratt added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 26, 2025
@BoxyUwU
Copy link
Member

BoxyUwU commented May 26, 2025

It feels somewhat surprising to me that the default value of an array iterator would be empty instead of N default values of T. Though every other collection::IntoIter: Default impl in std just makes an empty iterator so this arguably is the consistent choice.

@dtolnay
Copy link
Member

dtolnay commented May 26, 2025

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented May 26, 2025

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 May 26, 2025
@dtolnay dtolnay removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label May 26, 2025
@BurntSushi
Copy link
Member

This "feels" slightly odd to me. From what I can tell, this Default impl for array::IntoIter essentially exposes the currently unstable array::IntoIter::empty constructor. AFAIK, there is no other way to get an empty iterator for arbitrary const N.

I think the thing that makes me feel uneasy about this is that this makes it so array::IntoIter<T, N> might yield fewer than N elements. But maybe there is already a stable way to do that that I'm missing?

I'm not necessarily advocating for returning N elements via T::default() (although, if we add this impl as-is, we will never be able to do that AIUI) as I think that's also somewhat strange too.

@fee1-dead
Copy link
Member Author

makes it so array::IntoIter<T, N> might yield fewer than N elements

I mean... You can already do the following on stable:

fn empty<T: Default + Copy, const N: usize>() -> std::array::IntoIter<T, N> {
    let mut iter = [T::default(); N].into_iter();
    iter.by_ref().for_each(|_| {});
    iter
}

@BurntSushi
Copy link
Member

Yeah I guess that renders my concern there moot. That was my poor attempt at articulating my feeling here. It doesn't get rid of my sense of this being a bit weird to me. I guess I'm just echoing @BoxyUwU here, but I'm not convinced that adding the impl at all is worth it.

I don't feel strongly enough to render an objection though.

@fee1-dead
Copy link
Member Author

Well sure, there is a bit of a difference here:

for Vec<T>, Vec::default and vec::IntoIter::default are the same things.

for Option<T>, Option::default and option::IntoIter::default are the same things.

Here <[T; N]>::default and array::IntoIter::default are different.

But array::IntoIter is also different in that it has a dynamic size whereas an array doesn't. When you do a mem::take on [T; N] you zero-fill it (fill it with the default value), but on an iterator it might make slightly more sense to have an empty iterator?

I can see arguments on both sides but I felt that an empty iterator was stronger. Ability to use mem::take instead of having to deal with unsafe shenanigans on an IntoIter was the primary use case for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. 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.

8 participants