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

Implement iterator specialization traits on more adapters #85528

Merged
merged 4 commits into from
Jan 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 6 additions & 2 deletions library/alloc/tests/vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1166,10 +1166,14 @@ fn test_from_iter_partially_drained_in_place_specialization() {
#[test]
fn test_from_iter_specialization_with_iterator_adapters() {
fn assert_in_place_trait<T: InPlaceIterable>(_: &T) {}
let src: Vec<usize> = vec![0usize; 256];
let owned: Vec<usize> = vec![0usize; 256];
let refd: Vec<&usize> = owned.iter().collect();
let src: Vec<&&usize> = refd.iter().collect();
let srcptr = src.as_ptr();
let iter = src
.into_iter()
.copied()
.cloned()
.enumerate()
.map(|i| i.0 + i.1)
.zip(std::iter::repeat(1usize))
Expand All @@ -1180,7 +1184,7 @@ fn test_from_iter_specialization_with_iterator_adapters() {
assert_in_place_trait(&iter);
let sink = iter.collect::<Result<Vec<_>, _>>().unwrap();
let sinkptr = sink.as_ptr();
assert_eq!(srcptr, sinkptr as *const usize);
assert_eq!(srcptr as *const usize, sinkptr as *const usize);
}

#[test]
Expand Down
13 changes: 13 additions & 0 deletions library/core/benches/iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,19 @@ fn bench_skip_then_zip(b: &mut Bencher) {
});
}

#[bench]
fn bench_skip_trusted_random_access(b: &mut Bencher) {
let v: Vec<u64> = black_box(vec![42; 10000]);
let mut sink = [0; 10000];

b.iter(|| {
for (val, idx) in v.iter().skip(8).zip(0..10000) {
sink[idx] += val;
}
sink
});
}

#[bench]
fn bench_filter_count(b: &mut Bencher) {
b.iter(|| (0i64..1000000).map(black_box).filter(|x| x % 3 == 0).count())
Expand Down
25 changes: 23 additions & 2 deletions library/core/src/iter/adapters/cloned.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
use crate::iter::adapters::{
zip::try_get_unchecked, TrustedRandomAccess, TrustedRandomAccessNoCoerce,
zip::try_get_unchecked, SourceIter, TrustedRandomAccess, TrustedRandomAccessNoCoerce,
};
use crate::iter::{FusedIterator, TrustedLen, UncheckedIterator};
use crate::iter::{FusedIterator, InPlaceIterable, TrustedLen, UncheckedIterator};
use crate::ops::Try;
use core::num::NonZeroUsize;

/// An iterator that clones the elements of an underlying iterator.
///
Expand Down Expand Up @@ -167,3 +168,23 @@ impl<I: Default> Default for Cloned<I> {
Self::new(Default::default())
}
}

#[unstable(issue = "none", feature = "inplace_iteration")]
unsafe impl<I> SourceIter for Cloned<I>
where
I: SourceIter,
{
type Source = I::Source;

#[inline]
unsafe fn as_inner(&mut self) -> &mut I::Source {
// SAFETY: unsafe function forwarding to unsafe function with the same requirements
unsafe { SourceIter::as_inner(&mut self.it) }
}
}

#[unstable(issue = "none", feature = "inplace_iteration")]
unsafe impl<I: InPlaceIterable> InPlaceIterable for Cloned<I> {
const EXPAND_BY: Option<NonZeroUsize> = I::EXPAND_BY;
const MERGE_BY: Option<NonZeroUsize> = I::MERGE_BY;
}
24 changes: 22 additions & 2 deletions library/core/src/iter/adapters/copied.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::iter::adapters::{
zip::try_get_unchecked, TrustedRandomAccess, TrustedRandomAccessNoCoerce,
zip::try_get_unchecked, SourceIter, TrustedRandomAccess, TrustedRandomAccessNoCoerce,
};
use crate::iter::{FusedIterator, TrustedLen};
use crate::iter::{FusedIterator, InPlaceIterable, TrustedLen};
use crate::mem::MaybeUninit;
use crate::mem::SizedTypeProperties;
use crate::num::NonZeroUsize;
Expand Down Expand Up @@ -255,3 +255,23 @@ impl<I: Default> Default for Copied<I> {
Self::new(Default::default())
}
}

