From de775dd0c4dc8be68d730783ab265003b8b11c5f Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 12 Sep 2024 16:05:26 -0500 Subject: [PATCH] Beef up validity assertions on `Resolve` (#1785) This commit extends the `Resolve::assert_valid` function with more assertions about the structure of worlds notably to guarantee they are "elaborated" meaning that they always list all dependencies of imports. This is required by bindings generators and encoding. This property is already upheld internally and is intended to reflect a preexisting property with dynamic assertion checks. The underlying motivation for this is to assist in the development and fuzzing of #1784. --- crates/wit-parser/src/lib.rs | 25 +++- crates/wit-parser/src/live.rs | 189 ++++++++++++++++++++++++++----- crates/wit-parser/src/resolve.rs | 147 +++++++++++++++++++++++- 3 files changed, 328 insertions(+), 33 deletions(-) diff --git a/crates/wit-parser/src/lib.rs b/crates/wit-parser/src/lib.rs index a97353811a..f26b87f01f 100644 --- a/crates/wit-parser/src/lib.rs +++ b/crates/wit-parser/src/lib.rs @@ -23,7 +23,7 @@ pub use sizealign::*; mod resolve; pub use resolve::{Package, PackageId, Remap, Resolve}; mod live; -pub use live::LiveTypes; +pub use live::{LiveTypes, TypeIdVisitor}; #[cfg(feature = "serde")] use serde_derive::Serialize; @@ -808,6 +808,18 @@ pub enum FunctionKind { Constructor(TypeId), } +impl FunctionKind { + /// Returns the resource, if present, that this function kind refers to. + pub fn resource(&self) -> Option { + match self { + FunctionKind::Freestanding => None, + FunctionKind::Method(id) | FunctionKind::Static(id) | FunctionKind::Constructor(id) => { + Some(*id) + } + } + } +} + impl Function { pub fn item_name(&self) -> &str { match &self.kind { @@ -819,6 +831,17 @@ impl Function { } } + /// Returns an iterator over the types used in parameters and results. + /// + /// Note that this iterator is not transitive, it only iterates over the + /// direct references to types that this function has. + pub fn parameter_and_result_types(&self) -> impl Iterator + '_ { + self.params + .iter() + .map(|(_, t)| *t) + .chain(self.results.iter_types().copied()) + } + /// Gets the core export name for this function. pub fn core_export_name<'a>(&'a self, interface: Option<&str>) -> Cow<'a, str> { match interface { diff --git a/crates/wit-parser/src/live.rs b/crates/wit-parser/src/live.rs index 1b6986c839..e8dfac1994 100644 --- a/crates/wit-parser/src/live.rs +++ b/crates/wit-parser/src/live.rs @@ -1,5 +1,6 @@ use crate::{ - Function, FunctionKind, InterfaceId, Resolve, Type, TypeDefKind, TypeId, WorldId, WorldItem, + Function, FunctionKind, InterfaceId, Resolve, Type, TypeDef, TypeDefKind, TypeId, WorldId, + WorldItem, }; use indexmap::IndexSet; @@ -18,36 +19,88 @@ impl LiveTypes { } pub fn add_interface(&mut self, resolve: &Resolve, iface: InterfaceId) { + self.visit_interface(resolve, iface); + } + + pub fn add_world(&mut self, resolve: &Resolve, world: WorldId) { + self.visit_world(resolve, world); + } + + pub fn add_world_item(&mut self, resolve: &Resolve, item: &WorldItem) { + self.visit_world_item(resolve, item); + } + + pub fn add_func(&mut self, resolve: &Resolve, func: &Function) { + self.visit_func(resolve, func); + } + + pub fn add_type_id(&mut self, resolve: &Resolve, ty: TypeId) { + self.visit_type_id(resolve, ty); + } + + pub fn add_type(&mut self, resolve: &Resolve, ty: &Type) { + self.visit_type(resolve, ty); + } +} + +impl TypeIdVisitor for LiveTypes { + fn before_visit_type_id(&mut self, id: TypeId) -> bool { + !self.set.contains(&id) + } + + fn after_visit_type_id(&mut self, id: TypeId) { + assert!(self.set.insert(id)); + } +} + +/// Helper trait to walk the structure of a type and visit all `TypeId`s that +/// it refers to, possibly transitively. +pub trait TypeIdVisitor { + /// Callback invoked just before a type is visited. + /// + /// If this function returns `false` the type is not visited, otherwise it's + /// recursed into. + fn before_visit_type_id(&mut self, id: TypeId) -> bool { + let _ = id; + true + } + + /// Callback invoked once a type is finished being visited. + fn after_visit_type_id(&mut self, id: TypeId) { + let _ = id; + } + + fn visit_interface(&mut self, resolve: &Resolve, iface: InterfaceId) { let iface = &resolve.interfaces[iface]; for (_, id) in iface.types.iter() { - self.add_type_id(resolve, *id); + self.visit_type_id(resolve, *id); } for (_, func) in iface.functions.iter() { - self.add_func(resolve, func); + self.visit_func(resolve, func); } } - pub fn add_world(&mut self, resolve: &Resolve, world: WorldId) { + fn visit_world(&mut self, resolve: &Resolve, world: WorldId) { let world = &resolve.worlds[world]; for (_, item) in world.imports.iter().chain(world.exports.iter()) { - self.add_world_item(resolve, item); + self.visit_world_item(resolve, item); } } - pub fn add_world_item(&mut self, resolve: &Resolve, item: &WorldItem) { + fn visit_world_item(&mut self, resolve: &Resolve, item: &WorldItem) { match item { - WorldItem::Interface { id, .. } => self.add_interface(resolve, *id), - WorldItem::Function(f) => self.add_func(resolve, f), - WorldItem::Type(t) => self.add_type_id(resolve, *t), + WorldItem::Interface { id, .. } => self.visit_interface(resolve, *id), + WorldItem::Function(f) => self.visit_func(resolve, f), + WorldItem::Type(t) => self.visit_type_id(resolve, *t), } } - pub fn add_func(&mut self, resolve: &Resolve, func: &Function) { + fn visit_func(&mut self, resolve: &Resolve, func: &Function) { match func.kind { // This resource is live as it's attached to a static method but // it's not guaranteed to be present in either params or results, so // be sure to attach it here. - FunctionKind::Static(id) => self.add_type_id(resolve, id), + FunctionKind::Static(id) => self.visit_type_id(resolve, id), // The resource these are attached to is in the params/results, so // no need to re-add it here. @@ -57,70 +110,146 @@ impl LiveTypes { } for (_, ty) in func.params.iter() { - self.add_type(resolve, ty); + self.visit_type(resolve, ty); } for ty in func.results.iter_types() { - self.add_type(resolve, ty); + self.visit_type(resolve, ty); } } - pub fn add_type_id(&mut self, resolve: &Resolve, ty: TypeId) { - if self.set.contains(&ty) { - return; + fn visit_type_id(&mut self, resolve: &Resolve, ty: TypeId) { + if self.before_visit_type_id(ty) { + self.visit_type_def(resolve, &resolve.types[ty]); + self.after_visit_type_id(ty); } - match &resolve.types[ty].kind { + } + + fn visit_type_def(&mut self, resolve: &Resolve, ty: &TypeDef) { + match &ty.kind { TypeDefKind::Type(t) | TypeDefKind::List(t) | TypeDefKind::Option(t) - | TypeDefKind::Future(Some(t)) => self.add_type(resolve, t), + | TypeDefKind::Future(Some(t)) => self.visit_type(resolve, t), TypeDefKind::Handle(handle) => match handle { - crate::Handle::Own(ty) => self.add_type_id(resolve, *ty), - crate::Handle::Borrow(ty) => self.add_type_id(resolve, *ty), + crate::Handle::Own(ty) => self.visit_type_id(resolve, *ty), + crate::Handle::Borrow(ty) => self.visit_type_id(resolve, *ty), }, TypeDefKind::Resource => {} TypeDefKind::Record(r) => { for field in r.fields.iter() { - self.add_type(resolve, &field.ty); + self.visit_type(resolve, &field.ty); } } TypeDefKind::Tuple(r) => { for ty in r.types.iter() { - self.add_type(resolve, ty); + self.visit_type(resolve, ty); } } TypeDefKind::Variant(v) => { for case in v.cases.iter() { if let Some(ty) = &case.ty { - self.add_type(resolve, ty); + self.visit_type(resolve, ty); } } } TypeDefKind::Result(r) => { if let Some(ty) = &r.ok { - self.add_type(resolve, ty); + self.visit_type(resolve, ty); } if let Some(ty) = &r.err { - self.add_type(resolve, ty); + self.visit_type(resolve, ty); } } TypeDefKind::Stream(s) => { if let Some(ty) = &s.element { - self.add_type(resolve, ty); + self.visit_type(resolve, ty); } if let Some(ty) = &s.end { - self.add_type(resolve, ty); + self.visit_type(resolve, ty); } } TypeDefKind::Flags(_) | TypeDefKind::Enum(_) | TypeDefKind::Future(None) => {} TypeDefKind::Unknown => unreachable!(), } - assert!(self.set.insert(ty)); } - pub fn add_type(&mut self, resolve: &Resolve, ty: &Type) { + fn visit_type(&mut self, resolve: &Resolve, ty: &Type) { match ty { - Type::Id(id) => self.add_type_id(resolve, *id), + Type::Id(id) => self.visit_type_id(resolve, *id), _ => {} } } } + +#[cfg(test)] +mod tests { + use super::{LiveTypes, Resolve}; + + fn live(wit: &str, ty: &str) -> Vec { + let mut resolve = Resolve::default(); + resolve.push_str("test.wit", wit).unwrap(); + let (_, interface) = resolve.interfaces.iter().next_back().unwrap(); + let ty = interface.types[ty]; + let mut live = LiveTypes::default(); + live.add_type_id(&resolve, ty); + + live.iter() + .filter_map(|ty| resolve.types[ty].name.clone()) + .collect() + } + + #[test] + fn no_deps() { + let types = live( + " + package foo:bar; + + interface foo { + type t = u32; + } + ", + "t", + ); + assert_eq!(types, ["t"]); + } + + #[test] + fn one_dep() { + let types = live( + " + package foo:bar; + + interface foo { + type t = u32; + type u = t; + } + ", + "u", + ); + assert_eq!(types, ["t", "u"]); + } + + #[test] + fn chain() { + let types = live( + " + package foo:bar; + + interface foo { + resource t1; + record t2 { + x: t1, + } + variant t3 { + x(t2), + } + flags t4 { a } + enum t5 { a } + type t6 = tuple; + } + ", + "t6", + ); + assert_eq!(types, ["t5", "t4", "t1", "t2", "t3", "t6"]); + } +} diff --git a/crates/wit-parser/src/resolve.rs b/crates/wit-parser/src/resolve.rs index 08042d5ae1..91d74ca29c 100644 --- a/crates/wit-parser/src/resolve.rs +++ b/crates/wit-parser/src/resolve.rs @@ -15,8 +15,8 @@ use crate::serde_::{serialize_arena, serialize_id_map}; use crate::{ AstItem, Docs, Error, Function, FunctionKind, Handle, IncludeName, Interface, InterfaceId, InterfaceSpan, PackageName, Results, SourceMap, Stability, Type, TypeDef, TypeDefKind, TypeId, - TypeOwner, UnresolvedPackage, UnresolvedPackageGroup, World, WorldId, WorldItem, WorldKey, - WorldSpan, + TypeIdVisitor, TypeOwner, UnresolvedPackage, UnresolvedPackageGroup, World, WorldId, WorldItem, + WorldKey, WorldSpan, }; /// Representation of a fully resolved set of WIT packages. @@ -724,6 +724,9 @@ package {name} is defined in two different locations:\n\ } log::trace!("now have {} packages", self.packages.len()); + + #[cfg(debug_assertions)] + self.assert_valid(); Ok(remap) } @@ -796,6 +799,8 @@ package {name} is defined in two different locations:\n\ assert!(prev.is_none()); } + #[cfg(debug_assertions)] + self.assert_valid(); Ok(()) } @@ -1287,9 +1292,11 @@ package {name} is defined in two different locations:\n\ match item { WorldItem::Interface { .. } => {} WorldItem::Function(f) => { + assert!(!matches!(name, WorldKey::Interface(_))); assert_eq!(f.name, name.clone().unwrap_name()); } WorldItem::Type(ty) => { + assert!(!matches!(name, WorldKey::Interface(_))); assert!(types.insert(*ty)); let ty = &self.types[*ty]; assert_eq!(ty.name, Some(name.clone().unwrap_name())); @@ -1302,6 +1309,7 @@ package {name} is defined in two different locations:\n\ } } } + self.assert_world_elaborated(world); world_types.push(types); } @@ -1391,6 +1399,141 @@ package {name} is defined in two different locations:\n\ } } + fn assert_world_elaborated(&self, world: &World) { + for (key, item) in world.imports.iter() { + log::debug!( + "asserting elaborated world import {}", + self.name_world_key(key) + ); + match item { + WorldItem::Type(t) => self.assert_world_imports_type_deps(world, key, *t), + + // All types referred to must be imported. + WorldItem::Function(f) => self.assert_world_function_imports_types(world, key, f), + + // All direct dependencies of this interface must be imported. + WorldItem::Interface { id, .. } => { + for dep in self.interface_direct_deps(*id) { + assert!( + world.imports.contains_key(&WorldKey::Interface(dep)), + "world import of {} is missing transitive dep of {}", + self.name_world_key(key), + self.id_of(dep).unwrap(), + ); + } + } + } + } + for (key, item) in world.exports.iter() { + log::debug!( + "asserting elaborated world export {}", + self.name_world_key(key) + ); + match item { + // Types referred to by this function must be imported. + WorldItem::Function(f) => self.assert_world_function_imports_types(world, key, f), + + // Dependencies of exported interfaces must also be exported, or + // if imported then that entire chain of imports must be + // imported and not exported. + 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, + ); + } + } + } + + // exported types not allowed at this time + WorldItem::Type(_) => unreachable!(), + } + } + } + + fn assert_world_imports_type_deps(&self, world: &World, key: &WorldKey, ty: TypeId) { + // If this is a `use` statement then the referred-to interface must be + // imported into this world. + let ty = &self.types[ty]; + if let TypeDefKind::Type(Type::Id(other)) = ty.kind { + if let TypeOwner::Interface(id) = self.types[other].owner { + let key = WorldKey::Interface(id); + assert!(world.imports.contains_key(&key)); + return; + } + } + + // ... otherwise any named type that this type refers to, one level + // deep, must be imported into this world under that name. + + let mut visitor = MyVisit(self, Vec::new()); + visitor.visit_type_def(self, ty); + for ty in visitor.1 { + let ty = &self.types[ty]; + let Some(name) = ty.name.clone() else { + continue; + }; + let dep_key = WorldKey::Name(name); + assert!( + world.imports.contains_key(&dep_key), + "world import `{}` should also force an import of `{}`", + self.name_world_key(key), + self.name_world_key(&dep_key), + ); + } + + struct MyVisit<'a>(&'a Resolve, Vec); + + impl TypeIdVisitor for MyVisit<'_> { + fn before_visit_type_id(&mut self, id: TypeId) -> bool { + self.1.push(id); + // recurse into unnamed types to look at all named types + self.0.types[id].name.is_none() + } + } + } + + /// This asserts that all types referred to by `func` are imported into + /// `world` under `WorldKey::Name`. Note that this is only applicable to + /// named type + fn assert_world_function_imports_types(&self, world: &World, key: &WorldKey, func: &Function) { + for ty in func + .parameter_and_result_types() + .chain(func.kind.resource().map(Type::Id)) + { + let Type::Id(id) = ty else { + continue; + }; + self.assert_world_imports_type_deps(world, key, id); + } + } + + 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,