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

Async closure argument inference is broken with functions taking FnOnce() -> futures::TryFuture #127781

Closed
peku33 opened this issue Jul 15, 2024 · 2 comments · Fixed by #129072
Closed
Labels
A-closures Area: Closures (`|…| { … }`) A-inference Area: Type inference C-bug Category: This is a bug. F-async_closure `#![feature(async_closure)]` T-types Relevant to the types team, which will review and decide on the PR/issue. WG-async Working group: Async & await

Comments

@peku33
Copy link

peku33 commented Jul 15, 2024

I'm writing this as a continuation of #127425

While the problem from first message in issue seems to be fixed, there are still cases when this fails.

One of examples is when using try_for_each instead of for_each, when closure is expected to return Result:

#![feature(async_closure)]

use anyhow::Error;
use futures::{stream::repeat_with, StreamExt, TryStreamExt};

fn accept_str(_: &str) {}
fn accept_string(_: &String) {}

#[tokio::main(flavor = "current_thread")]
async fn main() -> Result<(), Error> {

    repeat_with(|| "foo".to_owned())
        .take(1)
        .map(Result::<_, Error>::Ok)
        .try_for_each(async move |value| {
            // error on whole closure, rust thinks that type of value is `str`
            accept_str(&value);

            // type annotations needed, cannot infer type
            accept_str(value.as_str());

            // this works
            accept_string(&value); // ok
            
            Ok(())
        })
        .await?;

    Ok(())
}

I expected to see this happen: All options used to work in past versions of rust

Instead, this happened: Without explicit type hint it fails

Version it worked on

It most recently worked on: Not 100% sure, some explanations have been made in original issue

Version with regression

rustc 1.81.0-nightly (d9284afea 2024-07-14)
binary: rustc
commit-hash: d9284afea99e0969a0e692b9e9fd61ea4ba21366
commit-date: 2024-07-14
host: x86_64-pc-windows-msvc
release: 1.81.0-nightly
LLVM version: 18.1.7
@peku33 peku33 added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Jul 15, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jul 15, 2024
@compiler-errors compiler-errors added A-closures Area: Closures (`|…| { … }`) A-inference Area: Type inference F-async_closure `#![feature(async_closure)]` WG-async Working group: Async & await T-types Relevant to the types team, which will review and decide on the PR/issue. and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. regression-untriaged Untriaged performance or correctness regression. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jul 15, 2024
@compiler-errors
Copy link
Member

compiler-errors commented Jul 15, 2024

This is because the signature of try_for_each is:

fn try_for_each<Fut, F>(self, f: F) -> TryForEach<Self, Fut, F>
where
    F: FnMut(Self::Ok) -> Fut,
    Fut: TryFuture<Ok = (), Error = Self::Error>,
    Self: Sized,

We only extract an async closure bound from cases where there is Future<Output = ???> bound on the trait, which TryFuture is not. There isn't even a supertrait bound that constrains the output type.

I also do not expect this to ever be fixed, because that would require overly complicating the closure signature inference algorithm in ways that make me quite uncomfortable12. The correct solution is to rework the futures crate to have first-class async closure support, so for now I'd recommend just ascribing your type names. Sorry!

Footnotes

  1. Firstly, this would break if another blanket implementation for TryFuture were ever added.

  2. Secondly, the combinator shouldn't really be using FnMut bounds either -- it should be using async FnMut bounds.

@compiler-errors
Copy link
Member

Alternatively, rust-lang/futures-rs#2763 would fix this when paired with a slight adjustment to the closure signature inference that I'd be happy to make, but it seems like that PR is blocked.