#[unstable(issue = "none", feature = "inplace_iteration")]
unsafe impl<I> SourceIter for Copied<I>
where
I: SourceIter,
{
type Source = I::Source;

#[inline]
unsafe fn as_inner(&mut self) -> &mut I::Source {
// SAFETY: unsafe function forwarding to unsafe function with the same requirements
unsafe { SourceIter::as_inner(&mut self.it) }
}
}

#[unstable(issue = "none", feature = "inplace_iteration")]
unsafe impl<I: InPlaceIterable> InPlaceIterable for Copied<I> {
const EXPAND_BY: Option<NonZeroUsize> = I::EXPAND_BY;
const MERGE_BY: Option<NonZeroUsize> = I::MERGE_BY;
}
52 changes: 51 additions & 1 deletion library/core/src/iter/adapters/skip.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
use crate::intrinsics::unlikely;
use crate::iter::adapters::zip::try_get_unchecked;
use crate::iter::TrustedFused;
use crate::iter::{adapters::SourceIter, FusedIterator, InPlaceIterable};
use crate::iter::{
adapters::SourceIter, FusedIterator, InPlaceIterable, TrustedLen, TrustedRandomAccess,
TrustedRandomAccessNoCoerce,
};
use crate::num::NonZeroUsize;
use crate::ops::{ControlFlow, Try};

Expand Down Expand Up @@ -152,6 +156,32 @@ where

NonZeroUsize::new(n).map_or(Ok(()), Err)
}

#[doc(hidden)]
unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item
where
Self: TrustedRandomAccessNoCoerce,
{
// SAFETY: the caller must uphold the contract for
// `Iterator::__iterator_get_unchecked`.
//
// Dropping the skipped prefix when index 0 is passed is safe
// since
// * the caller passing index 0 means that the inner iterator has more items than `self.n`
// * TRA contract requires that get_unchecked will only be called once
Copy link
Member

Choose a reason for hiding this comment

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

Nit: IIUC the guarantee for being called once is just once when idx == 0, but this kinda reads like it's universally true (even though that makes no sense).

// (unless elements are copyable)
// * it does not conflict with in-place iteration since index 0 must be accessed
// before something is written into the storage used by the prefix
unsafe {
if Self::MAY_HAVE_SIDE_EFFECT && idx == 0 {
for skipped_idx in 0..self.n {
drop(try_get_unchecked(&mut self.iter, skipped_idx));
}
Copy link
Member Author

Choose a reason for hiding this comment

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

In light of #85969 I must note that this alters (undocumented) side-effects for backwards iteration, albeit in less surprising ways.

This block is written to preserve side-effects of the skipped portion of the iterator during forward iteration. During backward iteration it will lead to the whole skipped portion being drained on the last (backwards) step instead of only the first item beyond the skipped portion, i.e. it behaves as if the last item were obtained by a next() instead of next_back().

Copy link
Member

Choose a reason for hiding this comment

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

Hm, so, my interpretation of what you're saying: we're picking idx == 0, which might not actually be the first index the user iterates over (and won't be in backwards iteration).

But I think my interpretation is probably wrong -- I've tried several times and I cannot manage to find a case where your patch causes different behavior in map(|i| dbg!(i)).skip(n) behavior in forward or backward iteration. Do you have an example where it makes causes an observable difference?

Copy link
Member Author

Choose a reason for hiding this comment

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

next_back doesn't touch the inner's skipped prefix. But anything that goes backwards via __iterator_get_unchecked (e.g. Zip) would once it hits index 0.

Copy link
Member

Choose a reason for hiding this comment

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

@the8472 Okay, this is a slight behavioral change to behavior we never made a promise about. I think it's a change we're allowed to make, but to be safe, that decision should be left to t-libs-api FCP.

Do you mind writing up the details about before/after differences so that that can be done?

}

try_get_unchecked(&mut self.iter, idx + self.n)
}
}
}

#[stable(feature = "rust1", since = "1.0.0")]
Expand Down Expand Up @@ -237,3 +267,23 @@ unsafe impl<I: InPlaceIterable> InPlaceIterable for Skip<I> {
const EXPAND_BY: Option<NonZeroUsize> = I::EXPAND_BY;
const MERGE_BY: Option<NonZeroUsize> = I::MERGE_BY;
}

