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

Tracking issue for RFC 2033: Experimentally add coroutines to Rust #43122

Open
aturon opened this Issue Jul 8, 2017 · 34 comments

Comments

Projects
None yet
@aturon
Member

aturon commented Jul 8, 2017

RFC.

This is an experimental RFC, which means that we have enough confidence in the overall direction that we're willing to land an early implementation to gain experience. However, a complete RFC will be required before any stabilization.

This issue tracks the initial implementation.

related issues

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 9, 2017

Member

cc #43076, an initial implementation

Member

alexcrichton commented Jul 9, 2017

cc #43076, an initial implementation

@alexcrichton alexcrichton referenced this issue Jul 22, 2017

Merged

Generator support #43076

7 of 7 tasks complete
@Mitranim

This comment has been minimized.

Show comment
Hide comment
@Mitranim

Mitranim Jul 25, 2017

Copied from #43076:


I'm using this branch for stream-heavy data processing. By streams I mean iterators with blocking FS calls. Because Generator is missing an Iterator or IntoIterator implementation, you must call your own wrapper. Zoxc kindly provided an example, but it's quite unergonomic. Consider:

Python:

def my_iter(iter):
    for value in iter:
        yield value

Rust with generators:

fn my_iter<A, I: Iterator<Item=A>>(iter: I) -> impl Iterator<Item=A> {
    gen_to_iter(move || {
        for value in iter {
            yield value;
        }
    })
}

Two extra steps: inner closure + wrapper, and, worse, you have to write the wrapper yourself. We should be able to do better.

TL:DR: There should be a built-in solution for GeneratorIterator.

Mitranim commented Jul 25, 2017

Copied from #43076:


I'm using this branch for stream-heavy data processing. By streams I mean iterators with blocking FS calls. Because Generator is missing an Iterator or IntoIterator implementation, you must call your own wrapper. Zoxc kindly provided an example, but it's quite unergonomic. Consider:

Python:

def my_iter(iter):
    for value in iter:
        yield value

Rust with generators:

fn my_iter<A, I: Iterator<Item=A>>(iter: I) -> impl Iterator<Item=A> {
    gen_to_iter(move || {
        for value in iter {
            yield value;
        }
    })
}

Two extra steps: inner closure + wrapper, and, worse, you have to write the wrapper yourself. We should be able to do better.

TL:DR: There should be a built-in solution for GeneratorIterator.

@silene

This comment has been minimized.

Show comment
Hide comment
@silene

silene Aug 21, 2017

I was a bit surprised that, during the RFC discussion, links to the C++ world seemed to reference documents dating back from 2015. There have been some progress since then. The latest draft TS for coroutines in C++ is n4680. I guess the content of that draft TS will be discussed again when the complete RFC for Rust's coroutines is worded, so here are some of the salient points.

First, it envisions coroutines in a way similar to what this experimental RFC proposes, that is, they are stackless state machines. A function is a coroutine if and only if its body contains the co_await keyword somewhere (or co_yield which is just syntactic sugar for co_await, or co_return). Any occurrence of co_await in the body marks a suspension point where control is returned to the caller.

The object passed to co_await should provide three methods. The first one tells the state machine whether the suspension should be skipped and the coroutine immediately resumed (kind of a degenerate case). The second method is executed before returning control to the caller; it is meant to be used for chaining asynchronous tasks, handling recursive calls, etc. The third method is executed once the coroutine is resumed, e.g. to construct the value returned by the co_await expression. When implementing most generators, these three methods would have trivial bodies, respectively { return false; }, {}, and {}.

Various customization mechanisms are also provided. They tell how to construct the object received by the caller, how to allocate the local variables of the state machine, what to do at the start of the coroutine (e.g. immediately suspend), what to do at the end, what do to in case of an unhandled exception, what to do with the value passed to co_yield or co_return (how yielded values are passed back to the caller is completely controlled by the code).

silene commented Aug 21, 2017

I was a bit surprised that, during the RFC discussion, links to the C++ world seemed to reference documents dating back from 2015. There have been some progress since then. The latest draft TS for coroutines in C++ is n4680. I guess the content of that draft TS will be discussed again when the complete RFC for Rust's coroutines is worded, so here are some of the salient points.

First, it envisions coroutines in a way similar to what this experimental RFC proposes, that is, they are stackless state machines. A function is a coroutine if and only if its body contains the co_await keyword somewhere (or co_yield which is just syntactic sugar for co_await, or co_return). Any occurrence of co_await in the body marks a suspension point where control is returned to the caller.

