Skip to content

Conversation

fee1-dead
Copy link
Member

Endless Iterators: iterators that are either empty or yields elements infinitely.

Implement ExactSizeIterator for Take when the underlying iterator
: ExactSizeIterator | EndlessIterator. This could have benefits since repeat(item).take(x) is pretty common.

@rust-highfive
Copy link
Contributor

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 20, 2021
@fee1-dead fee1-dead added the A-iterators Area: Iterators label Oct 20, 2021
Copy link
Member

@the8472 the8472 left a comment

Choose a reason for hiding this comment

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

I know it's a draft, so feel free to ignore comments that you're already aware of.

unsafe impl<A: EndlessIterator, B: Iterator> EndlessIterator for Zip<A, B> {}

#[unstable(issue = "none", feature = "endless")]
unsafe impl<A: Iterator, B: EndlessIterator> EndlessIterator for Zip<A, B> {}
Copy link
Member

Choose a reason for hiding this comment

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

If one is endless and the other is finite then the resulting Zip is also finite, so those bounds aren't correct, both need to be endless.

/// The implementation must either be endless or empty.
#[unstable(issue = "none", feature = "endless")]
#[marker]
pub unsafe trait EndlessIterator: Iterator {}
Copy link
Member

Choose a reason for hiding this comment

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

Do you intend to add specializations or implement other unsafe traits based on this one? Otherwise I can't see any unsafe use of that trait in the current PR.

Also, the name is confusing if it can be either empty or endless. I had to revise my reasoning several times after remembering that "endless" here can also mean empty.

impl<I> FusedIterator for Cycle<I> where I: Clone + Iterator {}

#[unstable(issue = "none", feature = "none")]
unsafe impl<I> EndlessIterator for Cycle<I> where I: Clone + EndlessOrExact {}
Copy link
Member

Choose a reason for hiding this comment

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

EndlessIterator is unsafe, EndlessOrExact can be implemented by ExactSizeIterator which isn't.

If this is intended to be unsafe then we need to introduce TrustedUsizeIterator to describe this propery or use TrustedRandomAccessNoCoerce which already happens to have this requirement but also has other ones on top.

Copy link
Member

Choose a reason for hiding this comment

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

If trying to reflect a "not more than an allocatable amount" property, that may be better off with a type representing the logic of fn alloc_guard.

// We need to guarantee the following:
// * We don't ever allocate `> isize::MAX` byte-size objects.
// * We don't overflow `usize::MAX` and actually allocate too little.
//
// On 64-bit we just need to check for overflow since trying to allocate
// `> isize::MAX` bytes will surely fail. On 32-bit and 16-bit we need to add
// an extra guard for this in case we're running on a platform which can use
// all 4GB in user-space, e.g., PAE or x32.
#[inline]
fn alloc_guard(alloc_size: usize) -> Result<(), TryReserveError> {
if usize::BITS < 64 && alloc_size > isize::MAX as usize {
Err(CapacityOverflow.into())
} else {
Ok(())
}
}

FusedIterator,
},
ops::Try,
};
Copy link
Member

Choose a reason for hiding this comment

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

nit: multiple separate use statements would be shorter than the nested ones here.

unsafe impl<A: EndlessIterator, B> EndlessIterator for Chain<A, B> where B: Iterator<Item = A::Item> {}

#[unstable(issue = "none", feature = "endless")]
unsafe impl<A, B: EndlessIterator> EndlessIterator for Chain<A, B> where A: Iterator<Item = B::Item> {}
Copy link
Member

Choose a reason for hiding this comment

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

Similar to the Zip issue: If A is non-empty but finite and B is empty then then Chain is non-empty and finite.


/// An iterator that is either empty or endless.
///
/// The iterator reports a size hint that is either `0, Some(0)` or `usize::MAX, None`.
Copy link
Member

@the8472 the8472 Oct 20, 2021

Choose a reason for hiding this comment

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

Does it also require the size hint to be correct at at all times? Because Flatten is lazy and initially returns 0, Some(0) even when it's not empty.

Also note that generally (usize::MAX, None) can also mean "length exceeds usize::MAX", so the comment should note that the trait gives it a more specific meaning.

Copy link
Member

Choose a reason for hiding this comment

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

You had be worried for a second here -- I think you mean 0, None even when it's not empty? https://doc.rust-lang.org/1.56.0/src/core/iter/adapters/flatten.rs.html#313-332

If Flatten can return 0, Some(0) when it's not empty, please file a bug!

Copy link
Member

Choose a reason for hiding this comment

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

I meant (0, None)

U::IntoIter: EndlessIterator,
F: FnMut(I::Item) -> U,
{
}
Copy link
Member

Choose a reason for hiding this comment

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

Some of the bounds here seem redundant. If the inner iterators are Endless then it doesn't matter how many items the outer iterator has, the final FlatMap will either yield zero or infinite items.

