-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
[experiment] break as many Zip side-effects as we can #83796
Conversation
r? @kennytm (rust-highfive has picked a reviewer for you, use r? to override) |
r? @m-ou-se |
This comment has been minimized.
This comment has been minimized.
423e6be
to
bed379a
Compare
@bors try |
⌛ Trying commit bed379a with merge 46592174fb13958bb9a2aa0d7af6c1224e1d7143... |
☀️ Try build successful - checks-actions |
@craterbot run mode=build-and-test |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🎉 Experiment
|
My first time using crater, analyzing the results will take some time. |
@rustbot label: -S-waiting-on-review +S-waiting-on-author |
ping from triage: |
Haven't had the time yet, I'll take a look on monday. |
Local testing setup:
Results: Build failed cases
Test failed casescan't reproduce, test passed on 1st tryaristeia-0.2.3 tests fail, but also fail on d408fdd.I don't know what crater does differently that they pass there ezhook-0.2.2 tests fail intermittentlyipc-orchestrator-0.3.3 Othernotp-2.1.0 - test fails on first run due to filesystem state, passes on subsequent runs because |
So unless I missed any critical step in reproducing the issues it appears that none of the failures (at least those that I could test) seem related to skipping side-effects on the iterators in question. Of course this does not prove that nobody is relying on those properties, but at least there is a chance that we could eliminate that logic and thus simplify In the meantime #85342 has been suggested, which offers a more conservative path: We implement Tangentially to the ping @m-ou-se @rustbot label +S-waiting-on-review -S-waiting-on-author |
@the8472 Would you mind removing the [experiment] part from title (and optionally give it a more descriptive title), if this is ready for review? Thanks! |
This is removes all the side-effect-preserving code from iter::Zip. On top it converts some
Iterator
internal iteration implementations to counted loops to skip additional side-effects on the default implementation of zip. That's a version of some optimizations I wanted to try but modified to increase coverage. Some of those already ran afoul of the*_sideffectful()
tests.It's designed to increase impact in a crater run for #83791 to see if anyone relies on side-effects.
This is a somewhat bigger change than merely omitting side-effects on the last
next()
during fowards because it can skips many steps worth of side-effects at the end of an iterator when using backwards iteration, which is normally necessary to bring the iterators to equal length before doing a backwards zip.Omitting the side-effects during a forward pass are easily justified and safely implementable with counted loops. There currently isn't anything safe or stable that would justify skipping during backwards iteration. On the other hand the
zip
documentaion doesn't spend a single word on itsDoubleEndedIterator
behavior.@rustbot label +S-experimental