The object passed to co_await should provide three methods. The first one tells the state machine whether the suspension should be skipped and the coroutine immediately resumed (kind of a degenerate case). The second method is executed before returning control to the caller; it is meant to be used for chaining asynchronous tasks, handling recursive calls, etc. The third method is executed once the coroutine is resumed, e.g. to construct the value returned by the co_await expression. When implementing most generators, these three methods would have trivial bodies, respectively { return false; }, {}, and {}.

Various customization mechanisms are also provided. They tell how to construct the object received by the caller, how to allocate the local variables of the state machine, what to do at the start of the coroutine (e.g. immediately suspend), what to do at the end, what do to in case of an unhandled exception, what to do with the value passed to co_yield or co_return (how yielded values are passed back to the caller is completely controlled by the code).

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Sep 20, 2017

Contributor

One subtle point that came up is how we handle the partially-empty boxes created inside of box statements with respect to OIBITs/borrows.

For example, if we have something like:

fn foo(...) -> Foo<...> {}
fn bar(...) -> Bar<...> {}
box (foo(...), yield, bar(...))

Then at the yield point, the generator obviously contains a live Foo<...> for OIBIT and borrow purposes. It also contains a semi-empty Box<(Foo<...>, (), Bar<...>)>, and we have to decide whether we should have that mean that it is to be treated like it contains a Box, just the Foo<...>, or something else.

Contributor

arielb1 commented Sep 20, 2017

One subtle point that came up is how we handle the partially-empty boxes created inside of box statements with respect to OIBITs/borrows.

For example, if we have something like:

fn foo(...) -> Foo<...> {}
fn bar(...) -> Bar<...> {}
box (foo(...), yield, bar(...))

Then at the yield point, the generator obviously contains a live Foo<...> for OIBIT and borrow purposes. It also contains a semi-empty Box<(Foo<...>, (), Bar<...>)>, and we have to decide whether we should have that mean that it is to be treated like it contains a Box, just the Foo<...>, or something else.

@masonk

This comment has been minimized.

Show comment
Hide comment
@masonk

masonk Jan 7, 2018

I might be missing something in the RFC, but based on the definition of resume in the Generator struct, and the given examples, it looks like these generators don't have two way communication. Ideally this language construct would allow us to yield values out and resume values into the generator.

Here's an example of implementing the async/await pattern using coroutines in ES6. The generator yields Promises and the coroutine resumes the generator with the unwrapped value of a Promise each time the Promise completes. There is no way this pattern could have been implemented without the two-way communication.

Rust has a problem here because what's the type of resume? In the ES6 example, the generator always yields out some kind of Promise and is always resumed with the unwrapped value of the Promise. However the contained type changes on each line. In other words, first it yields a Promise<X> and is resumed with an X, and then it yields a Promise<Y> and is resumed with a Y. I can imagine various ways of declaring that this generator first yields a Wrapper<X> and then a Wrapper<Y>, and expects to be resumed with an X and then a Y, but I can't imagine how the compiler will prove that this is what happens when the code runs.

TL;DR:
yield value is the less interesting half. It has the potential to be a much more ergonomic way to build an Iterator, but nothing more.

let resumedValue = yield value; is the fun half. It's what turns on the unique flow control possibilities of coroutines.

(Here are some more very interesting ideas for how to use two-way coroutines.)

masonk commented Jan 7, 2018

I might be missing something in the RFC, but based on the definition of resume in the Generator struct, and the given examples, it looks like these generators don't have two way communication. Ideally this language construct would allow us to yield values out and resume values into the generator.

Here's an example of implementing the async/await pattern using coroutines in ES6. The generator yields Promises and the coroutine resumes the generator with the unwrapped value of a Promise each time the Promise completes. There is no way this pattern could have been implemented without the two-way communication.

Rust has a problem here because what's the type of resume? In the ES6 example, the generator always yields out some kind of Promise and is always resumed with the unwrapped value of the Promise. However the contained type changes on each line. In other words, first it yields a Promise<X> and is resumed with an X, and then it yields a Promise<Y> and is resumed with a Y. I can imagine various ways of declaring that this generator first yields a Wrapper<X> and then a Wrapper<Y>, and expects to be resumed with an X and then a Y, but I can't imagine how the compiler will prove that this is what happens when the code runs.

TL;DR:
yield value is the less interesting half. It has the potential to be a much more ergonomic way to build an Iterator, but nothing more.

let resumedValue = yield value; is the fun half. It's what turns on the unique flow control possibilities of coroutines.

(Here are some more very interesting ideas for how to use two-way coroutines.)

@mikeyhew

This comment has been minimized.

Show comment
Hide comment
@mikeyhew

mikeyhew Jan 19, 2018

Contributor

@arielb1

Then at the yield point, the generator obviously contains a live Foo<...> for OIBIT and borrow purposes. It also contains a semi-empty Box<(Foo<...>, (), Bar<...>)>, and we have to decide whether we should have that mean that it is to be treated like it contains a Box, just the Foo<...>, or something else.

I don't know what you mean by "OIBIT". But at the yield point, you do not have a Box<(Foo<...>, (), Bar<...>)> yet. You have a <Box<(Foo<...>, (), Bar<...>)> as Boxed>::Place and a Foo<...> that would need to be dropped if the generator were dropped before resuming.

Contributor

mikeyhew commented Jan 19, 2018

@arielb1

Then at the yield point, the generator obviously contains a live Foo<...> for OIBIT and borrow purposes. It also contains a semi-empty Box<(Foo<...>, (), Bar<...>)>, and we have to decide whether we should have that mean that it is to be treated like it contains a Box, just the Foo<...>, or something else.

I don't know what you mean by "OIBIT". But at the yield point, you do not have a Box<(Foo<...>, (), Bar<...>)> yet. You have a <Box<(Foo<...>, (), Bar<...>)> as Boxed>::Place and a Foo<...> that would need to be dropped if the generator were dropped before resuming.

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Feb 11, 2018

Contributor

Looking at the API, it doesn't seem very ergonomic/idiomatic that you have to check if resume returns Yielded or Complete every single iteration. What makes the most sense is two methods:

fn resume(&mut self) -> Option<Self::Yield>;
fn await_done(self) -> Self::Return;

Note that this would technically require adding an additional state to closure-based generators which holds the return value, instead of immediately returning it. This would make futures and iterators more ergonomic, though.

I also think that explicitly clarifying that dropping a Generator does not exhaust it, stopping it entirely. This makes sense if we view the generator as a channel: resume requests a value from the channel, await_done waits until the channel is closed and returns a final state, and drop simply closes the channel.

Contributor

clarcharr commented Feb 11, 2018

Looking at the API, it doesn't seem very ergonomic/idiomatic that you have to check if resume returns Yielded or Complete every single iteration. What makes the most sense is two methods:

fn resume(&mut self) -> Option<Self::Yield>;
fn await_done(self) -> Self::Return;

Note that this would technically require adding an additional state to closure-based generators which holds the return value, instead of immediately returning it. This would make futures and iterators more ergonomic, though.

I also think that explicitly clarifying that dropping a Generator does not exhaust it, stopping it entirely. This makes sense if we view the generator as a channel: resume requests a value from the channel, await_done waits until the channel is closed and returns a final state, and drop simply closes the channel.

@jnferner

This comment has been minimized.

Show comment
Hide comment
@jnferner

jnferner Feb 28, 2018

Has there been any progress regarding the generator -> iterator conversion? If not, is there any active discussion about it somewhere? It would be useful to link it.
@Nemikolh and @uHOOCCOOHu, I'm curious about why you disagree with @clarcharr's suggestion. Care to share your thoughts?

jnferner commented Feb 28, 2018

Has there been any progress regarding the generator -> iterator conversion? If not, is there any active discussion about it somewhere? It would be useful to link it.
@Nemikolh and @uHOOCCOOHu, I'm curious about why you disagree with @clarcharr's suggestion. Care to share your thoughts?

@phaux

This comment has been minimized.

Show comment
Hide comment
@phaux

phaux Mar 2, 2018

Has there been any progress regarding the generator -> iterator conversion? If not, is there any active discussion about it somewhere?

https://internals.rust-lang.org/t/pre-rfc-generator-integration-with-for-loops/6625

phaux commented Mar 2, 2018

Has there been any progress regarding the generator -> iterator conversion? If not, is there any active discussion about it somewhere?

https://internals.rust-lang.org/t/pre-rfc-generator-integration-with-for-loops/6625

@Flupp

This comment has been minimized.

Show comment
Hide comment
@Flupp

Flupp Mar 28, 2018

I was looking at the current Generator-API and immediately felt uneasy when I read

If Complete is returned then the generator has completely finished with the value provided. It is invalid for the generator to be resumed again.

Instead of relying on the programmer to not resume after completion, I would strongly prefer if this was ensured by the compiler. This is easily possible by using slightly different types:

pub enum GeneratorState<S, Y, R> {
    Yielded(S, Y),
    Complete(R),
}

pub trait Generator where Self: std::marker::Sized {
    type Yield;
    type Return;
    fn resume(self) -> GeneratorState<Self, Self::Yield, Self::Return>;
}

(see this rust playground for a small usage example)

The current API documentation also states:

