Skip to content

Commit

Permalink
Fix some minor issues in WIT world merging (#1791)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@alexcrichton
alexcrichton committed Sep 17, 2024
1 parent 610b7aa commit a099de3
Show file tree
Hide file tree
Showing 8 changed files with 221 additions and 124 deletions.
222 changes: 127 additions & 95 deletions crates/wit-parser/src/resolve.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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()));
}
}
Expand Down Expand Up @@ -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<InterfaceId, WorldKey>,
) -> 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<Item = InterfaceId> + '_ {
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.
Expand Down Expand Up @@ -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);
})
}
}
}
Expand Down Expand Up @@ -1017,15 +1069,6 @@ package {name} is defined in two different locations:\n\
Ok(())
}

fn collect_interface_deps(&self, interface: InterfaceId, deps: &mut IndexSet<InterfaceId>) {
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];
Expand Down Expand Up @@ -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),
);
});
}
}

Expand Down Expand Up @@ -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<bool> {
Ok(match stability {
Stability::Unknown => true,
Expand Down
46 changes: 26 additions & 20 deletions fuzz/src/roundtrip_wit.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use arbitrary::{Result, Unstructured};
use std::path::Path;
use wasmparser::WasmFeatures;
use wit_component::*;
use wit_parser::{PackageId, Resolve};

Expand All @@ -14,13 +13,15 @@ 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);

let (resolve2, pkg2) = match wit_component::decode(&wasm).unwrap() {
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);
Expand All @@ -32,31 +33,27 @@ 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);
wit_component::embed_component_metadata(&mut dummy, &resolve, id, StringEncoding::UTF8)
.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)
.unwrap()
.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();
Expand All @@ -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(())
}

Expand Down
Loading

0 comments on commit a099de3

Please sign in to comment.