Add Future::map

[jonas-schievink]

This adds the simplest Future combinator to the standard library, mostly to act as a sort of "icebreaker" for adding more of them in the future. A compiler change to fix the miscompilation in #111264 is also included.

[rustbot]

r? @joshtriplett

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

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[taiki-e]

It is not great to see such complex unsafe pin projection in the standard library. (And it is actually unsound.)

[taiki-e]

If the future panics here, self.inner will not be set to None and will cause use-after-free on the second poll. (similar case to async-rs/async-std#903)

Polling the future after panic is a bit of an odd case, but since poll is a safe function, it must not cause unsoundness in such a case.

The efficient, correct, and safe pattern here is using pin-project{,-lite}'s project_replace as done in futures.

If adding dependencies to core is difficult, I personally recommend vendoring pin-project-lite and doing the same thing as futures. (see also previous discussion)

[taiki-e]

Adding methods to Future will break some existing code when stabilized due to conflicts with existing crates. There were some discussions about that (e.g., rust-lang/futures-rs#2362 (comment)), but I'm not aware of the current status. Any thoughts on how to address this issue?

Also, this can be written very easily using async-await. (I remember that when stjepang created futures-lite, these methods were not added to FutureExt.)

(I personally don't think we should piecemeal import combinators like this from ecosystems, as cramertj said in the past.)

[taiki-e]

cc @rust-lang/wg-async

[yoshuawuyts]

I wouldn't mind it if added some form of Future::map: it can sometimes be convenient to have a quick way to declare a named future. I do think we should discuss whether this method should take a sync or async closure. Since async/.await is common now, the reasons to use Future::map are different than what they may have been in the past. Because async closures should be the most flexible and useful, I'm inclined to say we should go with that? - The only issue is that we don't have those yet, so we'd be blocked on landing those first.

edit: On second thought, I actually don't know whether it should take an async closure or not. Maybe we should instead just expose a flatten method like we do for Option and Iterator? I do think this is a conversation worth having.

My understanding of why we haven't added this yet is that is because adding methods which are commonly added via extension traits would lead to name resolution conflicts. This can cause issues when upgrading Rust versions, which we'd like to avoid. So in order to avoid that we've been waiting for either #64797 or #64796 to stabilize first.

[vincenzopalazzo]

@rustbot label +I-async-nominated +A-async-await

[bors]

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

[taiki-e]

@yoshuawuyts

it can sometimes be convenient to have a quick way to declare a named future.

Note that the named future is not really available except in cases where the closure can be converted to a function pointer since map takes a closure.

[apiraino]

I guess I can switch the review flag for now until after the meeting and the following design decisions

@rustbot label -S-waiting-on-review +S-waiting-on-team

[tmandry]

We discussed this at today's wg-async meeting (notes).

We came to the conclusion that we do want this, but it would be a blocker if it breaks existing code that uses futures-rs, or regresses error messages significantly. I expect we will encounter these problems, though maybe not while the method is unstable, since I think we taught the resolver to deprioritize unstable items at some point. One way or another, we'll have to solve the ambiguity problem between this trait and FutureExt before stabilizing.

There were also concerns raised about whether this would impact performance by adding a vtable entry, or whether we should wait for sealed methods (not RFC'd). The conclusion was that as long as the signature continues to return a concrete type that's not constructible from outside the crate, we can tackle that optimization at the same time as the Iterator combinators. In the meantime we don't expect the overhead to be too onerous, but are open to contrary data points.

[jonas-schievink]

Future::map has a Self: Sized bound, so it won't end up in the vtable (unless rustc is still so bad at building vtables that it'll add an empty slot for it regardless, I recall that being an issue).

However, since I'm not currently employed to work on Rust, I'll close this PR regardless.