This function may panic if it is called after the Complete variant has been returned previously. While generator literals in the language are guaranteed to panic on resuming after Complete, this is not guaranteed for all implementations of the Generator trait.

So you might not immediately notice a resume-after-completion at runtime even when it actually occurs. A panic on resume-after-completion needs additional checks to be performed by resume, which would not be necessary with the above idea.

In fact, the same idea was already brought up in a different context, however, the focus of this discussion was not on type safety.

I assume there are good reasons for the current API. Nevertheless I think it is worth (re)considering the above idea to prevent resume-after-completion. This protects the programmer from a class of mistakes similar to use-after-free, which is already successfully prevented by rust.

Flupp commented Mar 28, 2018

I was looking at the current Generator-API and immediately felt uneasy when I read

If Complete is returned then the generator has completely finished with the value provided. It is invalid for the generator to be resumed again.

Instead of relying on the programmer to not resume after completion, I would strongly prefer if this was ensured by the compiler. This is easily possible by using slightly different types:

pub enum GeneratorState<S, Y, R> {
    Yielded(S, Y),
    Complete(R),
}

pub trait Generator where Self: std::marker::Sized {
    type Yield;
    type Return;
    fn resume(self) -> GeneratorState<Self, Self::Yield, Self::Return>;
}

(see this rust playground for a small usage example)

The current API documentation also states:

This function may panic if it is called after the Complete variant has been returned previously. While generator literals in the language are guaranteed to panic on resuming after Complete, this is not guaranteed for all implementations of the Generator trait.

So you might not immediately notice a resume-after-completion at runtime even when it actually occurs. A panic on resume-after-completion needs additional checks to be performed by resume, which would not be necessary with the above idea.

In fact, the same idea was already brought up in a different context, however, the focus of this discussion was not on type safety.

I assume there are good reasons for the current API. Nevertheless I think it is worth (re)considering the above idea to prevent resume-after-completion. This protects the programmer from a class of mistakes similar to use-after-free, which is already successfully prevented by rust.

@Nemo157

This comment has been minimized.

Show comment
Hide comment
@Nemo157

Nemo157 Mar 28, 2018

Contributor

I too would have preferred a similar construction for the compile time safety. Unfortunately, that construction doesn't work with immovable generators, once they have been resumed they can't ever be passed by value. I can't think of a way to encode that constraint in a similar way for pinned references, it seems you need some kind of affine reference that you can pass in and recieve back in the GeneratorState::Yielded variant rather than the current lifetime scoped Pin reference.

Contributor

Nemo157 commented Mar 28, 2018

I too would have preferred a similar construction for the compile time safety. Unfortunately, that construction doesn't work with immovable generators, once they have been resumed they can't ever be passed by value. I can't think of a way to encode that constraint in a similar way for pinned references, it seems you need some kind of affine reference that you can pass in and recieve back in the GeneratorState::Yielded variant rather than the current lifetime scoped Pin reference.

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Mar 28, 2018

Contributor

A resume/await_done version seems much more ergonomic than moving the generator every time resume is called. And plus, this would prevent all of @withoutboats' work on pinning from actually being applied.

Contributor

clarcharr commented Mar 28, 2018

A resume/await_done version seems much more ergonomic than moving the generator every time resume is called. And plus, this would prevent all of @withoutboats' work on pinning from actually being applied.

@rpjohnst

This comment has been minimized.

Show comment
Hide comment
@rpjohnst

rpjohnst Mar 28, 2018

Contributor

Note that Iterator has a similar constraint- it's not really a big deal, it doesn't affect safety, and the vast majority of users of the trait don't even have to worry about it.

Contributor

rpjohnst commented Mar 28, 2018

Note that Iterator has a similar constraint- it's not really a big deal, it doesn't affect safety, and the vast majority of users of the trait don't even have to worry about it.

@Lisoph

This comment has been minimized.

Show comment
Hide comment
@Lisoph

Lisoph Apr 5, 2018

Question regarding the current experimental implementation: Can the yield and return types of generators (move-like syntax) be annotated? I would like to do the following:

use std::hash::Hash;

// Somehow add annotations so that `generator` implements
// `Generator<Yield = Box<Hash>, Return = ()>`.
// As of now, `Box<i32>` gets deduced for the Yield type.
let mut generator = || {
    yield Box::new(123i32);
    yield Box::new("hello");
};

Lisoph commented Apr 5, 2018

Question regarding the current experimental implementation: Can the yield and return types of generators (move-like syntax) be annotated? I would like to do the following:

use std::hash::Hash;

// Somehow add annotations so that `generator` implements
// `Generator<Yield = Box<Hash>, Return = ()>`.
// As of now, `Box<i32>` gets deduced for the Yield type.
let mut generator = || {
    yield Box::new(123i32);
    yield Box::new("hello");
};
@Nemo157

