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.

Sign up for GitHub

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

impl IntoIterator for arrays #49000

Closed
wants to merge 1 commit into from
Closed

impl IntoIterator for arrays #49000

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
189 changes: 188 additions & 1 deletion src/libcore/array.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,10 @@ use cmp::Ordering;
use convert::TryFrom;
use fmt;
use hash::{Hash, self};
use marker::Unsize;
use iter::{FusedIterator, TrustedLen};
use marker::{PhantomData, Unsize};
use mem::{ManuallyDrop, self};
use ptr;
use slice::{Iter, IterMut};

/// Utility trait implemented only on arrays of fixed size
Expand Down Expand Up @@ -52,6 +55,7 @@ unsafe impl<T, A: Unsize<[T]>> FixedSizeArray<T> for A {
fn as_slice(&self) -> &[T] {
self
}

#[inline]
fn as_mut_slice(&mut self) -> &mut [T] {
self
Expand Down Expand Up @@ -210,6 +214,21 @@ macro_rules! array_impls {
}
}

#[unstable(feature = "array_into_iter", issue = "0")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Trait implementations are still insta-stable, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, yeah. I'll need to update the truly unstable items with a tracking issue (if this PR is accepted), so I'll change the impls to stable then.

Copy link
Member

Choose a reason for hiding this comment

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

Event if the IntoIter type is left as unstable, it will still be reachable through <<[u8; 0]> as IntoIter>::Iter>, right?

Copy link
Member Author

@cuviper cuviper Mar 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

True, you can write something like:

fn my_iter() -> <[u8; 0] as IntoIterator>::IntoIter {
    [].into_iter()
}

I think that's OK. We could change the associated type entirely without breaking such code.

impl<T> IntoIterator for [T; $N] {
type Item = T;
type IntoIter = IntoIter<T, Self>;

fn into_iter(self) -> Self::IntoIter {
Self::IntoIter {
array: ManuallyDrop::new(self),
index: 0,
index_back: $N,
_marker: PhantomData,
}
}
}

// NOTE: some less important impls are omitted to reduce code bloat
__impl_slice_eq1! { [A; $N], [B; $N] }
__impl_slice_eq2! { [A; $N], [B] }
Expand Down Expand Up @@ -285,3 +304,171 @@ macro_rules! array_impl_default {
}

array_impl_default!{32, T T T T T T T T T T T T T T T T T T T T T T T T T T T T T T T T}


/// An iterator that moves out of an array.
///
/// This `struct` is created by the `into_iter` method on [arrays]
/// (provided by the [`IntoIterator`] trait).
///
/// [arrays]: ../../std/primitive.array.html
/// [`IntoIterator`]: ../../std/iter/trait.IntoIterator.html
#[unstable(feature = "array_into_iter", issue = "0")]
pub struct IntoIter<T, A: FixedSizeArray<T>> {
// Invariants: index <= index_back <= array.len()
// Only values in array[index..index_back] are alive at any given time.
// Values from array[..index] and array[index_back..] are already moved/dropped.
array: ManuallyDrop<A>,
index: usize,
Copy link
Member

Choose a reason for hiding this comment

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

One thing I've personally enjoyed doing historically is storing these two indices as Range<usize> as it helps make implementations for the iterator methods easier

index_back: usize,
_marker: PhantomData<T>,
}

impl<T, A: FixedSizeArray<T>> IntoIter<T, A> {
/// Returns the remaining items of this iterator as a slice.
///
/// # Examples
///
/// ```
/// #![feature(array_into_iter)]
///
/// # fn main() {
/// let array = ['a', 'b', 'c'];
/// let mut into_iter = array.into_iter();
/// assert_eq!(into_iter.as_slice(), &['a', 'b', 'c']);
/// let _ = into_iter.next().unwrap();
/// assert_eq!(into_iter.as_slice(), &['b', 'c']);
/// # }
/// ```
#[inline]
#[unstable(feature = "array_into_iter", issue = "0")]
pub fn as_slice(&self) -> &[T] {
&self.array.as_slice()[self.index..self.index_back]
}

/// Returns the remaining items of this iterator as a mutable slice.
///
/// # Examples
///
/// ```
/// #![feature(array_into_iter)]
///
/// # fn main() {
/// let array = ['a', 'b', 'c'];
/// let mut into_iter = array.into_iter();
/// assert_eq!(into_iter.as_slice(), &['a', 'b', 'c']);
/// into_iter.as_mut_slice()[2] = 'z';
/// assert_eq!(into_iter.next().unwrap(), 'a');
/// assert_eq!(into_iter.next().unwrap(), 'b');
/// assert_eq!(into_iter.next().unwrap(), 'z');
/// # }
/// ```
#[inline]
#[unstable(feature = "array_into_iter", issue = "0")]
pub fn as_mut_slice(&mut self) -> &mut [T] {
&mut self.array.as_mut_slice()[self.index..self.index_back]
Copy link
Member

Choose a reason for hiding this comment

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

Same question here-- I think this implementation should use ptr::offset and from_raw_parts so as not to acquire a reference to a partially uninitialized slice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same question as above, how can I get a base pointer at all without forming some reference?

Copy link
Member

Choose a reason for hiding this comment

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

As above-- if you make ManuallyDrop repr(transparent), then you know that pointers to it will point directly to its element.

}
}

#[unstable(feature = "array_into_iter", issue = "0")]
impl<T, A: FixedSizeArray<T>> Drop for IntoIter<T, A> {
#[inline]
fn drop(&mut self) {
// Drop values that are still alive.
for p in self.as_mut_slice() {
unsafe { ptr::drop_in_place(p); }
}
}
}

#[unstable(feature = "array_into_iter", issue = "0")]
impl<T: Clone, A: FixedSizeArray<T>> Clone for IntoIter<T, A> {
fn clone(&self) -> Self {
unsafe {
let mut iter = Self {
array: ManuallyDrop::new(mem::uninitialized()),
index: 0,
index_back: 0,
_marker: PhantomData,
};

// Clone values that are still alive.
for (dst, src) in iter.array.as_mut_slice().iter_mut().zip(self.as_slice()) {
ptr::write(dst, src.clone());
iter.index_back += 1;
}

iter
}
}
}

#[unstable(feature = "array_into_iter", issue = "0")]
impl<T: fmt::Debug, A: FixedSizeArray<T>> fmt::Debug for IntoIter<T, A> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
f.debug_tuple("IntoIter")
.field(&self.as_slice())
.finish()
}
}

