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

Generators should implement either Iterator or IntoIterator. #60062

Open
lachlansneff opened this issue Apr 18, 2019 · 6 comments
Open

Generators should implement either Iterator or IntoIterator. #60062

lachlansneff opened this issue Apr 18, 2019 · 6 comments
Labels
A-coroutines Area: Coroutines A-iterators Area: Iterators C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@lachlansneff
Copy link

Generators (currently unstable) are used as an implementation detail for async/await support in rust, but they can also be useful for writing Iterators and similar.

If you want to turn a Generator into an Iterator, you must wrap it in a new type, but it'd be useful to implement Iterator directory on Generator.

Here's an example of how that could work:

impl<G> Iterator for G where G: Generator<Return = ()> + Unpin {
    type Item = G::Yield;
    
    fn next(&mut self) -> Option<Self::Item> {
        match Pin::new(&mut self.generator).resume() {
            GeneratorState::Yielded(item) => Some(item),
            GeneratorState::Complete(_) => None,
        }
    }
}
@estebank estebank added A-coroutines Area: Coroutines A-iterators Area: Iterators T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Apr 18, 2019
@DutchGhost
Copy link
Contributor

DutchGhost commented Apr 19, 2019

I've experiment with this in the past, and also talked to @Nemo157 about this.
Basically we came up with an implementation using a little wrapper:
https://gist.github.com/DutchGhost/5a6712f3d8b611ccbed6a9f120929e90

An interesting point is, do you allow panic's to happen? Your implementation does panic when .next() is called, and the previous call already finished the generator.

Another interesting point is, what to do with generators that are !Unpin? You can actually make an Iterator out of them directly using macro's, which handle's all the unsafe parts correctly. But that requires a macro, which may not be ideal.

@lachlansneff
Copy link
Author

I don't have a problem with a panic occurring if .next() continues to be called. If the user wants the iterator to allows return None after that, they can just call .fuse() to turn it into a FusedIterator. Unpin generators are a more niche usecase and it wouldn't be a breaking change to support them in the future if it becomes possible.

I've seen the gen_iter crate and it's cool, but I'd rather just have Generators be iterators instead of having to wrap them.

@Nemo157
Copy link
Member

Nemo157 commented Apr 22, 2019

I don't have a problem with a panic occurring if .next() continues to be called. If the user wants the iterator to allows return None after that, they can just call .fuse() to turn it into a FusedIterator.

This is my position as well, but when discussing it on the community Discord there was a lot of pushback against it. The common opinion appears to be that the Iterator::next API guarantees one of the two options it mentions, either returning None forever the same as a FusedIterator, or restarting from the beginning (or some combination of the two, e.g. return None 10 times then restart from the beginning).

@timvermeulen
Copy link
Contributor

@Nemo157 To be fair, the Iterator::next docs don't mention any possible panics, but other than that they don't really guarantee anything:

calling next() again may or may not eventually start returning Some(Item) again at some point.

Iterator::next isn't required to either be fused or restart at some point by any means (or the Chain adaptor, and I'm sure others, would currently be implemented incorrectly).

@alexreg
Copy link
Contributor

alexreg commented Jul 18, 2019

@DutchGhost That looks like a good approach to me. I actually quickly hacked together something similar myself in the past. What do you mean about making an iterator out of !Unpin generators? How is that possible?

Either way, I think we definitely need this in the stdlib, just needs a bit more planning first.

@tema3210
Copy link

If we had pin_cell in std then it could look like:

impl<G> Iterator for G where G: PinCell< Generator<Return = ()> > {
    type Item = G::Content::Yield;
    
    fn next(&mut self) -> Option<Self::Item> {
        match (self.borrow_pin_mut().unwrap()).resume() {
            GeneratorState::Yielded(item) => Some(item),
            GeneratorState::Complete(_) => None,
        }
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-coroutines Area: Coroutines A-iterators Area: Iterators C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants