From a099de30d0a8d46de64870cd1946739268e77d4b Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 17 Sep 2024 17:06:24 -0500 Subject: [PATCH] Fix some minor issues in WIT world merging (#1791) This commit updates the `roundtrip_wit` fuzzer to test out merging of worlds, not just merging of `Resolve`. This then fixes various bugs that cropped up where `ensure_can_add_world_exports` wasn't ensuring enough. --- crates/wit-parser/src/resolve.rs | 222 ++++++++++-------- fuzz/src/roundtrip_wit.rs | 46 ++-- tests/cli/fail-merge.wit | 22 ++ tests/cli/fail-merge.wit.stderr | 6 + tests/cli/merge-fail-to-add-import.wit | 23 ++ tests/cli/merge-fail-to-add-import.wit.stderr | 6 + .../world-merging-export-semantic-change.wit | 14 +- ...-merging-export-semantic-change.wit.stderr | 6 +- 8 files changed, 221 insertions(+), 124 deletions(-) create mode 100644 tests/cli/fail-merge.wit create mode 100644 tests/cli/fail-merge.wit.stderr create mode 100644 tests/cli/merge-fail-to-add-import.wit create mode 100644 tests/cli/merge-fail-to-add-import.wit.stderr diff --git a/crates/wit-parser/src/resolve.rs b/crates/wit-parser/src/resolve.rs index 975e86222e..3a9ee24e6d 100644 --- a/crates/wit-parser/src/resolve.rs +++ b/crates/wit-parser/src/resolve.rs @@ -767,6 +767,24 @@ package {name} is defined in two different locations:\n\ } } + // Build a set of interfaces which are required to be imported because + // of `into`'s exports. This set is then used below during + // `ensure_can_add_world_export`. + // + // This is the set of interfaces which exports depend on that are + // themselves not exports. + let mut must_be_imported = HashMap::new(); + for (key, export) in into_world.exports.iter() { + for dep in self.world_item_direct_deps(export) { + if into_world.exports.contains_key(&WorldKey::Interface(dep)) { + continue; + } + self.foreach_interface_dep(dep, &mut |id| { + must_be_imported.insert(id, key.clone()); + }); + } + } + // Next walk over exports of `from` and process these similarly to // imports. for (name, from_export) in from_world.exports.iter() { @@ -779,10 +797,15 @@ package {name} is defined in two different locations:\n\ None => { // See comments in `ensure_can_add_world_export` for why // this is slightly different than imports. - self.ensure_can_add_world_export(into_world, name, from_export) - .with_context(|| { - format!("failed to add export `{}`", self.name_world_key(name)) - })?; + self.ensure_can_add_world_export( + into_world, + name, + from_export, + &must_be_imported, + ) + .with_context(|| { + format!("failed to add export `{}`", self.name_world_key(name)) + })?; new_exports.push((name.clone(), from_export.clone())); } } @@ -855,74 +878,99 @@ package {name} is defined in two different locations:\n\ /// This method ensures that the world export of `name` and `item` can be /// added to the world `into` without changing the meaning of `into`. /// - /// This is somewhat tricky due to how exports/imports are elaborated today - /// but the basic idea is that the transitive dependencies of an `export` - /// will be implicitly `import`ed if they're not otherwise listed as - /// exports. That means that if a transitive dependency of a preexisting - /// export is added as a new export it might change the meaning of an - /// existing import if it was otherwise already hooked up to an import. + /// All dependencies of world exports must either be: + /// + /// * An export themselves + /// * An import with all transitive dependencies of the import also imported /// - /// This method rules out this situation. + /// It's not possible to depend on an import which then also depends on an + /// export at some point, for example. This method ensures that if `name` + /// and `item` are added that this property is upheld. fn ensure_can_add_world_export( &self, into: &World, name: &WorldKey, item: &WorldItem, + must_be_imported: &HashMap, ) -> Result<()> { assert!(!into.exports.contains_key(name)); - let interface = match name { - // Top-level exports always depend on imports, so these are always - // allowed to be added. - WorldKey::Name(_) => return Ok(()), - - // This is the case we're worried about. Here if the key is an - // interface then the item must also be an interface. - WorldKey::Interface(key) => { - match item { - WorldItem::Interface { id, .. } => assert_eq!(id, key), - _ => unreachable!(), - } - *key - } - }; + let name = self.name_world_key(name); - // For `interface` to be added as a new export of `into` then it must be - // the case that no previous export of `into` depends on `interface`. - // Test that by walking all interface exports and seeing if any types - // refer to this interface. - for (export_name, export) in into.exports.iter() { - let export_interface = match export_name { - WorldKey::Name(_) => continue, - WorldKey::Interface(key) => { - match export { - WorldItem::Interface { id, .. } => assert_eq!(id, key), - _ => unreachable!(), - } - *key - } - }; - assert!(export_interface != interface); - let iface = &self.interfaces[export_interface]; - for (name, ty) in iface.types.iter() { - let other_ty = match self.types[*ty].kind { - TypeDefKind::Type(Type::Id(ty)) => ty, - _ => continue, - }; - if self.types[other_ty].owner != TypeOwner::Interface(interface) { - continue; - } + // First make sure that all of this item's dependencies are either + // exported or the entire chain of imports rooted at that dependency are + // all imported. + for dep in self.world_item_direct_deps(item) { + if into.exports.contains_key(&WorldKey::Interface(dep)) { + continue; + } + self.ensure_not_exported(into, dep) + .with_context(|| format!("failed validating export of `{name}`"))?; + } - let export_name = self.name_world_key(export_name); + // Second make sure that this item, if it's an interface, will not alter + // the meaning of the preexisting world by ensuring that it's not in the + // set of "must be imported" items. + if let WorldItem::Interface { id, .. } = item { + if let Some(export) = must_be_imported.get(&id) { + let export_name = self.name_world_key(export); bail!( - "export `{export_name}` has a type `{name}` which could \ - change meaning if this world export were added" - ) + "export `{export_name}` depends on `{name}` \ + previously as an import which will change meaning \ + if `{name}` is added as an export" + ); } } Ok(()) } + fn ensure_not_exported(&self, world: &World, id: InterfaceId) -> Result<()> { + let key = WorldKey::Interface(id); + let name = self.name_world_key(&key); + if world.exports.contains_key(&key) { + bail!( + "world exports `{name}` but it's also transitively used by an \ + import \ + which means that this is not valid" + ) + } + for dep in self.interface_direct_deps(id) { + self.ensure_not_exported(world, dep) + .with_context(|| format!("failed validating transitive import dep `{name}`"))?; + } + Ok(()) + } + + /// Returns an iterator of all the direct interface dependencies of this + /// `item`. + /// + /// Note that this doesn't include transitive dependencies, that must be + /// followed manually. + fn world_item_direct_deps(&self, item: &WorldItem) -> impl Iterator + '_ { + let mut interface = None; + let mut ty = None; + match item { + WorldItem::Function(_) => {} + WorldItem::Type(id) => ty = Some(*id), + WorldItem::Interface { id, .. } => interface = Some(*id), + } + + interface + .into_iter() + .flat_map(move |id| self.interface_direct_deps(id)) + .chain(ty.and_then(|t| self.type_interface_dep(t))) + } + + /// Invokes `f` with `id` and all transitive interface dependencies of `id`. + /// + /// Note that `f` may be called with the same id multiple times. + fn foreach_interface_dep(&self, id: InterfaceId, f: &mut dyn FnMut(InterfaceId)) { + f(id); + for dep in self.interface_direct_deps(id) { + self.foreach_interface_dep(dep, f); + } + } + /// Returns the ID of the specified `interface`. /// /// Returns `None` for unnamed interfaces. @@ -950,16 +998,20 @@ package {name} is defined in two different locations:\n\ // Collect the set of interfaces which are depended on by exports. Also // all imported types are assumed to stay so collect any interfaces // they depend on. - let mut live_through_exports = IndexSet::default(); + let mut live_through_exports = IndexSet::new(); for (_, export) in self.worlds[world_id].exports.iter() { if let WorldItem::Interface { id, .. } = export { - self.collect_interface_deps(*id, &mut live_through_exports); + self.foreach_interface_dep(*id, &mut |id| { + live_through_exports.insert(id); + }) } } for (_, import) in self.worlds[world_id].imports.iter() { if let WorldItem::Type(ty) = import { if let Some(dep) = self.type_interface_dep(*ty) { - self.collect_interface_deps(dep, &mut live_through_exports); + self.foreach_interface_dep(dep, &mut |id| { + live_through_exports.insert(id); + }) } } } @@ -1017,15 +1069,6 @@ package {name} is defined in two different locations:\n\ Ok(()) } - fn collect_interface_deps(&self, interface: InterfaceId, deps: &mut IndexSet) { - if !deps.insert(interface) { - return; - } - for dep in self.interface_direct_deps(interface) { - self.collect_interface_deps(dep, deps); - } - } - /// Returns the ID of the specified `name` within the `pkg`. pub fn id_of_name(&self, pkg: PackageId, name: &str) -> String { let package = &self.packages[pkg]; @@ -1534,11 +1577,24 @@ package {name} is defined in two different locations:\n\ WorldItem::Interface { id, .. } => { for dep in self.interface_direct_deps(*id) { let dep_key = WorldKey::Interface(dep); - if !world.exports.contains_key(&dep_key) { - self.assert_interface_and_all_deps_imported_and_not_exported( - world, key, dep, - ); + if world.exports.contains_key(&dep_key) { + continue; } + self.foreach_interface_dep(dep, &mut |dep| { + let dep_key = WorldKey::Interface(dep); + assert!( + world.imports.contains_key(&dep_key), + "world should import {} (required by {})", + self.name_world_key(&dep_key), + self.name_world_key(key), + ); + assert!( + !world.exports.contains_key(&dep_key), + "world should not export {} (required by {})", + self.name_world_key(&dep_key), + self.name_world_key(key), + ); + }); } } @@ -1605,30 +1661,6 @@ package {name} is defined in two different locations:\n\ } } - fn assert_interface_and_all_deps_imported_and_not_exported( - &self, - world: &World, - key: &WorldKey, - id: InterfaceId, - ) { - let dep_key = WorldKey::Interface(id); - assert!( - world.imports.contains_key(&dep_key), - "world should import {} (required by {})", - self.name_world_key(&dep_key), - self.name_world_key(key), - ); - assert!( - !world.exports.contains_key(&dep_key), - "world should not export {} (required by {})", - self.name_world_key(&dep_key), - self.name_world_key(key), - ); - for dep in self.interface_direct_deps(id) { - self.assert_interface_and_all_deps_imported_and_not_exported(world, key, dep); - } - } - fn include_stability(&self, stability: &Stability, pkg_id: &PackageId) -> Result { Ok(match stability { Stability::Unknown => true, diff --git a/fuzz/src/roundtrip_wit.rs b/fuzz/src/roundtrip_wit.rs index b1b22fd3a0..c1cf5bdf8b 100644 --- a/fuzz/src/roundtrip_wit.rs +++ b/fuzz/src/roundtrip_wit.rs @@ -1,6 +1,5 @@ use arbitrary::{Result, Unstructured}; use std::path::Path; -use wasmparser::WasmFeatures; use wit_component::*; use wit_parser::{PackageId, Resolve}; @@ -14,6 +13,7 @@ pub fn run(u: &mut Unstructured<'_>) -> Result<()> { DecodedWasm::WitPackage(resolve, pkg) => (resolve, pkg), DecodedWasm::Component(..) => unreachable!(), }; + resolve.assert_valid(); roundtrip_through_printing("doc1", &resolve, pkg, &wasm); @@ -21,6 +21,7 @@ pub fn run(u: &mut Unstructured<'_>) -> Result<()> { DecodedWasm::WitPackage(resolve, pkgs) => (resolve, pkgs), DecodedWasm::Component(..) => unreachable!(), }; + resolve2.assert_valid(); let wasm2 = wit_component::encode(&resolve2, pkg2).expect("failed to encode WIT document"); write_file("doc2.wasm", &wasm2); @@ -32,7 +33,7 @@ pub fn run(u: &mut Unstructured<'_>) -> Result<()> { // If there's hundreds or thousands of worlds only work with the first few // to avoid timing out this fuzzer with asan enabled. - let mut decoded_worlds = Vec::new(); + let mut decoded_bindgens = Vec::new(); for (id, world) in resolve.worlds.iter().take(20) { log::debug!("embedding world {} as in a dummy module", world.name); let mut dummy = wit_component::dummy_module(&resolve, id); @@ -40,11 +41,6 @@ pub fn run(u: &mut Unstructured<'_>) -> Result<()> { .unwrap(); write_file("dummy.wasm", &dummy); - // Decode what was just created and record it later for testing merging - // worlds together. - let (_, decoded) = wit_component::metadata::decode(&dummy).unwrap(); - decoded_worlds.push(decoded.resolve); - log::debug!("... componentizing the world into a binary component"); let wasm = wit_component::ComponentEncoder::default() .module(&dummy) @@ -52,11 +48,12 @@ pub fn run(u: &mut Unstructured<'_>) -> Result<()> { .encode() .unwrap(); write_file("dummy.component.wasm", &wasm); - wasmparser::Validator::new_with_features( - WasmFeatures::default() | WasmFeatures::COMPONENT_MODEL, - ) - .validate_all(&wasm) - .unwrap(); + wasmparser::Validator::new().validate_all(&wasm).unwrap(); + + // Decode what was just created and record it later for testing merging + // worlds together. + let (_, decoded) = wit_component::metadata::decode(&dummy).unwrap(); + decoded_bindgens.push((decoded, dummy, world.name.clone())); log::debug!("... decoding the component itself"); wit_component::decode(&wasm).unwrap(); @@ -66,19 +63,28 @@ pub fn run(u: &mut Unstructured<'_>) -> Result<()> { log::debug!("... importizing this world"); let mut resolve2 = resolve.clone(); let _ = resolve2.importize(id); - resolve.assert_valid(); } - if decoded_worlds.len() < 2 { + if decoded_bindgens.len() < 2 { return Ok(()); } - log::debug!("merging worlds"); - let w1 = u.choose(&decoded_worlds)?; - let w2 = u.choose(&decoded_worlds)?; - let mut dst = w1.clone(); - dst.merge(w2.clone()).unwrap(); - dst.assert_valid(); + let i = u.choose_index(decoded_bindgens.len())?; + let (mut b1, wasm1, world1) = decoded_bindgens.swap_remove(i); + let i = u.choose_index(decoded_bindgens.len())?; + let (b2, wasm2, world2) = decoded_bindgens.swap_remove(i); + + log::debug!("merging bindgens world {world1} <- world {world2}"); + + write_file("bindgen1.wasm", &wasm1); + write_file("bindgen2.wasm", &wasm2); + + // Merging worlds may fail but if successful then a `Resolve` is asserted + // to be valid which is what we're interested in here. Note that failure + // here can be due to the structure of worlds which aren't reasonable to + // control in this generator, so it's just done to see what happens and try + // to trigger panics in `Resolve::assert_valid`. + let _ = b1.merge(b2); Ok(()) } diff --git a/tests/cli/fail-merge.wit b/tests/cli/fail-merge.wit new file mode 100644 index 0000000000..88f464fa21 --- /dev/null +++ b/tests/cli/fail-merge.wit @@ -0,0 +1,22 @@ +// FAIL: component embed --dummy --world into % | component embed --world %from % | component wit + +package a:b; + +interface i1 { + type t = u32; +} +interface i2 { + use i1.{t}; +} + +world into { + export x: interface { + use i2.{t}; + } +} + +// `into` has a transitive dep on importing i2 and must then import i1. Means +// we can't import `i1` when merging this in. +world %from { + export i1; +} diff --git a/tests/cli/fail-merge.wit.stderr b/tests/cli/fail-merge.wit.stderr new file mode 100644 index 0000000000..4fac37967a --- /dev/null +++ b/tests/cli/fail-merge.wit.stderr @@ -0,0 +1,6 @@ +error: updating metadata for section component-type + +Caused by: + 0: failed to merge worlds from two documents + 1: failed to add export `a:b/i1` + 2: export `x` depends on `a:b/i1` previously as an import which will change meaning if `a:b/i1` is added as an export diff --git a/tests/cli/merge-fail-to-add-import.wit b/tests/cli/merge-fail-to-add-import.wit new file mode 100644 index 0000000000..dd96013e72 --- /dev/null +++ b/tests/cli/merge-fail-to-add-import.wit @@ -0,0 +1,23 @@ +// FAIL: component embed --dummy --world into % | component embed --world %from % | component wit + +package a:b; + +interface i1 { + type t = u32; +} +interface i2 { + use i1.{t}; +} + +world into { + export i2; +} + +// This world cannot be merged into `into` because it would change the meaning +// of `into`. Previously in `into` i2's export of `i2` would refer to the +// implicit import of `i1`, but here the export of `i2` refers to the export of +// `i1`. +world %from { + export i1; + export i2; +} diff --git a/tests/cli/merge-fail-to-add-import.wit.stderr b/tests/cli/merge-fail-to-add-import.wit.stderr new file mode 100644 index 0000000000..bee6661206 --- /dev/null +++ b/tests/cli/merge-fail-to-add-import.wit.stderr @@ -0,0 +1,6 @@ +error: updating metadata for section component-type + +Caused by: + 0: failed to merge worlds from two documents + 1: failed to add export `a:b/i1` + 2: export `a:b/i2` depends on `a:b/i1` previously as an import which will change meaning if `a:b/i1` is added as an export diff --git a/tests/cli/world-merging-export-semantic-change.wit b/tests/cli/world-merging-export-semantic-change.wit index ec10ad06d5..ca4ab2db28 100644 --- a/tests/cli/world-merging-export-semantic-change.wit +++ b/tests/cli/world-merging-export-semantic-change.wit @@ -8,17 +8,17 @@ interface i1 { interface i2 { use i1.{t}; } +interface i3 { + use i2.{t}; +} world into { + export i1; export i2; } -// This world cannot be merged into `into` because it would change the meaning -// of `into`. Previously in `into` i2's export of `i2` would refer to the -// implicit import of `i1`, but here the export of `i2` refers to the export of -// `i1`. world %from { - export i1; - export i2; + export x: interface { + use i3.{t}; + } } - diff --git a/tests/cli/world-merging-export-semantic-change.wit.stderr b/tests/cli/world-merging-export-semantic-change.wit.stderr index 8d0bf87dc6..27c7420109 100644 --- a/tests/cli/world-merging-export-semantic-change.wit.stderr +++ b/tests/cli/world-merging-export-semantic-change.wit.stderr @@ -2,5 +2,7 @@ error: updating metadata for section component-type Caused by: 0: failed to merge worlds from two documents - 1: failed to add export `a:b/i1` - 2: export `a:b/i2` has a type `t` which could change meaning if this world export were added + 1: failed to add export `x` + 2: failed validating export of `x` + 3: failed validating transitive import dep `a:b/i3` + 4: world exports `a:b/i2` but it's also transitively used by an import which means that this is not valid