Remove async-trait in bindgen #9867
Conversation
github-actions
bot
added
wasi
There was a problem hiding this comment.
Thanks for this!
LGTM overall, but see my inline comment about avoiding the trait-variant dependency, which would address the cargo vet CI failure.
The other CI failure should be easy to fix: just run BINDGEN_TEST_BLESS=1 cargo test -p wasmtime-component-macro --test expanded.
| @@ -313,6 +315,7 @@ tracing = "0.1.26" | |||
| bitflags = "2.0" | |||
| thiserror = "1.0.43" | |||
| async-trait = "0.1.71" | |||
| trait-variant = "0.1.2" | |||
There was a problem hiding this comment.
Per this section of the contributing docs, the bar for adding new dependencies to Wasmtime is a bit higher than for other projects (hence the cargo vet CI failure), so we need to make sure this one is worth including.
From what I can tell, it's only being used to annotate wasmtime-wit-bindgen-generated traits. Given that we're generating those traits anyway, I wonder if we should modify the generator to emit a -> impl Future<Output = T> + Send return type directly rather than rely on trait-variant to transform the code it just generated. That would avoid the extra dependency and remove a layer of abstraction. What do you think?
There was a problem hiding this comment.
I would strongly recommend adding trait-variant because:
- it is maintained officially by the Rust project, so the code quality is reassured.
- this is the recommended way to add
Sendbound by the Rust announcement about async fn in traits - in the announcement, it's said future version of
trait-variantwill support dyn trait object. Usingtrait-variantwill enable dyn object support in the future without the need to modifybindgen - compared to writing
-> impl Future<Output = T> + Sendourselves, it's better to leaveasync fnsignatures for users to debug when users have IDEs like RustRover that can expand proc_macro layer by layer. So when they want to debug, they actually see#[trait_variant::make]andasync fninstead offn ... -> impl Future<Output = T> + Send - from my perspective, the use of
#[trait_variant::make]is sort of a marker indicating better support of async fn in traits is needed, which may benefit contributions.
In general, I think trait-variant is a better solution than async-trait when we take the future evolution of Rust into account.
PS: replacing async-trait with trait-variant is a breaking change because:
- traits by
async-traitsupport dyn trait object while traits withtrait-variantfor now do not - when implementing async traits, users need to remove
#[async_trait::async_trait]on the impl blocks
There was a problem hiding this comment.
Thanks for explaining! I hadn't realized trait-variant was a rust-lang project and that it has official status. I'll add a commit to tell cargo vet to trust that crate.
And yes, I realize the removal of async-trait will be a breaking change in any case.
Signed-off-by: Joel Dice <[email protected]>
This exempts the `trait-variant` crate from vetting since it is published and maintained by the rust-lang organization and is officially recommended by the Rust project. Signed-off-by: Joel Dice <[email protected]>
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Closes #9823
I ran
cargo test -p wasmtime-wasiand all tests passed, so now I'd like to go through the whole CI.There are only 2 cases remaining unresolved due to the use of dyn objects, i.e.,
wasmtime::runtime::store::CallHookHandlerandwasmtime::runtime::limits::ResourceLimiterAsync, but they are related to the runtime, so I don't think they are very relevant to the issue and I left them unchanged.