rust-lang / libs-team

The home of the library team
Apache License 2.0
116 stars 18 forks source link

Add infallible variant of RangeFrom::next() #262

Open nightkr opened 1 year ago

nightkr commented 1 year ago

Proposal

Problem statement

RangeFrom is almost a convenient way to count and index things, but its adherence to Iterator makes next fallible, even if there is no actual stop condition (as opposed to full Range, which has a defined endpoint).

This is effectively equivalent to the venerable i++.

Motivating examples or use cases

use std::collections::HashMap;

enum Identifier {
    Node,
    Listener(String),
}

fn item_labels(node: &str, ids: &[Identifier]) -> HashMap<String, String> {
    let mut listener_i = 0..;
    ids.iter()
        .map(|id| match id {
            Identifier::Node => ("node".to_string(), node.to_string()),
            Identifier::Listener(listener) => (
                format!("listener.{}", listener_i.next().unwrap()),
                listener.to_string(),
            ),
        })
        .collect()
}

The unwrap is required here to avoid bubbling up the impossible error, but will still set off alarms where panics are forbidden or discouraged (for example, this gets in the way of #[deny(clippy::unwrap_used)]).

Solution sketch

RangeFrom::next()'s implementation is already infallible and can be moved into RangeFrom::advance() (name pending). next() can then be redefined as Some(self.advance()).

Alternatives

Use Iterator::enumerate() (AKA Iterator::zip(0..))

This should be preferred where possible, but doesn't solve cases where you only want to count some subset of values (without filtering out the others entirely).

Increment manually

The same effect can be accomplished by incrementing the integer manually, for example:

fn item_labels(node: &str, ids: &[Identifier]) -> HashMap<String, String> {
    let mut listener_i = 0;
    ids.iter()
        .map(|id| match id {
            Identifier::Node => ("node".to_string(), node.to_string()),
            Identifier::Listener(listener) => {
                let key = format!("listener.{}", listener_i.next().unwrap());
                listener_i += 1;
                (key, listener.to_string())
            }
        })
        .collect()
}

However, the extra statement feels awkward, and breaks up the otherwise nice expression-oriented code into a read-modify-return sequence.

Publish to crates.io

This would be a possible (if a bit ugly) option, but it would have to wrap the fallible RangeFrom::next() (or reimplement all of the machinery from scratch).

Define an InfiniteIterator trait

Conceptually this makes sense, but it interacts awkwardly with Rust's type system. Ideally, Iterator::next() would be implied by InfiniteIterator::advance(), but that makes combinators impossible to implement neatly.

Links and related work

This is effectively equivalent to the venerable i++ operator.

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

Second, if there's a concrete solution:

scottmcm commented 1 year ago

What is it about RangeFrom that it should have this, but other infinite things (say, Repeat) shouldn't? Or would this set a precedent that other things would get the method too?

Personally, my instinct is that if it's worth having, then having a trait for it would be nice, so that Enumerate<Repeat<_>> would also have it, for example.

That said, making a crate to wrap .next().unwrap_unchecked() might also be sufficient.

nit: advance makes me think of the existing advance_by, so I'm not convinced it's a good name.

nightkr commented 1 year ago

What is it about RangeFrom that it should have this, but other infinite things (say, Repeat) shouldn't? Or would this set a precedent that other things would get the method too?

IMO Repeat is already covered for this by Clone, so the primary use-case would be in composition.

Personally, my instinct is that if it's worth having, then having a trait for it would be nice, so that Enumerate<Repeat<_>> would also have it, for example.

I'd like to see a trait, but I don't think it's possible in a way that's both clean and correct by construction. There are three obvious ways this could be done:

Implementations ```rust // 1. Independent implementation trait InfiniteIterator: Iterator { fn advance(&mut self) -> Self::Item; } impl InfiniteIterator for RangeFrom { fn advance(&mut self) -> Self::Item { // from existing Iterator::next() let n = Step::forward(self.start.clone(), 1); mem::replace(&mut self.start, n) } } impl Iterator for RangeFrom { type Item = T; fn next(&mut self) -> Option { Some(self.advance()) } } impl T> InfiniteIterator for Map { fn advance(&mut self) -> Self::Item { (self.f)(self.iter.advance()) } } impl T> Iterator for Map { type Item = T; fn next(&mut self) -> Option { // existing impl, can't reuse `self::advance()` because it is not always implemented self.iter.next().map(&mut self.f) } } // 2. Marker trait trait InfiniteIterator: Iterator { fn advance(&mut self) -> Self::Item { self.next().unwrap() } } impl InfiniteIterator for RangeFrom {} impl InfiniteIterator for Map {} // 3. Blanket impl for InfiniteIterator implies Iterator trait InfiniteIterator { // Can't reuse Iterator::Item if InfiniteIterator is the source of truth type Item; fn advance(&mut self) -> Self::Item; } impl Iterator for I { type Item = ::Item; fn next(&mut self) -> Option { Some(self.advance()) } } impl InfiniteIterator for RangeFrom { fn advance(&mut self) -> Self::Item { // from existing Iterator::next() let n = Step::forward(self.start.clone(), 1); mem::replace(&mut self.start, n) } } // causes conflict with the existing `impl Iterator for Map`, which we need for when `I: !InfiniteIterator` impl T> InfiniteIterator for Map { fn advance(&mut self) -> Self::Item { (self.f)(self.iter.advance()) } } ```

I personally don't like any of these. Option 1 leaves the door wide open for implementations to diverge. Option 2 isn't much better than unwrap(), it doesn't guarantee anything by its design. If we want it to be fast and use unwrap_unchecked() then it'd have to be an unsafe trait which just feels wrong. Option 3 makes it completely impossible for a type to be both always an Iterator and selectively an InfiniteOperator.

That said, making a crate to wrap .next().unwrap_unchecked() might also be sufficient.

Yeah.. this comes down to the correct-by-construction question again. In practice I don't see RangeFrom ever violating this, but I'd rather have it be a promise made by RangeFrom itself, rather than an external crate making the promise on its behalf.

nit: advance makes me think of the existing advance_by, so I'm not convinced it's a good name.

Me neither, consider it a placeholder.

nightkr commented 1 year ago

Another alternative would be to redefine Iterator as a special case of a generic supertrait, but that makes the compatibility story pretty hairy:

Implementation ```rust #![feature(step_trait)] use std::{iter::Step, mem, ops::RangeFrom}; trait GenericIterator { type OptionalItem: MaybeOption; fn next_generic(&mut self) -> Self::OptionalItem; // renamed for demonstration purposes, to avoid clashing with std's Iterator fn map_gen(self, f: F) -> Map where Self: Sized, { Map { iter: self, f } } } trait MaybeOption { type Value; type Map: MaybeOption; fn map(self, f: F) -> Self::Map where F: FnOnce(Self::Value) -> T; fn into_option(self) -> Option; } trait Infinite: MaybeOption { fn into_value(self) -> Self::Value; } impl MaybeOption for Option { type Value = T; type Map = Option; fn map(self, f: F) -> Self::Map where F: FnOnce(Self::Value) -> U, { self.map(f) } fn into_option(self) -> Option { self } } // identity functor struct Id(T); impl MaybeOption for Id { type Value = T; type Map = Id; fn map(self, f: F) -> Self::Map where F: FnOnce(Self::Value) -> U, { Id(f(self.0)) } fn into_option(self) -> Option { Some(self.0) } } impl Infinite for Id { fn into_value(self) -> Self::Value { self.0 } } type Item = <::OptionalItem as MaybeOption>::Value; // Iterator and InfiniteIterator both become special cases trait InfiniteIterator: GenericIterator { fn advance(&mut self) -> Item; } impl InfiniteIterator for I where I: GenericIterator, I::OptionalItem: Infinite, { fn advance(&mut self) -> Item { self.next_generic().into_value() } } trait Iterator: GenericIterator { // renamed for demonstration purposes, to avoid clashing with std's Iterator fn next_fin(&mut self) -> Option>; } impl Iterator for I where I: GenericIterator, { fn next_fin(&mut self) -> Option> { self.next_generic().into_option() } } // RangeFrom is always infinite impl GenericIterator for RangeFrom { type OptionalItem = Id; fn next_generic(&mut self) -> Self::OptionalItem { let n = Step::forward(self.start.clone(), 1); Id(mem::replace(&mut self.start, n)) } } // slices are finite impl<'a, T> GenericIterator for &'a [T] { type OptionalItem = Option<&'a T>; fn next_generic(&mut self) -> Self::OptionalItem { let item = self.first()?; *self = &self[1..]; Some(item) } } // Map is generic over finiteness! struct Map { iter: I, f: F, } impl GenericIterator for Map where I: GenericIterator, F: FnMut(Item) -> T, { type OptionalItem = ::Map; fn next_generic(&mut self) -> Self::OptionalItem { self.iter.next_generic().map(&mut self.f) } } fn main() { let mut infinite = 0..; assert_eq!(infinite.advance(), 0); assert_eq!(infinite.advance(), 1); assert_eq!(infinite.next_fin(), Some(2)); assert_eq!(infinite.map_gen(|i| i + 1).advance(), 4); let finite = [0, 1, 2, 3]; let mut finite = finite.as_slice(); assert_eq!(finite.next_fin(), Some(&0)); assert_eq!(finite.next_fin(), Some(&1)); // Can't .advance(), not an InfiniteIterator // finite.advance(); assert_eq!(finite.map_gen(|i| i + 1).next_fin(), Some(3)); } ```
ChayimFriedman2 commented 1 year ago

When we'll have pattern types, and if we'll be able to refine trait methods using pattern types, we will be able to declare fn next(&mut self) -> Option<Self::Item> is Option::Some or whatever syntax it will be.

nightkr commented 1 year ago

Oh, I didn't know those were in the works. Is there an RFC somewhere?

scottmcm commented 3 months ago

One nice thing about an InfiniteIterator is that it could also enable things like a zip_infinite that wouldn't have some of the optimization/bounds complications that apply to the existing zip.

Such a trait would also open up possibilities like a to-array method that doesn't have to worry about handling the "iterator is too short" cases.