This comment has been minimized.

Show comment
Hide comment
@Nemo157

Nemo157 Apr 5, 2018

Contributor

I was hopeful that let mut generator: impl Generator<Yield = Box<Debug>> = || { ... }; might allow this, but testing with

fn foo() -> impl Generator<Yield = Box<Debug + 'static>> {
    || {
        yield Box::new(123i32);
        yield Box::new("hello");
    }
}

it seems the associated types of the return value aren't used to infer the types for the yield expression; this could be different once let _: impl Trait is implemented, but I wouldn't expect it to be.

(Note that Hash can't be used as a trait object because its methods have generic type parameters which must go through monomorphization).

One terrible way to do this is to place an unreachable yield at the start of the generator declaring its yield and return types, e.g.:

let mut generator = || {
    if false { yield { return () } as Box<Debug> };
    yield Box::new(123i32);
    yield Box::new("hello");
};

EDIT: The more I look at yield { return () } as Box<Debug> the more I wonder how long till Cthulu truly owns me.

Contributor

Nemo157 commented Apr 5, 2018

I was hopeful that let mut generator: impl Generator<Yield = Box<Debug>> = || { ... }; might allow this, but testing with

fn foo() -> impl Generator<Yield = Box<Debug + 'static>> {
    || {
        yield Box::new(123i32);
        yield Box::new("hello");
    }
}

it seems the associated types of the return value aren't used to infer the types for the yield expression; this could be different once let _: impl Trait is implemented, but I wouldn't expect it to be.

(Note that Hash can't be used as a trait object because its methods have generic type parameters which must go through monomorphization).

One terrible way to do this is to place an unreachable yield at the start of the generator declaring its yield and return types, e.g.:

let mut generator = || {
    if false { yield { return () } as Box<Debug> };
    yield Box::new(123i32);
    yield Box::new("hello");
};

EDIT: The more I look at yield { return () } as Box<Debug> the more I wonder how long till Cthulu truly owns me.

@Lisoph

This comment has been minimized.

Show comment
Hide comment
@Lisoph

Lisoph Apr 5, 2018

Yeah, I was hoping as well impl Trait would do the trick, but couldn't get it to work either. Your if false { yield { return () } as Box<Debug> }; hack does indeed work, though after seeing that, I don't think I will be able to sleep for tonight.

I guess the only way is to introduce more syntax to annotate the types?

Lisoph commented Apr 5, 2018

Yeah, I was hoping as well impl Trait would do the trick, but couldn't get it to work either. Your if false { yield { return () } as Box<Debug> }; hack does indeed work, though after seeing that, I don't think I will be able to sleep for tonight.

I guess the only way is to introduce more syntax to annotate the types?

@Diggsey

This comment has been minimized.

Show comment
Hide comment
@Diggsey

Diggsey Apr 5, 2018

Contributor

Will the Generator::resume() method be changed to use Pin<Self> and be safe, or is the idea to add a new SafeGenerator trait?

Contributor

Diggsey commented Apr 5, 2018

Will the Generator::resume() method be changed to use Pin<Self> and be safe, or is the idea to add a new SafeGenerator trait?

@Nemo157

This comment has been minimized.

Show comment
Hide comment
@Nemo157

Nemo157 Apr 7, 2018

Contributor

I assumed that it would be changed, and I happened to be looking at the Pin RFC just now and noticed that it agrees, but it is blocked on object safety of arbitrary self types (which is currently an open RFC):

Once the arbitrary_self_types feature becomes object safe, we will make three changes to the generator API:

  1. We will change the resume method to take self by self: Pin<Self> instead of &mut self.
  2. We will implement !Unpin for the anonymous type of an immovable generator.
  3. We will make it safe to define an immovable generator.

The third point has actually happened already, but it doesn't help much since that required making Generator::resume unsafe.

Contributor

Nemo157 commented Apr 7, 2018

I assumed that it would be changed, and I happened to be looking at the Pin RFC just now and noticed that it agrees, but it is blocked on object safety of arbitrary self types (which is currently an open RFC):

Once the arbitrary_self_types feature becomes object safe, we will make three changes to the generator API:

  1. We will change the resume method to take self by self: Pin<Self> instead of &mut self.
  2. We will implement !Unpin for the anonymous type of an immovable generator.
  3. We will make it safe to define an immovable generator.

The third point has actually happened already, but it doesn't help much since that required making Generator::resume unsafe.

@dylanede

This comment has been minimized.

Show comment
Hide comment
@dylanede

dylanede Apr 12, 2018

Contributor

I've found a use case that suggests that the Yield associated type should be parameterised by a lifetime (and thus rely on GATs). The trait would then look like this (with explicit lifetimes for clarity):

pub trait Generator {
    type Yield<'a>;
    type Return;
    unsafe fn resume<'a>(self: Pin<'a, Self>) -> GeneratorState<Self::Yield<'a>, Self::Return>;
}