#[unstable(feature = "array_into_iter", issue = "0")]
impl<T, A: FixedSizeArray<T>> Iterator for IntoIter<T, A> {
type Item = T;

#[inline]
fn next(&mut self) -> Option<T> {
if self.index < self.index_back {
let p = &self.array.as_slice()[self.index];
Copy link
Member

Choose a reason for hiding this comment

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

Should these indexes use ptr::offset so as not to acquire &[T] to a partially uninitialized [T]?

The same applies down in next_back.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is an uninitialized slice really a problem if we never read from those parts? I would think it's fine, otherwise sequences like mem::uninitialized() + ptr::write() would be a problem too.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's certainly problematic under the types-as-contracts view. AFAICT the unsafe code guidelines aren't solid enough to really give a hard answer yet, but it would certainly be better to not create invalid slices. Incidentially, yes, mem::uninitialized is problematic :)

Copy link
Member Author

Choose a reason for hiding this comment

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

If that's the case, how can I even get the pointer base to offset from? Deref for ManuallyDrop will create a momentary reference here too. Or even with a local union type, getting the pointer to the field will require forming a reference first.

Copy link
Member

@cramertj cramertj Mar 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I'd add an unstable ManuallyDrop::as_ptr and mark ManuallyDrop as repr(transparent). This will tell you that *const ManuallyDrop<T> points to the same place as*const T would.

Copy link
Member Author

Choose a reason for hiding this comment

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

At the moment, repr(transparent) is only allowed on a struct. The RFC left unions as future work: rust-lang/rfcs#1758 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need repr(transparent) for this, since this is just a question of memory layout, not calling convention. repr(C) would suffice, or inside libstd we could even simply rely on what rustc currently does (which should place the union contents at the same address as the union).

Copy link
Member

Choose a reason for hiding this comment

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

@rkruppe I've been thinking more about this, and if my conclusion in #49056 (comment) is correct, then this PR would be fine with the current "invalid slice" usage, since the values left in the slice would still be valid values of type T, just ones in the "dropped" typestate.

Copy link
Member Author

Choose a reason for hiding this comment

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

FWIW, here's a comparison using pointers to avoid possibly dropped/uninit refs:
cuviper/rust@array-intoiter...cuviper:array-intoiter-ptr

I can submit the ManuallyDrop additions in a separate PR, if you think it's worth it.

since the values left in the slice would still be valid values of type T, just ones in the "dropped" typestate.

The Clone implementation will create truly uninitialized entries too. IMO the unsafe rules should not declare such references invalid if they're not read, but we can deal with it in pointers if necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that clone implementation is certainly become UB due to the uninitialized, but that's true of basically every use of mem::uninitialized, so we'll have to go clean them all up later no matter what once the uninit RFC lands

self.index += 1;
unsafe { Some(ptr::read(p)) }
} else {
None
}
}

#[inline]
fn size_hint(&self) -> (usize, Option<usize>) {
let len = self.len();
(len, Some(len))
}

#[inline]
fn count(self) -> usize {
self.len()
}
}

