rust-lang / impl-trait-utils

Utilities for working with impl traits in Rust.
Apache License 2.0
89 stars 9 forks source link

async function with default implementation won't compile #17

Open rodrigorc opened 8 months ago

rodrigorc commented 8 months ago

I have a trait such as this:

#[trait_variant::make(Test: Send)]
trait LocalTest {
    async fn foo() -> i32 { 0 }
}

but if fails with an error:

  |
3 | #[trait_variant::make(Test: Send)]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `{integer}` is not a future
  |

It looks like the body of the default impl is translated to the Test trait, unchanged.

If I instead I try:

    async fn foo() -> i32 { std::future::ready(0) }

Then it looks like the Test works, but LocalTest fails with a somewhat opposite error:

5 |     async fn foo() -> i32 { std::future::ready(0) }
  |                             ^^^^^^^^^^^^^^^^^^^^^ expected `i32`, found `Ready<{integer}>`

I'm not sure if this is a limitation of this crate, if so I think it should be documented somewhere; or if it is an oversight in the implementation. Or maybe I'm missing something?

Then, just to see what would happen, I tried:

async fn foo() -> i32 { async move { 0 } }

And I got an ICE!

5 |     async fn foo() -> i32 { async move { 0 } }
  |                             ^^^^^^^^^^^^^^^^ expected `i32`, found `async` block
  |
  = note:       expected type `i32`
          found `async` block `{async block@src/main.rs:5:29: 5:45}`

error: internal compiler error: compiler/rustc_mir_build/src/build/mod.rs:655:72: impossible case reached
...
milesj commented 8 months ago

Also ran into this. I have a trait with this function and a default body:

async fn is_version_supported(&self, req: &str) -> miette::Result<bool> {
    let version = self.get_version().await?;

    Ok(VersionReq::parse(req).into_diagnostic()?.matches(&version))
}

This fails to compile with:

error[E0728]: `await` is only allowed inside `async` functions and blocks
  --> nextgen/vcs/src/vcs.rs:75:42
   |
75 |         let version = self.get_version().await?;
   |                                          ^^^^^ only allowed inside `async` functions and blocks

    Checking moon_deno_tool v0.1.0 (/crates/deno/tool)
    Checking moon_system_platform v0.1.0 (/crates/system/platform)
    Checking moon_app v0.1.0 (/nextgen/app)
error[E0277]: `Result<bool, _>` is not a future
 --> nextgen/vcs/src/vcs.rs:9:1
  |
9 | #[trait_variant::make(Vcs: Send)]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `Result<bool, _>` is not a future
  |
  = help: the trait `Future` is not implemented for `Result<bool, _>`
  = note: Result<bool, _> must be a future or must implement `IntoFuture` to be awaited

When looking at the trait_variant compiled output, it makes sense that it would fail. I'm surprised this wasn't tested at all?

    #[doc = r" Return true if the current binary version matches the provided requirement."]
    fn is_version_supported(
        &self,
        req: &str,
    ) -> impl ::core::future::Future<Output = miette::Result<bool>> + Send {
        let version = self.get_version().await?;
        Ok(VersionReq::parse(req).into_diagnostic()?.matches(&version))
    }
ryo33 commented 8 months ago

I almost had opened an issue with the same problem with title, "making a variant with panic!() in async fn's body causes a compile error"

==== The issue body ====

Adding #[trait_variant::make(Test: Send)] to the following no-error code causes an error

pub trait LocalTest {
    async fn test(&self) -> String {
        panic!()
    }
}
rustc: `()` is not a future
the trait `std::future::Future` is not implemented for `()`
() must be a future or must implement `IntoFuture` to be awaited [E0277]

Macros that desugar async fn have to emit async { panic!() } as the body instead of just panic!() to avoid the error, though there are complex cases, like:

  1. todo! or unimplemented! is used instead
  2. the method has more statements above the panic!()
  3. (trivial) a function that returns ! is used instead.

As the user side, I cannot find a way to work around this, especially when using the trait_variant::make variant along with other macros that emit default implementation containing panic!().

Related: https://github.com/rust-lang/rust/issues/35121 https://github.com/rust-lang/rfcs/pull/1637 https://github.com/rust-lang/rust/issues/20021

kingwingfly commented 7 months ago

It’s not silky smooth to use for me also. However, I could give a relatively silky example.

#[trait_variant::make(Test: Send)]
trait LocalTest {
    // Below can not work (`Result<(), _>` is not a future)
    // async fn foo() -> anyhow::Result<()> {
    //     Ok(())
    // }

    // Below can work
    fn foo() -> impl core::future::Future<Output = anyhow::Result<()>> {
        async { Ok(()) }
    }

    async fn bar() -> anyhow::Result<()>;

    async fn baz() -> anyhow::Result<()>;
}

struct Foo;

impl LocalTest for Foo {
    async fn foo() -> anyhow::Result<()> {
        Ok(())
    }

    async fn bar() -> anyhow::Result<()> {
        Ok(())
    }

    async fn baz() -> anyhow::Result<()> {
        todo!()
    }
}

struct Bar;

impl Test for Bar {
    async fn foo() -> anyhow::Result<()> {
        Ok(())
    }

    async fn bar() -> anyhow::Result<()> {
        Ok(())
    }

    async fn baz() -> anyhow::Result<()> {
        todo!()
    }
}

To conclude,

  1. Cannot impl LocalTest and Test for the same struct.
  2. When define the trait, only default impl shouldn't use async fn, use fn foo() -> impl core::future::Future<Output = ... > with inner async block instead.
  3. When impl trait, the LocalTest and Test are completely the same.
AlvaroSierra commented 3 months ago

I gave both PRs a shot in a code base where I encountered this issue which involved lifetimes and #28 worked while with #20 I two errors where the lifetime 'the_self_lt needed to outlive 'static and that the implementation of Send is not general enough.

I have not been successful at creating a simpler example so sry I can't provide one.

tmandry commented 1 month ago

@AlvaroSierra I'm surprised #28 worked with your example, I couldn't get it to compile without adding where Self: Sync to the macro output.

AlvaroSierra commented 1 month ago

@tmandry In my case, where the self is taken as &mut self, I guess Sync is not required due to the single mutable reference enforced by the compiler.