This version of the trait allows a generator to yield a reference to a local variable or other variables constrained by the lifetime of the generator.

Contributor

dylanede commented Apr 12, 2018

I've found a use case that suggests that the Yield associated type should be parameterised by a lifetime (and thus rely on GATs). The trait would then look like this (with explicit lifetimes for clarity):

pub trait Generator {
    type Yield<'a>;
    type Return;
    unsafe fn resume<'a>(self: Pin<'a, Self>) -> GeneratorState<Self::Yield<'a>, Self::Return>;
}

This version of the trait allows a generator to yield a reference to a local variable or other variables constrained by the lifetime of the generator.

@dylanede

This comment has been minimized.

Show comment
Hide comment
@dylanede

dylanede Apr 12, 2018

Contributor

What's curious is that currently on nightly you can write a generator that yields a reference to a local variable, but not use it. As soon as you put in a call to resume, compilation fails.

Contributor

dylanede commented Apr 12, 2018

What's curious is that currently on nightly you can write a generator that yields a reference to a local variable, but not use it. As soon as you put in a call to resume, compilation fails.

@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats Apr 12, 2018

Contributor

@dylanede that is curious, can you post a playpen example?

Contributor

withoutboats commented Apr 12, 2018

@dylanede that is curious, can you post a playpen example?

@dylanede

This comment has been minimized.

Show comment
Hide comment
@dylanede

dylanede Apr 12, 2018

Contributor

Here it is in the playpen: https://play.rust-lang.org/?gist=4cc04defbebf06fa55a3074499d256ad&version=nightly

The line to uncomment to trigger the compilation error is not mentioned in the error.

Contributor

dylanede commented Apr 12, 2018

Here it is in the playpen: https://play.rust-lang.org/?gist=4cc04defbebf06fa55a3074499d256ad&version=nightly

The line to uncomment to trigger the compilation error is not mentioned in the error.

@dylanede

This comment has been minimized.

Show comment
Hide comment
@dylanede

dylanede Apr 13, 2018

Contributor

I've come up with a hacky wrapper around generators that implements a trait like the one I mentioned above, so that you can return local references from generators. Caveats are mentioned inline. Take this as a proof-of-concept and motivating use case for changing the Generator trait.

https://play.rust-lang.org/?gist=74fe6554d3fa5503594bb48889caed49&version=nightly

Contributor

dylanede commented Apr 13, 2018

I've come up with a hacky wrapper around generators that implements a trait like the one I mentioned above, so that you can return local references from generators. Caveats are mentioned inline. Take this as a proof-of-concept and motivating use case for changing the Generator trait.

https://play.rust-lang.org/?gist=74fe6554d3fa5503594bb48889caed49&version=nightly

@dylanede

This comment has been minimized.

Show comment
Hide comment
@dylanede

dylanede Apr 13, 2018

Contributor

One possible signature for the Generator trait that avoids the need for GATs is

pub trait Generator<'a> {
    type Yield;
    type Return;
    unsafe fn resume(self: Pin<'a, Self>) -> GeneratorState<Self::Yield, Self::Return>;
}

Users of the trait would then typically refer to for<'a> Generator<'a>.

Contributor

dylanede commented Apr 13, 2018

One possible signature for the Generator trait that avoids the need for GATs is

pub trait Generator<'a> {
    type Yield;
    type Return;
    unsafe fn resume(self: Pin<'a, Self>) -> GeneratorState<Self::Yield, Self::Return>;
}

Users of the trait would then typically refer to for<'a> Generator<'a>.

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Apr 15, 2018

Contributor

@dylanede given that GATs are already on the list to be implemented, I'd rather wait on them. It'd be very unfortunate to be locked into a more frustrating syntax which is inconsistent with Iterator.

Contributor

clarcharr commented Apr 15, 2018

@dylanede given that GATs are already on the list to be implemented, I'd rather wait on them. It'd be very unfortunate to be locked into a more frustrating syntax which is inconsistent with Iterator.

@newpavlov

This comment has been minimized.

Show comment
Hide comment
@newpavlov

newpavlov Apr 20, 2018

Contributor

When suggesting Generator changes, please, do not forget about the fact that they can be used outside of async use-cases. Also I think it's worth to consider renaming Yield to Item for better compatibility with Iterator and potential future unification.

