wasmtime / PR #9867 Remove async-trait in bindgen · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #9867 Remove async-trait in bindgen

Wasmtime GitHub notifications bot (Dec 19 2024 at 17:32):

ifsheldon opened PR #9867 from ifsheldon:remove-async-trait-in-bindgen to bytecodealliance:main:

Closes #9823

I ran cargo test -p wasmtime-wasi and 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::CallHookHandler and wasmtime::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.

Wasmtime GitHub notifications bot (Dec 19 2024 at 17:32):

ifsheldon requested dicej for a review on PR #9867.

Wasmtime GitHub notifications bot (Dec 19 2024 at 17:32):

ifsheldon requested wasmtime-core-reviewers for a review on PR #9867.

Wasmtime GitHub notifications bot (Dec 19 2024 at 17:32):

ifsheldon requested wasmtime-default-reviewers for a review on PR #9867.

Wasmtime GitHub notifications bot (Dec 19 2024 at 20:25):

dicej submitted PR review:

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.

Wasmtime GitHub notifications bot (Dec 19 2024 at 20:25):

dicej created PR review 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?

Wasmtime GitHub notifications bot (Dec 20 2024 at 05:59):

ifsheldon submitted PR review.

Wasmtime GitHub notifications bot (Dec 20 2024 at 05:59):

ifsheldon created PR review 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 Send bound by the Rust announcement about async fn in traits

in the announcement, it's said future version of trait-variant will support dyn trait object. Using trait-variant will enable dyn object support in the future without the need to modify bindgen

compared to writing -> impl Future<Output = T> + Send ourselves, it's better to leave async fn signatures 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] and async fn instead of fn ... -> 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-trait support dyn trait object while traits with trait-variant for now do not

when implementing async traits, users need to remove #[async_trait::async_trait] on the impl blocks

Wasmtime GitHub notifications bot (Dec 20 2024 at 15:31):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Dec 20 2024 at 15:31):

dicej created PR review 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.

Wasmtime GitHub notifications bot (Dec 20 2024 at 15:36):

dicej updated PR #9867.

Wasmtime GitHub notifications bot (Dec 20 2024 at 15:53):

dicej updated PR #9867.

Wasmtime GitHub notifications bot (Dec 20 2024 at 16:13):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Dec 20 2024 at 17:12):

alexcrichton merged PR #9867.

Last updated: Dec 23 2024 at 12:05 UTC