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 #1832

Closed
wants to merge 2 commits into from
Closed

Generators #1832

wants to merge 2 commits into from

Conversation

Zoxc
Copy link

@Zoxc Zoxc commented Dec 27, 2016

This is some friendly competition to Stackless coroutines #1823.

Rendered

@withoutboats withoutboats added the T-lang Relevant to the language team, which will review and decide on the RFC. label Dec 27, 2016
@ahicks92
Copy link

I don't like this, some concrete reasons:

  • The RFC wants to introduce both await and yield at the same time, but there's no really good reason to do this. It is possible to implement await as macros on top of yield, and await is not an already-reserved keyword.

  • The RFC is going out of the way to introduce executors in the standard library. Given that futures-rs hasn't even finalized what an executor needs to do as of the last time I checked, having executor traits set in stone seems premature.

  • Your enum makes it such that code that cares only about generators has to worry about the Blocked case, and code that cares only about async/await functionality has to care about the Yielded case. The competing RFC to this one does not have this problem: instead, the complexity of the Blocked case is increased but in such a way that it mostly falls to a library author. Furthermore, this is probably only one library author for the whole ecosystem, as other things can be built on top of it.

  • Nothing precludes the impl Iterator stuff in the other RFC, so what you're doing here doesn't seem to gain you that. If coroutines implement Iterator, it doesn't matter how they're implemented: you can still use impl trait with them.

  • People who only want generator functionality have to care about executors, because resume takes one. This poses a problem for teaching.

  • Your scheme for passing arguments in is very complicated, as compared to the other RFC. I'm having trouble even following it.

  • Your approach to iterators is fine, but I really do not like making () into an executor just to deal with the fact that the lack of one isn't accounted for in this scheme.

  • Finally, the RFC feels very hastily written, is not well-ordered, and is generally hard to follow. In addition, I don't see anything that points out specific advantages over the other one, just additional complexity that's not necessary.

type Blocked = !;
}
```
Here we give `Blocked` the never type. This means that our executor can only run generators which do return `Blocked` from the `State` enum; since there aren't any instances of `!`. Our executor can only run generators which cannot block. This is why we call `()` the synchronous executor.
Copy link
Member

Choose a reason for hiding this comment

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

"do" should be don't

@nikomatsakis nikomatsakis self-assigned this Dec 30, 2016
@mark-i-m
Copy link
Member

mark-i-m commented Jan 2, 2017

The generality of your abstractions (Executor and Generator) is great. In particular, I think you did a nice job reducing other language constructs to yours. There is a certain appeal in having some unifying abstractions in the library (which I think might address the last bullet point by @camlorn). That, I think, is its primary appeal over #1823.

This all comes at the cost of potentially requiring some significant changes to standard library and possibly the compiler. I would appreciate the addition to this RFC of

  • more detail in the "Motivation" section that specifies the exact problem being solved. It seems that this RFC aims to solve a more general problem than Stackless coroutines #1823, but maybe I'm wrong?
  • more detail in the "Alternatives" section. It's not very convincing... Why are the points you mentioned so valuable that they make this RFC worth it?
  • a concise and specific list of proposed changes to the language and the libraries.

Organizationally, it would be nice to move "Examples" to the end of the document.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Feb 17, 2017

There hasn't been much activity here, but from things I've heard on IRC or elsewhere, I gather that @Zoxc wanted to close it and iterate some more in any case...@Zoxc, is that true?

In general, my take on both this RFC and the stackless coroutines RFC is that we probably want to close them for now and spend a bit more time working through the constraints and goals. I know that @eddyb at some point felt like there was a big design space, and that it didn't make sense to pick a random point, but we should aim for something maximally minimal as a starting point. That intuitively makes sense to me -- but this RFC doesn't quite feel like that "max min" point to me. (The other RFC feels closer, but perhaps it also closes off too many avenues.)

@eddyb
Copy link
Member

eddyb commented Feb 17, 2017

@nikomatsakis TBH I think both went into their own potentially dead-end paths, and we need to start with some smaller subset. Also, tunnel vision must be actively defended against (but I know that's hard 😢).

@Zoxc
Copy link
Author

Zoxc commented Feb 17, 2017

@nikomatsakis I do want to close it, but I left it open to see if there was some valuable input. Must of the discussion seems to be in the other RFC though.

I went down the path of removing the Yielded variant of my state enum, await, await for and the Executor trait. This would make the design for minimal and closer to the other RFC, but it still allows functions to be generators and has a Generator trait instead of reusing the function traits. Such a design is also compatible with futures-rs.

I also added immovable generators in addition to movable generators (hence the immovable types RFC), which isn't minimal, but would help immensely for writing efficient asynchronous code. We still want movable generators so we can write movable iterators, so having just immovable generators isn't a great option. I'd pick immovable generators over movable generators if we had to be minimal though, since they are more expressive and you can get movable generators by boxing them.

I've run into problems thinking about implicit argument with lifetime variables to generators and a way to access them. My understanding of lifetimes need some improvement to resolve that.

I also think we should consider other ways of having await for be part of the language, since that has an impact on the design. There are trait coherence issues with that however. I know @eddyb also wants to combine it with for-loops, which doesn't help with those issues.

@nikomatsakis
Copy link
Contributor

@rfcbot fcp close

I propose that we close this RFC. It seems clear that we are still iterating on the final design here, and (as @Zoxc notes) most of the discussion has been taking place in the other RFC anyhow.

@Zoxc thanks for the thoughtful RFC in any case. =)

@rfcbot
Copy link
Collaborator

rfcbot commented Feb 23, 2017

Team member @nikomatsakis has proposed to close this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot
Copy link
Collaborator

rfcbot commented Apr 27, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Apr 27, 2017
@rfcbot
Copy link
Collaborator

rfcbot commented May 7, 2017

The final comment period is now complete.

@aturon
Copy link
Member

aturon commented May 8, 2017

Closing as per FCP. Discussion on this topic continues here.

@aturon aturon closed this May 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants