Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upadd IntoFuture trait and support for await #65244
Open
+98
−23
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
r? @kennytm (rust_highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
|
r? @nikomatsakis cc @cramertj |
This comment has been minimized.
This comment has been minimized.
|
(My machine dies trying to compile rustc, so I'll be needing CI to tell me if I goofed a small thing. And I can add a tracking issue if accepted.) |
This comment was marked as outdated.
This comment was marked as outdated.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The code generally looks good to me -- though I feel like the test ought not to be passing. Let's see what travis says. Anyway, the other question is whether we want to enable this. It'd be good to capture some of the motivation. The main thing I've heard (which I find persuasive) came from the async-finalizers blog post by @yoshuawuyts. The main reason I've heard for removing |
| @@ -99,6 +99,18 @@ pub trait Future { | |||
| fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output>; | |||
| } | |||
|
|
|||
| /// Conversion into a `Future`. | |||
| #[unstable(feature = "into_future", issue = "0")] | |||
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Oct 9, 2019
Contributor
We should file a tracking issue, but let's hold off just a bit.
This comment has been minimized.
This comment has been minimized.
tmandry
Nov 18, 2019
Contributor
There should be a tracking issue before this gets merged, though, I think.
This comment has been minimized.
This comment has been minimized.
|
I don't think it was forgotten about-- |
This comment has been minimized.
This comment has been minimized.
|
@cramertj did you see the async-finalizers (since renamed to async-builders, I think) blog post I was referring to? That's at least one use case. (I'd like to know if @seanmonstar had another use case in mind, though!) |
This comment has been minimized.
This comment has been minimized.
|
Yes, I've long had various builders in reqwest and hyper that I've previously felt would be nice if they could easily become futures, but they themselves aren't futures. (My office internet is flaking hard, so might take a bit to update the PR.) |
This was referenced Oct 9, 2019
This comment was marked as outdated.
This comment was marked as outdated.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment was marked as outdated.
This comment was marked as outdated.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
seanmonstar
force-pushed the
seanmonstar:into-future
branch
2 times, most recently
from
bd3eb16
to
5e3d3bb
This comment was marked as outdated.
This comment was marked as outdated.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
|
Besides the slightly changed diagnostics, it seems using the @eddyb claims that rebasing this on his fix in #65743 does fix the |
Centril
added a commit
to Centril/rust
that referenced
this pull request
Oct 26, 2019
…asper rustc_typeck: don't record direct callees in generator_interior. For expressions like `f(g().await)` we were recording `f` as needing to be kept in a temporary (and therefore be tracked by the generator type) across the suspend, even if a function/method path. However, this is never needed, and can cause issues with complex function types (see rust-lang#65244). cc @Zoxc @nikomatsakis
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
danieleades
commented
Nov 2, 2019
|
pretty excited for this. any further progress? |
seanmonstar
force-pushed the
seanmonstar:into-future
branch
2 times, most recently
from
ec098e7
to
7073a1f
This comment has been minimized.
This comment has been minimized.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
|
I've rebased on master such that it includes @eddyb's fix, and in doing so, removed the Does this still need an FCP before I open a tracking issue and update here? |
This comment has been minimized.
This comment has been minimized.
|
|
| &[sym::future, sym::IntoFuture, sym::into_future], | ||
| hir_vec![expr], | ||
| )); | ||
|
|
||
| // match <expr> { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@bors delegate=seanmonstar r=me with a tracking issue opened and conflicts fixed. |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
@tmandry I hadn't done the tracking issue stuff yet since it seemed this needed an FCP. It's still tagged as such... |
This comment has been minimized.
This comment has been minimized.
|
@Centril as you added the |
This comment has been minimized.
This comment has been minimized.
|
Let's do it. :) Dear language team, lib team, and the community; I propose that we experimentally try out @rfcbot merge |
This comment has been minimized.
This comment has been minimized.
rfcbot
commented
Nov 22, 2019
•
|
Team member @Centril has proposed to merge this. The next step is review by the rest of the tagged team members:
No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
danieleades
commented
Dec 11, 2019
|
The suspense is killing me |
This comment has been minimized.
This comment has been minimized.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
seanmonstar commentedOct 9, 2019
The async-await RFC mentions being able to
awaitanything implementingIntoFuture. Somewhere along the way, it was left out.