Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GetHost to generated bindings for more flexible linking #8448

Merged
merged 10 commits into from
May 13, 2024

Conversation

lann
Copy link
Contributor

@lann lann commented Apr 23, 2024

Work in progress for #8382

@lann lann changed the title Bindgen get host Add GetHost to generated bindings for more flexible linking Apr 23, 2024
@lann lann force-pushed the bindgen-get-host branch 2 times, most recently from 9581950 to 3ed3078 Compare May 7, 2024 17:35
@lann
Copy link
Contributor Author

lann commented May 8, 2024

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).

@lann
Copy link
Contributor Author

lann commented May 8, 2024

@alexcrichton
re: #8382 (comment) (?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.

@alexcrichton
Copy link
Member

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?

@lann
Copy link
Contributor Author

lann commented May 8, 2024

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

@alexcrichton
Copy link
Member

This has run into a bit of a snag. rust-lang/rust#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.

Use <https://users.rust-lang.org/t/generic-closure-returns-that-can-capture-arguments/76513/3>
as a guide of how to implement this by making the `GetHost` trait a bit
uglier.
Also enable this for WASI crates since they do their own thing with
`WasiView` for now. A future refactoring should be able to remove this
option entirely and switch wasi crates to a new design of `WasiView`.
@alexcrichton
Copy link
Member

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.

@alexcrichton
Copy link
Member

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"

Copy link
Contributor Author

@lann lann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach lgtm

crates/component-macro/tests/expanded/char.rs Outdated Show resolved Hide resolved
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@alexcrichton alexcrichton marked this pull request as ready for review May 13, 2024 16:30
@alexcrichton alexcrichton requested a review from a team as a code owner May 13, 2024 16:30
@alexcrichton alexcrichton requested review from alexcrichton and removed request for a team May 13, 2024 16:30
@alexcrichton alexcrichton added this pull request to the merge queue May 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 13, 2024
@alexcrichton alexcrichton added this pull request to the merge queue May 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 13, 2024
@alexcrichton alexcrichton added this pull request to the merge queue May 13, 2024
Merged via the queue into bytecodealliance:main with commit 3cd96e1 May 13, 2024
22 checks passed
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request May 13, 2024
This commit builds on the support from bytecodealliance#8448 to remove all blanket impls
from the WASI crates and instead replace them with concrete impls. This
is slightly functionally different from before where impls are now on
trait objects meaning dynamic dispatch is involved where previously
dynamic dispatch was used. That being said the perf hit here is expected
to be negligible-to-nonexistent since the implementations are large
enough that the dynamic dispatch won't be the hot path.

The motivations for this commit are:

* Removes the need for an odd `skip_mut_forwarding_impls` option - but
  this'll be left for a bit in case others need it.
* Improves incremental compile time of these crates where the crates
  themselves now contain all object code for all of WASI instead of
  forcing the final consume to codegen everything (although there's
  still a significant amount monomorphized).
* Improves future compatibility with refactorings of
  bindgen-generated-traits and such because blanket impls are pretty
  hard to work around where concrete impls are easier to reason about
  (and document).
@lann lann deleted the bindgen-get-host branch May 14, 2024 14:02
github-merge-queue bot pushed a commit that referenced this pull request May 14, 2024
This commit builds on the support from #8448 to remove all blanket impls
from the WASI crates and instead replace them with concrete impls. This
is slightly functionally different from before where impls are now on
trait objects meaning dynamic dispatch is involved where previously
dynamic dispatch was used. That being said the perf hit here is expected
to be negligible-to-nonexistent since the implementations are large
enough that the dynamic dispatch won't be the hot path.

The motivations for this commit are:

* Removes the need for an odd `skip_mut_forwarding_impls` option - but
  this'll be left for a bit in case others need it.
* Improves incremental compile time of these crates where the crates
  themselves now contain all object code for all of WASI instead of
  forcing the final consume to codegen everything (although there's
  still a significant amount monomorphized).
* Improves future compatibility with refactorings of
  bindgen-generated-traits and such because blanket impls are pretty
  hard to work around where concrete impls are easier to reason about
  (and document).
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request May 14, 2024
…dealliance#8448)

* Remove unused generated `add_root_to_linker` method

* WIP: bindgen GetHost

* Compile with Rust 1.78+

Use <https://users.rust-lang.org/t/generic-closure-returns-that-can-capture-arguments/76513/3>
as a guide of how to implement this by making the `GetHost` trait a bit
uglier.

* Add an option to skip `&mut T -> T` impls

Also enable this for WASI crates since they do their own thing with
`WasiView` for now. A future refactoring should be able to remove this
option entirely and switch wasi crates to a new design of `WasiView`.

* Update test expectations

* Review comments

* Undo temporary change

* Handle some TODOs

* Remove no-longer-relevant note

* Fix wasmtime-wasi-http doc link

---------

Co-authored-by: Alex Crichton <[email protected]>
alexcrichton added a commit that referenced this pull request May 14, 2024
…#8620)

* Remove unused generated `add_root_to_linker` method

* WIP: bindgen GetHost

* Compile with Rust 1.78+

Use <https://users.rust-lang.org/t/generic-closure-returns-that-can-capture-arguments/76513/3>
as a guide of how to implement this by making the `GetHost` trait a bit
uglier.

* Add an option to skip `&mut T -> T` impls

Also enable this for WASI crates since they do their own thing with
`WasiView` for now. A future refactoring should be able to remove this
option entirely and switch wasi crates to a new design of `WasiView`.

* Update test expectations

* Review comments

* Undo temporary change

* Handle some TODOs

* Remove no-longer-relevant note

* Fix wasmtime-wasi-http doc link

---------

Co-authored-by: Lann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants