RFC: movable array iterators #2185

ishitatsuyuki · 2017-10-21T07:52:02Z

SimonSapin · 2017-10-21T08:32:44Z

I very much want this, but what is return type of [T; N]::into_iter? I think it should be IntoIter<T, N>, with const generics. So this needs to wait for that to be implemented.

Or maybe IntoIter<A> where A: Unsize<[T]> might work, but stabilizing with Unsize itself unstable doesn’t feel right.

Transmute the array so it has ManuallyDrop signature. This should be memory safe.

I believe that in a method taking self, the array can be safely moved into ManuallyDrop::new(…) without transmute. Hopefully this move optimizes to a no-op?

ishitatsuyuki · 2017-10-21T08:38:58Z

Please prefer line comments so I can respond without copying :)

I believe that in a method taking self, the array can be safely moved into ManuallyDrop::new(…) without transmute. Hopefully this move optimizes to a no-op?

I think there's a need to be able to drop each elements individually (in other words, only in the range that is not moved out). So we need ManuallyDrop on each element, and trasmuting should be the easiest so far.

SimonSapin · 2017-10-21T09:07:22Z

You can call drop_in_place(…) selectively on individual items or a sub-slice inside of ManuallyDrop<[T; N]>.

ishitatsuyuki · 2017-10-21T09:14:50Z

Good. I will update the text later.

bluss · 2017-10-24T21:22:33Z

text/0000-array_into_iter_move.md

+```rust
+[1, 2].into_iter();
+// This was originally yielding references, but now values.
+```


This is pretty much ensured to break actual code for many users, both in crater (that we can test, find and maybe fix), and lots of code we can't find or test. Maybe we can begin warning for this long before the new iterator is implemented?

Personally speaking, I don't think people would use into_iter despite the fact it yields references, and they would just iter instead. Thus I think fixing after a crater should be sufficient.

We don't reach all rust code in the world with crater. If the break is wide spread, we have to be more careful.

And what about for &x in [1,2,3], isn't that just into_iter?

That does not compile today I think so it's not a backwards compat concern.

Oh I see. [1].into_iter() currently resolves to the impl for &[_; _] which the new [_; _] impl would override, right?

cristicbz · 2017-10-31T13:58:56Z

@SimonSapin

I very much want this, but what is return type of [T; N]::into_iter? I think it should be IntoIter<T, N>, with const generics. So this needs to wait for that to be implemented.

This could be done without stabilising Unsize, by doing:

impl<T> Iterator for IntoIter<[T; 1]> { ... }
impl<T> Iterator for IntoIter<[T; 2]> { ... }
impl<T> Iterator for IntoIter<[T; 3]> { ... }
// ...
impl<T> Iterator for IntoIter<[T; 32]> { ... }

with all the impls calling to some shared unsafe method to avoid duplication. Has a nice migration story for when const generics land, just replacing with

impl<T, const N> Iterator for IntoIter<[T; N]> {}

ishitatsuyuki · 2017-11-13T07:05:15Z

Please take a look, I think I have addressed the concerns.

SimonSapin · 2017-11-13T07:18:35Z

This looks great to me, thanks for writing it up!

sinkuu · 2017-11-13T12:54:44Z

Drop impls cannot be specialized, so we may have to use ~~Unsafe~~Unsize or FixedSizeArray if we implement this RFC before const generics.

struct IntoIter<T> { ... }
impl<T> Drop for IntoIter<[T; 0]> { ... }
impl<T> Drop for IntoIter<[T; 1]> { ... } // error: Implementations of Drop cannot be specialized
...

// I expect this will work
struct IntoIter<T, const N: usize> { ... }
impl<T, const N: usize> Drop for IntoIter<T, N>

// This works today
struct IntoIter<T, A> where A: FixedSizeArray<T> { ... }
impl<T, A> Drop for IntoIter<T, A> where A: FixedSizeArray<T> { ... }

EDIT: typo

eddyb · 2017-11-13T12:59:59Z

@sinkuu What do you mean by Unsafe?

ishitatsuyuki · 2017-11-13T13:00:50Z

Presumably Unsize.

