From 54d3695896c8f20fddf3dc6dfdbe1d022784f7b2 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 31 May 2024 14:53:52 -0500 Subject: [PATCH] Always generate the same output structure with `bindgen!` (#8721) Currently with the `bindgen!` macro when the `with` key is used then the generated bindings are different than if the `with` key was not used. Not only for replacement purposes but the generated bindings are missing two key pieces: * In the generated `add_to_linker` functions bounds and invocations of `with`-overridden interfaces are all missing. This means that the generated `add_to_linker` functions don't actually represent the full world. * The generated module hierarchy has "holes" for all the modules that are overridden. While it's mostly a minor inconvenience it's also easy enough to generate everything via `pub use` to have everything hooked up correctly. After this PR it means that each `bindgen!` macro should, in isolation, work for any other `bindgen!` macro invocation. It shouldn't be necessary to weave things together and remember how each macro was invoked along the way. This is primarily to unblock #8715 which is running into a case where tcp/udp are generated as sync but their dependency, `wasi:sockets/network`, is used from an upstream async version. The generated `add_to_linker` does not compile because tcp/udp depend on `wasi:sockets/network` isn't added to the linker. After fixing that it then required more modules to be generated, hence this PR. --- crates/component-macro/src/bindgen.rs | 9 ++ crates/component-macro/tests/codegen.rs | 75 ++++++++++++++ crates/wasi-http/src/proxy.rs | 3 +- crates/wasi/src/bindings.rs | 1 + crates/wasmtime/src/runtime/component/mod.rs | 6 ++ crates/wit-bindgen/src/lib.rs | 100 ++++++++++++++----- 6 files changed, 169 insertions(+), 25 deletions(-) diff --git a/crates/component-macro/src/bindgen.rs b/crates/component-macro/src/bindgen.rs index eb05d4ad81..e5113a5e16 100644 --- a/crates/component-macro/src/bindgen.rs +++ b/crates/component-macro/src/bindgen.rs @@ -154,6 +154,7 @@ impl Parse for Config { Opt::Features(f) => { features.extend(f.into_iter().map(|f| f.value())); } + Opt::RequireStoreDataSend(val) => opts.require_store_data_send = val, } } } else { @@ -228,6 +229,7 @@ mod kw { syn::custom_keyword!(stringify); syn::custom_keyword!(skip_mut_forwarding_impls); syn::custom_keyword!(features); + syn::custom_keyword!(require_store_data_send); } enum Opt { @@ -245,6 +247,7 @@ enum Opt { Stringify(bool), SkipMutForwardingImpls(bool), Features(Vec), + RequireStoreDataSend(bool), } impl Parse for Opt { @@ -402,6 +405,12 @@ impl Parse for Opt { syn::bracketed!(contents in input); let list = Punctuated::<_, Token![,]>::parse_terminated(&contents)?; Ok(Opt::Features(list.into_iter().collect())) + } else if l.peek(kw::require_store_data_send) { + input.parse::()?; + input.parse::()?; + Ok(Opt::RequireStoreDataSend( + input.parse::()?.value, + )) } else { Err(l.error()) } diff --git a/crates/component-macro/tests/codegen.rs b/crates/component-macro/tests/codegen.rs index f68f5930b5..df3c3954da 100644 --- a/crates/component-macro/tests/codegen.rs +++ b/crates/component-macro/tests/codegen.rs @@ -574,3 +574,78 @@ mod custom_derives { } } } + +mod with_and_mixing_async { + mod with_async { + wasmtime::component::bindgen!({ + inline: " + package my:inline; + interface foo { + type t = u32; + foo: func() -> t; + } + interface bar { + use foo.{t}; + bar: func() -> t; + } + world x { + import bar; + } + ", + async: { + only_imports: ["bar"], + }, + }); + } + + mod without_async { + wasmtime::component::bindgen!({ + inline: " + package my:inline; + interface foo { + type t = u32; + foo: func() -> t; + } + interface bar { + use foo.{t}; + bar: func() -> t; + } + world x { + import bar; + } + ", + with: { + "my:inline/foo": super::with_async::my::inline::foo, + }, + require_store_data_send: true, + }); + } + + mod third { + wasmtime::component::bindgen!({ + inline: " + package my:inline; + interface foo { + type t = u32; + foo: func() -> t; + } + interface bar { + use foo.{t}; + bar: func() -> t; + } + interface baz { + use bar.{t}; + baz: func() -> t; + } + world x { + import baz; + } + ", + with: { + "my:inline/foo": super::with_async::my::inline::foo, + "my:inline/bar": super::without_async::my::inline::bar, + }, + require_store_data_send: true, + }); + } +} diff --git a/crates/wasi-http/src/proxy.rs b/crates/wasi-http/src/proxy.rs index 2d18f12430..23454af117 100644 --- a/crates/wasi-http/src/proxy.rs +++ b/crates/wasi-http/src/proxy.rs @@ -136,9 +136,10 @@ pub mod sync { async: false, with: { "wasi:http": crate::bindings::http, // http is in this crate - "wasi:io": wasmtime_wasi::bindings::sync, // io is sync + "wasi:io": wasmtime_wasi::bindings::sync::io, // io is sync "wasi": wasmtime_wasi::bindings, // everything else }, + require_store_data_send: true, }); } diff --git a/crates/wasi/src/bindings.rs b/crates/wasi/src/bindings.rs index 6c2c10b715..374647fb21 100644 --- a/crates/wasi/src/bindings.rs +++ b/crates/wasi/src/bindings.rs @@ -34,6 +34,7 @@ pub mod sync { "wasi:io/streams/input-stream": super::super::io::streams::InputStream, "wasi:io/streams/output-stream": super::super::io::streams::OutputStream, }, + require_store_data_send: true, }); } pub use self::generated::exports; diff --git a/crates/wasmtime/src/runtime/component/mod.rs b/crates/wasmtime/src/runtime/component/mod.rs index d7fd4e1037..7fb7f7ef8c 100644 --- a/crates/wasmtime/src/runtime/component/mod.rs +++ b/crates/wasmtime/src/runtime/component/mod.rs @@ -529,6 +529,12 @@ pub(crate) use self::store::ComponentStoreData; /// // /// // This option defaults to an empty array. /// features: ["foo", "bar", "baz"], +/// +/// // An niche configuration option to require that the `T` in `Store` +/// // is always `Send` in the generated bindings. Typically not needed +/// // but if synchronous bindings depend on asynchronous bindings using +/// // the `with` key then this may be required. +/// require_store_data_send: false, /// }); /// ``` pub use wasmtime_component_macro::bindgen; diff --git a/crates/wit-bindgen/src/lib.rs b/crates/wit-bindgen/src/lib.rs index 23b9e8c4b2..7b5c449db9 100644 --- a/crates/wit-bindgen/src/lib.rs +++ b/crates/wit-bindgen/src/lib.rs @@ -31,10 +31,25 @@ use source::Source; enum InterfaceName { /// This interface was remapped using `with` to some other Rust code. Remapped { + /// This is the `::`-separated string which is the path to the mapped + /// item relative to the root of the `bindgen!` macro invocation. + /// + /// This path currently starts with `__with_name$N` and will then + /// optionally have `::` projections through to the actual item + /// depending on how `with` was configured. name_at_root: String, + + /// This is currently only used for exports and is the relative path to + /// where this mapped name would be located if `with` were not + /// specified. Basically it's the same as the `Path` variant of this + /// enum if the mapping weren't present. local_path: Vec, }, + /// This interface is generated in the module hierarchy specified. + /// + /// The path listed here is the path, from the root of the `bindgen!` macro, + /// to where this interface is generated. Path(Vec), } @@ -42,6 +57,11 @@ enum InterfaceName { struct Wasmtime { src: Source, opts: Opts, + /// A list of all interfaces which were imported by this world. + /// + /// The first value here is the contents of the module that this interface + /// generated. The second value is the name of the interface as also present + /// in `self.interface_names`. import_interfaces: Vec<(String, InterfaceName)>, import_functions: Vec, exports: Exports, @@ -129,6 +149,13 @@ pub struct Opts { /// `wasmtime-wasi` crate while that's given a chance to update its b /// indings. pub skip_mut_forwarding_impls: bool, + + /// Indicates that the `T` in `Store` should be send even if async is not + /// enabled. + /// + /// This is helpful when sync bindings depend on generated functions from + /// async bindings as is the case with WASI in-tree. + pub require_store_data_send: bool, } #[derive(Debug, Clone)] @@ -204,6 +231,10 @@ impl Opts { r.opts = self.clone(); r.generate(resolve, world) } + + fn is_store_data_send(&self) -> bool { + self.async_.maybe_async() || self.require_store_data_send + } } impl Wasmtime { @@ -351,17 +382,7 @@ impl Wasmtime { } WorldItem::Interface { id, .. } => { gen.gen.interface_last_seen_as_import.insert(*id, true); - if gen.gen.name_interface(resolve, *id, name, false) { - return; - } gen.current_interface = Some((*id, name, false)); - gen.types(*id); - let key_name = resolve.name_world_key(name); - - gen.generate_add_to_linker(*id, &key_name); - - let module = &gen.src[..]; - let snake = match name { WorldKey::Name(s) => s.to_snake_case(), WorldKey::Interface(id) => resolve.interfaces[*id] @@ -370,17 +391,48 @@ impl Wasmtime { .unwrap() .to_snake_case(), }; - let module = format!( - " - #[allow(clippy::all)] - pub mod {snake} {{ - #[allow(unused_imports)] - use wasmtime::component::__internal::anyhow; - - {module} - }} - " - ); + let module = if gen.gen.name_interface(resolve, *id, name, false) { + // If this interface is remapped then that means that it was + // provided via the `with` key in the bindgen configuration. + // That means that bindings generation is skipped here. To + // accomodate future bindgens depending on this bindgen + // though we still generate a module which reexports the + // original module. This helps maintain the same output + // structure regardless of whether `with` is used. + let name_at_root = match &gen.gen.interface_names[id] { + InterfaceName::Remapped { name_at_root, .. } => name_at_root, + InterfaceName::Path(_) => unreachable!(), + }; + let path_to_root = gen.path_to_root(); + format!( + " + pub mod {snake} {{ + #[allow(unused_imports)] + pub use {path_to_root}{name_at_root}::*; + }} + " + ) + } else { + // If this interface is not remapped then it's time to + // actually generate bindings here. + gen.types(*id); + let key_name = resolve.name_world_key(name); + gen.generate_add_to_linker(*id, &key_name); + + let module = &gen.src[..]; + + format!( + " + #[allow(clippy::all)] + pub mod {snake} {{ + #[allow(unused_imports)] + use wasmtime::component::__internal::anyhow; + + {module} + }} + " + ) + }; self.import_interfaces .push((module, self.interface_names[id].clone())); } @@ -979,7 +1031,7 @@ impl Wasmtime { .iter() .map(|(_, name)| match name { InterfaceName::Path(path) => path.join("::"), - InterfaceName::Remapped { .. } => unreachable!("imported a remapped module"), + InterfaceName::Remapped { name_at_root, .. } => name_at_root.clone(), }) .collect() } @@ -1007,7 +1059,7 @@ impl Wasmtime { } let camel = to_rust_upper_camel_case(&resolve.worlds[world].name); - let data_bounds = if self.opts.async_.maybe_async() { + let data_bounds = if self.opts.is_store_data_send() { "T: Send," } else { "" @@ -1937,7 +1989,7 @@ impl<'a> InterfaceGenerator<'a> { } uwriteln!(self.src, "}}"); - let (data_bounds, mut host_bounds) = if self.gen.opts.async_.maybe_async() { + let (data_bounds, mut host_bounds) = if self.gen.opts.is_store_data_send() { ("T: Send,", "Host + Send".to_string()) } else { ("", "Host".to_string())