Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upGenerator support #43076
Conversation
rust-highfive
assigned
nikomatsakis
Jul 5, 2017
This comment has been minimized.
This comment has been minimized.
|
(rust_highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
|
cc @rust-lang/compiler, I suspect a number of you may be interested in this! |
This comment has been minimized.
This comment has been minimized.
|
|
alexcrichton
added
the
S-waiting-on-review
label
Jul 6, 2017
Zoxc
force-pushed the
Zoxc:gen
branch
2 times, most recently
from
18a9c9b
to
0cacc20
Jul 6, 2017
nikomatsakis
reviewed
Jul 7, 2017
| @@ -1059,6 +1071,12 @@ pub enum Expr_ { | |||
| /// For example, `[1; 5]`. The first expression is the element | |||
| /// to be repeated; the second is the number of times to repeat it. | |||
| ExprRepeat(P<Expr>, BodyId), | |||
|
|
|||
| /// A suspension point for generators | |||
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Jul 7, 2017
Contributor
It would be good to give an example of Rust syntax; this corresponds to a yield, right?
This comment has been minimized.
This comment has been minimized.
Zoxc
Jul 8, 2017
Author
Contributor
In an older version this was separate from yield, now it's the same and it could use an renaming.
nikomatsakis
reviewed
Jul 7, 2017
| ExprSuspend(P<Expr>), | ||
|
|
||
| /// The argument to a generator | ||
| ExprImplArg(NodeId), |
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Jul 7, 2017
Contributor
This corresponds to a gen arg, right? I am wondering if we should think about removing this from the PR, since it is not needed for async-await, and I find it hard to imagine this syntax ultimately being stabilized (it just seems rather unlike any other syntax we have in the language). I'm not sure how much actual simplicity would result though, maybe very little.
This comment has been minimized.
This comment has been minimized.
eddyb
Jul 7, 2017
Member
I am also against the concept as a whole - it's there to get rid of thread-local state but I do not think it's a satisfactory solution.
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Jul 7, 2017
Contributor
Skimming through the PR, it seems like this would be a reasonably nice simplification.
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Jul 7, 2017
Contributor
I am also against the concept as a whole - it's there to get rid of thread-local state but I do not think it's a satisfactory solution.
Does this mean you are also against yield returning a value, sort of like let x = yield 22?
This comment has been minimized.
This comment has been minimized.
eddyb
Jul 7, 2017
Member
Ah, I see what you're hinting at. In my hand-desugared examples I ended up having the generator equivalent of IntoIterator produce the first Yield/CoResult/etc. alongside the generator itself. The other options are multiple entry points, taking Option<Input>, or maybe a different suspend model. Maybe a yield without a value and produce the first input.
Anyway, what I'm against is any such scheme used with async IO, when it can be avoided.
This comment has been minimized.
This comment has been minimized.
Zoxc
Jul 8, 2017
Author
Contributor
Having explicitly named arguments would be an alternative which is just as expressive and could probably reuse most of the code dealing with regular arguments. One of the problems with this is that if we allow yield inside function and closure bodies in addition to generator literal bodies we need syntax for another set of arguments. This is less of a problem if we're committed to using compiler plugins for ergonomic async I/O.
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Jul 10, 2017
Contributor
I have to admit that I am finding these last 2 comments a bit confusing. e.g., @eddyb, when you write:
The other options are multiple entry points
I don't really understand what you are referring to. The other options for.. what exactly? In any case, it seems like neither iterators nor futures require the ability to provide "feedback" during execution, so I would personally be happy to "defer" that part for later PRs.
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Jul 10, 2017
Contributor
Having explicitly named arguments would be an alternative which is just as expressive and could probably reuse most of the code dealing with regular arguments.
I would think so -- those regular arguments wind up being assigned to standard locals in MIR, iirc, almost immediately upon function entry. In fact, I thikn it would "just work" just fine -- unless you meant that, after a yield, the function arguments would be updated "in place" with the new values?
This comment has been minimized.
This comment has been minimized.
Zoxc
Jul 10, 2017
Author
Contributor
unless you meant that, after a yield, the function arguments would be updated "in place" with the new values?
This is indeed what I meant.
This comment has been minimized.
This comment has been minimized.
eddyb
Jul 10, 2017
Member
@nikomatsakis By "multiple entry points" I meant that you can also have yield produce a value by providing multiple ways to resume a given generator, the one taking no value for the very first time, and then using the one requiring a value. Using the wrong one would "just" panic. If you do have multiple entry points, though, you can also start doing crazier things, like having multiple types that yield can produce, similar to what ends up being done in dynamic language. However, while quite flexible, the dynamic nature of it makes me uneasy and I'm very glad that yield producing no value back to the generator can be used for async I/O.
nikomatsakis
reviewed
Jul 7, 2017
| @@ -1014,7 +1026,7 @@ pub enum Expr_ { | |||
| /// A closure (for example, `move |a, b, c| {a + b + c}`). | |||
| /// | |||
| /// The final span is the span of the argument block `|...|` | |||
| ExprClosure(CaptureClause, P<FnDecl>, BodyId, Span), | |||
| ExprClosure(CaptureClause, P<FnDecl>, BodyId, Span, Option<GeneratorClause>), | |||
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Jul 7, 2017
Contributor
It's worth adding a comment -- what is an Option<GeneratorClause>?
(Also, I wonder if we should convert this to a struct variant at some point.)
nikomatsakis
reviewed
Jul 7, 2017
| if gen.is_some() { | ||
| self.head("gen")?; | ||
| space(&mut self.s)?; | ||
| } |
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Jul 7, 2017
Contributor
This isn't the syntax that users use, right? In that case, we probably shouldn't either. Also, it seems weird that we don't print if this is "movable" or "immovable" -- iirc from the last time I looked, that stuff isn't really used in this branch, maybe we should remove it from this PR too.
This comment has been minimized.
This comment has been minimized.
Zoxc
Jul 8, 2017
Author
Contributor
We need syntax to mark a generator as movable or immovable. I haven't come up with any good ideas for that. The best idea I got is using static for immovable generators. The only syntax that seems to be nailed down is yield
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Jul 10, 2017
Contributor
Isn't this idea of movable vs immovable a kind of separate, orthogonal proposal? That is, at the moment, I thought we would be starting with only the "movable" kind...
This comment has been minimized.
This comment has been minimized.
Zoxc
Jul 10, 2017
Author
Contributor
Immovable types is a separate orthogonal proposal. Immovable generators would be a extension to generators which depend on immovable types. Immovable generators are very important for async I/O experiments however.
I do intend to implement immovable generators, but landing that in master would block on the immovable types RFC.
nikomatsakis
reviewed
Jul 7, 2017
| @@ -150,6 +150,9 @@ pub enum TypeVariants<'tcx> { | |||
| /// `|a| a`. | |||
| TyClosure(DefId, ClosureSubsts<'tcx>), | |||
|
|
|||
| /// The anonymous type of a generator. Pairs with a TyClosure for closure generators. | |||
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Jul 7, 2017
Contributor
I think this comment could be expanded. For example, what does "pairs with" mean? My guess is that this is the type assigned to an expression like || yield 22, right? In other words, it represents a "resumable" function that, each time it is called, picks up from the last yield point? (Only it works through the generator trait, not the function traits?)
This comment has been minimized.
This comment has been minimized.
nikomatsakis
reviewed
Jul 7, 2017
| impl Iterator<Item=Ty<'tcx>> + 'tcx | ||
| { | ||
| let state = tcx.generator_layout(def_id).fields.iter(); | ||
| let state: Vec<_> = state.map(|d| d.ty.subst(tcx, self.substs)).collect(); |
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Jul 7, 2017
Contributor
Silly question, but is it necessary to collect into an intermediate vec here, given that we're returning an impl Iterator?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Zoxc
Jul 12, 2017
Author
Contributor
This is the error I get if I just return state.map(|d| d.ty.subst(tcx, self.substs)):
error[E0477]: the type `core::iter::Map<core::slice::Iter<'_, mir::LocalDecl<'_>>, [closure@src\librustc\ty\sty.rs:290:19: 290:51 tcx:&ty::context::TyCtxt<'a, 'gcx, 'tcx>, self:&ty::sty::ClosureSubsts<'tcx>]>` does not fulfill the required lifetime
--> src\librustc\ty\sty.rs:287:9
|
287 | impl Iterator<Item=Ty<'tcx>> + 'tcx
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: type must outlive the lifetime 'tcx as defined on the impl at 282:1
--> src\librustc\ty\sty.rs:282:1
|
282 | / impl<'a, 'gcx, 'tcx> ClosureSubsts<'tcx> {
283 | | /// This returns the types of the MIR locals which had to be stored across suspension points.
284 | | /// It is calculated in rustc_mir::transform::generator::StateTransform.
285 | | /// All the types here must be in the tuple in GeneratorInterior.
... |
302 | | }
303 | | }
| |_^
error[E0495]: cannot infer an appropriate lifetime for capture of `tcx` by closure due to conflicting requirements
--> src\librustc\ty\sty.rs:290:19
|
290 | state.map(|d| d.ty.subst(tcx, self.substs))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: first, the lifetime cannot outlive the lifetime 'a as defined on the impl at 282:1...
--> src\librustc\ty\sty.rs:282:1
|
282 | / impl<'a, 'gcx, 'tcx> ClosureSubsts<'tcx> {
283 | | /// This returns the types of the MIR locals which had to be stored across suspension points.
284 | | /// It is calculated in rustc_mir::transform::generator::StateTransform.
285 | | /// All the types here must be in the tuple in GeneratorInterior.
... |
302 | | }
303 | | }
| |_^
note: ...so that the reference type `&ty::context::TyCtxt<'a, 'gcx, 'tcx>` does not outlive the data it points at
--> src\librustc\ty\sty.rs:290:9
|
290 | state.map(|d| d.ty.subst(tcx, self.substs))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: but, the lifetime must be valid for the lifetime 'tcx as defined on the impl at 282:1...
--> src\librustc\ty\sty.rs:282:1
|
282 | / impl<'a, 'gcx, 'tcx> ClosureSubsts<'tcx> {
283 | | /// This returns the types of the MIR locals which had to be stored across suspension points.
284 | | /// It is calculated in rustc_mir::transform::generator::StateTransform.
285 | | /// All the types here must be in the tuple in GeneratorInterior.
... |
302 | | }
303 | | }
| |_^
note: ...so that the type `core::iter::Map<core::slice::Iter<'_, mir::LocalDecl<'_>>, [closure@src\librustc\ty\sty.rs:290:19: 290:51 tcx:&ty::context::TyCtxt<'a, 'gcx, 'tcx>, self:&ty::sty::ClosureSubsts<'tcx>]>` will meet its required lifetime bounds
--> src\librustc\ty\sty.rs:287:9
|
287 | impl Iterator<Item=Ty<'tcx>> + 'tcx
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: aborting due to previous error(s)
This comment has been minimized.
This comment has been minimized.
eddyb
Jul 12, 2017
Member
error[E0495]: cannot infer an appropriate lifetime for capture of
tcxby closure due to conflicting requirements
You forgot a move.
This comment has been minimized.
This comment has been minimized.
Zoxc
Jul 13, 2017
Author
Contributor
state.map(move |d| d.ty.subst(tcx, self.substs)) gives this error:
error[E0477]: the type `core::iter::Map<core::slice::Iter<'_, mir::LocalDecl<'_>>, [closure@src\librustc\ty\sty.rs:289:19: 289:56 tcx:ty::context::TyCtxt<'a, 'gcx, 'tcx>, self:ty::sty::ClosureSubsts<'tcx>]>` does not fulfill the required lifetime
--> src\librustc\ty\sty.rs:287:9
|
287 | impl Iterator<Item=Ty<'tcx>> + 'tcx {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: type must outlive the lifetime 'tcx as defined on the impl at 282:1
--> src\librustc\ty\sty.rs:282:1
|
282 | / impl<'a, 'gcx, 'tcx> ClosureSubsts<'tcx> {
283 | | /// This returns the types of the MIR locals which had to be stored across suspension points.
284 | | /// It is calculated in rustc_mir::transform::generator::StateTransform.
285 | | /// All the types here must be in the tuple in GeneratorInterior.
... |
301 | | }
302 | | }
| |_^
error: aborting due to previous error(s)
This comment has been minimized.
This comment has been minimized.
arielb1
Aug 1, 2017
•
Contributor
You need move and + 'a I think (to capture the 'a lifetime in 'tcx).
This comment has been minimized.
This comment has been minimized.
arielb1
Aug 3, 2017
Contributor
I mean, to change the return type to impl Iterator<Item=Ty<'tcx>> + 'a and use a move closure. This error message really needs to be improved.
nikomatsakis
reviewed
Jul 7, 2017
| @@ -276,6 +279,50 @@ impl<'a, 'gcx, 'acx, 'tcx> ClosureSubsts<'tcx> { | |||
| } | |||
| } | |||
|
|
|||
| impl<'a, 'gcx, 'tcx> ClosureSubsts<'tcx> { | |||
| pub fn state_tys(self, def_id: DefId, tcx: TyCtxt<'a, 'gcx, 'tcx>) -> | |||
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Jul 7, 2017
Contributor
I think this function could use a comment. I am guessing it returns the types of local variables that are (potentially) saved/restore in the state?
nikomatsakis
reviewed
Jul 7, 2017
| state.into_iter() | ||
| } | ||
|
|
||
| pub fn field_tys(self, def_id: DefId, tcx: TyCtxt<'a, 'gcx, 'tcx>) -> |
This comment has been minimized.
This comment has been minimized.
nikomatsakis
reviewed
Jul 7, 2017
| @@ -201,6 +219,19 @@ fn check_aliasability<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>, | |||
| } | |||
| } | |||
|
|
|||
| fn check_yields<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>, | |||
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Jul 7, 2017
Contributor
I would expect this check to be done in the check_loans code -- basically, each time we reach a yield point, we would check that no loans are in scope.
This comment has been minimized.
This comment has been minimized.
Zoxc
Jul 13, 2017
Author
Contributor
Could you point me to an appropriate location in the check_loans code?
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Jul 14, 2017
Contributor
Well, I forgot that check_loans uses the ExprUseVisitor. I would add a "callback" to that interface for "yields". So, specifically the expr_use_visitor::Delegate interface. We could add a fn yield(id: ast::NodeId) method. Then, on each callback, we could check whether there are any loans in scope at that point, e.g. by calling CheckLoans::each_in_scope_loan().
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Zoxc
Jul 14, 2017
Author
Contributor
I was looking around there and that doesn't seem correct. No loan records are created when we get RestrictionResult::Safe, so I assume that means the loans don't show up with CheckLoans::each_in_scope_loan() leading to unsoundness. We also want to ignore ReEarlyBound and ReFree borrows and it looks like that information is lost by then.
aturon
referenced this pull request
Jul 8, 2017
Merged
eRFC: Experimentally add coroutines to Rust #2033
This comment has been minimized.
This comment has been minimized.
|
|
Zoxc
force-pushed the
Zoxc:gen
branch
from
4de910e
to
e4febfa
Jul 8, 2017
This was referenced Jul 9, 2017
Zoxc
reviewed
Jul 10, 2017
| types and such. | ||
|
|
||
| * Traits like `Send` and `Sync` are automatically implemented for a `Generator` | ||
| depending on the captured variables of the environment. Note, though, that |
This comment has been minimized.
This comment has been minimized.
Zoxc
Jul 10, 2017
Author
Contributor
Unlike closures, this also depends on the values inside the generator which are live across a suspension point.
apasel422
reviewed
Jul 10, 2017
| generator progresses. | ||
|
|
||
| * Generator literals produce a value with a unique type which implements the | ||
| `std::ops::Generator` trait. This allows actual execution of the genrator |
This comment has been minimized.
This comment has been minimized.
| @@ -189,6 +190,13 @@ pub use self::range::{RangeInclusive, RangeToInclusive}; | |||
| #[unstable(feature = "try_trait", issue = "42327")] | |||
| pub use self::try::Try; | |||
|
|
|||
| #[unstable(feature = "generator_trait", issue = "0")] | |||
| #[cfg(not(stage0))] | |||
| pub use self::generator::State; | |||
This comment has been minimized.
This comment has been minimized.
apasel422
Jul 10, 2017
Member
core::ops::State on its own has no connection to generators, so we might want to export this with a more descriptive name or introduce a core::generator module, though this can certainly wait until the future RFC that stabilizes this feature.
This comment has been minimized.
This comment has been minimized.
Zoxc
Jul 13, 2017
Author
Contributor
It is in a generator module. Perhaps we should remove the reexport of it?
This comment has been minimized.
This comment has been minimized.
leonardo-m
commented
Jul 11, 2017
|
Is a simpler to remember syntax like this possible?
|
arielb1
added
S-waiting-on-author
and removed
S-waiting-on-review
labels
Jul 11, 2017
Zoxc
force-pushed the
Zoxc:gen
branch
from
d3b2ba1
to
b593f0a
Jul 11, 2017
This comment has been minimized.
This comment has been minimized.
|
So I've read over the code again -- not in depth -- and I remain fairly positive. @Zoxc I want to say that this is nice work. =) That said, I still think we could (and should) try to simplify it as much as possible -- it's basically impossible to review a branch of this size, so the more we can do to prune it down (and then add things in separately later) the better, since those additions can then get reviewed in more depth. The obvious thing for removal remains The other candidate is the "movable/immovable" stuff, which if I recall is not (yet) deeply integrated. I'd prefer to layer that in separately as well. So, before r+, I would like to see:
|
This comment has been minimized.
This comment has been minimized.
rozaliev
commented
Jul 11, 2017
|
I'm updating my prototype lib based on coroutines right now. So I wonder if this kind of code is supposed to be working on this branch?
|
This comment has been minimized.
This comment has been minimized.
|
RLS test still failed. (BTW could some of the commits be squashed? This PR will bring in 127 commits, many of which are "fixing this and that".) |
alexcrichton
added a commit
to rust-lang/rls
that referenced
this pull request
Aug 28, 2017
This comment has been minimized.
This comment has been minimized.
|
@bors: r=arielb1 At this point the commit history is basically an accurate reflection of this history of this feature... |
This comment has been minimized.
This comment has been minimized.
|
|
alexcrichton
force-pushed the
Zoxc:gen
branch
from
98e3ebe
to
a996d5e
Aug 28, 2017
This comment has been minimized.
This comment has been minimized.
|
@bors: r=arielb1 |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Aug 28, 2017
This comment has been minimized.
This comment has been minimized.
|
|
bors
merged commit a996d5e
into
rust-lang:master
Aug 28, 2017
This was referenced Aug 28, 2017
This comment has been minimized.
This comment has been minimized.
|
This is super exciting and I can't wait to play around with it on the next nightly |
whitequark
referenced this pull request
Aug 29, 2017
Closed
Consider interactions with real generators #72
Frederick888
referenced this pull request
Aug 30, 2017
Closed
Failed to build with rustc 1.21.0-nightly (c11f689d2 2017-08-29) #2002
This comment has been minimized.
This comment has been minimized.
phaazon
commented
Aug 31, 2017
|
Hells yeah, great success! <3 Is there any plan on adding support for coroutines as well? Anyway, ultra good job, I’ll play with it as soon as the nightly lands! |
This comment has been minimized.
This comment has been minimized.
rushmorem
commented
Aug 31, 2017
What do you mean? This is already in the latest nightly. I've been playing with this since yesterday. |
This comment has been minimized.
This comment has been minimized.
phaazon
commented
Aug 31, 2017
|
Yeah I just saw that, and I’ve started playing with it as well! I’ve already pushed a patch to Congrats to everyone! |
jonasbb
added a commit
to jonasbb/rls
that referenced
this pull request
Sep 1, 2017
This comment has been minimized.
This comment has been minimized.
lilianmoraru
commented
Sep 5, 2017
|
This does not seem to use stackless coroutines support from LLVM or I am understanding it wrongly? |
This comment has been minimized.
This comment has been minimized.
lilianmoraru
commented
Sep 5, 2017
|
Specifically, the first LLVM RFC(cannot edit my comment on mobile): http://lists.llvm.org/pipermail/llvm-dev/2016-July/102337.html |
This comment has been minimized.
This comment has been minimized.
|
You can look at the comments here: |
This comment has been minimized.
This comment has been minimized.
|
Is there a better way to get a generator which doesn't yield? let mut generator = || {
if false {
yield;
}
0xDEAD_BEEF
}; |
This comment has been minimized.
This comment has been minimized.
|
@lilianmoraru LLVM's coroutine support require heap allocation, so it's not suitable for Rust. @valff Currently, that is the best way. The |
Zoxc commentedJul 5, 2017
•
edited
This adds experimental support for generators intended to land once rust-lang/rfcs#2033 is approved.
This is not yet ready to be merged. Things to do:
Make the implicit argument type default to()