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

Rename core::future::poll_fn to core::future::from_fn #76789

Closed
wants to merge 1 commit into from
Closed

Rename core::future::poll_fn to core::future::from_fn #76789

wants to merge 1 commit into from

Conversation

yoshuawuyts
Copy link
Member

@yoshuawuyts yoshuawuyts commented Sep 16, 2020

Tracking issue: #72302.

This patch renames core::future::poll_fn to core::future::from_fn as suggested by @Koxiaet in #72303 (comment). This makes the API more consistent with similar functions already in the stdlib, for example std::iter::from_fn, and removes some awkwardness from the naming (I remember being confused by what poll_fn meant when I first encountered it).

Drawbacks

The main rationale not to name this core::future::from_fn is that the futures crate also exposes futures::future::lazy which also takes a closure. That would mean we'd have two functions that take a closure at top level. However the stdlib exposes std::iter::once, which exists next to std::iter::from_fn with both functions creating an iterator from a function; meaning there would be precedent for having both functions in the same hierarchy.

However I don't think futures::future::lazy is a good fit for the stdlib, and I'd be surprised if we saw it stabilized. It was designed before we had async/.await in the language, as a convenience function to create "lazy values" . But most of its uses have been replaced by async blocks since:

// lazy
use std::future::lazy;
let fut = lazy(|_| "world");
println!("hello {}", fut.await);

// async blocks
let fut = async { "world" };
println!("hello {}", fut.await);

With the expected addition of async closures to the language, it's likely the uses for futures::future::lazy will only decrease further. Async closures are expected to be able to take closures, take borrows as arguments, and support .await inside of the closure body. Which makes it seem quite likely we'll only ever expose a single "create a future from a closure" method in the core::future hierarchy.


cc/ @rust-lang/wg-async-foundations

@rust-highfive
Copy link
Collaborator

r? @LukasKalbertodt

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 16, 2020
@LukasKalbertodt
Copy link
Member

I have basically no experience with async APIs and don't feel qualified to judge if this rename is good or not. I will reassign to @dtolnay as they reviewed the original PR:

r? @dtolnay

@bors
Copy link
Contributor

bors commented Sep 20, 2020

☔ The latest upstream changes (presumably #76964) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@tmandry
Copy link
Member

tmandry commented Sep 22, 2020

I find from_fn to be a clearer name than poll_fn. From a very unscientific poll of Fuchsia developers, it seems like most people agree; at least I couldn't find anyone who disagreed. So I'm in support of this change.

@yoshuawuyts
Copy link
Member Author

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@tmandry
Copy link
Member

tmandry commented Sep 23, 2020

@carllerche told me he was -1 on this change but can't post right now for technical reasons.

He asked me to post this on his behalf:

the short is, the poll terminology is already well established and it matches Future::poll
from_fn is ambigious, and as hinted at, there are multiple ways to take a "fn" and turn it into a future
being more specific is better

@LucioFranco
Copy link
Member

I agree with @carllerche on this one, I think poll_fn is extremely clear and obvious what it does. from_fn really confused me looking at this PR in my emails.

@tmandry
Copy link
Member

tmandry commented Sep 23, 2020

Based on my own experience and others I've asked, I think a lot of people find the poll_fn name to be confusing.

At the same time, I can see that from_fn is ambiguous.

A modest proposal: What about from_poll_fn?

@LucioFranco
Copy link
Member

LucioFranco commented Sep 23, 2020

To be honest, I don't think from_fn is very discoverable either...

A modest proposal: What about from_poll_fn?

I think this is even more confusing :D

Looking at the signatures it looks like from_fn is called that because it returns a type called FromFn. While poll_fn is called that because it returns PollFn? So to me the reason to call this from_fn is incorrect due to the origins of that naming scheme? Are there other examples of a method called from_fn? Is one enough to set this precedent?

In the end, I don't have a super strong opinion but ambiguity is a problem here imo.

@tmandry
Copy link
Member

tmandry commented Sep 23, 2020

from_poll_fn returns a Future given a poll function.

What about the name do you find confusing? The type of the future we can name however we want. People don't usually write the name of the type directly, so it's not as important as making the function name clear in my opinion.

@LucioFranco
Copy link
Member

I think its just longer than it needs to be, but maybe this is just me being used to using poll_fn.

People don't usually write the name of the type directly, so it's not as important as making the function name clear in my opinion.

Right, my point was that the name from_fn comes from that struct and maybe isn't the right grounds to rename things similar to this. But that is just my 2c.

@skade
Copy link
Contributor

skade commented Sep 23, 2020

Right, my point was that the name from_fn comes from that struct and maybe isn't the right grounds to rename things similar to this. But that is just my 2c.

Usually, it's the other way around, especially around iterators: the return value is named after the combinator function. This reflects in the docs where the documentation is on the function, and FromFn is the rarely named struct that has the sole job of implementing Iterator.

I'd like to add that poll_fn breaks Rusts idiom to use from and into when you create a value from another one.

Late thought: This may come from the habit that we rarely talk about Futures being a thing we construct, mainly about that "something is a future".

@yoshuawuyts
Copy link
Member Author

yoshuawuyts commented Sep 24, 2020

from_poll_fn returns a Future given a poll function.

@tmandry I think that's a reasonable suggestion; it clears up what kind of function we're expecting, and feels consistent enough with what we have. As @skade correctly points out the most pressing concern is that poll_fn breaks with the established stdlib convention of using from and into for conversions.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I wouldn't otherwise weigh in, but since I am the assigned reviewer: my preference would be to keep poll_fn.

I think the "idiom to use from and into when you create a value from another one" is overstated. What does this mean? Functions with 1 argument (or 2, like ptr+len?) which return non-() and do not perform I/O? There are plenty of counterexamples with a clearer name, e.g. iter::repeat I don't think would be clearer as iter::from / from_val / from_repeat / from_repeated_val / into_repeating / into_repeater / ...

future::poll_fn makes a future which polls a fn (poll as a verb) or you give it the implementation for its Future's poll fn (poll as a noun), so there are two sensible ways to internalize what this does that both draw exactly the correct distinction from other ways of treating a fn as a future.

@taiki-e
Copy link
Member

taiki-e commented Sep 25, 2020

@yoshuawuyts

This makes the API more consistent with similar functions already in the stdlib, for example std::iter::from_fn, and removes some awkwardness from the naming (I remember being confused by what poll_fn meant when I first encountered it).

The main rationale not to name this core::future::from_fn is that the futures crate also exposes futures::future::lazy which also takes a closure. That would mean we'd have two functions that take a closure at top level. However the stdlib exposes std::iter::once, which exists next to std::iter::from_fn with both functions creating an iterator from a function; meaning there would be precedent for having both functions in the same hierarchy.

With the expected addition of async closures to the language, it's likely the uses for futures::future::lazy will only decrease further. Async closures are expected to be able to take closures, take borrows as arguments, and support .await inside of the closure body. Which makes it seem quite likely we'll only ever expose a single "create a future from a closure" method in the core::future hierarchy.

I think future API naming should consider consistency with stream than iterator.
stream has some more fn -> Stream functions (e.g., poll_fn, async closure -> Stream, Equivalent to futures::stream::iter(std::iter::from_fn)), and if we accept this PR it will be difficult to keep consistency.

@yoshuawuyts yoshuawuyts deleted the future_from_fn branch September 25, 2020 18:44
@yoshuawuyts
Copy link
Member Author

I'm noticing I'm low on energy and don't have the energy to engage in the dialog needed to see this change through to completion. If someone else wants to see this through, by all means feel free to pick this up.

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.

9 participants