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

Using Zip & Take allows accessing the middle a Map iterator without “getting there first” from either side. #85969

Closed
steffahn opened this issue Jun 3, 2021 · 5 comments · Fixed by #85975
Labels
A-iterators Area: Iterators A-specialization Area: Trait impl specialization C-bug Category: This is a bug. regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@steffahn
Copy link
Member

steffahn commented Jun 3, 2021

Look at this iterator.

let iterator = (0..100).map(|x| println!("item {}", x));

We all know that when map’s resulting Iterator implements DoubleEndedIterator, then the side-effects can be re-ordered. However, judging by the implementation of map one would probably at least expect that it’s impossible to hit the "item 50" without first either going through at least 0, 1, 2, … 49 or 100, 99, 98, … 51 first. Right?

Well, here we go!

let mut y = iterator.take(51).zip(0..100);
y.next_back();

on beta

item 50

on stable 1.52.0

item 99
item 98
item 97
item 96
item 95
item 94
item 93
item 92
item 91
item 90
item 89
item 88
item 87
item 86
item 85
item 84
item 83
item 82
item 81
item 80
item 79
item 78
item 77
item 76
item 75
item 74
item 73
item 72
item 71
item 70
item 69
item 68
item 67
item 66
item 65
item 64
item 63
item 62
item 61
item 60
item 59
item 58
item 57
item 56
item 55
item 54
item 53
item 52
item 51
item 50

(playground)

@rustbot label T-libs-impl, A-iterators, A-specialization, regression-from-stable-to-beta

@steffahn steffahn added the C-bug Category: This is a bug. label Jun 3, 2021
@rustbot rustbot added A-iterators Area: Iterators A-specialization Area: Trait impl specialization regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs Relevant to the library team, which will review and decide on the PR/issue. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jun 3, 2021
@steffahn
Copy link
Member Author

steffahn commented Jun 3, 2021

cc @the8472

@the8472
Copy link
Member

the8472 commented Jun 3, 2021

As mentioned in the other issue, I think this is just another instance of #82303

Edit: Nevermind, not really since it doesn't involve calling __iterator_get_unchecked on Zip, rather this is about use of __iterator_get_unchecked in Zip.

@steffahn
Copy link
Member Author

steffahn commented Jun 3, 2021

It’s related to #82303. I guess the “random” aspect of TrustedRandomAccess is a bit weird for iterators like Map.

You can do the same thing (accessing the map from the center without “walking” there) with Zip only, too, on stable already like so:

let mut y = iterator.zip(0..51).zip(0..100);
y.next_back();

This issue is only “really” a regression if out-of-order side-effects for iterators aren’t wanted.

@the8472
Copy link
Member

the8472 commented Jun 3, 2021

steffahn brought up some general concerns about the TrustedRandomAccess implemention on Take (#83990) in #85873 (comment)
I'll have to think some more about whether combining Take, TRA and backwards iteration is even doing the right thing under all circumstances, beyond the side-effects issue raised here.

I am generally in favor of eliminating iterator side-effects that are not necessary to produce the final results. But of course that's currently undocumented and possibly surprising behavior and needs to be approved by the libs team, which hasn't happened so far. So while I like what it does it shouldn't have landed without discussion.

You can probably do the same thing (accessing the map from the center without “walking” there) with Zip only, too, on stable already.

I don't think so? At least that would be another implementation gap. That's what the whole MAY_HAVE_SIDE_EFFECT thing is supposed to prevent.

let mut y = iterator.zip(0..51).zip(0..100);
y.next_back();

Oh, sure, that definitely is #82303

@the8472
Copy link
Member

the8472 commented Jun 3, 2021

https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/eliminating.20side-effects.20of.20iterator.20adapters/near/241436500

We had written a fair number of specializations over time that were later either removed or just not accepted because they would fail to consider side-effects.

I'll submit a revert PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-iterators Area: Iterators A-specialization Area: Trait impl specialization C-bug Category: This is a bug. regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants