Added Range iterator

[TrolledWoods]

I thought an iterator that iterates over a range of values would be pretty neat to have. Use if you want to!

[phimuemue]

Hi there. Surprising to see that there is apparently no existing functionality like this. Could polish it a bit and remove the (probably accidental) formatting changes?

[me22]

One potential reason it doesn't exist is that it can be accomplished with skip-take: If I'm reading things correctly here, .range(a..b) is equivalent to .skip(a).take(b.saturating_sub(a)).

It may still be worth keeping the range method, but consider whether the new iterator struct is actually necessary -- reusing Skip and Take would mean things are automatically double-ended and such where possible. (And I'm not sure whether supporting .range(1..) and .range(..10) are actually valuable, since they're equivalent to .skip(1) and .take(10).)

[jswrenn]

This is very similar to Vec::drain. Should it use the same name?

[TrolledWoods]

@jswrenn I don't think it should be called drain, because it consumes the whole iterator, not just a part of a vector, like drain does. But yeah, the formatting changes were accidental, and reusing skip and take may be a better idea. The problem with those though is that they wouldn't work too well with unlimited iterators. range(5..) only corresponds to the call skip(5) and doesn't need a take, but that changes the type of the iterator. This could potentially be fixed by using an enum, though.

[scottmcm]

I feel pretty strongly that it shouldn't be drain because the important part to me about drain is that it modifies the receiver even if the return value is not used. To me this is far more like Vec::get.

@TrolledWoods One could also use an associated type for the return type so that .range(5..) returns Skip<_> and .range(..5) returns Take<_> and .range(5..10) return Skip<Take<_>>.

[TrolledWoods]

That's a great idea @scottmcm

[TrolledWoods]

Yeah, this is definitely better, now we can have range(..) be the same as a noop, so we keep ALL the functionality of the underlying iterator!

[TrolledWoods]

I was thinking maybe the range standalone function should use IntoIterator instead of Iterator, since it seems like most other standalone functions do that in itertools

[TrolledWoods]

And do we really want the access to the range function to be via itertools::range::range? I wanted to avoid the name clash that pub use range::range; could cause, but maybe I should rename the range.rs file to something like range_traits.rs

[phimuemue]

I like the idea of exploiting existing iterator implementations. Three questions:

• Should we introduce a type alias RangeIterator<R, I> so that our public interface does not expose that the return type is realized as an associated type?

• I guess at some point users want to write:

  fn doit(r: impl std::ops::RangeBounds) {
   give_me_usize_iterator().range(r); // somewhere within the function
  }
  

  This - as far as I can see - does not work because range expects an IntoRangeIter, so the user probably goes ahead and tries:

  fn doit(r: impl IntoRangeIter) {
   give_me_usize_iterator().range(r); // somewhere within the function
  }
  

  But if I am not mistaken this will not work either, because IntoRangeIter requires to specify an iterator upfront. Is it possible to implement IntoRangeIter in a way that does not require the user to specify the iterator?

• Should we try to inform the type system that each IntoRangeIter also satisfies std::ops::RangeBounds so that users can write

  fn doit(r: impl IntoRangeIter<(possibly iter type)> {
    give_me_iter().range(r.clone());
    vec![1,2,3,4,5].drain(r);
  }
  

  without having to specify the (imho opinion) implied RangeBounds trait bound?

[TrolledWoods]

I'm not sure that the TraitBounds -> IntoRangeIter conversion can work, because IntoRangeIter type is implement specifically for each TraitBounds type, so implementing it for all TraitBounds types would be a conflict. I'm not sure if rust team is working on prioritized implementations, but if it's possible to somehow say "use these implementations, or this other even more generic one if the specific ones are not possible".

IntoIterRange implementing RangeBounds is definitely something we should add.

And for some reason I had the idea that trait methods couldn't have generics, but that doesn't seem to be the case, so we shouldn't need to pass around the iterator information in the trait.

[TrolledWoods]

And I'll see if I can figure out that type alias thing. I am not too used to working with some of these things, I have stuck in my little rust box and not gone into other places too much, but it's great that I am now!

[TrolledWoods]

I was thinking that maybe we should rename "IntoRangeIter" to "IterRangeBounds", because I feel like that makes more sense. The bounds aren't the thing that turn into an iterator after all, they just are a RangeBounds that work on an iterator

[TrolledWoods]

I'm going to be honest though, it feels like the associated type version has a few more drawbacks than the previous approach. It adds a new trait, it changes its return type based on the input, it cannot take a generic RangeBounds type, and(giving it some more thought I realized this) you have to pass around the iterator with the range as a type parameter until generic trait types are stable. The current approach is sure a lot easier to implement and less error prone, but I'm not so sure anymore it's the right approach

[jswrenn]

The trait approach is very close to how the core library implements slice indexing: it defines a trait, SliceIndex, and then implements ops::Index for all indices implementing that trait. In practice, the burden of this extra trait is small; I think most end-users are completely unaware that SliceIndex exists!

Likewise, I don't think implementing iterator indexing with an “IteratorIndex” trait would be terribly unergonomic. The symmetry with SliceIndex is a point in its favor, too.

Here's what an implementation mirroring SliceIndex as closely as possible looks like: https://play.rust-lang.org/?edition=2018&gist=2d908fb28c16fb69070ea857c5aa1143

I'm curious if this is a candidate for standard library inclusion.

[TrolledWoods]

Then, making it a trait seems to be very nicely consistant with the standard library! I have really terrible internet at the moment, so it's kindof hard for me to push changes at the moment.

[TrolledWoods]

One thing though, since range is so closely mapped by slice.get(), maybe it should just be called get, and support usize indexing as well? So .range(4) (or .get(4)), is just the same as .nth(4). This range thing is for convenience to begin with, so I don't think that this addition would be strange

[jswrenn]

Agree completely. Both of those changes are already incorporated into the playground URL I linked. :-)

[TrolledWoods]

Welp, I don't know about the naming, but I think it's starting to come together quite nicely now

[jswrenn]

Let's stick to mirroring SliceIndex as closely as possible:

• IterIndex → IteratorIndex
• IterIndex::get → IteratorIndex::index

Could you also just make sure that indentation consistently uses four spaces? Right now it's a mix of tabs and spaces.

[TrolledWoods]

Oh sorry, I prefer tabs for my personal projects, so my editor uses them. I'll rename the things and fix the tabs

[TrolledWoods]

Should I rename the module to iterator_index too, or just keep it as iter_index?

[jswrenn]

iter_index is fine, as it's just an internal implementation detail and not publically exposed.

[TrolledWoods]

Are there any more tweaks that I should do?

[TrolledWoods]

I could limit the IteratorIndex::Output to be an IntoIterator, which could allow people to write some nice generic code with the get if they wanted to. Another option would be to limit the Output to Iterator, but then the .get(5) case would have to return the Option iterator, which I think could be kinda clunky for users

[jswrenn]