Contributor

newpavlov commented Apr 20, 2018

When suggesting Generator changes, please, do not forget about the fact that they can be used outside of async use-cases. Also I think it's worth to consider renaming Yield to Item for better compatibility with Iterator and potential future unification.

@nugend

This comment has been minimized.

Show comment
Hide comment
@nugend

nugend Apr 28, 2018

fwiw, yield is used in Python and Javascript for generators not related to async behavior.

nugend commented Apr 28, 2018

fwiw, yield is used in Python and Javascript for generators not related to async behavior.

@ishitatsuyuki

This comment has been minimized.

Show comment
Hide comment
@ishitatsuyuki

ishitatsuyuki May 6, 2018

Member

Has anyone looked into the experimental LLVM coroutine support? @Zoxc on IRC mentioned that the current IR generated is hard to optimize, and the situation may improve if we passed it using the native LLVM facilities.

Member

ishitatsuyuki commented May 6, 2018

Has anyone looked into the experimental LLVM coroutine support? @Zoxc on IRC mentioned that the current IR generated is hard to optimize, and the situation may improve if we passed it using the native LLVM facilities.

@rpjohnst

This comment has been minimized.

Show comment
Hide comment
@rpjohnst

rpjohnst May 6, 2018

Contributor

Last time I looked at it, LLVM coroutines were basically purpose-built for C++ coroutines, which default to allocating their state block. I'm not sure how easy it would be to get around that, and even without that there are layering problems.

It would be nice to be able to optimize the code pre-state-machine-transformation without lifting all of LLVM into MIR, though. :)

Contributor

rpjohnst commented May 6, 2018

Last time I looked at it, LLVM coroutines were basically purpose-built for C++ coroutines, which default to allocating their state block. I'm not sure how easy it would be to get around that, and even without that there are layering problems.

It would be nice to be able to optimize the code pre-state-machine-transformation without lifting all of LLVM into MIR, though. :)

bors added a commit that referenced this issue May 26, 2018

Auto merge of #51090 - kennytm:tidy-check-missing-tracking-issue, r=a…
…lexcrichton

Ensure every unstable language feature has a tracking issue.

Filled in the missing numbers:

* `abi_ptx` → #38788
* `generators` → #43122
* `global_allocator` → #27389

Reused existing tracking issues because they were decomposed from a larger feature

* `*_target_feature` → #44839 (reusing the old `target_feature` number)
* `proc_macros_*` → #38356 (reusing the to-be-stabilized `proc_macros` number)

Filed new issues

* `exhaustive_patterns` → #51085
* `pattern_parentheses` → #51087
* `wasm_custom_section` and `wasm_import_module` → #51088

bors added a commit that referenced this issue May 27, 2018

Auto merge of #51090 - kennytm:tidy-check-missing-tracking-issue, r=a…
…lexcrichton

Ensure every unstable language feature has a tracking issue.

Filled in the missing numbers:

* `abi_ptx` → #38788
* `generators` → #43122
* `global_allocator` → #27389

Reused existing tracking issues because they were decomposed from a larger feature

* `*_target_feature` → #44839 (reusing the old `target_feature` number)
* `proc_macros_*` → #38356 (reusing the to-be-stabilized `proc_macros` number)

Filed new issues

* `exhaustive_patterns` → #51085
* `pattern_parentheses` → #51087
* `wasm_custom_section` and `wasm_import_module` → #51088
@zroug

This comment has been minimized.

Show comment
Hide comment
@zroug

zroug Oct 4, 2018

I noticed that Generator is implemented for mutable references to generators. I think that this is unfortunate because it prevents any safe abstractions for the Generator trait.

Consider this seemingly safe function:

fn get_first_yield<T>(mut gen: impl Generator<Yield = T>) -> Option<T> {
    // We know `resume` wasn't called on `gen` before because the caller needs to
    // move `gen` to call this method. Since we now own `gen` and we won't move it
    // anymore, it is safe for us to call `resume`.
    match unsafe { gen.resume() } {
        GeneratorState::Yielded(value) => Some(value),
        GeneratorState::Complete(_) => None,
    }
    // `gen` gets dropped here, therefore we are sure it isn't moved in the future.
}

According to the documentation of the Generator trait this function should be sound. But it isn't because it can be called with mutable references to generators. Here is a full example on how this function fails: https://play.rust-lang.org/?gist=94d0081f2f4e8fd08e4e0b97989422f1&version=nightly&mode=debug&edition=2018

I know that generators will eventually get a safe api but depending on how long this will take I think it would make sense to remove the implementation of Generator for mutable references to generators now. Once there is a safe api, this implementation isn't possible anyway.