#[doc(hidden)]
#[unstable(feature = "trusted_random_access", issue = "none")]
unsafe impl<I> TrustedRandomAccess for Skip<I> where I: TrustedRandomAccess {}

#[doc(hidden)]
#[unstable(feature = "trusted_random_access", issue = "none")]
unsafe impl<I> TrustedRandomAccessNoCoerce for Skip<I>
where
I: TrustedRandomAccessNoCoerce,
{
const MAY_HAVE_SIDE_EFFECT: bool = I::MAY_HAVE_SIDE_EFFECT;
}

// SAFETY: This adapter is shortening. TrustedLen requires the upper bound to be calculated correctly.
// These requirements can only be satisfied when the upper bound of the inner iterator's upper
// bound is never `None`. I: TrustedRandomAccess happens to provide this guarantee while
// I: TrustedLen would not.
#[unstable(feature = "trusted_len", issue = "37572")]
unsafe impl<I> TrustedLen for Skip<I> where I: Iterator + TrustedRandomAccess {}
16 changes: 9 additions & 7 deletions library/core/src/iter/adapters/step_by.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::convert::TryFrom;
use crate::{
intrinsics,
iter::{from_fn, TrustedLen},
iter::{from_fn, TrustedLen, TrustedRandomAccess},
ops::{Range, Try},
};

Expand Down Expand Up @@ -124,6 +124,14 @@ where
#[stable(feature = "iterator_step_by", since = "1.28.0")]
impl<I> ExactSizeIterator for StepBy<I> where I: ExactSizeIterator {}

// SAFETY: This adapter is shortening. TrustedLen requires the upper bound to be calculated correctly.
// These requirements can only be satisfied when the upper bound of the inner iterator's upper
// bound is never `None`. I: TrustedRandomAccess happens to provide this guarantee while
// I: TrustedLen would not.
// This also covers the Range specializations since the ranges also implement TRA
#[unstable(feature = "trusted_len", issue = "37572")]
unsafe impl<I> TrustedLen for StepBy<I> where I: Iterator + TrustedRandomAccess {}

trait SpecRangeSetup<T> {
fn setup(inner: T, step: usize) -> T;
}
Expand Down Expand Up @@ -480,12 +488,6 @@ macro_rules! spec_int_ranges {
acc
}
}

/// Safety: This macro is only applied to ranges over types <= usize
/// which means the inner length is guaranteed to fit into a usize and so
/// the outer length calculation won't encounter clamped values
#[unstable(feature = "trusted_len", issue = "37572")]
unsafe impl TrustedLen for StepBy<Range<$t>> {}
)*)
}

Expand Down
7 changes: 5 additions & 2 deletions library/core/tests/iter/adapters/step_by.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,8 @@ fn test_iterator_step_by_size_hint() {
assert_eq!(it.len(), 3);

// Cannot be TrustedLen as a step greater than one makes an iterator
// with (usize::MAX, None) no longer meet the safety requirements
// with (usize::MAX, None) no longer meet the safety requirements.
// Exception: The inner iterator is known to have a len() <= usize::MAX
trait TrustedLenCheck {
fn test(self) -> bool;
}
Expand All @@ -235,7 +236,9 @@ fn test_iterator_step_by_size_hint() {
}
}
assert!(TrustedLenCheck::test(a.iter()));
assert!(!TrustedLenCheck::test(a.iter().step_by(1)));
assert!(TrustedLenCheck::test(a.iter().step_by(1)));
assert!(TrustedLenCheck::test(a.iter().chain(a.iter())));
assert!(!TrustedLenCheck::test(a.iter().chain(a.iter()).step_by(1)));
}

#[test]
Expand Down
10 changes: 10 additions & 0 deletions library/core/tests/iter/range.rs
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,16 @@ fn test_range_inclusive_size_hint() {
assert_eq!((imin..=imax + 1).size_hint(), (usize::MAX, None));
}

#[test]
fn test_range_trusted_random_access() {
let mut range = 0..10;
unsafe {
assert_eq!(range.next(), Some(0));
assert_eq!(range.__iterator_get_unchecked(0), 1);
assert_eq!(range.__iterator_get_unchecked(1), 2);
}
}

#[test]
fn test_double_ended_range() {
assert_eq!((11..14).rev().collect::<Vec<_>>(), [13, 12, 11]);
Expand Down
Loading