From 6083199341903f03376327f0a05e349600e1cb05 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 6 Oct 2023 07:09:52 -0700 Subject: [PATCH 1/2] Update syntax for `trappable_error_type` in `bindgen!` Following through on a suggestion from #7152 this makes the syntax a bit more consistent with the rest of `bindgen!` today. This additionally updates the documentation on the `bindgen!` macro itself. --- crates/component-macro/src/bindgen.rs | 23 +---- crates/component-macro/tests/codegen.rs | 4 +- crates/wasi/src/preview2/mod.rs | 14 ++- crates/wasmtime/src/component/mod.rs | 11 +-- crates/wit-bindgen/src/lib.rs | 93 ++++++-------------- tests/all/component_model/bindgen/results.rs | 10 +-- 6 files changed, 46 insertions(+), 109 deletions(-) diff --git a/crates/component-macro/src/bindgen.rs b/crates/component-macro/src/bindgen.rs index c5a9d2920ce8..bae15a3a5586 100644 --- a/crates/component-macro/src/bindgen.rs +++ b/crates/component-macro/src/bindgen.rs @@ -318,28 +318,11 @@ impl Parse for Opt { } fn trappable_error_field_parse(input: ParseStream<'_>) -> Result { - // Accept a Rust identifier or a string literal. This is required - // because not all wit identifiers are Rust identifiers, so we can - // smuggle the invalid ones inside quotes. - fn ident_or_str(input: ParseStream<'_>) -> Result { - let l = input.lookahead1(); - if l.peek(syn::LitStr) { - Ok(input.parse::()?.value()) - } else if l.peek(syn::Ident) { - Ok(input.parse::()?.to_string()) - } else { - Err(l.error()) - } - } - - let wit_package_path = input.parse::()?.value(); - input.parse::()?; - let wit_type_name = ident_or_str(input)?; - input.parse::()?; + let wit_path = input.parse::()?.value(); + input.parse::]>()?; let rust_type_name = input.parse::()?.to_token_stream().to_string(); Ok(TrappableError { - wit_package_path, - wit_type_name, + wit_path, rust_type_name, }) } diff --git a/crates/component-macro/tests/codegen.rs b/crates/component-macro/tests/codegen.rs index 49bb8c6d5726..50a2f8a02446 100644 --- a/crates/component-macro/tests/codegen.rs +++ b/crates/component-macro/tests/codegen.rs @@ -140,8 +140,8 @@ mod trappable_errors { } ", trappable_error_type: { - "demo:pkg/a"::"b": MyX, - "demo:pkg/c"::"b": MyX, + "demo:pkg/a/b" => MyX, + "demo:pkg/c/b" => MyX, }, }); diff --git a/crates/wasi/src/preview2/mod.rs b/crates/wasi/src/preview2/mod.rs index aecc414ea4b2..aefdcb44ab42 100644 --- a/crates/wasi/src/preview2/mod.rs +++ b/crates/wasi/src/preview2/mod.rs @@ -71,8 +71,8 @@ pub mod bindings { ", tracing: true, trappable_error_type: { - "wasi:io/streams"::"stream-error": StreamError, - "wasi:filesystem/types"::"error-code": FsError, + "wasi:io/streams/stream-error" => StreamError, + "wasi:filesystem/types/error-code" => FsError, }, with: { "wasi:clocks/wall-clock": crate::preview2::bindings::clocks::wall_clock, @@ -149,17 +149,15 @@ pub mod bindings { "poll-one", ], }, - with: { - "wasi:sockets/ip-name-lookup/resolve-address-stream": super::ip_name_lookup::ResolveAddressStream, - }, trappable_error_type: { - "wasi:io/streams"::"stream-error": crate::preview2::StreamError, - "wasi:filesystem/types"::"error-code": crate::preview2::FsError, - "wasi:sockets/network"::"error-code": crate::preview2::SocketError, + "wasi:io/streams/stream-error" => crate::preview2::StreamError, + "wasi:filesystem/types/error-code" => crate::preview2::FsError, + "wasi:sockets/network/error-code" => crate::preview2::SocketError, }, with: { "wasi:sockets/network/network": super::network::Network, "wasi:sockets/tcp/tcp-socket": super::tcp::TcpSocket, + "wasi:sockets/ip-name-lookup/resolve-address-stream": super::ip_name_lookup::ResolveAddressStream, "wasi:filesystem/types/directory-entry-stream": super::filesystem::ReaddirIterator, "wasi:filesystem/types/descriptor": super::filesystem::Descriptor, "wasi:io/streams/input-stream": super::stream::InputStream, diff --git a/crates/wasmtime/src/component/mod.rs b/crates/wasmtime/src/component/mod.rs index 73aaad8e1cfc..460d1d08e35d 100644 --- a/crates/wasmtime/src/component/mod.rs +++ b/crates/wasmtime/src/component/mod.rs @@ -299,14 +299,15 @@ pub(crate) use self::store::ComponentStoreData; /// /// // This can be used to translate WIT return values of the form /// // `result` into `Result` in Rust. -/// // The `RustErrorType` structure will have an automatically generated -/// // implementation of `From for RustErrorType`. The -/// // `RustErrorType` additionally can also represent a trap to -/// // conveniently flatten all errors into one container. +/// // Users must define `RustErrorType` and the `Host` trait for the +/// // interface which defines `error-type` will have a method +/// // called `convert_error_type` which converts `RustErrorType` +/// // into `wasmtime::Result`. This conversion can either +/// // return the raw WIT error (`ErrorType` here) or a trap. /// // /// // By default this option is not specified. /// trappable_error_type: { -/// interface::ErrorType: RustErrorType, +/// "wasi:io/streams/stream-error" => RustErrorType, /// }, /// /// // All generated bindgen types are "owned" meaning types like `String` diff --git a/crates/wit-bindgen/src/lib.rs b/crates/wit-bindgen/src/lib.rs index 7183e0bf232c..08d355b46887 100644 --- a/crates/wit-bindgen/src/lib.rs +++ b/crates/wit-bindgen/src/lib.rs @@ -1,6 +1,6 @@ use crate::rust::{to_rust_ident, to_rust_upper_camel_case, RustGenerator, TypeMode}; use crate::types::{TypeInfo, Types}; -use anyhow::{anyhow, bail, Context}; +use anyhow::{anyhow, Context}; use heck::*; use indexmap::{IndexMap, IndexSet}; use std::collections::{BTreeMap, HashMap, HashSet}; @@ -114,11 +114,8 @@ pub struct Opts { #[derive(Debug, Clone)] pub struct TrappableError { - /// The package and interface that define the error type being mapped. - pub wit_package_path: String, - - /// The name of the error type in WIT that is being mapped. - pub wit_type_name: String, + /// Full path to the error, such as `wasi:io/streams/error`. + pub wit_path: String, /// The name, in Rust, of the error type to generate. pub rust_type_name: String, @@ -220,7 +217,7 @@ impl Wasmtime { fn generate(&mut self, resolve: &Resolve, id: WorldId) -> String { self.types.analyze(resolve, id); for (i, te) in self.opts.trappable_error_type.iter().enumerate() { - let id = resolve_type_in_package(resolve, &te.wit_package_path, &te.wit_type_name) + let id = resolve_type_in_package(resolve, &te.wit_path) .context(format!("resolving {:?}", te)) .unwrap(); let name = format!("_TrappableError{i}"); @@ -770,77 +767,37 @@ impl Wasmtime { } } -fn resolve_type_in_package( - resolve: &Resolve, - package_path: &str, - type_name: &str, -) -> anyhow::Result { +fn resolve_type_in_package(resolve: &Resolve, wit_path: &str) -> anyhow::Result { // foo:bar/baz - let (namespace, rest) = package_path - .split_once(':') - .ok_or_else(|| anyhow!("Invalid package path: missing package identifier"))?; - - let (package_name, iface_name) = rest - .split_once('/') - .ok_or_else(|| anyhow!("Invalid package path: missing namespace separator"))?; - - // TODO: we should handle version annotations - if package_name.contains('@') { - bail!("Invalid package path: version parsing is not currently handled"); - } - - let packages = Vec::from_iter( - resolve - .package_names - .iter() - .filter(|(pname, _)| pname.namespace == namespace && pname.name == package_name), - ); - - if packages.len() != 1 { - if packages.is_empty() { - bail!("No package named `{}`", namespace); - } else { - // Getting here is a bug, parsing version identifiers would disambiguate the intended - // package. - bail!( - "Multiple packages named `{}` found ({:?})", - namespace, - packages - ); - } - } + let (_, package) = resolve + .packages + .iter() + .find(|(_, p)| wit_path.starts_with(&p.name.to_string())) + .ok_or_else(|| anyhow!("no package found to match `{wit_path}`"))?; - let (_, &package_id) = packages[0]; - let package = &resolve.packages[package_id]; + let wit_path = wit_path.strip_prefix(&package.name.to_string()).unwrap(); + let wit_path = wit_path + .strip_prefix('/') + .ok_or_else(|| anyhow!("expected `/` after package name"))?; - let (_, &iface_id) = package + let (iface_name, interface) = package .interfaces .iter() - .find(|(name, _)| name.as_str() == iface_name) - .ok_or_else(|| { - anyhow!( - "Unknown interface `{}` in package `{}`", - iface_name, - package_path - ) - })?; + .find(|(name, _)| wit_path.starts_with(name.as_str())) + .ok_or_else(|| anyhow!("no interface found to match `{wit_path}` in package"))?; - let iface = &resolve.interfaces[iface_id]; + let wit_path = wit_path.strip_prefix(iface_name.as_str()).unwrap(); + let wit_path = wit_path + .strip_prefix('/') + .ok_or_else(|| anyhow!("expected `/` after interface name"))?; - let (_, &type_id) = iface + let (_, id) = resolve.interfaces[*interface] .types .iter() - .find(|(n, _)| n.as_str() == type_name) - .ok_or_else(|| { - anyhow!( - "No type named `{}` in package `{}`", - package_name, - package_path - ) - })?; - - Ok(type_id) + .find(|(name, _)| wit_path == name.as_str()) + .ok_or_else(|| anyhow!("no types found to match `{wit_path}` in interface"))?; + Ok(*id) } struct InterfaceGenerator<'a> { diff --git a/tests/all/component_model/bindgen/results.rs b/tests/all/component_model/bindgen/results.rs index 32a645682629..8a3dede07f38 100644 --- a/tests/all/component_model/bindgen/results.rs +++ b/tests/all/component_model/bindgen/results.rs @@ -238,7 +238,7 @@ mod enum_error { enum-error: func(a: float64) -> result } }", - trappable_error_type: { "inline:inline/imports"::e1: TrappableE1 } + trappable_error_type: { "inline:inline/imports/e1" => TrappableE1 } }); // You can create concrete trap types which make it all the way out to the @@ -418,9 +418,7 @@ mod record_error { record-error: func(a: float64) -> result } }", - // Literal strings can be used for the interface and typename fields instead of - // identifiers, because wit identifiers arent always Rust identifiers. - trappable_error_type: { "inline:inline/imports"::"e2": TrappableE2 } + trappable_error_type: { "inline:inline/imports/e2" => TrappableE2 } }); pub enum TrappableE2 { @@ -591,7 +589,7 @@ mod variant_error { variant-error: func(a: float64) -> result } }", - trappable_error_type: { "inline:inline/imports"::e3: TrappableE3 } + trappable_error_type: { "inline:inline/imports/e3" => TrappableE3 } }); pub enum TrappableE3 { @@ -786,7 +784,7 @@ mod multiple_interfaces_error { enum-error: func(a: float64) -> result } }", - trappable_error_type: { "inline:inline/types"::e1: TrappableE1 } + trappable_error_type: { "inline:inline/types/e1" => TrappableE1 } }); pub enum TrappableE1 { From d0d0f79e28af7017dbd4fd8b2fdde40edae43146 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 6 Oct 2023 08:46:49 -0700 Subject: [PATCH 2/2] Update parsing to afford for versions --- crates/wit-bindgen/src/lib.rs | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/crates/wit-bindgen/src/lib.rs b/crates/wit-bindgen/src/lib.rs index 08d355b46887..c432140892f1 100644 --- a/crates/wit-bindgen/src/lib.rs +++ b/crates/wit-bindgen/src/lib.rs @@ -770,24 +770,16 @@ impl Wasmtime { fn resolve_type_in_package(resolve: &Resolve, wit_path: &str) -> anyhow::Result { // foo:bar/baz - let (_, package) = resolve + let (_, interface) = resolve .packages .iter() - .find(|(_, p)| wit_path.starts_with(&p.name.to_string())) - .ok_or_else(|| anyhow!("no package found to match `{wit_path}`"))?; + .flat_map(|(_, p)| p.interfaces.iter()) + .find(|(_, id)| wit_path.starts_with(&resolve.id_of(**id).unwrap())) + .ok_or_else(|| anyhow!("no package/interface found to match `{wit_path}`"))?; - let wit_path = wit_path.strip_prefix(&package.name.to_string()).unwrap(); let wit_path = wit_path - .strip_prefix('/') - .ok_or_else(|| anyhow!("expected `/` after package name"))?; - - let (iface_name, interface) = package - .interfaces - .iter() - .find(|(name, _)| wit_path.starts_with(name.as_str())) - .ok_or_else(|| anyhow!("no interface found to match `{wit_path}` in package"))?; - - let wit_path = wit_path.strip_prefix(iface_name.as_str()).unwrap(); + .strip_prefix(&resolve.id_of(*interface).unwrap()) + .unwrap(); let wit_path = wit_path .strip_prefix('/') .ok_or_else(|| anyhow!("expected `/` after interface name"))?;