#[unstable(feature = "array_into_iter", issue = "0")]
impl<T, A: FixedSizeArray<T>> DoubleEndedIterator for IntoIter<T, A> {
#[inline]
fn next_back(&mut self) -> Option<T> {
if self.index < self.index_back {
self.index_back -= 1;
let p = &self.array.as_slice()[self.index_back];
unsafe { Some(ptr::read(p)) }
} else {
None
}
}
}

#[unstable(feature = "array_into_iter", issue = "0")]
impl<T, A: FixedSizeArray<T>> ExactSizeIterator for IntoIter<T, A> {
#[inline]
fn len(&self) -> usize {
self.index_back - self.index
}

#[inline]
fn is_empty(&self) -> bool {
self.index_back == self.index
}
}

#[unstable(feature = "array_into_iter", issue = "0")]
impl<T, A: FixedSizeArray<T>> FusedIterator for IntoIter<T, A> {}

// #[unstable(feature = "trusted_len", issue = "37572")]
#[unstable(feature = "array_into_iter", issue = "0")]
unsafe impl<T, A: FixedSizeArray<T>> TrustedLen for IntoIter<T, A> {}
32 changes: 16 additions & 16 deletions src/libcore/iter/iterator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ pub trait Iterator {
/// ```
/// #![feature(iterator_step_by)]
/// let a = [0, 1, 2, 3, 4, 5];
/// let mut iter = a.into_iter().step_by(2);
/// let mut iter = a.iter().step_by(2);
///
/// assert_eq!(iter.next(), Some(&0));
/// assert_eq!(iter.next(), Some(&2));
Expand Down Expand Up @@ -461,7 +461,7 @@ pub trait Iterator {
/// ```
/// let a = [1, 2, 3];
///
/// let mut iter = a.into_iter().map(|x| 2 * x);
/// let mut iter = a.iter().map(|x| 2 * x);
///
/// assert_eq!(iter.next(), Some(2));
/// assert_eq!(iter.next(), Some(4));
Expand Down Expand Up @@ -550,7 +550,7 @@ pub trait Iterator {
/// ```
/// let a = [0i32, 1, 2];
///
/// let mut iter = a.into_iter().filter(|x| x.is_positive());
/// let mut iter = a.iter().filter(|x| x.is_positive());
///
/// assert_eq!(iter.next(), Some(&1));
/// assert_eq!(iter.next(), Some(&2));
Expand All @@ -564,7 +564,7 @@ pub trait Iterator {
/// ```
/// let a = [0, 1, 2];
///
/// let mut iter = a.into_iter().filter(|x| **x > 1); // need two *s!
/// let mut iter = a.iter().filter(|x| **x > 1); // need two *s!
///
/// assert_eq!(iter.next(), Some(&2));
/// assert_eq!(iter.next(), None);
Expand All @@ -576,7 +576,7 @@ pub trait Iterator {
/// ```
/// let a = [0, 1, 2];
///
/// let mut iter = a.into_iter().filter(|&x| *x > 1); // both & and *
/// let mut iter = a.iter().filter(|&x| *x > 1); // both & and *
///
/// assert_eq!(iter.next(), Some(&2));
/// assert_eq!(iter.next(), None);
Expand All @@ -587,7 +587,7 @@ pub trait Iterator {
/// ```
/// let a = [0, 1, 2];
///
/// let mut iter = a.into_iter().filter(|&&x| x > 1); // two &s
/// let mut iter = a.iter().filter(|&&x| x > 1); // two &s
///
/// assert_eq!(iter.next(), Some(&2));
/// assert_eq!(iter.next(), None);
Expand Down Expand Up @@ -767,7 +767,7 @@ pub trait Iterator {
/// ```
/// let a = [-1i32, 0, 1];
///
/// let mut iter = a.into_iter().skip_while(|x| x.is_negative());
/// let mut iter = a.iter().skip_while(|x| x.is_negative());
///
/// assert_eq!(iter.next(), Some(&0));
/// assert_eq!(iter.next(), Some(&1));
Expand All @@ -781,7 +781,7 @@ pub trait Iterator {
/// ```
/// let a = [-1, 0, 1];
///
/// let mut iter = a.into_iter().skip_while(|x| **x < 0); // need two *s!
/// let mut iter = a.iter().skip_while(|x| **x < 0); // need two *s!
///
/// assert_eq!(iter.next(), Some(&0));
/// assert_eq!(iter.next(), Some(&1));
Expand All @@ -793,7 +793,7 @@ pub trait Iterator {
/// ```
/// let a = [-1, 0, 1, -2];
///
/// let mut iter = a.into_iter().skip_while(|x| **x < 0);
/// let mut iter = a.iter().skip_while(|x| **x < 0);
///
/// assert_eq!(iter.next(), Some(&0));
/// assert_eq!(iter.next(), Some(&1));
Expand Down Expand Up @@ -828,7 +828,7 @@ pub trait Iterator {
/// ```
/// let a = [-1i32, 0, 1];
///
/// let mut iter = a.into_iter().take_while(|x| x.is_negative());
/// let mut iter = a.iter().take_while(|x| x.is_negative());
///
/// assert_eq!(iter.next(), Some(&-1));
/// assert_eq!(iter.next(), None);
Expand All @@ -841,7 +841,7 @@ pub trait Iterator {
/// ```
/// let a = [-1, 0, 1];
///
/// let mut iter = a.into_iter().take_while(|x| **x < 0); // need two *s!
/// let mut iter = a.iter().take_while(|x| **x < 0); // need two *s!
///
/// assert_eq!(iter.next(), Some(&-1));
/// assert_eq!(iter.next(), None);
Expand All @@ -852,7 +852,7 @@ pub trait Iterator {
/// ```
/// let a = [-1, 0, 1, -2];
///
/// let mut iter = a.into_iter().take_while(|x| **x < 0);
/// let mut iter = a.iter().take_while(|x| **x < 0);
///
/// assert_eq!(iter.next(), Some(&-1));
///
Expand All @@ -867,7 +867,7 @@ pub trait Iterator {
///
/// ```
/// let a = [1, 2, 3, 4];
/// let mut iter = a.into_iter();
/// let mut iter = a.iter();
///
/// let result: Vec<i32> = iter.by_ref()
/// .take_while(|n| **n != 3)
Expand Down Expand Up @@ -1229,7 +1229,7 @@ pub trait Iterator {
/// ```
/// let a = [1, 2, 3];
///
/// let iter = a.into_iter();
/// let iter = a.iter();
///
/// let sum: i32 = iter.take(5).fold(0, |acc, i| acc + i );
///
Expand All @@ -1242,7 +1242,7 @@ pub trait Iterator {
/// // let's try that again
/// let a = [1, 2, 3];
///
/// let mut iter = a.into_iter();
Copy link
Member

Choose a reason for hiding this comment

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

Oh dear, these changes mean that this is technically a breaking change, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly what's going on, this "breaking change" is no different from the way adding a new method to Iterator which (if e.g. itertools already implements that method) would break all users of itertools who used the method syntax instead of the UFC syntax to call said method.

Copy link
Member

Choose a reason for hiding this comment

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

Correct yeah. This doesn't mean it's impossible to land, just that it needs more scrutiny/review.

Copy link
Member

Choose a reason for hiding this comment

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

This does feel like one of those changes that, while (allowed) breaking, people have wanted for a very long time and we definitely want to do at some point.

/// let mut iter = a.iter();
///
/// // instead, we add in a .by_ref()
/// let sum: i32 = iter.by_ref().take(2).fold(0, |acc, i| acc + i );
Expand Down Expand Up @@ -1386,7 +1386,7 @@ pub trait Iterator {
/// let a = [1, 2, 3];
///
/// let (even, odd): (Vec<i32>, Vec<i32>) = a
/// .into_iter()
/// .iter()
/// .partition(|&n| n % 2 == 0);
///
/// assert_eq!(even, vec![2]);
Expand Down
Loading