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

step_by + skip produce incorrect results #55985

Closed
newpavlov opened this issue Nov 15, 2018 · 3 comments · Fixed by #56049
Closed

step_by + skip produce incorrect results #55985

newpavlov opened this issue Nov 15, 2018 · 3 comments · Fixed by #56049
Labels
C-bug Category: This is a bug. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@newpavlov
Copy link
Contributor

newpavlov commented Nov 15, 2018

The following snippet demonstrates the problem:

println!("first");
for i in (0..640).step_by(128) {
    println!("{}", i)
}
println!("second");
for i in (0..640).step_by(128).skip(1) {
    println!("{}", i)
}
println!("third");
for i in (0..640).step_by(128).map(|i| i).skip(1) {
    println!("{}", i)
}

It prints:

first
0
128
256
384
512
second
128
129
257
385
513
third
128
256
384
512

I expect second iterator to produce the same result as for the third one.

@jethrogb
Copy link
Contributor

Regression from 1.28.0 to 1.29.0. Introduced in #51601

@m1el
Copy link
Contributor

m1el commented Nov 15, 2018

The problem is interaction between StepBy::nth and StepBySpecIterator::spec_next. spec_next ignores the inner working of StepBy.

Test-case reduced to using StepBy without Skip.

fn main() {
    let mut it = (0_usize..640).step_by(128);
    it.nth(0);
    assert_eq!(it.next(), Some(128));
}

@nagisa nagisa added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. C-bug Category: This is a bug. labels Nov 17, 2018
@Emerentius
Copy link
Contributor

Emerentius commented Nov 19, 2018

From a cursory look, it seems to me that the problem is that nth and next are out of sync. StepBy specifically allows two different kind of semantics for the underlying iterator, either next(), nth(step-1), nth(step-1), … or advance_n_and_return_first. If both next and nth follow the same one, the bug shouldn't occur.

In other words, if next is specialized and nth overrides the default, then nth needs to be specialized as well.

bors added a commit that referenced this issue Nov 20, 2018
Revert #51601

Closes: #55985

Specialization of `StepBy<Range(Inclusive)>` results in an incorrectly behaving code when `step_by` is combined with `skip` or `nth`.

If this will get merged we probably should reopen issues previously closed by #51601 (if there was any).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants