Skip to content

Document iterator protocol better #8276

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

Merged
merged 2 commits into from
Sep 1, 2013
Merged

Conversation

lilyball
Copy link
Contributor

@lilyball lilyball commented Aug 3, 2013

/// Resets the fuse such that the next call to .next() or .next_back() will
/// call the underlying iterator again even if it prevously returned None.
#[inline]
fn resetFuse(&mut self) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the should be reset_fuse per the naming conventions

@lilyball
Copy link
Contributor Author

lilyball commented Aug 4, 2013

I've sent a proposal to the list about this: https://mail.mozilla.org/pipermail/rust-dev/2013-August/005113.html

@catamorphism
Copy link
Contributor

@kballard , @thestinger - what's the status of this PR? Can it move forward?

@lilyball
Copy link
Contributor Author

I'm still in favor of this approach. I don't know if @thestinger has changed his position at all. It would be nice if other developers would weigh in.

@bluss
Copy link
Member

bluss commented Aug 29, 2013

This change is about defining the guarantees and non-guarantees of the iterator protocol, or conversely, the expectations we place on implementations of trait Iterator, if I understand correctly? Just to put it here so others realize what can be discussed.

@lilyball
Copy link
Contributor Author

Yes. The mail message I linked in my first comment gives an overview of the 3 proposed approaches, and this PR implements the third approach (or more accurately, standardizes it; the current iterators already basically implement approach 3 in an ad-hoc fashion).

@bluss
Copy link
Member

bluss commented Aug 29, 2013

In general, you cannot rely on the behavior of the next() method after it has returned None

it seems rather controversial to me. Even if the side effects question is not resolved, at the very least the Iterator protocol should expect each iterator to return None indefinitely.

@lilyball
Copy link
Contributor Author

@blake2-ppc Have you read the mail message I linked? This is covered there. But the short version is, I believe that if we guarantee None forever, we should guarantee no side-effects too, and Daniel believes that guaranteeing no side-effects is too onerous to require on every iterator adaptor. Also, not guaranteeing None forever allows more flexibility.

@bluss
Copy link
Member

bluss commented Aug 29, 2013

of course, I don't agree with you on the 2nd case. I share Armin R.'s concerns.

@lilyball
Copy link
Contributor Author

As I said in my reply to Armin R., most clients aren't ever going to touch an iterator after it returns None anyway, and those that need to rely on this behavior (without being able to rely on any one particular iterator that they know returns None forever) can just call .fuse() on it. That method will turn any iterator into one behaving along proposal 1, which I'm calling the "strong guarantee" (i.e. it guarantees that it returns None forever without invoking any side-effects).

@bluss
Copy link
Member

bluss commented Aug 29, 2013

It does add a certain complexity for iterator uses outside plain for-loops. You'll have to judge each iterator if you know it is "sane" (for example VecIterator) or if you have to use .fuse(). I don't see any problems with adopting the 2nd case instead of the 3rd.

@lilyball
Copy link
Contributor Author

I feel like we're just re-hashing the ML convo here.

Anyway, I just feel it's subtly dangerous to say that an iterator must return None forever, without handling the side-effect case. Most people won't even consider side-effects when consuming iterators, and their assumption will be that once an iterator has returned None, calling .next() will be idempotent.

Also, declaring this as the behavior all iterators must follow rules out a class of interesting iterators. I posted one to the ML during the conversation, one that allows you to push an element back onto the front of the iterator (but only one). This iterator would allow you to decide that you don't want to handle a particular value yet, push it back, break out of your inner loop, and let the outer loop handle it. Basically equivalent to the Peekable iterator but perhaps a bit easier to use. However, it relies on the ability to return a value after returning None.

@bluss
Copy link
Member

bluss commented Aug 29, 2013

The case of that kind of Peekable is not under this umbrella. If you modify an iterator like that, the iterator protocol guarantees don't apply. Some iterators might even have a .reset() method.

I do read the ML, you don't have to repeat it if you don't want to. I did read it and I still didn't/don't agree that there is consensus or that the chosen guarantee is the best,

Document the fact that the iterator protocol only defines behavior up
until the first None is returned. After this point, iterators are free
to behave how they wish.

Add a new iterator adaptor Fuse<T> that modifies iterators to return
None forever if they returned None once.
Python's zip() short-circuits by not even querying its right-hand
iterator if the left-hand one is done. Match that behavior here by not
calling .next() on the right iterator if the left one returns None.
@lilyball
Copy link
Contributor Author

Fixed the test failure.

@bors bors closed this Sep 1, 2013
@bors bors merged commit a3d18bc into rust-lang:master Sep 1, 2013
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jan 27, 2022
Add `msrv` config for `map_clone`

Just a small PR to have some fun with Clippy and to clear my head a bit 😅

---

changelog: [`map_clone`]: The suggestion takes `msrv` into account
changelog: Track `msrv` attribute for `manual_bits` and `borrow_as_prt`

fixes: rust-lang#8276
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants