Ensure async trait impls are async (or otherwise return an opaque type)

[ComputerDruid]

As a workaround for the full #[refine] semantics not being implemented
yet, forbit returning a concrete future type like Box<dyn Future> or a
manually implemented Future.

-> impl Future is still permitted; while that can also cause
accidental refinement, that's behind a different feature gate
(return_position_impl_trait_in_trait) and that problem exists
regardless of whether the trait method is async, so will have to be
solved more generally.

Fixes #102745

[rustbot]

r? @fee1-dead

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

[ComputerDruid]

r? @compiler-errors

cc @tmandry

While this would help unblock stabilization we might prefer not to land this if we're worried about artificially blocking testing.

[compiler-errors]

Why not make it both ways?

[ComputerDruid]

There's not really a problem with using an async impl if the trait is -> impl Future + '_. I think it's not possible to create a refinement hazard with an async fn, but even if it is, that's already a problem with RPITIT that'll have to be solved before it can be stabilized, and I expect the solution to that to automatically cover whatever hypothetical refinement problem this might expose.

And we don't have to get it right in order to stabilize async_fn_in_trait since the RPITIT stuff is behind a different feature gate.

[compiler-errors]

While this would help unblock stabilization we might prefer not to land this if we're worried about artificially blocking testing.

Can you elaborate what you mean here?

[compiler-errors]

@rustbot author

[tmandry]

Hm, I forgot about this. I think I'd prefer to allow the actual mixing and matching allowed in the RFC, even if it means we have to allow too many other things in the meantime. This made sense as an early implementation step, but now that the call for testing blog post is out (and even uses rpitit) I think it will really restrict the cases where it makes sense to use async in a trait.

How difficult would it be to make the check allow mixing async fn() with fn() -> impl Future + '_?

[compiler-errors]

How difficult would it be to make the check allow mixing

@tmandry, Can you explain what kind of mixing you mean? Right now, the RPITIT feature allows both impl Future trait + async fn impl and async fn trait + impl Future impl. If we want both, then nothing needs to be done, and this can just be closed.

[tmandry]

Sorry, I said that this was allowed in the async fn RFC, but it's only mentioned as a future possibility (that requires return position impl Trait, which is not RFC'd yet.)

Ideally I would like to allow mixing both ways, so that implementers of an async fn in a trait can run code before returning the future. But the impl Future signature should match the async fn desugaring exactly, which means the future should capture all input lifetimes. (This could also be made more clear in the RFC.) For example,

trait Foo {
    async fn foo(&self, x: &u32);
}

impl<'blah> Foo for &'blah () {
    // Accepted: Matches desugared form (these are equivalent)
    fn foo<'me, 'x>(&'me self, x: &'x u32) -> impl Future<Output = ()> + 'me + 'x { async {} }
    fn foo<'x>(&'me self, x: &'x u32) -> impl Future<Output = ()> + '_ + 'x { async {} }

    // Not accepted: return type must include all input lifetimes
    fn foo<'x>(&self, x: &'x u32) -> impl Future<Output = ()> + '_ { async {} }
    fn foo<'x>(&self, x: &'x u32) -> impl Future<Output = ()> + 'x { async {} }
    fn foo<'x>(&self, x: &'x u32) -> impl Future<Output = ()> { async {} }

    // Not accepted: return type must not include additional lifetimes, like `'static`
    fn foo<'x>(&self, x: &'x u32) -> impl Future<Output = ()> + '_ + 'x + 'static { async {} }

    // Not accepted (this already works): return type must specify `Output`
    fn foo<'x>(&self, x: &'x u32) -> impl Future + '_ + 'x { async {} }

    // This one is acceptable because the extra `+ 'blah` bound is already implied by `+ '_`
    // (anything that outlives a reference to `self` must also outlive `self`),
    // but it's also okay to give an error here for now.
    fn foo<'x>(&self, x: &'x u32) -> impl Future<Output = ()> + '_ + 'x + 'blah { async {} }
}

The rationale for this is that according to rust-lang/rfcs#3245, any other form in the impl should require #[refine]. Until this RFC is implemented we should disallow the divergence altogether.

