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 by-value iterator for arrays #62959

Merged
merged 3 commits into from
Oct 25, 2019

Conversation

LukasKalbertodt
Copy link
Member

@LukasKalbertodt LukasKalbertodt commented Jul 24, 2019

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

@rust-highfive

This comment has been minimized.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 24, 2019
Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/libcore/array.rs Outdated Show resolved Hide resolved
src/libcore/array.rs Outdated Show resolved Hide resolved
src/libcore/array.rs Outdated Show resolved Hide resolved
src/libcore/array.rs Outdated Show resolved Hide resolved
src/libcore/array.rs Outdated Show resolved Hide resolved
src/libcore/array.rs Outdated Show resolved Hide resolved
src/libcore/array.rs Outdated Show resolved Hide resolved
@jonas-schievink jonas-schievink added the relnotes Marks issues that should be documented in the release notes of the next release. label Jul 24, 2019
@jonas-schievink jonas-schievink added this to the 1.38 milestone Jul 24, 2019
@jonas-schievink jonas-schievink added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jul 24, 2019
@Centril
Copy link
Contributor

Centril commented Jul 24, 2019

r? @scottmcm

@Mark-Simulacrum
Copy link
Member

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.

@rust-highfive

This comment has been minimized.

@LukasKalbertodt
Copy link
Member Author

@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
Copy link
Contributor

Centril commented Jul 25, 2019

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.

@Centril Centril reopened this Jul 25, 2019
Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/libcore/array.rs Outdated Show resolved Hide resolved
src/libcore/array.rs Outdated Show resolved Hide resolved
src/libcore/array.rs Outdated Show resolved Hide resolved
@cuviper
Copy link
Member

cuviper commented Jul 25, 2019

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
Copy link
Contributor

Centril commented Jul 25, 2019

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

@scottmcm
Copy link
Member

scottmcm commented Jul 25, 2019

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.

src/libcore/array.rs Outdated Show resolved Hide resolved
@rust-highfive

This comment has been minimized.

@RalfJung
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

The part about the lint still stands.

@Centril
Copy link
Contributor

Centril commented Jul 25, 2019

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:

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
Copy link
Member

cuviper commented Jul 25, 2019

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

@bors

This comment has been minimized.

src/libcore/array/mod.rs Outdated Show resolved Hide resolved
The iterator is implemented using const generics. It implements the
traits `Iterator`, `DoubleEndedIterator`, `ExactSizeIterator`,
`FusedIterator` and `TrustedLen`. It also contains a public method
`new` to create it from an array.

`IntoIterator` was not implemented for arrays yet, as there are still
some open questions regarding backwards compatibility. This commit
only adds the iterator impl and does not yet offer a convenient way
to obtain that iterator.
Many tests are based on tests by Josh Stone <[email protected]>
This it to make sure traits are implemented for arrays with length 32
and below, while they are not implemented for >= 33.
@LukasKalbertodt

This comment has been minimized.

@Centril Centril added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Oct 24, 2019
@scottmcm
Copy link
Member

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

@bors r+

@bors
Copy link
Contributor

bors commented Oct 25, 2019

📌 Commit c36b9dd has been approved by scottmcm

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 25, 2019
Centril added a commit to Centril/rust that referenced this pull request Oct 25, 2019
…=scottmcm

Add by-value iterator for arrays

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):

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

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

**TODO**:
- [x] Get initial feedback
- [x] Add tests
- [x] Figure out why stage1 produces weird bugs ([comment](rust-lang#62959 (comment)))
- [x] Add UI tests as mentioned [here](rust-lang#62959 (comment)) (will do that soon-ish)
- [x] Fix [this new bug](rust-lang#62959 (comment))

**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 rust-lang#25725
bors added a commit that referenced this pull request Oct 25, 2019
Rollup of 9 pull requests

Successful merges:

 - #62959 (Add by-value iterator for arrays )
 - #65390 (Add long error explanation for E0576)
 - #65408 (reorder config.toml.example options and add one missing option)
 - #65414 (ignore uninhabited non-exhaustive variant fields)
 - #65666 (Deprecated proc_macro doesn't trigger warning on build library)
 - #65742 (Pre-expansion gate most of the things)
 - #65747 (Adjust the tracking issue for `untagged_unions`.)
 - #65763 (Changed APIT with explicit generic args span to specific arg spans)
 - #65775 (Fix more `ReEmpty` ICEs)

Failed merges:

 - #65519 (trait-based structural match implementation)

r? @ghost
@bors bors merged commit c36b9dd into rust-lang:master Oct 25, 2019
@LukasKalbertodt LukasKalbertodt deleted the array-value-iter branch October 25, 2019 08:53
Centril added a commit to Centril/rust that referenced this pull request Oct 25, 2019
…er-tracking-issue, r=Centril

Fill tracking issue number for `array_value_iter`

Thanks for [noticing](rust-lang#62959 (comment))!

r? @Centril
Centril added a commit to Centril/rust that referenced this pull request Oct 25, 2019
…er-tracking-issue, r=Centril

Fill tracking issue number for `array_value_iter`

Thanks for [noticing](rust-lang#62959 (comment))!

r? @Centril
Centril added a commit to Centril/rust that referenced this pull request Oct 25, 2019
…er-tracking-issue, r=Centril

Fill tracking issue number for `array_value_iter`

Thanks for [noticing](rust-lang#62959 (comment))!

r? @Centril
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 25, 2019
…er-tracking-issue, r=Centril

Fill tracking issue number for `array_value_iter`

Thanks for [noticing](rust-lang#62959 (comment))!

r? @Centril
@HalidOdat HalidOdat mentioned this pull request Jun 11, 2020
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-const_generics `#![feature(const_generics)]` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue. 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.