ishitatsuyuki · 2017-11-13T14:00:16Z

Should we make multiple IntoIter structs until const generic lands? Like this: IntoIter32<T>.

I think this should resolve the specialization problem, alongside reducing the template complexity, so we can store pointers (or unsafe slices) inside the struct.

SimonSapin · 2017-11-13T14:04:39Z

IntoIter32<T>

I want this feature, but I don’t think I want to stabilize such an API.

It’d be harder to generalize once const generics land, too.

so we can store pointers (or unsafe slices) inside the struct

I don’t understand what you mean here. The point of into_iter is that the items move when the iterator moves. Pointers should not be involved.

ishitatsuyuki · 2017-11-13T14:08:24Z

but I don’t think I want to stabilize such an API.

I'm afraid that even IntoIter<T> has to change into IntoIter<T, const N> so signature change is unavoidable.

I don’t understand what you mean here.

We need to save the range we have consumed with the iterator. It can be either usize-indexed or just start-end pointer.

SimonSapin · 2017-11-13T14:15:22Z

Changing the signature of stable APIs is not acceptable, so I’m afraid that this implies we should not stabilize anything here without const generics.

Self-referential pointers would become invalid when the iterator is moved. It has to be indices. (Two of them, or a Range, to support DoubleEndedIterator.)

ishitatsuyuki · 2017-11-20T09:07:06Z

I've come up with a partial stabilization plan, please let me know if this is possible.

As the signature of the iterator struct is subject to change, we will be keeping that struct(s) unstable until const generics land. I will call this struct UnstableIntoIter below.

IntoIterator is implemented like below, using the length of 32 as an example:

impl<T> IntoIterator for [T; 32] {
    type Item = T;
    type IntoIter = UnstableIntoIter32<T>;

    fn into_iter(self) -> T { ... }
}

I have no idea if this implementation will be accepted as stable in the compiler, so we can use it without the ability to annotate the type.

kennytm · 2017-11-20T14:42:39Z

Instead of a specific IntoIter or UnstableIntoIter method, could it just return an existential type impl Iterator<Item=T>?

scottmcm · 2017-11-20T21:37:38Z

Would it be compatible to replace

struct IntoIter0<T>{ .. };
..
struct IntoIter32<T>{ .. };

with

type IntoIter0<T> = IntoIter<T, const 0>;
..
type IntoIter32<T> = IntoIter<T, const 32>;

once const generics land?

Then the eventual type would be the nice one, and the residual cruft is just a bunch of deprecated type aliases, which should be easy to maintain and ignore in the docs.

bluss · 2017-11-20T21:53:05Z

