wasmtime / PR #8448 Add `GetHost` to generated bindings f... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #8448 Add `GetHost` to generated bindings f...

Wasmtime GitHub notifications bot (Apr 23 2024 at 19:17):

lann edited PR #8448.

Wasmtime GitHub notifications bot (May 07 2024 at 16:13):

lann updated PR #8448.

Wasmtime GitHub notifications bot (May 07 2024 at 17:35):

lann updated PR #8448.

Wasmtime GitHub notifications bot (May 08 2024 at 14:14):

lann commented on PR #8448:

Tracking for myself: next step is to update the top-level world linking: add a e.g. add_root_to_linker_get_host, revert the world add_to_linker impl to call child trait closure-form add_to_linkers directly (avoiding the need for WorldGetHost to somehow impl all imported interface/resource GetHosts as well).

Wasmtime GitHub notifications bot (May 08 2024 at 15:26):

lann commented on PR #8448:

@alexcrichton
re: https://github.com/bytecodealliance/wasmtime/issues/8382#issuecomment-2072986390 (?Sized for impl <T: Host> Host for &mut T), this doesn't work for methods that return the type itself:
27 | impl<T: FooImports + ?Sized> FooImports for &mut T {
   |      - this type parameter needs to be `Sized`
28 |     fn x(&mut self) -> T {
   |                        ^ doesn't have a size known at compile-time
I'm just going to drop the ?Sized unless you think its worth trying to special case somehow.

Wasmtime GitHub notifications bot (May 08 2024 at 15:27):

lann edited a comment on PR #8448:

@alexcrichton
re: https://github.com/bytecodealliance/wasmtime/issues/8382#issuecomment-2072986390 (?Sized for impl <T: Host> Host for &mut T), this doesn't work for resource methods that return the type itself:
27 | impl<T: FooImports + ?Sized> FooImports for &mut T {
   |      - this type parameter needs to be `Sized`
28 |     fn x(&mut self) -> T {
   |                        ^ doesn't have a size known at compile-time
I'm just going to drop the ?Sized unless you think its worth trying to special case somehow.

Wasmtime GitHub notifications bot (May 08 2024 at 15:28):

lann edited a comment on PR #8448:

@alexcrichton
re: https://github.com/bytecodealliance/wasmtime/issues/8382#issuecomment-2072986390 (?Sized for impl <T: Host> Host for &mut T), this doesn't work for methods that return the type itself:
27 | impl<T: FooImports + ?Sized> FooImports for &mut T {
   |      - this type parameter needs to be `Sized`
28 |     fn x(&mut self) -> T {
   |                        ^ doesn't have a size known at compile-time
I'm just going to drop the ?Sized unless you think its worth trying to special case somehow.

Wasmtime GitHub notifications bot (May 08 2024 at 15:29):

alexcrichton commented on PR #8448:

Is that actually the type parameter T shadowing a WIT type T? I don't think that any WIT api should return Self, and that might be fixable by renaming the type parameter to _T perhaps?

Wasmtime GitHub notifications bot (May 08 2024 at 15:35):

lann commented on PR #8448:

Ah, you are correct. That explains why I couldn't figure out where this would be happening...

Wasmtime GitHub notifications bot (May 10 2024 at 17:14):

lann updated PR #8448.

Wasmtime GitHub notifications bot (May 10 2024 at 20:26):

alexcrichton commented on PR #8448:

This has run into a bit of a snag. https://github.com/rust-lang/rust/issues/124984 shows code this PR was going to generate which compiled with Rust 1.77.0 but does not compile with Rust 1.78.0.

Wasmtime GitHub notifications bot (May 10 2024 at 20:56):

alexcrichton updated PR #8448.

Wasmtime GitHub notifications bot (May 10 2024 at 21:39):

alexcrichton updated PR #8448.

Wasmtime GitHub notifications bot (May 10 2024 at 21:45):

alexcrichton commented on PR #8448:

Ok I think most tests should be passing now. A summary of changes now at its current state:

Previously bindgen! would generate:

            pub fn add_to_linker<T, U>(
                linker: &mut wasmtime::component::Linker<T>,
                get: impl Fn(&mut T) -> &mut U + Send + Sync + Copy + 'static,
            ) -> wasmtime::Result<()>
            where
                U: Host,

Now it generates:

            pub trait GetHost<
                T,
            >: Fn(T) -> <Self as GetHost<T>>::Output + Send + Sync + Copy + 'static {
                type Output: Host;
            }

            impl<F, T, O> GetHost<T> for F
            where
                F: Fn(T) -> O + Send + Sync + Copy + 'static,
                O: Host,
            {
                type Output = O;
            }

            pub fn add_to_linker_get_host<T>(
                linker: &mut wasmtime::component::Linker<T>,
                host_getter: impl for<'a> GetHost<&'a mut T>,
            ) -> wasmtime::Result<()> {
                // ...
            }

            pub fn add_to_linker<T, U>(
                linker: &mut wasmtime::component::Linker<T>,
                get: impl Fn(&mut T) -> &mut U + Send + Sync + Copy + 'static,
            ) -> wasmtime::Result<()>
            where
                U: Host,
            {
                add_to_linker_get_host(linker, get)
            }

Effectively the old add_to_linker is still there but everything is actually built on a more-general GetHost trait which enables not being requried to use literally &mut U but instead you can use something like MyStruct<'a>. Usage of add_to_linker_get_host will probably require type-annotating closures. For example wasmtime-wasi contains:

    fn type_annotate<T, F>(val: F) -> F
    where
        F: Fn(&mut T) -> &mut T,
    {
        val
    }
    let closure = type_annotate::<T, _>(|t| t);

Usage of add_to_linker will not require type annotations over what's already required today.

I would like to, in a future commit I want to refactor all of wasmtime-wasi{,-http} to remove imp<T: WasiView> Host for T and replace that with impl Host for dyn WasiView using this support. That'll enable removing the odd skip_mut_forwarding_impls I've added to bindgen! here which is just to get WASI bits as-is compiling.

Wasmtime GitHub notifications bot (May 10 2024 at 21:46):

alexcrichton commented on PR #8448:

I should also say there are other minor details here and there but the comment above is the main thrust of the change, everything else is "smith a few things until everything compiles again"

Wasmtime GitHub notifications bot (May 13 2024 at 12:49):

lann submitted PR review:

Approach lgtm

Wasmtime GitHub notifications bot (May 13 2024 at 12:49):

lann submitted PR review:

Approach lgtm

Wasmtime GitHub notifications bot (May 13 2024 at 12:49):

lann created PR review comment:

Side note: we should pick a small subset of these tests with good feature coverage to expand. As-is these are useful for review but its hard to know how many/which files to review before stopping, and having so many changed files screws with Github's UI...

Wasmtime GitHub notifications bot (May 13 2024 at 12:49):

lann created PR review comment:

Slightly confusing in the definition but maybe more obvious for implementers?
                type Host: Host;

Wasmtime GitHub notifications bot (May 13 2024 at 16:15):

alexcrichton updated PR #8448.

Wasmtime GitHub notifications bot (May 13 2024 at 16:16):

alexcrichton created PR review comment:

That's a good point yeah I've been a bit sad that the actual changes were all hidden by default as well...

Wasmtime GitHub notifications bot (May 13 2024 at 16:16):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 13 2024 at 16:30):

alexcrichton updated PR #8448.

Wasmtime GitHub notifications bot (May 13 2024 at 16:30):

alexcrichton has marked PR #8448 as ready for review.

Wasmtime GitHub notifications bot (May 13 2024 at 16:30):

alexcrichton requested alexcrichton for a review on PR #8448.

Wasmtime GitHub notifications bot (May 13 2024 at 16:30):

alexcrichton requested wasmtime-core-reviewers for a review on PR #8448.

Wasmtime GitHub notifications bot (May 13 2024 at 16:43):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 13 2024 at 16:43):

alexcrichton has enabled auto merge for PR #8448.

Wasmtime GitHub notifications bot (May 13 2024 at 17:18):

alexcrichton updated PR #8448.

Wasmtime GitHub notifications bot (May 13 2024 at 17:19):

alexcrichton has enabled auto merge for PR #8448.

Wasmtime GitHub notifications bot (May 13 2024 at 17:45):

alexcrichton updated PR #8448.

Wasmtime GitHub notifications bot (May 13 2024 at 17:45):

alexcrichton has enabled auto merge for PR #8448.

Wasmtime GitHub notifications bot (May 13 2024 at 18:31):

alexcrichton merged PR #8448.

Last updated: Nov 22 2024 at 16:03 UTC