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

Infer async closure signature from (old-style) two-part Fn + Future bounds #127482

Merged

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Jul 8, 2024

When an async closure is passed to a function that has a "two-part" Fn and Future trait bound, like:

use std::future::Future;

fn not_exactly_an_async_closure(_f: F)
where
    F: FnOnce(String) -> Fut,
    Fut: Future<Output = ()>,
{}

The we want to be able to extract the signature to guide inference in the async closure, like:

not_exactly_an_async_closure(async |string| {
    for x in string.split('\n') { ... }
    //~^ We need to know that the type of `string` is `String` to call methods on it.
})

Closure signature inference will see two bounds: <?F as FnOnce<Args>>::Output = ?Fut, <?Fut as Future>::Output = String. We need to extract the signature by looking through both projections.

Why?

I expect the ecosystem's move onto async Fn trait bounds (which are not affected by this PR, and already do signature inference fine) to be slow. In the mean time, I don't see major overhead to supporting this "old–style" of trait bounds that were used to model async closures.

r? oli-obk
Fixes #127468
Fixes #127425

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 8, 2024
@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member Author

oh lol let me fix the doc comment

@compiler-errors
Copy link
Member Author

OK this is ready :3

@oli-obk
Copy link
Contributor

oli-obk commented Jul 8, 2024

bors r plus

@bors
Copy link
Contributor

bors commented Jul 8, 2024

📌 Commit f4f678f has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 8, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 8, 2024
…llaumeGomez

Rollup of 4 pull requests

Successful merges:

 - rust-lang#126427 (Rewrite `intrinsic-unreachable`, `sepcomp-cci-copies`, `sepcomp-inlining` and `sepcomp-separate` `run-make` tests to rmake.rs)
 - rust-lang#127237 (Improve code of `run-make/llvm-ident`)
 - rust-lang#127325 (Migrate `target-cpu-native`,  `target-specs` and `target-without-atomic-cas` `run-make` tests to rmake)
 - rust-lang#127482 (Infer async closure signature from (old-style) two-part `Fn` + `Future` bounds)

Failed merges:

 - rust-lang#127357 (Remove `StructuredDiag`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 72199b2 into rust-lang:master Jul 8, 2024
6 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 8, 2024
Rollup merge of rust-lang#127482 - compiler-errors:closure-two-par-sig-inference, r=oli-obk

Infer async closure signature from (old-style) two-part `Fn` + `Future` bounds

When an async closure is passed to a function that has a "two-part" `Fn` and `Future` trait bound, like:

```rust
use std::future::Future;

fn not_exactly_an_async_closure(_f: F)
where
    F: FnOnce(String) -> Fut,
    Fut: Future<Output = ()>,
{}
```

The we want to be able to extract the signature to guide inference in the async closure, like:

```rust
not_exactly_an_async_closure(async |string| {
    for x in string.split('\n') { ... }
    //~^ We need to know that the type of `string` is `String` to call methods on it.
})
```

Closure signature inference will see two bounds: `<?F as FnOnce<Args>>::Output = ?Fut`, `<?Fut as Future>::Output = String`. We need to extract the signature by looking through both projections.

### Why?

I expect the ecosystem's move onto `async Fn` trait bounds (which are not affected by this PR, and already do signature inference fine) to be slow. In the mean time, I don't see major overhead to supporting this "old–style" of trait bounds that were used to model async closures.

r? oli-obk
Fixes rust-lang#127468
Fixes rust-lang#127425
@rustbot rustbot added this to the 1.81.0 milestone Jul 8, 2024
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 15, 2024
…c-closure-inference, r=lcnr

Infer async closure args from `Fn` bound even if there is no corresponding `Future` bound on return

In rust-lang#127482, I implemented the functionality to infer an async closure signature when passed into a function that has `Fn` + `Future` where clauses that look like:

```
fn whatever(callback: F)
where
  F: Fn(Arg) -> Fut,
  Fut: Future<Output = Out>,
```

However, rust-lang#127781 demonstrates that this is still incomplete to address the cases users care about. So let's not bail when we fail to find a `Future` bound, and try our best to just use the args from the `Fn` bound if we find it. This is *fine* since most users of closures only really care about the *argument* types for inference guidance, since we require the receiver of a `.` method call to be known in order to probe methods.

When I experimented with programmatically rewriting `|| async {}` to `async || {}` in rust-lang#127827, this also seems to have fixed ~5000 regressions (probably all coming from usages `TryFuture`/`TryStream` from futures-rs): the [before](rust-lang#127827 (comment)) and [after](rust-lang#127827 (comment)) crater runs.

Fixes rust-lang#127781.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 15, 2024
…c-closure-inference, r=lcnr

Infer async closure args from `Fn` bound even if there is no corresponding `Future` bound on return

In rust-lang#127482, I implemented the functionality to infer an async closure signature when passed into a function that has `Fn` + `Future` where clauses that look like:

```
fn whatever(callback: F)
where
  F: Fn(Arg) -> Fut,
  Fut: Future<Output = Out>,
```

However, rust-lang#127781 demonstrates that this is still incomplete to address the cases users care about. So let's not bail when we fail to find a `Future` bound, and try our best to just use the args from the `Fn` bound if we find it. This is *fine* since most users of closures only really care about the *argument* types for inference guidance, since we require the receiver of a `.` method call to be known in order to probe methods.

When I experimented with programmatically rewriting `|| async {}` to `async || {}` in rust-lang#127827, this also seems to have fixed ~5000 regressions (probably all coming from usages `TryFuture`/`TryStream` from futures-rs): the [before](rust-lang#127827 (comment)) and [after](rust-lang#127827 (comment)) crater runs.

Fixes rust-lang#127781.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 15, 2024
Rollup merge of rust-lang#129072 - compiler-errors:more-powerful-async-closure-inference, r=lcnr

Infer async closure args from `Fn` bound even if there is no corresponding `Future` bound on return

In rust-lang#127482, I implemented the functionality to infer an async closure signature when passed into a function that has `Fn` + `Future` where clauses that look like:

```
fn whatever(callback: F)
where
  F: Fn(Arg) -> Fut,
  Fut: Future<Output = Out>,
```

However, rust-lang#127781 demonstrates that this is still incomplete to address the cases users care about. So let's not bail when we fail to find a `Future` bound, and try our best to just use the args from the `Fn` bound if we find it. This is *fine* since most users of closures only really care about the *argument* types for inference guidance, since we require the receiver of a `.` method call to be known in order to probe methods.

When I experimented with programmatically rewriting `|| async {}` to `async || {}` in rust-lang#127827, this also seems to have fixed ~5000 regressions (probably all coming from usages `TryFuture`/`TryStream` from futures-rs): the [before](rust-lang#127827 (comment)) and [after](rust-lang#127827 (comment)) crater runs.

Fixes rust-lang#127781.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
5 participants