From 663a812f35b2e5141c56231b15e52cf863c4ac23 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 13 Jun 2024 09:00:43 -0700 Subject: [PATCH 1/3] Implement semver compatibility for exports This commit is an implementation of component model semver compatibility for export lookups. Previously in #7994 component imports were made semver-aware to ensure that bumping version numbers would not be a breaking change. This commit implements the same feature for component exports. This required some refactoring to move the definition of semver compat around and the previous refactoring in #8786 enables frontloading this work to happen before instantiation. Closes #8395 --- Cargo.lock | 1 + Cargo.toml | 1 + crates/environ/Cargo.toml | 3 +- crates/environ/src/component.rs | 2 + crates/environ/src/component/dfg.rs | 39 +-- crates/environ/src/component/info.rs | 4 +- crates/environ/src/component/names.rs | 271 ++++++++++++++++++ crates/environ/src/component/types_builder.rs | 2 +- crates/wasmtime/Cargo.toml | 2 +- .../src/runtime/component/component.rs | 2 +- .../src/runtime/component/instance.rs | 2 +- .../wasmtime/src/runtime/component/linker.rs | 202 +------------ .../src/runtime/component/matching.rs | 8 +- tests/all/component_model/instance.rs | 118 ++++++++ 14 files changed, 431 insertions(+), 226 deletions(-) create mode 100644 crates/environ/src/component/names.rs diff --git a/Cargo.lock b/Cargo.lock index d446e72ab678..b463ea22fca7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3647,6 +3647,7 @@ dependencies = [ "object 0.36.0", "postcard", "rustc-demangle", + "semver", "serde", "serde_derive", "target-lexicon", diff --git a/Cargo.toml b/Cargo.toml index 4a3f1158044d..e40074023490 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -320,6 +320,7 @@ postcard = { version = "1.0.8", default-features = false, features = ['alloc'] } criterion = { version = "0.5.0", default-features = false, features = ["html_reports", "rayon"] } rustc-hash = "1.1.0" libtest-mimic = "0.7.0" +semver = { version = "1.0.17", default-features = false } # ============================================================================= # diff --git a/crates/environ/Cargo.toml b/crates/environ/Cargo.toml index 5cef16b18ee5..7190ea09d5e0 100644 --- a/crates/environ/Cargo.toml +++ b/crates/environ/Cargo.toml @@ -34,6 +34,7 @@ target-lexicon = { workspace = true } wasm-encoder = { workspace = true, optional = true } wasmprinter = { workspace = true, optional = true } wasmtime-component-util = { workspace = true, optional = true } +semver = { workspace = true, optional = true, features = ['serde'] } [dev-dependencies] clap = { workspace = true, features = ['default'] } @@ -45,7 +46,7 @@ name = "factc" required-features = ['component-model', 'compile'] [features] -component-model = ["dep:wasmtime-component-util"] +component-model = ["dep:wasmtime-component-util", "dep:semver"] demangle = ['std', 'dep:rustc-demangle', 'dep:cpp_demangle'] gc = [] compile = [ diff --git a/crates/environ/src/component.rs b/crates/environ/src/component.rs index 96846182b130..10741a59c5e9 100644 --- a/crates/environ/src/component.rs +++ b/crates/environ/src/component.rs @@ -39,10 +39,12 @@ pub const MAX_FLAT_RESULTS: usize = 1; mod artifacts; mod info; +mod names; mod types; mod vmcomponent_offsets; pub use self::artifacts::*; pub use self::info::*; +pub use self::names::*; pub use self::types::*; pub use self::vmcomponent_offsets::*; diff --git a/crates/environ/src/component/dfg.rs b/crates/environ/src/component/dfg.rs index 9fb422f14d59..a271e27e8393 100644 --- a/crates/environ/src/component/dfg.rs +++ b/crates/environ/src/component/dfg.rs @@ -378,21 +378,12 @@ impl ComponentDfg { // Next the exports of the instance are handled which will likely end up // creating some lowered imports, perhaps some saved modules, etc. let mut export_items = PrimaryMap::new(); - let exports = self - .exports - .iter() - .map(|(name, export)| { - Ok(( - name.clone(), - linearize.export( - export, - &mut export_items, - wasmtime_types, - wasmparser_types, - )?, - )) - }) - .collect::>()?; + let mut exports = NameMap::default(); + for (name, export) in self.exports.iter() { + let export = + linearize.export(export, &mut export_items, wasmtime_types, wasmparser_types)?; + exports.insert(name, &mut (), false, export)?; + } // With all those pieces done the results of the dataflow-based // linearization are recorded into the `Component`. The number of @@ -531,15 +522,15 @@ impl LinearizeDfg<'_> { }, Export::Instance { ty, exports } => info::Export::Instance { ty: *ty, - exports: exports - .iter() - .map(|(name, export)| { - Ok(( - name.clone(), - self.export(export, items, wasmtime_types, wasmparser_types)?, - )) - }) - .collect::>()?, + exports: { + let mut map = NameMap::default(); + for (name, export) in exports { + let export = + self.export(export, items, wasmtime_types, wasmparser_types)?; + map.insert(name, &mut (), false, export)?; + } + map + }, }, Export::Type(def) => info::Export::Type(*def), }; diff --git a/crates/environ/src/component/info.rs b/crates/environ/src/component/info.rs index 0150904f85e6..401d4bafd5e1 100644 --- a/crates/environ/src/component/info.rs +++ b/crates/environ/src/component/info.rs @@ -107,7 +107,7 @@ pub struct Component { pub imports: PrimaryMap)>, /// This component's own root exports from the component itself. - pub exports: IndexMap, + pub exports: NameMap, /// All exports of this component and exported instances of this component. /// @@ -426,7 +426,7 @@ pub enum Export { /// Instance type index, if such is assigned ty: TypeComponentInstanceIndex, /// Instance export map - exports: IndexMap, + exports: NameMap, }, /// An exported type from a component or instance, currently only /// informational. diff --git a/crates/environ/src/component/names.rs b/crates/environ/src/component/names.rs new file mode 100644 index 000000000000..2991a990c33e --- /dev/null +++ b/crates/environ/src/component/names.rs @@ -0,0 +1,271 @@ +use crate::prelude::*; +use anyhow::{bail, Result}; +use core::hash::Hash; +use semver::Version; +use serde_derive::{Deserialize, Serialize}; + +/// A semver-aware map for imports/exports of a component. +/// +/// This data structure is used when looking up the names of imports/exports of +/// a component to enable semver-compatible matching of lookups. This will +/// enable lookups of `a:b/c@0.2.0` to match entries defined as `a:b/c@0.2.1` +/// which is currently considered a key feature of WASI's compatibility story. +/// +/// On the outside this looks like a map of `K` to `V`. +#[derive(Clone, Serialize, Deserialize, Debug)] +pub struct NameMap { + /// A map of keys to the value that they define. + /// + /// Note that this map is "exact" where the name here is the exact name that + /// was specified when the `insert` was called. This doesn't have any + /// semver-mangling or anything like that. + /// + /// This map is always consulted first during lookups. + definitions: IndexMap, + + /// An auxiliary map tracking semver-compatible names. This is a map from + /// "semver compatible alternate name" to a name present in `definitions` + /// and the semver version it was registered at. + /// + /// The `usize` entries here map to intern'd keys, so an example map could + /// be: + /// + /// ```text + /// { + /// "a:b/c@0.2": ("a:b/c@0.2.1", 0.2.1), + /// "a:b/c@2": ("a:b/c@2.0.0+abc", 2.0.0+abc), + /// } + /// ``` + /// + /// As names are inserted into `definitions` each name may have up to one + /// semver-compatible name with extra numbers/info chopped off which is + /// inserted into this map. This map is the lookup table from `@0.2` to + /// `@0.2.x` where `x` is what was inserted manually. + /// + /// The `Version` here is tracked to ensure that when multiple versions on + /// one track are defined that only the maximal version here is retained. + alternate_lookups: IndexMap, +} + +impl NameMap +where + K: Clone + Hash + Eq + Ord, +{ + /// Inserts the `name` specified into this map. + /// + /// The name is intern'd through the `cx` argument and shadowing is + /// controlled by the `allow_shadowing` variable. + /// + /// This function will automatically insert an entry in + /// `self.alternate_lookups` if `name` is a semver-looking name. + /// + /// Returns an error if `allow_shadowing` is `false` and the `name` is + /// already present in this map (by exact match). Otherwise returns the + /// intern'd version of `name`. + pub fn insert(&mut self, name: &str, cx: &mut I, allow_shadowing: bool, item: V) -> Result + where + I: NameMapIntern, + { + // Always insert `name` and `item` as an exact definition. + let key = cx.intern(name); + if let Some(prev) = self.definitions.insert(key.clone(), item) { + if !allow_shadowing { + self.definitions.insert(key, prev); + bail!("map entry `{name}` defined twice") + } + } + + // If `name` is a semver-looking thing, like `a:b/c@1.0.0`, then also + // insert an entry in the semver-compatible map under a key such as + // `a:b/c@1`. + // + // This key is used during `get` later on. + if let Some((alternate_key, version)) = alternate_lookup_key(name) { + let alternate_key = cx.intern(alternate_key); + if let Some((prev_key, prev_version)) = self + .alternate_lookups + .insert(alternate_key.clone(), (key.clone(), version.clone())) + { + // Prefer the latest version, so only do this if we're + // greater than the prior version. + if version < prev_version { + self.alternate_lookups + .insert(alternate_key, (prev_key, prev_version)); + } + } + } + Ok(key) + } + + /// Looks up `name` within this map, using the interning specified by + /// `cx`. + /// + /// This may return a definition even if `name` wasn't exactly defined in + /// this map, such as looking up `a:b/c@0.2.0` when the map only has + /// `a:b/c@0.2.1` defined. + pub fn get(&self, name: &str, cx: &I) -> Option<&V> + where + I: NameMapIntern, + { + // First look up an exact match and if that's found return that. This + // enables defining multiple versions in the map and the requested + // version is returned if it matches exactly. + let candidate = cx.lookup(name).and_then(|k| self.definitions.get(&k)); + if let Some(def) = candidate { + return Some(def); + } + + // Failing that, then try to look for a semver-compatible alternative. + // This looks up the key based on `name`, if any, and then looks to see + // if that was intern'd in `strings`. Given all that look to see if it + // was defined in `alternate_lookups` and finally at the end that exact + // key is then used to look up again in `self.definitions`. + let (alternate_name, _version) = alternate_lookup_key(name)?; + let alternate_key = cx.lookup(alternate_name)?; + let (exact_key, _version) = self.alternate_lookups.get(&alternate_key)?; + self.definitions.get(exact_key) + } + + /// Returns an iterator over inserted values in this map. + /// + /// Note that the iterator return yields intern'd keys and additionally does + /// not do anything special with semver names and such, it only literally + /// yields what's been inserted with [`NameMap::insert`]. + pub fn raw_iter(&self) -> impl Iterator { + self.definitions.iter() + } + + /// TODO + pub fn raw_get_mut(&mut self, key: &K) -> Option<&mut V> { + self.definitions.get_mut(key) + } +} + +impl Default for NameMap +where + K: Clone + Hash + Eq + Ord, +{ + fn default() -> NameMap { + NameMap { + definitions: Default::default(), + alternate_lookups: Default::default(), + } + } +} + +/// A helper trait used in conjunction with [`NameMap`] to optionally intern +/// keys to non-strings. +pub trait NameMapIntern { + /// The key that this interning context generates. + type Key; + + /// Inserts `s` into `self` and returns the intern'd key `Self`. + fn intern(&mut self, s: &str) -> Self::Key; + + /// Looks up `s` in `self` returning `Some` if it was found or `None` if + /// it's not present. + fn lookup(&self, s: &str) -> Option; +} + +impl NameMapIntern for () { + type Key = String; + + fn intern(&mut self, s: &str) -> String { + s.to_string() + } + + fn lookup(&self, s: &str) -> Option { + Some(s.to_string()) + } +} + +/// Determines a version-based "alternate lookup key" for the `name` specified. +/// +/// Some examples are: +/// +/// * `foo` => `None` +/// * `foo:bar/baz` => `None` +/// * `foo:bar/baz@1.1.2` => `Some(foo:bar/baz@1)` +/// * `foo:bar/baz@0.1.0` => `Some(foo:bar/baz@0.1)` +/// * `foo:bar/baz@0.0.1` => `None` +/// * `foo:bar/baz@0.1.0-rc.2` => `None` +/// +/// This alternate lookup key is intended to serve the purpose where a +/// semver-compatible definition can be located, if one is defined, at perhaps +/// either a newer or an older version. +fn alternate_lookup_key(name: &str) -> Option<(&str, Version)> { + let at = name.find('@')?; + let version_string = &name[at + 1..]; + let version = Version::parse(version_string).ok()?; + if !version.pre.is_empty() { + // If there's a prerelease then don't consider that compatible with any + // other version number. + None + } else if version.major != 0 { + // If the major number is nonzero then compatibility is up to the major + // version number, so return up to the first decimal. + let first_dot = version_string.find('.')? + at + 1; + Some((&name[..first_dot], version)) + } else if version.minor != 0 { + // Like the major version if the minor is nonzero then patch releases + // are all considered to be on a "compatible track". + let first_dot = version_string.find('.')? + at + 1; + let second_dot = name[first_dot + 1..].find('.')? + first_dot + 1; + Some((&name[..second_dot], version)) + } else { + // If the patch number is the first nonzero entry then nothing can be + // compatible with this patch, e.g. 0.0.1 isn't' compatible with + // any other version inherently. + None + } +} + +#[cfg(test)] +mod tests { + use super::NameMap; + + #[test] + fn alternate_lookup_key() { + fn alt(s: &str) -> Option<&str> { + super::alternate_lookup_key(s).map(|(s, _)| s) + } + + assert_eq!(alt("x"), None); + assert_eq!(alt("x:y/z"), None); + assert_eq!(alt("x:y/z@1.0.0"), Some("x:y/z@1")); + assert_eq!(alt("x:y/z@1.1.0"), Some("x:y/z@1")); + assert_eq!(alt("x:y/z@1.1.2"), Some("x:y/z@1")); + assert_eq!(alt("x:y/z@2.1.2"), Some("x:y/z@2")); + assert_eq!(alt("x:y/z@2.1.2+abc"), Some("x:y/z@2")); + assert_eq!(alt("x:y/z@0.1.2"), Some("x:y/z@0.1")); + assert_eq!(alt("x:y/z@0.1.3"), Some("x:y/z@0.1")); + assert_eq!(alt("x:y/z@0.2.3"), Some("x:y/z@0.2")); + assert_eq!(alt("x:y/z@0.2.3+abc"), Some("x:y/z@0.2")); + assert_eq!(alt("x:y/z@0.0.1"), None); + assert_eq!(alt("x:y/z@0.0.1-pre"), None); + assert_eq!(alt("x:y/z@0.1.0-pre"), None); + assert_eq!(alt("x:y/z@1.0.0-pre"), None); + } + + #[test] + fn name_map_smoke() { + let mut map = NameMap::default(); + + map.insert("a", &mut (), false, 0).unwrap(); + map.insert("b", &mut (), false, 1).unwrap(); + + assert!(map.insert("a", &mut (), false, 0).is_err()); + assert!(map.insert("a", &mut (), true, 0).is_ok()); + + assert_eq!(map.get("a", &()), Some(&0)); + assert_eq!(map.get("b", &()), Some(&1)); + assert_eq!(map.get("c", &()), None); + + map.insert("a:b/c@1.0.0", &mut (), false, 2).unwrap(); + map.insert("a:b/c@1.0.1", &mut (), false, 3).unwrap(); + assert_eq!(map.get("a:b/c@1.0.0", &()), Some(&2)); + assert_eq!(map.get("a:b/c@1.0.1", &()), Some(&3)); + assert_eq!(map.get("a:b/c@1.0.2", &()), Some(&3)); + assert_eq!(map.get("a:b/c@1.1.0", &()), Some(&3)); + } +} diff --git a/crates/environ/src/component/types_builder.rs b/crates/environ/src/component/types_builder.rs index d5ee00a1d400..1c471cc1e299 100644 --- a/crates/environ/src/component/types_builder.rs +++ b/crates/environ/src/component/types_builder.rs @@ -121,7 +121,7 @@ impl ComponentTypesBuilder { for (_, (name, ty)) in component.import_types.iter() { component_ty.imports.insert(name.clone(), *ty); } - for (name, ty) in component.exports.iter() { + for (name, ty) in component.exports.raw_iter() { component_ty.exports.insert( name.clone(), self.export_type_def(&component.export_items, *ty), diff --git a/crates/wasmtime/Cargo.toml b/crates/wasmtime/Cargo.toml index c76b3f90d8b6..2c52a4a768b4 100644 --- a/crates/wasmtime/Cargo.toml +++ b/crates/wasmtime/Cargo.toml @@ -54,7 +54,7 @@ bumpalo = "3.11.0" fxprof-processed-profile = { version = "0.6.0", optional = true } gimli = { workspace = true, optional = true } addr2line = { workspace = true, optional = true } -semver = { version = "1.0.17", optional = true, default-features = false } +semver = { workspace = true, optional = true } smallvec = { workspace = true, optional = true } hashbrown = { workspace = true } libm = "0.2.7" diff --git a/crates/wasmtime/src/runtime/component/component.rs b/crates/wasmtime/src/runtime/component/component.rs index 26017195c638..64076a8b1ab6 100644 --- a/crates/wasmtime/src/runtime/component/component.rs +++ b/crates/wasmtime/src/runtime/component/component.rs @@ -726,7 +726,7 @@ impl Component { } None => &info.exports, }; - exports.get(name).copied() + exports.get(name, &()).copied() } pub(crate) fn id(&self) -> CompiledModuleId { diff --git a/crates/wasmtime/src/runtime/component/instance.rs b/crates/wasmtime/src/runtime/component/instance.rs index 4c73084a93f7..9eb26ac4bdd7 100644 --- a/crates/wasmtime/src/runtime/component/instance.rs +++ b/crates/wasmtime/src/runtime/component/instance.rs @@ -350,7 +350,7 @@ where impl InstanceExportLookup for str { fn lookup(&self, component: &Component) -> Option { - component.env_component().exports.get(self).copied() + component.env_component().exports.get(self, &()).copied() } } diff --git a/crates/wasmtime/src/runtime/component/linker.rs b/crates/wasmtime/src/runtime/component/linker.rs index cb7795af9e45..fd98445b7f56 100644 --- a/crates/wasmtime/src/runtime/component/linker.rs +++ b/crates/wasmtime/src/runtime/component/linker.rs @@ -11,8 +11,8 @@ use alloc::sync::Arc; use core::future::Future; use core::marker; use core::pin::Pin; -use hashbrown::hash_map::{Entry, HashMap}; -use semver::Version; +use hashbrown::hash_map::HashMap; +use wasmtime_environ::component::{NameMap, NameMapIntern}; use wasmtime_environ::PrimaryMap; /// A type used to instantiate [`Component`]s. @@ -59,7 +59,7 @@ use wasmtime_environ::PrimaryMap; pub struct Linker { engine: Engine, strings: Strings, - map: NameMap, + map: NameMap, path: Vec, allow_shadowing: bool, _marker: marker::PhantomData T>, @@ -94,49 +94,14 @@ pub struct LinkerInstance<'a, T> { path: &'a mut Vec, path_len: usize, strings: &'a mut Strings, - map: &'a mut NameMap, + map: &'a mut NameMap, allow_shadowing: bool, _marker: marker::PhantomData T>, } -#[derive(Clone, Default)] -pub(crate) struct NameMap { - /// A map of interned strings to the name that they define. - /// - /// Note that this map is "exact" where the name here is the exact name that - /// was specified when the `Linker` was configured. This doesn't have any - /// semver-mangling or anything like that. - /// - /// This map is always consulted first during lookups. - definitions: HashMap, - - /// An auxiliary map tracking semver-compatible names. This is a map from - /// "semver compatible alternate name" to a name present in `definitions` - /// and the semver version it was registered at. - /// - /// The `usize` entries here map to intern'd keys, so an example map could - /// be: - /// - /// ```text - /// { - /// "a:b/c@0.2": ("a:b/c@0.2.1", 0.2.1), - /// "a:b/c@2": ("a:b/c@2.0.0+abc", 2.0.0+abc), - /// } - /// ``` - /// - /// As names are inserted into `definitions` each name may have up to one - /// semver-compatible name with extra numbers/info chopped off which is - /// inserted into this map. This map is the lookup table from `@0.2` to - /// `@0.2.x` where `x` is what was inserted manually. - /// - /// The `Version` here is tracked to ensure that when multiple versions on - /// one track are defined that only the maximal version here is retained. - alternate_lookups: HashMap, -} - #[derive(Clone)] pub(crate) enum Definition { - Instance(NameMap), + Instance(NameMap), Func(Arc), Module(Module), Resource(ResourceType, Arc), @@ -668,7 +633,7 @@ impl LinkerInstance<'_, T> { /// parameters. pub fn into_instance(mut self, name: &str) -> Result { let name = self.insert(name, Definition::Instance(NameMap::default()))?; - self.map = match self.map.definitions.get_mut(&name) { + self.map = match self.map.raw_get_mut(&name) { Some(Definition::Instance(map)) => map, _ => unreachable!(), }; @@ -680,136 +645,17 @@ impl LinkerInstance<'_, T> { fn insert(&mut self, name: &str, item: Definition) -> Result { self.map - .insert(name, &mut self.strings, self.allow_shadowing, item) + .insert(name, self.strings, self.allow_shadowing, item) } fn get(&self, name: &str) -> Option<&Definition> { - self.map.get(name, &self.strings) - } -} - -impl NameMap { - /// Looks up `name` within this map, using the interning specified by - /// `strings`. - /// - /// This may return a definition even if `name` wasn't exactly defined in - /// this map, such as looking up `a:b/c@0.2.0` when the map only has - /// `a:b/c@0.2.1` defined. - pub(crate) fn get(&self, name: &str, strings: &Strings) -> Option<&Definition> { - // First look up an exact match and if that's found return that. This - // enables defining multiple versions in the map and the requested - // version is returned if it matches exactly. - let candidate = strings.lookup(name).and_then(|k| self.definitions.get(&k)); - if let Some(def) = candidate { - return Some(def); - } - - // Failing that, then try to look for a semver-compatible alternative. - // This looks up the key based on `name`, if any, and then looks to see - // if that was intern'd in `strings`. Given all that look to see if it - // was defined in `alternate_lookups` and finally at the end that exact - // key is then used to look up again in `self.definitions`. - let (alternate_name, _version) = alternate_lookup_key(name)?; - let alternate_key = strings.lookup(alternate_name)?; - let (exact_key, _version) = self.alternate_lookups.get(&alternate_key)?; - self.definitions.get(exact_key) - } - - /// Inserts the `name` specified into this map. - /// - /// The name is intern'd through the `strings` argument and shadowing is - /// controlled by the `allow_shadowing` variable. - /// - /// This function will automatically insert an entry in - /// `self.alternate_lookups` if `name` is a semver-looking name. - fn insert( - &mut self, - name: &str, - strings: &mut Strings, - allow_shadowing: bool, - item: Definition, - ) -> Result { - // Always insert `name` and `item` as an exact definition. - let key = strings.intern(name); - match self.definitions.entry(key) { - Entry::Occupied(_) if !allow_shadowing => { - bail!("import of `{}` defined twice", strings.strings[key]) - } - Entry::Occupied(mut e) => { - e.insert(item); - } - Entry::Vacant(v) => { - v.insert(item); - } - } - - // If `name` is a semver-looking thing, like `a:b/c@1.0.0`, then also - // insert an entry in the semver-compatible map under a key such as - // `a:b/c@1`. - // - // This key is used during `get` later on. - if let Some((alternate_key, version)) = alternate_lookup_key(name) { - let alternate_key = strings.intern(alternate_key); - match self.alternate_lookups.entry(alternate_key) { - Entry::Occupied(mut e) => { - let (_, prev_version) = e.get(); - // Prefer the latest version, so only do this if we're - // greater than the prior version. - if version > *prev_version { - e.insert((key, version)); - } - } - Entry::Vacant(v) => { - v.insert((key, version)); - } - } - } - Ok(key) + self.map.get(name, self.strings) } } -/// Determines a version-based "alternate lookup key" for the `name` specified. -/// -/// Some examples are: -/// -/// * `foo` => `None` -/// * `foo:bar/baz` => `None` -/// * `foo:bar/baz@1.1.2` => `Some(foo:bar/baz@1)` -/// * `foo:bar/baz@0.1.0` => `Some(foo:bar/baz@0.1)` -/// * `foo:bar/baz@0.0.1` => `None` -/// * `foo:bar/baz@0.1.0-rc.2` => `None` -/// -/// This alternate lookup key is intended to serve the purpose where a -/// semver-compatible definition can be located, if one is defined, at perhaps -/// either a newer or an older version. -fn alternate_lookup_key(name: &str) -> Option<(&str, Version)> { - let at = name.find('@')?; - let version_string = &name[at + 1..]; - let version = Version::parse(version_string).ok()?; - if !version.pre.is_empty() { - // If there's a prerelease then don't consider that compatible with any - // other version number. - None - } else if version.major != 0 { - // If the major number is nonzero then compatibility is up to the major - // version number, so return up to the first decimal. - let first_dot = version_string.find('.')? + at + 1; - Some((&name[..first_dot], version)) - } else if version.minor != 0 { - // Like the major version if the minor is nonzero then patch releases - // are all considered to be on a "compatible track". - let first_dot = version_string.find('.')? + at + 1; - let second_dot = name[first_dot + 1..].find('.')? + first_dot + 1; - Some((&name[..second_dot], version)) - } else { - // If the patch number is the first nonzero entry then nothing can be - // compatible with this patch, e.g. 0.0.1 isn't' compatible with - // any other version inherently. - None - } -} +impl NameMapIntern for Strings { + type Key = usize; -impl Strings { fn intern(&mut self, string: &str) -> usize { if let Some(idx) = self.string2idx.get(string) { return *idx; @@ -821,33 +667,7 @@ impl Strings { idx } - pub fn lookup(&self, string: &str) -> Option { + fn lookup(&self, string: &str) -> Option { self.string2idx.get(string).cloned() } } - -#[cfg(test)] -mod tests { - #[test] - fn alternate_lookup_key() { - fn alt(s: &str) -> Option<&str> { - super::alternate_lookup_key(s).map(|(s, _)| s) - } - - assert_eq!(alt("x"), None); - assert_eq!(alt("x:y/z"), None); - assert_eq!(alt("x:y/z@1.0.0"), Some("x:y/z@1")); - assert_eq!(alt("x:y/z@1.1.0"), Some("x:y/z@1")); - assert_eq!(alt("x:y/z@1.1.2"), Some("x:y/z@1")); - assert_eq!(alt("x:y/z@2.1.2"), Some("x:y/z@2")); - assert_eq!(alt("x:y/z@2.1.2+abc"), Some("x:y/z@2")); - assert_eq!(alt("x:y/z@0.1.2"), Some("x:y/z@0.1")); - assert_eq!(alt("x:y/z@0.1.3"), Some("x:y/z@0.1")); - assert_eq!(alt("x:y/z@0.2.3"), Some("x:y/z@0.2")); - assert_eq!(alt("x:y/z@0.2.3+abc"), Some("x:y/z@0.2")); - assert_eq!(alt("x:y/z@0.0.1"), None); - assert_eq!(alt("x:y/z@0.0.1-pre"), None); - assert_eq!(alt("x:y/z@0.1.0-pre"), None); - assert_eq!(alt("x:y/z@1.0.0-pre"), None); - } -} diff --git a/crates/wasmtime/src/runtime/component/matching.rs b/crates/wasmtime/src/runtime/component/matching.rs index 94c24af87caa..4222daa6dc62 100644 --- a/crates/wasmtime/src/runtime/component/matching.rs +++ b/crates/wasmtime/src/runtime/component/matching.rs @@ -1,5 +1,5 @@ use crate::component::func::HostFunc; -use crate::component::linker::{Definition, NameMap, Strings}; +use crate::component::linker::{Definition, Strings}; use crate::component::ResourceType; use crate::prelude::*; use crate::runtime::vm::component::ComponentInstance; @@ -8,8 +8,8 @@ use crate::Module; use alloc::sync::Arc; use core::any::Any; use wasmtime_environ::component::{ - ComponentTypes, ResourceIndex, TypeComponentInstance, TypeDef, TypeFuncIndex, TypeModule, - TypeResourceTableIndex, + ComponentTypes, NameMap, ResourceIndex, TypeComponentInstance, TypeDef, TypeFuncIndex, + TypeModule, TypeResourceTableIndex, }; use wasmtime_environ::PrimaryMap; @@ -145,7 +145,7 @@ impl TypeChecker<'_> { fn instance( &mut self, expected: &TypeComponentInstance, - actual: Option<&NameMap>, + actual: Option<&NameMap>, ) -> Result<()> { // Like modules, every export in the expected type must be present in // the actual type. It's ok, though, to have extra exports in the actual diff --git a/tests/all/component_model/instance.rs b/tests/all/component_model/instance.rs index f24694dd26bb..a398c2e5cfeb 100644 --- a/tests/all/component_model/instance.rs +++ b/tests/all/component_model/instance.rs @@ -52,3 +52,121 @@ fn instance_exports() -> Result<()> { Ok(()) } + +#[test] +fn export_old_get_new() -> Result<()> { + let engine = super::engine(); + let component = r#" + (component + (core module $m) + (export "a:b/m@1.0.0" (core module $m)) + + (instance $i (export "m" (core module $m))) + (export "a:b/i@1.0.0" (instance $i)) + ) + "#; + + let component = Component::new(&engine, component)?; + component.export_index(None, "a:b/m@1.0.1").unwrap(); + let (_, i) = component.export_index(None, "a:b/i@1.0.1").unwrap(); + component.export_index(Some(&i), "m").unwrap(); + + let mut store = Store::new(&engine, ()); + let linker = Linker::new(&engine); + let instance = linker.instantiate(&mut store, &component)?; + + instance.get_module(&mut store, "a:b/m@1.0.1").unwrap(); + instance + .get_export(&mut store, None, "a:b/m@1.0.1") + .unwrap(); + + let i = instance + .get_export(&mut store, None, "a:b/i@1.0.1") + .unwrap(); + instance.get_export(&mut store, Some(&i), "m").unwrap(); + + Ok(()) +} + +#[test] +fn export_new_get_old() -> Result<()> { + let engine = super::engine(); + let component = r#" + (component + (core module $m) + (export "a:b/m@1.0.1" (core module $m)) + + (instance $i (export "m" (core module $m))) + (export "a:b/i@1.0.1" (instance $i)) + ) + "#; + + let component = Component::new(&engine, component)?; + component.export_index(None, "a:b/m@1.0.0").unwrap(); + let (_, i) = component.export_index(None, "a:b/i@1.0.0").unwrap(); + component.export_index(Some(&i), "m").unwrap(); + + let mut store = Store::new(&engine, ()); + let linker = Linker::new(&engine); + let instance = linker.instantiate(&mut store, &component)?; + + instance.get_module(&mut store, "a:b/m@1.0.0").unwrap(); + instance + .get_export(&mut store, None, "a:b/m@1.0.0") + .unwrap(); + + let i = instance + .get_export(&mut store, None, "a:b/i@1.0.0") + .unwrap(); + instance.get_export(&mut store, Some(&i), "m").unwrap(); + + Ok(()) +} + +#[test] +fn export_missing_get_max() -> Result<()> { + let engine = super::engine(); + let component = r#" + (component + (core module $m1) + (core module $m2 (import "" "" (func))) + (export "a:b/m@1.0.1" (core module $m1)) + (export "a:b/m@1.0.3" (core module $m2)) + ) + "#; + + fn assert_m2(module: &Module) { + assert_eq!(module.imports().len(), 1); + } + fn assert_m1(module: &Module) { + assert_eq!(module.imports().len(), 0); + } + + let component = Component::new(&engine, component)?; + let mut store = Store::new(&engine, ()); + let instance = Linker::new(&engine).instantiate(&mut store, &component)?; + + let tests = [ + ("a:b/m@1.0.0", assert_m2 as fn(&_)), // no exact, should pick max available + ("a:b/m@1.0.1", assert_m1), // exact hit + ("a:b/m@1.0.2", assert_m2), // no exact, should pick max available + ("a:b/m@1.0.3", assert_m2), // exact hit + ("a:b/m@1.0.4", assert_m2), // no exact, should pick max available + ]; + + for (name, test_fn) in tests { + println!("test {name}"); + let (_, m) = component.export_index(None, name).unwrap(); + let m = instance.get_module(&mut store, &m).unwrap(); + test_fn(&m); + + let m = instance.get_module(&mut store, name).unwrap(); + test_fn(&m); + + let m = instance.get_export(&mut store, None, name).unwrap(); + let m = instance.get_module(&mut store, &m).unwrap(); + test_fn(&m); + } + + Ok(()) +} From f3d44c7613e613c07c2dfc2030dcc83834607781 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 18 Jun 2024 10:34:11 -0700 Subject: [PATCH 2/3] Review comments --- crates/environ/src/component/dfg.rs | 4 ++-- crates/environ/src/component/names.rs | 11 +++++++---- crates/wasmtime/src/runtime/component/component.rs | 6 +++--- crates/wasmtime/src/runtime/component/instance.rs | 6 +++++- 4 files changed, 17 insertions(+), 10 deletions(-) diff --git a/crates/environ/src/component/dfg.rs b/crates/environ/src/component/dfg.rs index a271e27e8393..f308aeaf3783 100644 --- a/crates/environ/src/component/dfg.rs +++ b/crates/environ/src/component/dfg.rs @@ -382,7 +382,7 @@ impl ComponentDfg { for (name, export) in self.exports.iter() { let export = linearize.export(export, &mut export_items, wasmtime_types, wasmparser_types)?; - exports.insert(name, &mut (), false, export)?; + exports.insert(name, &mut NameMapNoIntern, false, export)?; } // With all those pieces done the results of the dataflow-based @@ -527,7 +527,7 @@ impl LinearizeDfg<'_> { for (name, export) in exports { let export = self.export(export, items, wasmtime_types, wasmparser_types)?; - map.insert(name, &mut (), false, export)?; + map.insert(name, &mut NameMapNoIntern, false, export)?; } map }, diff --git a/crates/environ/src/component/names.rs b/crates/environ/src/component/names.rs index 2991a990c33e..10931f5329bc 100644 --- a/crates/environ/src/component/names.rs +++ b/crates/environ/src/component/names.rs @@ -27,8 +27,7 @@ pub struct NameMap { /// "semver compatible alternate name" to a name present in `definitions` /// and the semver version it was registered at. /// - /// The `usize` entries here map to intern'd keys, so an example map could - /// be: + /// An example map would be: /// /// ```text /// { @@ -159,7 +158,7 @@ pub trait NameMapIntern { /// The key that this interning context generates. type Key; - /// Inserts `s` into `self` and returns the intern'd key `Self`. + /// Inserts `s` into `self` and returns the intern'd key `Self::Key`. fn intern(&mut self, s: &str) -> Self::Key; /// Looks up `s` in `self` returning `Some` if it was found or `None` if @@ -167,7 +166,11 @@ pub trait NameMapIntern { fn lookup(&self, s: &str) -> Option; } -impl NameMapIntern for () { +/// For use with [`NameMap`] when no interning should happen and instead string +/// keys are copied as-is. +pub struct NameMapNoIntern; + +impl NameMapIntern for NameMapNoIntern { type Key = String; fn intern(&mut self, s: &str) -> String { diff --git a/crates/wasmtime/src/runtime/component/component.rs b/crates/wasmtime/src/runtime/component/component.rs index 64076a8b1ab6..00cdb9e47e88 100644 --- a/crates/wasmtime/src/runtime/component/component.rs +++ b/crates/wasmtime/src/runtime/component/component.rs @@ -20,8 +20,8 @@ use core::ptr::NonNull; use std::path::Path; use wasmtime_environ::component::{ AllCallFunc, CompiledComponentInfo, ComponentArtifacts, ComponentTypes, Export, ExportIndex, - GlobalInitializer, InstantiateModule, StaticModuleIndex, TrampolineIndex, TypeComponentIndex, - TypeDef, VMComponentOffsets, + GlobalInitializer, InstantiateModule, NameMapNoIntern, StaticModuleIndex, TrampolineIndex, + TypeComponentIndex, TypeDef, VMComponentOffsets, }; use wasmtime_environ::{FunctionLoc, HostPtr, ObjectKind, PrimaryMap}; @@ -726,7 +726,7 @@ impl Component { } None => &info.exports, }; - exports.get(name, &()).copied() + exports.get(name, &NameMapNoIntern).copied() } pub(crate) fn id(&self) -> CompiledModuleId { diff --git a/crates/wasmtime/src/runtime/component/instance.rs b/crates/wasmtime/src/runtime/component/instance.rs index 9eb26ac4bdd7..4f1e710919be 100644 --- a/crates/wasmtime/src/runtime/component/instance.rs +++ b/crates/wasmtime/src/runtime/component/instance.rs @@ -350,7 +350,11 @@ where impl InstanceExportLookup for str { fn lookup(&self, component: &Component) -> Option { - component.env_component().exports.get(self, &()).copied() + component + .env_component() + .exports + .get(self, &NameMapNoIntern) + .copied() } } From 433dd17ec3cb6a037be4bb0ffedf73b19138dfc3 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 18 Jun 2024 10:59:42 -0700 Subject: [PATCH 3/3] Fix tests --- crates/environ/src/component/names.rs | 29 ++++++++++++++------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/crates/environ/src/component/names.rs b/crates/environ/src/component/names.rs index 10931f5329bc..e19cdd382f58 100644 --- a/crates/environ/src/component/names.rs +++ b/crates/environ/src/component/names.rs @@ -225,7 +225,7 @@ fn alternate_lookup_key(name: &str) -> Option<(&str, Version)> { #[cfg(test)] mod tests { - use super::NameMap; + use super::{NameMap, NameMapNoIntern}; #[test] fn alternate_lookup_key() { @@ -253,22 +253,23 @@ mod tests { #[test] fn name_map_smoke() { let mut map = NameMap::default(); + let mut intern = NameMapNoIntern; - map.insert("a", &mut (), false, 0).unwrap(); - map.insert("b", &mut (), false, 1).unwrap(); + map.insert("a", &mut intern, false, 0).unwrap(); + map.insert("b", &mut intern, false, 1).unwrap(); - assert!(map.insert("a", &mut (), false, 0).is_err()); - assert!(map.insert("a", &mut (), true, 0).is_ok()); + assert!(map.insert("a", &mut intern, false, 0).is_err()); + assert!(map.insert("a", &mut intern, true, 0).is_ok()); - assert_eq!(map.get("a", &()), Some(&0)); - assert_eq!(map.get("b", &()), Some(&1)); - assert_eq!(map.get("c", &()), None); + assert_eq!(map.get("a", &intern), Some(&0)); + assert_eq!(map.get("b", &intern), Some(&1)); + assert_eq!(map.get("c", &intern), None); - map.insert("a:b/c@1.0.0", &mut (), false, 2).unwrap(); - map.insert("a:b/c@1.0.1", &mut (), false, 3).unwrap(); - assert_eq!(map.get("a:b/c@1.0.0", &()), Some(&2)); - assert_eq!(map.get("a:b/c@1.0.1", &()), Some(&3)); - assert_eq!(map.get("a:b/c@1.0.2", &()), Some(&3)); - assert_eq!(map.get("a:b/c@1.1.0", &()), Some(&3)); + map.insert("a:b/c@1.0.0", &mut intern, false, 2).unwrap(); + map.insert("a:b/c@1.0.1", &mut intern, false, 3).unwrap(); + assert_eq!(map.get("a:b/c@1.0.0", &intern), Some(&2)); + assert_eq!(map.get("a:b/c@1.0.1", &intern), Some(&3)); + assert_eq!(map.get("a:b/c@1.0.2", &intern), Some(&3)); + assert_eq!(map.get("a:b/c@1.1.0", &intern), Some(&3)); } }