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

Do not expose PollFn struct #79585

Closed
wants to merge 2 commits into from
Closed

Do not expose PollFn struct #79585

wants to merge 2 commits into from

Conversation

win-t
Copy link

@win-t win-t commented Dec 1, 2020

Tracking issue: #72302.

We don't need to expose PollFn struct to the public, returning impl Future is enough

We don't need to expose PollFn struct to the public, returning `impl Future` is enought
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cramertj (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 1, 2020
@taiki-e
Copy link
Member

taiki-e commented Dec 1, 2020

I oppose hiding return type. Other iterators/futures combinators are exposed, and the current one matches this.

See also tokio-rs/tokio#2723.

@win-t
Copy link
Author

win-t commented Dec 1, 2020

Sorry for questioning this dumb question, but what is the reason for exposing others iterators/future combinators return type?
the only reason that I can think is that at that time impl Trait is not available
and also, I considered the case of let a = poll_fn(...) as the same as let a = async { ... }, and the concrete type of the second case is not known either

@win-t
Copy link
Author

win-t commented Dec 1, 2020

I just done reading tokio-rs/tokio#2723.
and yes, I agree that one drawback is that we can store it in another struct because impl Trait only works as an argument or return of a function

EDIT:
it also true that we can't store async { ... } either

@win-t
Copy link
Author

win-t commented Dec 1, 2020

Hmm, after some thinking, I think it is still okay to not expose it,
most of the time, we use poll_fn to get the polling behavior back in the async fn like

async fn something() {
    // do something here
    let some_variable = poll_fn(...).await;
    // do something else here
}

if anyone wants a concrete type, they should create their own type and implement the Future trait, instead of relying on poll_fn,
my argument is looked similar to another debate on general programming: when to use anonymous function vs named one

@taiki-e
Copy link
Member

taiki-e commented Dec 1, 2020

I disagree with the opinion that "async fns/blocks can't do that, so combinators don't have to do that". The current async-await is MVP (minimum viable product).

One benefit of "can storing a return type" is that you can combine existing combinators to create a new combinator. futures and core have a lot of code that uses this pattern. (there are probably many others)

Note that if the return type is not exposed, there is currently no stable efficient way to do this. (you need boxing)
This is also the reason why there are combinators like std::future::ready.

Also, note that impl Trait cannot be used in trait method's return type.

EDIT: Closures can't be named in stable Rust, but if you don't capture the local variable, you can convert it to a function pointer. And there are also several ways to share values without going through local variables.

@win-t
Copy link
Author

win-t commented Dec 1, 2020

I disagree with the opinion that "async fns/blocks can't do that, so combinators don't have to do that". The current async-await is MVP (minimum viable product).

I also dissagre with that, that is not my point.
My point is about that I consider poll_fn as utility to get polling behaviout back in async block, which mean it should used only as poll_fn(...).await (always .await after it), it should not used in another way.

but, It also true that because impl Trait is not complete yet (can be only be used in function param and return), I think this is not a good appoach,

so I will close it, not because of async/await is still MVP, I close it because impl Trait is not completed yet

thanks for the discussion

@win-t win-t closed this Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants