Stabilize step_by by adding it to Iterator (issue #27741) #41439

ivandardi · 2017-04-21T06:29:44Z

Inspired by itertools' take() method. See issue #27741

rust-highfive · 2017-04-21T06:29:58Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @BurntSushi (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

ivandardi · 2017-04-21T06:33:25Z

This is my first commit to Rust! :D ~~I probably screwed something up~~

I may need help with the docstrings. I feel like they can be improved. Also, I couldn't get it to compile without adding the #[stable] tags. Which are the proper tags to put there?

killercup · 2017-04-21T07:26:54Z

src/libcore/iter/iterator.rs

+    #[inline]
+    #[stable(feature = "rust1", since = "1.0.0")]
+    fn every_nth(self, step: usize) -> EveryNth<Self> where Self: Sized {
+        assert!(step != 0);


Not sure if I'd expect this method to panic, especially if I'm using it programmatically. I'm in no position to define this, but: If this assert stays, I'd add a custom message.

I would personally expect the behaviour of .peek() for every_nth(0).next(). Troublesome to implement though.

@nagisa more like impossible without cloning (peak returns a reference, next would return a value).

From the description ("skipping step elements at a time"), I'd expect every_nth(0) to return every item and every_nth(1) to skip every other item. This would also be consistent with nth.

Yeah, I think it would be best to have that behavior instead. I'll make the changes now.

killercup · 2017-04-21T07:28:12Z

src/libcore/tests/iter.rs

@@ -145,6 +145,22 @@ fn test_iterator_chain_find() {
 }

 #[test]
+fn test_every_nth_one() {


Maybe add another test (with #[should_panic]) to test .every_nth(0)?

Yes, such a test should be added.

ollie27 · 2017-04-21T17:58:40Z

How about if the number passed to every_nth is the number of elements that it should skip? It would mean every_nth(0) would return every element, every_nth(1) would return every other element and so on. It would match nth and remove the awkward panic.

ivandardi · 2017-04-21T19:38:37Z

@ollie27 How would every_nth(1) work?

Like this:

let mut it = (0..).every_nth(1).take(3);
assert_eq!(it.next(), Some(0));
assert_eq!(it.next(), Some(2));
assert_eq!(it.next(), Some(4));
assert_eq!(it.next(), None);

Or like this:

let mut it = (0..).every_nth(1).take(3);
assert_eq!(it.next(), Some(1));
assert_eq!(it.next(), Some(3));
assert_eq!(it.next(), Some(5));
assert_eq!(it.next(), None);

ollie27 · 2017-04-21T20:01:50Z

@ivandardi that's a good question. Both seem reasonable to me.

nagisa · 2017-04-21T21:03:36Z

How would every_nth(1) work?

Probably the first. You can get the behaviour of the 2nd code sample by skipping before hand, whereas you cannot get the first behaviour from the second.

leonardo-m · 2017-04-22T05:30:35Z

This is how Python behaves:

>>> range(10)[::0]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: slice step cannot be zero
>>> range(10)[::1]
[0, 1, 2, 3, 4, 5, 6, 7, 8, 9]
>>> range(10)[::2]
[0, 2, 4, 6, 8]

ivandardi · 2017-04-22T07:55:08Z

Hm... It would be good to keep consistency with other languages too. If we keep the Python behavior, then the current implementation is correct and all it's needed is the #[should_panic] test.

After that we need to discuss whether or not to keep the name every_nth for clarity, how to fix the #[stable] tags and possibly the removal of the Step trait altogether.

nagisa · 2017-04-24T21:11:40Z

If we keep the Python behavior, then the current implementation is correct

Too bad Python has a terrible behaviour in a lot of cases. I feel that at this point it is more in the garden of T-libs to decide what’s the optimal behaviour here.

Here’s what the documentation for the behaviour I would prefer:

Returns an iterator adaptor which yields the next element and then skips the specified number of elements before yielding another element. `every_nth(0)` is an identity adaptor.

# Examples

```
let mut it = (0..).every_nth(0).take(2);
assert_eq!(it.next(), Some(0));
assert_eq!(it.next(), Some(1));
assert_eq!(it.next(), None);
```

```
let mut it = (0..).every_nth(2).take(3);
assert_eq!(it.next(), Some(0));
assert_eq!(it.next(), Some(3));
assert_eq!(it.next(), Some(6));
assert_eq!(it.next(), None);
```

Implementation wise it also needs to be decided whether it is fine for iterator to be implemented as

fn next(&mut self) -> Option<Self::Item> { let n = self.next(); self.take(self.n).count(); n }
// as opposed to
fn next(&mut self) -> Option<Self::Item> { if self.first_take { self.next() } else { self.skip(self.n).next() } }

as these two have distinct behaviour in terms of the state the underlying iterator is after a next. Implementation approach will likely dictate the documentation wording as well.

nagisa · 2017-04-24T21:12:21Z

I’ll take liberty of tagging this as waiting-on-team, as there’s clearly nothing the author can act on.

johncf · 2017-04-26T04:47:47Z

src/libcore/iter/mod.rs

+    fn next(&mut self) -> Option<Self::Item> {
+        let elt = self.iter.next();
+        if self.step > 0 {
+            self.iter.nth(self.step - 1);


If nth returns None here, then the behavior of calling next again is unspecified. Shouldn't that be avoided?

Well, the iterator could be fused so that it never returns a Some(T) again. But I don't know if that's desired behavior, because one could voluntarily .fuse() beforehand.

But the problem here is that iter.next() could return Some(..) and internally hit a None, but calling next() again could return Some(..). So the user is not actually informed that the iterator returned None once, making the behavior of every_nth itself unspecified without using fuse.

Itertools fuses the iterator internally too, so I don't see why not fuse it here too. We'd just need to leave it explicit in the documentation.

I can think of a way to avoid fusing. But that would involve consuming an element when every_nth is called, which might be (slightly?) against the "laziness" principle of iterator combinators.

At least in the basic use case of stepping through ranges, the compiler should be able to inline the whole thing and get rid of every abstraction (the fuse iterator or the first_take flag, nth etc). For other use cases, it might be impossible to optimise away the branch, but it might also be very bad to consume elements in advance.

but it might also be very bad to consume elements in advance.

I wasn't clear enough, I think. I said consume "the first element" in advance, not "all." I'm not quite sure if even consuming the first element in advance could be "bad," except perhaps weakly breaking a principle.

Even if the fused/first_take approaches have the same branching, the current implementation and the one I pointed out do not have the same complexity from the point of view of work on the inner iterator, because of the early nth.
If the final consumer of the every_nth iterator only takes the first element, the underlying iterator will consume 1+n elements with the current implementation (as in @nagisa's first option), while one might expect it to only consume 1 element (as in the second option).

@johncf I think a similar rationale applies to Peekable. It would be easier to write it so that it always contains the current element (or None). Instead it was explicitly coded to keep track of whether the element has been pulled from the inner iterator.

@ranma42 I'm guessing, that rationale would indeed matter when the iterator is consuming data from a network stream, for instance. In that case, consuming any element in advance might cause unexpected blocking behavior.

BurntSushi · 2017-05-09T00:08:56Z

src/libcore/iter/iterator.rs

+    /// assert_eq!(iter.next(), None);
+    /// ```
+    #[inline]
+    #[stable(feature = "rust1", since = "1.0.0")]


You'll want to fix this attribute. It should be given its own feature name and it should be unstable.

Yes, I plan to do that. I just didn't know how when I made the pull request. Also, the feature name depends on whether we get rid of the Step trait or if we'll keep the step trait and have this as every_nth. I personally prefer the former, since there's some code that already uses step_by and wouldn't break as bad with a change in the implementation. That would mean, however, that I'd have to reimplement Iterator for Range, which is not hard, but would definitely take some time. Once all that gets figured out, I'll put the proper attributes.

Does taking the name step_by really require removing the Step trait?

Removing step, steps_between, and is_negative from Step seems logical, as does replacing core::iter::StepBy with the logic worked out here. But redoing all the Range iterators feels like it shouldn't be necessary for this change, and I don't see how the existence of Step conflicts with it.

Because if we rename every_nth to step_by, then there's going to be a method name conflict for ranges, because there would be a step_by from the Iterator trait, and a step_by from the Range struct.

Technically we wouldn't need to remove the Step trait, but if we switch the Range::step_by implementation to the iterator version, then there's going to be no use for the Step trait in the core lib. Besides, it looks like the only implementors of Step are the normal integers. Nobody really uses the Step trait to increment an integer when they can just use += 1. And if people really wanted it, we could just implement Iterator for them and have them return the integer plus one. It would work the same.

@ivandardi I think we should just go with a separate feature name for now. We can do the removal in another PR.

BurntSushi · 2017-05-09T00:10:44Z

cc @rust-lang/libs Are we sufficiently past the step_by stuff that we'd be willing to take every_nth?

BurntSushi · 2017-05-09T20:30:53Z

@ivandardi The libs team discussed this today, and it seems like we're all on board with this direction. It looks like there are still some changes you wanted to make before merging though? For example, is it intentional that every_nth(0) panics?

nagisa · 2017-05-09T20:33:59Z

@BurntSushi I feel like there has been some mis-comunication? It was on the libs team to decide on one of the many proposed behaviours, and I do not see any decision wrt that.

BurntSushi · 2017-05-09T20:43:19Z

@nagisa I see. I'll take a closer look and try to summarize.

ivandardi · 2017-05-09T22:06:50Z

Yes, it's expected that every_nth(0) panics, otherwise it would enter an infinite loop of yielding the first element forever. It's consistent with the behavior seen in Python too.

I'll submit a commit making the changes to the feature name and making it unstable hopefully later today.

nagisa · 2017-05-10T00:30:38Z

Yes, it's expected that every_nth(0) panics, otherwise it would enter an infinite loop of yielding the first element forever. It's consistent with the behavior seen in Python too.

You did agree before that having every_nth(0) be identity iterator transformer could make sense. Have you changed your mind since?

ivandardi · 2017-05-10T01:47:32Z

@nagisa Nope. Makes more sense for every_nth(1) to be the identity, because it means that it will give every 1st element, aka the next element. It also goes in hand with what other languages have, and it's important to have familiar behavior. Yeah, I know I went back and forth in this, but this hasn't been easy to decide! XD But I think everyone is pretty settled on it now.

Also, I was testing with step_by(0) always yielding the first element of the iterable and I actually accidentally crashed my browser a lot of times in the playground while testing it out. If you have a for i in (0..10).every_nth(0), it will loop forever unless you have a break inside the loop. Combine that with the fact that people might have something like

usize step = do_calculation()
for i in (0..len).step_by(step)

where step might accidentally be 0 and it's a really bad thing. I now see why Python throws an exception when the step parameter in their ranges is 0 and that's why I'm also making it panic on step_by(0).

ollie27 · 2017-05-10T02:00:23Z

Makes more sense for every_nth(1) to be the identity, because it means that it will give every 1st element, aka the next element.

That doesn't match nth:

Like most indexing operations, the count starts from zero, so nth(0) returns the first value, nth(1) the second, and so on.

ivandardi · 2017-05-19T03:19:59Z

Awesome! Yeah, it took one month to get this going! At least I'm glad it works. After this, there needs to be another PR to take care of StepByDeprecated and reimplement Iterator for Ranges.

Oh, and a last question: Is it my Github name that gets added to the contributors list automatically? If yes, then I just updated it. :D

bors · 2017-05-19T06:51:37Z

⌛ Testing commit 4955517 with merge 2ed2222...

bors · 2017-05-19T07:10:52Z

💔 Test failed - status-appveyor

BurntSushi · 2017-05-19T10:41:03Z

Oh, and a last question: Is it my Github name that gets added to the contributors list automatically? If yes, then I just updated it. :D

Hmm not sure. @steveklabnik ?

CryZe · 2017-05-19T10:45:32Z

I'm pretty sure it's just the name and email you committed as.

alexcrichton · 2017-05-19T14:40:09Z

@bors: retry

appveyor network issues Tracking issue for spurious network failures on bots #40474

bors · 2017-05-19T17:42:36Z

⌛ Testing commit 4955517 with merge 543691d...

Stabilize step_by by adding it to Iterator (issue #27741) Inspired by itertools' `take()` method. See issue #27741

bors · 2017-05-19T20:41:14Z

☀️ Test successful - status-appveyor, status-travis
Approved by: BurntSushi
Pushing 543691d to master...

Follow-up to rust-lang#41439 (comment) While doing so, remove the now-unused `step`, `steps_between`, and `is_negative` methods from `Step`. Mostly simple, but needed two interesting changes: * Override `Iterator::size_hint` for `iter::StepBy` (so hints aren't lost) * Override `Iterator::size_hint` for `ops::RangeFrom` (so `(0..).size_hint()` returns what `(0..).step_by(1).size_hint()` used to) (It turns out that `(0..).step_by(d)` is used in a bunch of tests, from `cycle` to `vec_deque`.) Incidentally fixes rust-lang#41477

birkenfeld · 2017-05-24T05:56:23Z

Isn't there a specialization for the Range types missing?

@alexcrichton

…alexcrichton Deprecate range-specific `step_by` Deprecation attributes and test updates only. Was replaced by an any-iterator version in rust-lang#41439 Last follow-up (this release) to rust-lang#42110 (comment) r? @alexcrichton

@alexcrichton

…alexcrichton Deprecate range-specific `step_by` Deprecation attributes and test updates only. Was replaced by an any-iterator version in rust-lang#41439 Last follow-up (this release) to rust-lang#42110 (comment) r? @alexcrichton

@alexcrichton

…alexcrichton Deprecate range-specific `step_by` Deprecation attributes and test updates only. Was replaced by an any-iterator version in rust-lang#41439 Last follow-up (this release) to rust-lang#42110 (comment) r? @alexcrichton

@alexcrichton

…alexcrichton Deprecate range-specific `step_by` Deprecation attributes and test updates only. Was replaced by an any-iterator version in rust-lang#41439 Last follow-up (this release) to rust-lang#42110 (comment) r? @alexcrichton

Delete deprecated & unstable range-specific `step_by` Using the new one is annoying while this one exists, since the inherent method hides the one on iterator. Tracking issue: #27741 Replacement: #41439 Deprecation: #42310 for 1.19 Fixes #41477

rust-highfive assigned BurntSushi Apr 21, 2017

killercup reviewed Apr 21, 2017

View reviewed changes

shepmaster added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 21, 2017

alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 21, 2017

nagisa added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 24, 2017

johncf reviewed Apr 26, 2017

View reviewed changes

BurntSushi reviewed May 9, 2017

View reviewed changes

BurntSushi added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels May 9, 2017

BurntSushi added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 9, 2017

frewsxcv mentioned this pull request May 18, 2017

Rollup of 7 pull requests #42095

Closed

Mark-Simulacrum added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 19, 2017

bors added a commit that referenced this pull request May 19, 2017

Auto merge of #41439 - ivandardi:master, r=BurntSushi

543691d

Stabilize step_by by adding it to Iterator (issue #27741) Inspired by itertools' `take()` method. See issue #27741

bors merged commit 4955517 into rust-lang:master May 19, 2017

photino mentioned this pull request May 20, 2017

Tracking issue for step_by stabilization #27741

Closed

scottmcm mentioned this pull request May 20, 2017

Remove deprecated Range::step_by (use Iterator::step_by instead) #42110

Closed

alexcrichton mentioned this pull request May 22, 2017

RFC for prepublication dependencies rust-lang/rfcs#1969

Merged

scottmcm mentioned this pull request May 23, 2017

Tracking issue for step_trait stabilization #42168

Open

4 tasks

scottmcm mentioned this pull request May 30, 2017

Deprecate range-specific step_by #42310

Merged

SimonSapin mentioned this pull request Jun 5, 2017

Range* should overrride more methods of Iterator #39975

Open

scottmcm mentioned this pull request Jul 2, 2017

Delete deprecated & unstable range-specific step_by #43012

Merged

alexcrichton mentioned this pull request Aug 12, 2017

Slower binaries since some days #42935

Closed

Stabilize step_by by adding it to Iterator (issue #27741) #41439

Stabilize step_by by adding it to Iterator (issue #27741) #41439

Conversation

ivandardi commented Apr 21, 2017

rust-highfive commented Apr 21, 2017

ivandardi commented Apr 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ollie27 commented Apr 21, 2017

ivandardi commented Apr 21, 2017

ollie27 commented Apr 21, 2017

nagisa commented Apr 21, 2017 • edited Loading

leonardo-m commented Apr 22, 2017

ivandardi commented Apr 22, 2017

nagisa commented Apr 24, 2017

nagisa commented Apr 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johncf Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottmcm May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BurntSushi commented May 9, 2017

BurntSushi commented May 9, 2017

nagisa commented May 9, 2017 • edited Loading

BurntSushi commented May 9, 2017

ivandardi commented May 9, 2017

nagisa commented May 10, 2017 • edited Loading

ivandardi commented May 10, 2017

ollie27 commented May 10, 2017

ivandardi commented May 19, 2017

bors commented May 19, 2017

bors commented May 19, 2017

BurntSushi commented May 19, 2017

CryZe commented May 19, 2017

alexcrichton commented May 19, 2017

bors commented May 19, 2017

bors commented May 19, 2017

birkenfeld commented May 24, 2017

nagisa commented Apr 21, 2017 •

edited

Loading

johncf Apr 27, 2017 •

edited

Loading

scottmcm May 9, 2017 •

edited

Loading

nagisa commented May 9, 2017 •

edited

Loading

nagisa commented May 10, 2017 •

edited

Loading