@compiler-errors compiler-errors changed the title Async closure argument inference is broken with closures returning Result Async closure argument inference is broken with functions taking futures::TryFuture Jul 16, 2024
@compiler-errors compiler-errors changed the title Async closure argument inference is broken with functions taking futures::TryFuture Async closure argument inference is broken with functions taking FnOnce() -> futures::TryFuture Jul 16, 2024
kpreid added a commit to kpreid/all-is-cubes that referenced this issue Aug 10, 2024
Note: This causes type inference failures, requiring us to specify the
closures’ parameter types explicitly. I gather from reading issue
<rust-lang/rust#127781> that this may be
rectified by using native `AsyncFnOnce` / `async FnOnce` syntax,
but that currently cannot specify the `+ Send + 'static` bound without
also using `return_type_notation`, which is an incomplete feature.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue 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.
kpreid added a commit to kpreid/all-is-cubes that referenced this issue Aug 15, 2024
Note: This causes type inference failures, requiring us to specify the
closures’ parameter types explicitly. I gather from reading issue
<rust-lang/rust#127781> that this may be
rectified by using native `AsyncFnOnce` / `async FnOnce` syntax,
but that currently cannot specify the `+ Send + 'static` bound without
also using `return_type_notation`, which is an incomplete feature.
@bors bors closed this as completed in 53bf554 Aug 15, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue 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.
kpreid added a commit to kpreid/all-is-cubes that referenced this issue Aug 27, 2024
Note: This causes type inference failures, requiring us to specify the
closures’ parameter types explicitly. I gather from reading issue
<rust-lang/rust#127781> that this may be
rectified by using native `AsyncFnOnce` / `async FnOnce` syntax,
but that currently cannot specify the `+ Send + 'static` bound without
also using `return_type_notation`, which is an incomplete feature.
kpreid added a commit to kpreid/all-is-cubes that referenced this issue Sep 6, 2024
Note: This causes type inference failures, requiring us to specify the
closures’ parameter types explicitly. I gather from reading issue
<rust-lang/rust#127781> that this may be
rectified by using native `AsyncFnOnce` / `async FnOnce` syntax,
but that currently cannot specify the `+ Send + 'static` bound without
also using `return_type_notation`, which is an incomplete feature.
kpreid added a commit to kpreid/all-is-cubes that referenced this issue Sep 6, 2024
Note: This causes type inference failures, requiring us to specify the
closures’ parameter types explicitly. I gather from reading issue
<rust-lang/rust#127781> that this may be
rectified by using native `AsyncFnOnce` / `async FnOnce` syntax,
but that currently cannot specify the `+ Send + 'static` bound without
also using `return_type_notation`, which is an incomplete feature.
kpreid added a commit to kpreid/all-is-cubes that referenced this issue Sep 14, 2024
Note: This causes type inference failures, requiring us to specify the
closures’ parameter types explicitly. I gather from reading issue
<rust-lang/rust#127781> that this may be
rectified by using native `AsyncFnOnce` / `async FnOnce` syntax,
but that currently cannot specify the `+ Send + 'static` bound without
also using `return_type_notation`, which is an incomplete feature.
kpreid added a commit to kpreid/all-is-cubes that referenced this issue Sep 14, 2024
Note: This causes type inference failures, requiring us to specify the
closures’ parameter types explicitly. I gather from reading issue
<rust-lang/rust#127781> that this may be
rectified by using native `AsyncFnOnce` / `async FnOnce` syntax,
but that currently cannot specify the `+ Send + 'static` bound without
also using `return_type_notation`, which is an incomplete feature.
kpreid added a commit to kpreid/all-is-cubes that referenced this issue Sep 16, 2024
Note: This causes type inference failures, requiring us to specify the
closures’ parameter types explicitly. I gather from reading issue
<rust-lang/rust#127781> that this may be
rectified by using native `AsyncFnOnce` / `async FnOnce` syntax,
but that currently cannot specify the `+ Send + 'static` bound without
also using `return_type_notation`, which is an incomplete feature.
kpreid added a commit to kpreid/all-is-cubes that referenced this issue Sep 24, 2024
Note: This causes type inference failures, requiring us to specify the
closures’ parameter types explicitly. I gather from reading issue
<rust-lang/rust#127781> that this may be
rectified by using native `AsyncFnOnce` / `async FnOnce` syntax,
but that currently cannot specify the `+ Send + 'static` bound without
also using `return_type_notation`, which cannot yet be used upon
`F: FnOnce()` bounds, as opposed to non-`Fn` trait bounds.
kpreid added a commit to kpreid/all-is-cubes that referenced this issue Oct 4, 2024
Note: This causes type inference failures, requiring us to specify the
closures’ parameter types explicitly. I gather from reading issue
<rust-lang/rust#127781> that this may be
rectified by using native `AsyncFnOnce` / `async FnOnce` syntax,
but that currently cannot specify the `+ Send + 'static` bound without
also using `return_type_notation`, which cannot yet be used upon
`F: FnOnce()` bounds, as opposed to non-`Fn` trait bounds.
kpreid added a commit to kpreid/all-is-cubes that referenced this issue Oct 22, 2024
Note: This causes type inference failures, requiring us to specify the
closures’ parameter types explicitly. I gather from reading issue
<rust-lang/rust#127781> that this may be
rectified by using native `AsyncFnOnce` / `async FnOnce` syntax,
but that currently cannot specify the `+ Send + 'static` bound without
also using `return_type_notation`, which cannot yet be used upon
`F: FnOnce()` bounds, as opposed to non-`Fn` trait bounds.
kpreid added a commit to kpreid/all-is-cubes that referenced this issue Nov 7, 2024
Note: This causes type inference failures, requiring us to specify the
closures’ parameter types explicitly. I gather from reading issue
<rust-lang/rust#127781> that this may be
rectified by using native `AsyncFnOnce` / `async FnOnce` syntax,
but that currently cannot specify the `+ Send + 'static` bound without
also using `return_type_notation`, which cannot yet be used upon
`F: FnOnce()` bounds, as opposed to non-`Fn` trait bounds.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-closures Area: Closures (`|…| { … }`) A-inference Area: Type inference C-bug Category: This is a bug. F-async_closure `#![feature(async_closure)]` T-types Relevant to the types team, which will review and decide on the PR/issue. WG-async Working group: Async & await
Projects
None yet
3 participants