Limit IteratorIndex to Iterator (that way it'll play nice with method chaining).

The usize indexing case should be this:

impl<I> IteratorIndex<I> for usize
where
    I: Iterator,
{
    type Output = <Option<I::Item> as IntoIterator>::IntoIter;

    fn index(self, mut iter: I) -> Self::Output {
        iter.nth(self).into_iter()
    }
}

[TrolledWoods]

Done!

[jswrenn]

I'm excited to merge this, but I want to first be sure we won't eventually be in conflict with the core library. I've opened an issue on @rust-lang/rust asking whether this PR would be a good core library addition: rust-lang/rust#73482

[cuviper]

Did you consider using dropping instead of skip? That changes the laziness, but you did use nth in IteratorIndex for usize, which has the same immediate effect.

edit: I guess you'd need to fuse though, in case dropping exhausts the iterator.

[TrolledWoods]

That's a good point. nth isn't consistant with the rest of the things. I think however the better approach is to make IteratorIndex for usize lazy instead of making everything else pre-evaluated. I think having it be lazy makes more sense. Not sure what you think, though

[cuviper]

Another consideration -- slice get(range) returns new slices, but get(usize) just returns the element, whereas you have IteratorIndex returning new iterators in all cases. But if iterator get(usize) were to return the element directly, that's literally just nth.

It's probably more consistent for iterators to be lazy all around.

[jswrenn]

I'm of two minds.

Approach 1: It's just a shorthand

We could have a get method that's just a shorthand for nth, take and skip. In this possibility, we have get(usize) produce an Option<Item>. Ergonomics are maybe slightly diminished, but the explanation of get is simpler: get is just a shorthand for nth, take and skip.

If an end-user wants to lazily get the n^th element, they still can: they just index on n..=n.

I like the ease of explanation, ease of implementation, and flexibility of this approach.

The downside of this approach is get(usize) consumes the underlying iterator, but the other index types do not. This is weird for two reasons:

1. First, when using Iterator::get in a context that's generic over the indexing type, it's weird that certain instantiations of that type would mutate the consume iterator, but others would not. I don't anticipate IteratorIndex to be used by end-users often, so I'm not too worried about this scenario.
2. Second, we're clearly drawing an analogy to Slice::get, but this lazy/eager discrepancy weakens the analogy: Slice::get never consumes the underlying slice.

Approach 2: Consistency above all

If we keep the Iterator bound on Output, then we probably ought to ensure that get(usize) lazily consumes the underlying iterator. However, there's no lazy nth in the standard library, so we'd either need to use a combination of skip and take for this case, or introduce a new lazy_nth construct.

I like the semantic consistency of this approach. However, it's not as easy to implement, and (IMO) it's not as easy to explain. It's a lot of work just for a lazy_nth. But is there any demand for a lazy_nth construct? I struggle to think of a case where I would use it, and a cursory glance at the issues here and on rust-lang/rust doesn't turn up any requests for it.

---

In the absence of demonstrated demand for a lazy_nth construct, I have a slight preference towards the first approach, but I'm not really convinced either way.

If this issue proves to be major roadblock, let's just remove the IteratorIndex implementation for usize (and change the bound on IteratorIndex::Output to IntoIterator).

[jswrenn]

@TrolledWoods, could you:

1. make the requested changes
2. rustfmt src/index_iter.rs
3. fix the merge conflict

After that, I'd be delighted to merge this!

[TrolledWoods]

I'm terribly sorry it took so long, I completely forgot about this and it seems github is not very adamant about telling you new messages have appeared. If there's anything more you need do tell!

[Philippe-Cholet]

@jswrenn @phimuemue
Hi everyone, I had a mental draft for this. This seems quite promising and I agree to not include .get(usize).
I'm not sure what is the blocker here? Are we waiting for rust-lang/rust#73482 (RFC2845 would fix the painpoint, see #702 (comment))? Or maybe it just got lost in the limbs?! 👻

It would have to be rebased (to run CI) and (thankfully) formatted (for CI to pass).

[Philippe-Cholet]

[`Iterator::take`], [`Iterator::skip`] should be enough.
Remove nth since it is not used anymore.

[Philippe-Cholet]

Remove links.

[Philippe-Cholet]

Remove "an element at a specific location, or returns".

[Philippe-Cholet]

No test about RangeToInclusive (such as ..=2) yet, add one or codecov will rightfully complain.

[jswrenn]

I'd love to see this merged! It just needs a bit of attention and rebasing.

[Philippe-Cholet]

Nice.

@TrolledWoods Are you around to finish this awesome feature of yours?

[TrolledWoods]

I will try to do the changes! Turns out formatting and then un-formatting everything makes it hard for git to merge though, will have to try to clean these commits up somehow...

[TrolledWoods]

Oh, sorry didn't see that it was being worked on already. Nice work!

[Philippe-Cholet]

@TrolledWoods
When I realized it was gonna be painful for anyone to rebase on master, I copied change diffs, pasted on master, formatted, committed with you as co-author (as I could not remove myself from it) and opened #891. There are still small changes to do (like corner cases) that I'll probably do next week.

Thanks for this! I guess this was delayed partially because maintainers were busy elsewhere.