The same would apply for the other direction; this implies that you must use the correct desugared form in the trait to be able to use async fn in any impl. This is already enforced. The async fn desugaring is maximally conservative in that it includes all input lifetimes in the output type, so there is no way to satisfy a different signature using async fn. Right now it looks like this direction is not actually accepted even if you do write the desugaring correctly:

trait Foo {
    fn foo<'me, 'x>(&'me self, x: &'x u32) -> impl Future<Output = ()> + 'me + 'x;
}

impl Foo for () {
    async fn foo(&self, x: &u32) {}
    // ^ error[E0495]: cannot infer an appropriate lifetime for lifetime parameter '_ in generic type due to conflicting requirements
}

...but it's also less important (you can almost always just switch to using async fn in the trait; this is only a problem if you want to do some magic in a default implementation).

[tmandry]

I should add there is one kind of divergence we should allow, auto traits. This actually works today, and must work before stabilizing return_position_impl_trait_in_trait:

trait Foo {
    fn foo(&self) -> impl Future<Output = ()> + '_ + Send;
}

impl Foo for () {
    async fn foo(&self) { }
}

I suppose it's okay if we have to break it in the interim, though. (I did mention that this would work in the blog post, but it's not critical, I can take that out!)

[tmandry]

After typing this out I realize this amounts to a lot of work :). Looking at the PR again, I think it's find to land except we should just accept any -> impl Future, since that's behind a different feature flag. (Returning e.g. Pin<Box<dyn Future>> should not be accepted, as is already the case in this PR.)

That would be enough to ensure that the behavior is correct when only async_fn_in_trait is enabled, without removing flexibility from those who want to experiment with the (further off) return_position_impl_trait_in_trait.

[rustbot]

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

[compiler-errors]

@bors r+

approving for now, we can always tweak or roll back semantics if needed.

[bors]

📌 Commit 259c37e has been approved by compiler-errors

It is now in the queue for this repository.

[tmandry]

@bors rollup

[matthiaskrgr]

@bors r- failed in a rollup :(
may just need a rebase / compilier errors fixed

https://github.com/rust-lang/rust/actions/runs/3699320469/jobs/6266555110#step:26:1115

   Compiling rustc_const_eval v0.0.0 (/checkout/compiler/rustc_const_eval)
   Compiling rustc_lint v0.0.0 (/checkout/compiler/rustc_lint)
   Compiling rustc_traits v0.0.0 (/checkout/compiler/rustc_traits)
   Compiling rustc_ty_utils v0.0.0 (/checkout/compiler/rustc_ty_utils)
   Compiling rustc_hir_analysis v0.0.0 (/checkout/compiler/rustc_hir_analysis)
error[E0532]: expected tuple struct or tuple variant, found unit variant `ty::Opaque`
   --> compiler/rustc_hir_analysis/src/check/compare_method.rs:339:13
    |
339 |             ty::Opaque(..) => {
    |             ^^^^^^^^^^^^^^
    |
   ::: /checkout/compiler/rustc_type_ir/src/sty.rs:42:5
    |
42  |     Opaque,
    |     ------ `ty::Opaque` defined here
    |
help: use this syntax instead
    |
339 |             ty::Opaque => {
    |             ~~~~~~~~~~
help: consider importing one of these items instead
    |
1   | use crate::check::compare_method::infer::outlives::components::Component::Opaque;
    |
1   | use crate::check::compare_method::infer::region_constraints::GenericKind::Opaque;
    |
1   | use rustc_infer::infer::outlives::components::Component::Opaque;
    |
1   | use rustc_infer::infer::region_constraints::GenericKind::Opaque;
    |
      and 2 other candidates
help: if you import `Opaque`, refer to it directly
    |
339 -             ty::Opaque(..) => {
339 +             Opaque(..) => {
    |

For more information about this erro

[compiler-errors]

Ah yeah, ty::Opaque(..) needs to be turned into ty::Alias(ty::Opaque, ..)

[compiler-errors]

@bors r+

[bors]

📌 Commit da98ef9 has been approved by compiler-errors

It is now in the queue for this repository.