We can do one thing before we even start the implementation: Work on deprecating & future warning the existing array_value.into_iter() calls (those that resolve to the slice's iterator).

ishitatsuyuki · 2017-11-21T09:24:59Z

I experimented with an implementation of deprecation warning, but it seems that I can't avoid one particular false positive: https://play.rust-lang.org/?gist=8b69b43bc947bf112c0f20d0e58b3add&version=stable

Basically, the differences between the below cannot be detected without changing the signature:

a.into_iter();
(&a).into_iter();

sfackler · 2018-01-24T00:08:15Z

We discussed this at the @rust-lang/libs meeting today, and feel like this won't really be actionable until integer generics are implemented.

@rfcbot fcp postpone

rfcbot · 2018-01-24T00:08:16Z

Team member @sfackler has proposed to postpone this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, 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.

bluss · 2018-01-24T00:33:34Z

@sfackler What do you think about starting to warn / soft deprecate [1, 2, 3].into_iter() in favour of [1, 2, 3].iter() ?

ishitatsuyuki · 2018-01-24T00:35:40Z

I have mentioned that if we don't stabilize the iterator types, we can implement it with macro generated iterators and partially stabilize the into_iter() function only.

sfackler · 2018-01-24T00:37:03Z

@bluss

Not sure - we haven't really done anything like that before. @aturon thoughts?

@ishitatsuyuki

I am personally not a fan of the current "up to size 32" approach, and don't really want to expand that further.

ishitatsuyuki · 2018-01-24T09:50:45Z

@sfackler - what we only can do currently is that "up to size 32" approach; it should not be a blocker to improve array usability.

I originally thought that the main concern is backwards compatibility, which needs a lint to be developed. As I emphasized above, const generics is not a blocker for this, just a wishlist item. As const generics is an approved RFC, we can expect the stabilization at the same time.

aturon · 2018-01-25T21:43:13Z

@bluss I'd be in favor.

rfcbot · 2018-02-14T07:03:41Z

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

rfcbot · 2018-02-14T07:03:41Z

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

rfcbot · 2018-02-14T07:03:41Z

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

rfcbot · 2018-02-14T07:03:41Z

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

rfcbot · 2018-02-24T07:07:45Z

The final comment period is now complete.

Centril · 2018-02-24T07:16:12Z

Closing since FCP with the proposal to postpone is now complete.

array_into_iter_move

00d9129

Address review comments

7aaafd5

bluss reviewed Oct 24, 2017

View reviewed changes

scottmcm added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Oct 25, 2017

Address comments regarding generics and lint

4c1505b

aturon assigned sfackler Jan 23, 2018

rfcbot added the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Jan 24, 2018

rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Feb 14, 2018

rfcbot added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Feb 14, 2018

Centril added the postponed RFCs that have been postponed and may be revisited at a later time. label Feb 24, 2018

Centril closed this Feb 24, 2018

ishitatsuyuki mentioned this pull request Mar 14, 2018

impl IntoIterator for arrays rust-lang/rust#49000

Closed

RFC: movable array iterators #2185

RFC: movable array iterators #2185

Conversation

ishitatsuyuki commented Oct 21, 2017 • edited Loading

SimonSapin commented Oct 21, 2017

ishitatsuyuki commented Oct 21, 2017

SimonSapin commented Oct 21, 2017

ishitatsuyuki commented Oct 21, 2017

bluss Oct 24, 2017 • edited Loading

Choose a reason for hiding this comment

ishitatsuyuki Oct 25, 2017

Choose a reason for hiding this comment

bluss Oct 25, 2017

Choose a reason for hiding this comment

main-- Oct 25, 2017

Choose a reason for hiding this comment

bluss Oct 25, 2017 • edited Loading

Choose a reason for hiding this comment

main-- Oct 25, 2017

Choose a reason for hiding this comment

bluss Oct 31, 2017

Choose a reason for hiding this comment

cristicbz commented Oct 31, 2017

ishitatsuyuki commented Nov 13, 2017

SimonSapin commented Nov 13, 2017

sinkuu commented Nov 13, 2017 • edited Loading

eddyb commented Nov 13, 2017

ishitatsuyuki commented Nov 13, 2017

ishitatsuyuki commented Nov 13, 2017

SimonSapin commented Nov 13, 2017

ishitatsuyuki commented Nov 13, 2017

SimonSapin commented Nov 13, 2017

ishitatsuyuki commented Nov 20, 2017

kennytm commented Nov 20, 2017

scottmcm commented Nov 20, 2017

bluss commented Nov 20, 2017 • edited Loading

ishitatsuyuki commented Nov 21, 2017

sfackler commented Jan 24, 2018

rfcbot commented Jan 24, 2018 • edited by aturon Loading

bluss commented Jan 24, 2018

ishitatsuyuki commented Jan 24, 2018

sfackler commented Jan 24, 2018

ishitatsuyuki commented Jan 24, 2018

aturon commented Jan 25, 2018

rfcbot commented Feb 14, 2018

rfcbot commented Feb 14, 2018

rfcbot commented Feb 14, 2018

rfcbot commented Feb 14, 2018

rfcbot commented Feb 24, 2018

Centril commented Feb 24, 2018

ishitatsuyuki commented Oct 21, 2017 •

edited

Loading

bluss Oct 24, 2017 •

edited

Loading

bluss Oct 25, 2017 •

edited

Loading

sinkuu commented Nov 13, 2017 •

edited

Loading

bluss commented Nov 20, 2017 •

edited

Loading

rfcbot commented Jan 24, 2018 •

edited by aturon

Loading