Add by-value iterator for arrays

[LukasKalbertodt]

This adds an iterator that can iterate over arrays by value, yielding all elements by value. However, this PR does not add a corresponding IntoIterator impl for arrays. The IntoIterator impl needs some discussion about backwards-compatibility that should take place in a separate PR. With this patch, this code should work (but there is currently still a bug):

#![feature(array_value_iter)]
use std::array::IntoIter;

let arr = [1, 2, 3];
for x in IntoIter::new(arr) {
    println!("{}", x);
}

TODO:

• Get initial feedback
• Add tests
• Figure out why stage1 produces weird bugs (comment)
• Add UI tests as mentioned here (will do that soon-ish)
• Fix this new bug

Notes for reviewers

• Is the use of MaybeUninit correct here? I think it has to be used due to the Clone impl which has to fill the dead array elements with something, but cannot fill it with a correct instance.
• Are the unit tests sufficient?

CC #25725

[Centril]

Overall this looks nice. Here's some food for thought however:

• I think some tests should also be added that exercise each impl you've provided here.
• In particular, there should be tests collecting into a Vec<...> and then checking the contents.
• It would be good to have tests that drop some elements on the floor in some positions (let _ = iter.next();)
• Checking the boundary conditions around N and self.pos is a good idea.
• Tests that also format!(...) the iterator to exercise Debug would be good.

(Some of these checks can be done in the same tests).

Also cc @scottmcm

[Centril]

r? @scottmcm

[Mark-Simulacrum]

This is unfortunately not backwards compatible. Arrays deref to slices and calling .into_iter currently returns &T but would begin returning T. cc @cuviper who has done investigation into this in the past - we should probably update the relevant issue.

[LukasKalbertodt]

@Centril Thanks a lot for the fairly in-depth review already! I will fix all of that once the bigger questions are resolved.

@Mark-Simulacrum I wasn't aware this is a problem. Sigh, that's unfortunate indeed. (I am a bit surprised about quickly closing the PR without a discussion though...)

From #49000 it seems like there has been a fair amount of discussion already. To sidestep the IntoIterator problem entirely for now, what do you think about changing this PR to only add the iterator type and an unstable fn owned_iter(self) -> IntoIter for arrays? That way the actual functionality is already there and we can discuss how we deal with IntoIterator afterwards. Makes sense?

[Centril]

I also wonder if there has already been a crater run and what the fallout was? Maybe @cuviper knows.

Ostensibly some minor breakage could (it's a choice!) be justified as "inference breakage" and this is a rather large paper cut in the language which would be good to finally solve. (I'm also thinking of #59500 which technically was not "allowed" due to #[fundamental] but which we merged anyways and did not revert even with the ensuing reported regression).

To sidestep the IntoIterator problem entirely for now, what do you think about changing this PR to only add the iterator type and an unstable fn owned_iter(self) -> IntoIter for arrays? That way the actual functionality is already there and we can discuss how we deal with IntoIterator afterwards. Makes sense?

In my view a solid interim plan! Reopening so that we may consider that.

[cuviper]

Please compare with #49000 for feature parity of what I did before. Most of it should be similar, apart from having real const generics now. I had as_slice() and as_mut_slice(), Clone, DoubleEndedIterator, ExactSizedIterator, FusedIterator, and TrustedLen. You could take my added tests too.

[cuviper]

I also wonder if there has already been a crater run and what the fallout was?

I don't think we did a direct crater run with the new IntoIterator, but there was an attempt at running with the clippy lint denied, which failed horribly for unrelated reasons. (The rustflags interfered with build scripts, or something like that.)

[Centril]

@cuviper In that case this is as good a time as any to try out a crater run then.

[scottmcm]

My personal opinion: having this impl is so obviously the correct thing that I'd be willing to bend stability guarantees to have it, but we don't even need to because adding a new trait impl is an allowed change, no matter whether it breaks code. And the only code that it breaks, today, is code that was doing a.into_iter() when a.iter() would have done exactly the same thing for less typing, so any workaround needed to not trigger this change will make the code strictly better regardless.

[RalfJung]

And the only code that it breaks, today, is code that was doing a.into_iter() when a.iter() would have done exactly the same thing for less typing,

Is that true? I think it also breaks code that does for x in a because for implicitly does into_iter().

We could probably have a clippy/rustc lint to start moving people away from that, though.

[Mark-Simulacrum]

for _ in a doesn't actually go through deref on a as we call IntoIterator::into_iter(a) instead of a.into_iter() in the desugaring, that is, https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=a367b046176dcf7a3eb762da2ace045a doesn't compile.

[RalfJung]

@Mark-Simulacrum ah, good point. That's great then!

The part about the lint still stands.

[Centril]

for _ in a doesn't actually go through deref on a as we call IntoIterator::into_iter(a) instead of a.into_iter() in the desugaring, that is, https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=a367b046176dcf7a3eb762da2ace045a doesn't compile.

(Relevant lowering code:

rust/src/librustc/hir/lowering.rs

Lines 4887 to 4894 in eedf6ce

	let iter = P(self.expr_ident(head_sp, iter, iter_pat_nid));
	let ref_mut_iter = self.expr_mut_addr_of(head_sp, iter);
	let next_path = &[sym::iter, sym::Iterator, sym::next];
	let next_expr = P(self.expr_call_std_path(
	head_sp,
	next_path,
	hir_vec![ref_mut_iter],
	));

)

[cuviper]

The clippy lint exists, although I don't think it was default-deny when we tried the crater run before.

https://rust-lang.github.io/rust-clippy/master/index.html#into_iter_on_array

[scottmcm]

Huzzah! Kudos to everyone working on const generics for getting this to work ❤️

@bors r+

[bors]

📌 Commit c36b9dd has been approved by scottmcm