///
/// The iterator reports a size hint that is either `0, Some(0)` or `usize::MAX, None`.
/// Since this is used to implement `ExactSizeIterator` for some adapters, iterators that
/// are only empty should not implement this.
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite understand this one. Implementing ExactSizeIterator for an always-empty iterator would be valid.

Copy link
Member Author

Choose a reason for hiding this comment

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

This can be used to implement ExactSizeIterator for Take<Repeat<T>>.

@fee1-dead
Copy link
Member Author

Here is a revised idea

InfiniteIterator - An iterator that yields elements infinitely. - Safe trait because used to implement safe traits only. (I don't see any potential unsafe specialization would use this trait)

EmptyIterator - An iterator that will never yield any element.

NonEmptyIterator - Basically !EmptyIterator

with that:

impl InfiniteIterator for Cycle<T: NonEmptyIterator>

impl EmptyIterator for Cycle<T: EmptyIterator>

impl EmptyIterator for Chain<A: EmptyIterator, B: EmptyIterator>

impl InfiniteIterator for Chain<A: Iterator, B: InfiniteIterator>

impl InfiniteIterator for Chain<A: InfiniteIterator, B: Iterator>

impl InfiniteIterator for Zip<A: InfiniteIterator, B: InfiniteIterator>

impl EmptyIterator for Zip<A: EmptyIterator, B: EmptyIterator> 

impl InfiniteIterator for Flatten<I: IntoIterator> where I::IntoIter: InfiniteIterator

impl InfiniteIterator for Flatten<I: IntoIterator<Item = T>, T> where T: InfiniteIterator, I::IntoIter: NonEmptyIterator

impl EmptyIterator for Flatten<I: IntoIterator> where I::IntoIter: EmptyIterator

impl EmptyIterator for Flatten<I: IntoIterator> where I::Item: EmptyIterator

other adapters should be easy to figure out.

Now we implement ExactSizeIterator for Take<I> where I: InfiniteIterator | ExactSizeIterator.

It does seem like a lot of traits just to have one purpose with unclear benefits. Closing

@fee1-dead fee1-dead closed this Oct 21, 2021
@the8472
Copy link
Member

the8472 commented Oct 21, 2021

You could try narrowing the scope, e.g. impl<T> ExactSizeIterator for Take<Repeat<T>> would be trivial.

Safe trait because used to implement safe traits only. (I don't see any potential unsafe specialization would use this trait)

I think once it is there a few other uses could be found. E.g. when one arm of Zip is TrustedLen or ExactSize and the other is Infinite then the adapter as a whole could still be exact/trusted because the shorter one wins.

Perhaps in the fare future when const eval and const generics are more powerful this can all be encoded in a single trait which has an associated generic const and a bunch of const functions that encode the range of possible sizes including such things whether it exceeds usize or is infinite and can be queried in where bounds. But at the current pace that's years off into the future.

@scottmcm
Copy link
Member

scottmcm commented Dec 6, 2021

TBH there are a few places where InfiniteIterator would be handy -- especially since it could have a default method, like

pub trait InfiniteIterator: Iterator {
    fn next_infinite(&mut self) -> Self::Item {
        match self.next() {
            Some(x) => x,
            None => infinite_iterator_was_finite(),
        }
    }
}

#[cold]
fn infinite_iterator_was_finite() -> ! {
    unreachable!()
}

And iter::repeat is not the only infinite one -- 0.. is infinite too, for example.

Hmm, and it might be handy for diagnostics, too -- calling for_each or last or count or collect or ... on an InfiniteIterator would be worth linting -- arguably anything on Iterator that takes self (instead of &self or &mut self).

@the8472
Copy link
Member

the8472 commented Dec 6, 2021

There are many more optimizations we could apply to iterators, but the problem is that they all require a swarm of unsafe marker traits.

  • infinite
  • size guaranteed fit into usize (but not necessarily an exact size).
    It could be factored out from from TrustedRandomAccess so it can also be combined with TrustedLen instead (the combination would give us TrustedExactLength). Together with the infinite case it would make Zip specializations more powerful.
  • sorted (consistent with Ord) useful for btree and binary heap, this could allow min() or max() to shortcircuit even after skip() or filter()
  • clone does not mess with the size (so we can optimize Cycle)

And a bunch of other things I have thought of before but can't recall atm.

I would submit some of these myself but they are very niche (at least for the initial optimizations that I can think of) and it already takes a long time to get more implementations for existing traits in (e.g. #85528). Even if the libs mind is willing the reviewer flesh is weak 😅.

@scottmcm
Copy link
Member

scottmcm commented Dec 6, 2021

There are many more optimizations we could apply to iterators, but the problem is that they all require a swarm of unsafe marker traits.

Yes, but that's why I mentioned infinite as a safe trait -- not just something that could be used internally for magic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-iterators Area: Iterators S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants