Stream: git-wasmtime

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


view this post on Zulip Wasmtime GitHub notifications bot (Apr 23 2024 at 19:17):

lann edited PR #8448.

view this post on Zulip Wasmtime GitHub notifications bot (May 07 2024 at 16:13):

lann updated PR #8448.

view this post on Zulip Wasmtime GitHub notifications bot (May 07 2024 at 17:35):

lann updated PR #8448.

view this post on Zulip 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).

view this post on Zulip 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.

view this post on Zulip 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.

view this post on Zulip 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.

view this post on Zulip 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?

view this post on Zulip 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...

view this post on Zulip Wasmtime GitHub notifications bot (May 10 2024 at 17:14):

lann updated PR #8448.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (May 10 2024 at 20:56):

alexcrichton updated PR #8448.

view this post on Zulip Wasmtime GitHub notifications bot (May 10 2024 at 21:39):

alexcrichton updated PR #8448.

view this post on Zulip 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.

view this post on Zulip 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"

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2024 at 12:49):

lann submitted PR review:

Approach lgtm

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2024 at 12:49):

lann submitted PR review:

Approach lgtm

view this post on Zulip 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...

view this post on Zulip 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;

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2024 at 16:15):

alexcrichton updated PR #8448.

view this post on Zulip 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...

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2024 at 16:16):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2024 at 16:30):

alexcrichton updated PR #8448.

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2024 at 16:30):

alexcrichton has marked PR #8448 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2024 at 16:30):

alexcrichton requested alexcrichton for a review on PR #8448.

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2024 at 16:30):

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

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2024 at 16:43):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2024 at 16:43):

alexcrichton has enabled auto merge for PR #8448.

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2024 at 17:18):

alexcrichton updated PR #8448.

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2024 at 17:19):

alexcrichton has enabled auto merge for PR #8448.

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2024 at 17:45):

alexcrichton updated PR #8448.

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2024 at 17:45):

alexcrichton has enabled auto merge for PR #8448.

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2024 at 18:31):

alexcrichton merged PR #8448.


Last updated: Dec 23 2024 at 12:05 UTC