zroug commented Oct 4, 2018

I noticed that Generator is implemented for mutable references to generators. I think that this is unfortunate because it prevents any safe abstractions for the Generator trait.

Consider this seemingly safe function:

fn get_first_yield<T>(mut gen: impl Generator<Yield = T>) -> Option<T> {
    // We know `resume` wasn't called on `gen` before because the caller needs to
    // move `gen` to call this method. Since we now own `gen` and we won't move it
    // anymore, it is safe for us to call `resume`.
    match unsafe { gen.resume() } {
        GeneratorState::Yielded(value) => Some(value),
        GeneratorState::Complete(_) => None,
    }
    // `gen` gets dropped here, therefore we are sure it isn't moved in the future.
}

According to the documentation of the Generator trait this function should be sound. But it isn't because it can be called with mutable references to generators. Here is a full example on how this function fails: https://play.rust-lang.org/?gist=94d0081f2f4e8fd08e4e0b97989422f1&version=nightly&mode=debug&edition=2018

I know that generators will eventually get a safe api but depending on how long this will take I think it would make sense to remove the implementation of Generator for mutable references to generators now. Once there is a safe api, this implementation isn't possible anyway.

@Nemo157

This comment has been minimized.

Show comment
Hide comment
@Nemo157

Nemo157 Oct 4, 2018

Contributor

@zroug #54383 should allow creating a safe api for generators soon ™️ (currently doing a local build of that branch to see if it works, will update in a few hours once it completes), as you mention impl<G> Generator for &mut G where G: Generator will have to disappear and instead we'll get impl<G> Generator for Pin<&mut G> where G: Generator or similar.

Contributor

Nemo157 commented Oct 4, 2018

@zroug #54383 should allow creating a safe api for generators soon ™️ (currently doing a local build of that branch to see if it works, will update in a few hours once it completes), as you mention impl<G> Generator for &mut G where G: Generator will have to disappear and instead we'll get impl<G> Generator for Pin<&mut G> where G: Generator or similar.

@jplatte

This comment has been minimized.

Show comment
Hide comment
@jplatte

jplatte Oct 4, 2018

Contributor

In the same vein, last time I checked generators with borrows across yield points didn't impl !Unpin. That seems like the prime example of an !Unpin type, or am I misunderstanding something?

Contributor

jplatte commented Oct 4, 2018

In the same vein, last time I checked generators with borrows across yield points didn't impl !Unpin. That seems like the prime example of an !Unpin type, or am I misunderstanding something?

@Nemo157

This comment has been minimized.

Show comment
Hide comment
@Nemo157

Nemo157 Oct 4, 2018

Contributor

I can confirm that the following definition of Generator works with the changes in #54383 (full playground of what I tested):

trait Generator {
    type Yield;
    type Return;

    fn resume(self: Pin<&mut Self>) -> GeneratorState<Self::Yield, Self::Return>;
}

impl<G> Generator for Pin<G>
where
    G: DerefMut,
    G::Target: Generator
{
    type Yield = <<G as Deref>::Target as Generator>::Yield;
    type Return = <<G as Deref>::Target as Generator>::Return;

    fn resume(self: Pin<&mut Self>) -> GeneratorState<Self::Yield, Self::Return> {
        <G::Target as Generator>::resume(Pin::get_mut(self).as_mut())
    }
}

I'm going to have a look whether I can figure out the changes to make the MIR transform match this trait as well.

Contributor

Nemo157 commented Oct 4, 2018

I can confirm that the following definition of Generator works with the changes in #54383 (full playground of what I tested):

trait Generator {
    type Yield;
    type Return;

    fn resume(self: Pin<&mut Self>) -> GeneratorState<Self::Yield, Self::Return>;
}

impl<G> Generator for Pin<G>
where
    G: DerefMut,
    G::Target: Generator
{
    type Yield = <<G as Deref>::Target as Generator>::Yield;
    type Return = <<G as Deref>::Target as Generator>::Return;

    fn resume(self: Pin<&mut Self>) -> GeneratorState<Self::Yield, Self::Return> {
        <G::Target as Generator>::resume(Pin::get_mut(self).as_mut())
    }
}

I'm going to have a look whether I can figure out the changes to make the MIR transform match this trait as well.

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Oct 4, 2018

Contributor

Going to again point out what I mentioned earlier: rather than a single resume method, I honestly think that resume should return Option<Yield> and that there should be a separate await method that consumes self and returns Return.

Contributor

clarcharr commented Oct 4, 2018

Going to again point out what I mentioned earlier: rather than a single resume method, I honestly think that resume should return Option<Yield> and that there should be a separate await method that consumes self and returns Return.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment