From 1ad14479cc3633963abb3b43eef2e98311fa4f65 Mon Sep 17 00:00:00 2001 From: Peter Huene Date: Wed, 10 Apr 2024 12:56:07 -0700 Subject: [PATCH] Fix the encoding of imported resources in the `CompositionGraph`. This commit fixes the encoding of imported resources in the `CompositionGraph`. Previously a panic was encountered when the graph attempts to import, either explicitly or implicitly, a resource. The fix requires tracking the "owner" of an aliased resource so that the export on the owning interface may be aliased and used for type equality when encoding the resource import. Fixes #62. --- crates/wac-graph/src/encoding.rs | 59 +++++-- crates/wac-graph/src/graph.rs | 10 +- .../tests/graphs/used-resource/encoded.wat | 36 +++++ .../tests/graphs/used-resource/foo.wit | 12 ++ .../tests/graphs/used-resource/graph.json | 25 +++ crates/wac-types/src/aggregator.rs | 147 ++++++++++++------ crates/wac-types/src/component.rs | 23 ++- crates/wac-types/src/package.rs | 17 +- 8 files changed, 258 insertions(+), 71 deletions(-) create mode 100644 crates/wac-graph/tests/graphs/used-resource/encoded.wat create mode 100644 crates/wac-graph/tests/graphs/used-resource/foo.wit create mode 100644 crates/wac-graph/tests/graphs/used-resource/graph.json diff --git a/crates/wac-graph/src/encoding.rs b/crates/wac-graph/src/encoding.rs index 1606708..3efd2a2 100644 --- a/crates/wac-graph/src/encoding.rs +++ b/crates/wac-graph/src/encoding.rs @@ -14,6 +14,7 @@ use wasm_encoder::{ /// A type used to abstract the API differences between a component builder, /// component type, and instance type from `wasm-encoder`. +#[derive(Debug)] enum Encodable { Builder(ComponentBuilder), Instance(InstanceType), @@ -94,7 +95,7 @@ impl Default for Encodable { } } -#[derive(Default)] +#[derive(Debug, Default)] pub struct Scope { /// The map from types to encoded type index. pub type_indexes: IndexMap, @@ -108,7 +109,7 @@ pub struct Scope { encodable: Encodable, } -#[derive(Default)] +#[derive(Debug, Default)] pub struct State { /// The stack of encoding scopes. scopes: Vec, @@ -630,7 +631,8 @@ impl<'a> TypeEncoder<'a> { fn import(&self, state: &mut State, name: &str, kind: ItemKind) { if let ItemKind::Type(Type::Resource(id)) = kind { - return self.import_resource(state, name, id); + self.import_resource(state, name, id); + return; } log::debug!("encoding {kind} import `{name}`", kind = kind.desc(self.0)); @@ -669,9 +671,9 @@ impl<'a> TypeEncoder<'a> { } } - fn import_resource(&self, state: &mut State, name: &str, id: ResourceId) { - if state.current.resources.contains_key(name) { - return; + pub fn import_resource(&self, state: &mut State, name: &str, id: ResourceId) -> u32 { + if let Some(index) = state.current.resources.get(name) { + return *index; } log::debug!("encoding import of resource `{name}`"); @@ -687,17 +689,43 @@ impl<'a> TypeEncoder<'a> { log::debug!("encoded outer alias for resource `{name}` to type index {index}"); index - } else if let Some(alias_of) = resource.alias_of { - // This is an alias to another resource at the same scope - let orig = - state.current.resources[self.0[self.0.resolve_resource(alias_of)].name.as_str()]; + } else if let Some(alias) = resource.alias { + let source = self.0.resolve_resource(alias.source); + let source_index = if let Some(index) = + state.current.resources.get(self.0[source].name.as_str()) + { + // The source resource was previously imported + *index + } else if let Some(index) = state.current.type_indexes.get(&Type::Resource(source)) { + // The source resource isn't directly imported, but was previously aliased + *index + } else { + // Otherwise, we need to alias the source resource + // This should only occur for resources owned by interfaces + let source_index = state.current.encodable.type_count(); + let iid = self.0[alias.owner.expect("should have owner")] + .id + .as_deref() + .expect("expected an interface with an id"); + state.current.encodable.alias(Alias::InstanceExport { + instance: state.current.instances[iid], + kind: ComponentExportKind::Type, + name: self.0[source].name.as_str(), + }); + state + .current + .type_indexes + .insert(Type::Resource(source), source_index); + source_index + }; + let index = state.current.encodable.type_count(); state .current .encodable - .import_type(name, ComponentTypeRef::Type(TypeBounds::Eq(orig))); + .import_type(name, ComponentTypeRef::Type(TypeBounds::Eq(source_index))); - log::debug!("encoded import for resource `{name}` as type index {index} (alias of type index {orig})"); + log::debug!("encoded import for resource `{name}` as type index {index} (alias of type index {source_index})"); index } else { // Otherwise, this is a new resource type, import with a subtype bounds @@ -712,6 +740,7 @@ impl<'a> TypeEncoder<'a> { }; state.current.resources.insert(resource.name.clone(), index); + index } fn export(&self, state: &mut State, name: &str, kind: ItemKind) -> u32 { @@ -759,10 +788,10 @@ impl<'a> TypeEncoder<'a> { Self::export_type(state, name, ComponentTypeRef::Type(TypeBounds::Eq(outer))); log::debug!("encoded outer alias for resource `{name}` as type index {index}"); index - } else if let Some(alias_of) = resource.alias_of { + } else if let Some(alias) = resource.alias { // This is an alias to another resource at the same scope - let index = - state.current.resources[self.0[self.0.resolve_resource(alias_of)].name.as_str()]; + let index = state.current.resources + [self.0[self.0.resolve_resource(alias.source)].name.as_str()]; let index = Self::export_type(state, name, ComponentTypeRef::Type(TypeBounds::Eq(index))); log::debug!("encoded alias for resource `{name}` as type index {index}"); diff --git a/crates/wac-graph/src/graph.rs b/crates/wac-graph/src/graph.rs index ec41cb0..949ef5e 100644 --- a/crates/wac-graph/src/graph.rs +++ b/crates/wac-graph/src/graph.rs @@ -1458,13 +1458,19 @@ impl<'a> CompositionGraphEncoder<'a> { } } + let encoder = TypeEncoder::new(types); + + // Defer to special handling if the item being imported is a resource + if let ItemKind::Type(Type::Resource(id)) = kind { + return encoder.import_resource(state, name, id); + } + log::debug!( "encoding import of {kind} `{name}`", kind = kind.desc(types) ); // Encode the type and import - let encoder = TypeEncoder::new(types); let ty = encoder.ty(state, kind.ty(), None); let index = state.builder().import( name, @@ -1712,7 +1718,7 @@ mod test { let mut graph = CompositionGraph::new(); let id = graph.types_mut().add_resource(Resource { name: "a".to_string(), - alias_of: None, + alias: None, }); assert!(matches!( graph.define_type("foo", Type::Resource(id)).unwrap_err(), diff --git a/crates/wac-graph/tests/graphs/used-resource/encoded.wat b/crates/wac-graph/tests/graphs/used-resource/encoded.wat new file mode 100644 index 0000000..ad6dfc2 --- /dev/null +++ b/crates/wac-graph/tests/graphs/used-resource/encoded.wat @@ -0,0 +1,36 @@ +(component + (type (;0;) + (instance + (export (;0;) "my-resource" (type (sub resource))) + ) + ) + (import "test:foo/my-interface" (instance (;0;) (type 0))) + (alias export 0 "my-resource" (type (;1;))) + (import "my-resource" (type (;2;) (eq 1))) + (import "my-resource2" (type (;3;) (eq 2))) + (type (;4;) + (component + (type (;0;) + (instance + (export (;0;) "my-resource" (type (sub resource))) + ) + ) + (import "test:foo/my-interface" (instance (;0;) (type 0))) + (alias export 0 "my-resource" (type (;1;))) + (import "my-resource" (type (;2;) (eq 1))) + (import "my-resource2" (type (;3;) (eq 2))) + (type (;4;) (own 3)) + (type (;5;) (func (param "r" 4))) + (export (;0;) "my-func" (func (type 5))) + ) + ) + (import "unlocked-dep=" (component (;0;) (type 4))) + (instance (;1;) (instantiate 0 + (with "test:foo/my-interface" (instance 0)) + (with "my-resource" (type 2)) + (with "my-resource2" (type 3)) + ) + ) + (alias export 1 "my-func" (func (;0;))) + (export (;1;) "my-func" (func 0)) +) diff --git a/crates/wac-graph/tests/graphs/used-resource/foo.wit b/crates/wac-graph/tests/graphs/used-resource/foo.wit new file mode 100644 index 0000000..d1b699d --- /dev/null +++ b/crates/wac-graph/tests/graphs/used-resource/foo.wit @@ -0,0 +1,12 @@ +package test:foo; + +world my-world { + use my-interface.{my-resource}; + type my-resource2 = my-resource; + + export my-func: func(r: my-resource2); +} + +interface my-interface { + resource my-resource {} +} diff --git a/crates/wac-graph/tests/graphs/used-resource/graph.json b/crates/wac-graph/tests/graphs/used-resource/graph.json new file mode 100644 index 0000000..06d453c --- /dev/null +++ b/crates/wac-graph/tests/graphs/used-resource/graph.json @@ -0,0 +1,25 @@ +{ + "packages": [ + { + "name": "test:foo", + "path": "foo.wit" + } + ], + "nodes": [ + { + "type": "instantiation", + "package": 0 + }, + { + "type": "alias", + "source": 0, + "export": "my-func" + } + ], + "exports": [ + { + "node": 1, + "name": "my-func" + } + ] +} diff --git a/crates/wac-types/src/aggregator.rs b/crates/wac-types/src/aggregator.rs index 83072db..a46508f 100644 --- a/crates/wac-types/src/aggregator.rs +++ b/crates/wac-types/src/aggregator.rs @@ -1,7 +1,7 @@ use crate::{ DefinedType, DefinedTypeId, FuncResult, FuncType, FuncTypeId, Interface, InterfaceId, ItemKind, - ModuleTypeId, Record, Resource, ResourceId, SubtypeCheck, SubtypeChecker, Type, Types, - UsedType, ValueType, Variant, World, WorldId, + ModuleTypeId, Record, Resource, ResourceAlias, ResourceId, SubtypeCheck, SubtypeChecker, Type, + Types, UsedType, ValueType, Variant, World, WorldId, }; use anyhow::{bail, Context, Result}; use indexmap::IndexMap; @@ -544,7 +544,7 @@ impl TypeAggregator { ) -> Result { match kind { ItemKind::Type(ty) => Ok(ItemKind::Type(self.remap_type(types, ty, checker)?)), - ItemKind::Func(id) => Ok(ItemKind::Func(self.remap_func_type(types, id))), + ItemKind::Func(id) => Ok(ItemKind::Func(self.remap_func_type(types, id, checker)?)), ItemKind::Instance(id) => Ok(ItemKind::Instance( self.remap_interface(types, id, checker)?, )), @@ -552,7 +552,7 @@ impl TypeAggregator { Ok(ItemKind::Component(self.remap_world(types, id, checker)?)) } ItemKind::Module(id) => Ok(ItemKind::Module(self.remap_module_type(types, id))), - ItemKind::Value(ty) => Ok(ItemKind::Value(self.remap_value_type(types, ty))), + ItemKind::Value(ty) => Ok(ItemKind::Value(self.remap_value_type(types, ty, checker)?)), } } @@ -563,19 +563,24 @@ impl TypeAggregator { checker: &mut SubtypeChecker, ) -> Result { match ty { - Type::Resource(id) => Ok(Type::Resource(self.remap_resource(types, id))), - Type::Func(id) => Ok(Type::Func(self.remap_func_type(types, id))), - Type::Value(ty) => Ok(Type::Value(self.remap_value_type(types, ty))), + Type::Resource(id) => Ok(Type::Resource(self.remap_resource(types, id, checker)?)), + Type::Func(id) => Ok(Type::Func(self.remap_func_type(types, id, checker)?)), + Type::Value(ty) => Ok(Type::Value(self.remap_value_type(types, ty, checker)?)), Type::Interface(id) => Ok(Type::Interface(self.remap_interface(types, id, checker)?)), Type::World(id) => Ok(Type::World(self.remap_world(types, id, checker)?)), Type::Module(id) => Ok(Type::Module(self.remap_module_type(types, id))), } } - fn remap_resource(&mut self, types: &Types, id: ResourceId) -> ResourceId { + fn remap_resource( + &mut self, + types: &Types, + id: ResourceId, + checker: &mut SubtypeChecker, + ) -> Result { if let Some(kind) = self.remapped.get(&Type::Resource(id)) { return match kind { - Type::Resource(id) => *id, + Type::Resource(id) => Ok(*id), _ => panic!("expected a resource"), }; } @@ -583,7 +588,18 @@ impl TypeAggregator { let resource = &types[id]; let remapped = Resource { name: resource.name.clone(), - alias_of: resource.alias_of.map(|id| self.remap_resource(types, id)), + alias: resource + .alias + .map(|a| -> Result<_> { + Ok(ResourceAlias { + owner: a + .owner + .map(|id| self.remap_interface(types, id, checker)) + .transpose()?, + source: self.remap_resource(types, a.source, checker)?, + }) + }) + .transpose()?, }; let remapped_id = self.types.add_resource(remapped); @@ -591,13 +607,18 @@ impl TypeAggregator { .remapped .insert(Type::Resource(id), Type::Resource(remapped_id)); assert!(prev.is_none()); - remapped_id + Ok(remapped_id) } - fn remap_func_type(&mut self, types: &Types, id: FuncTypeId) -> FuncTypeId { + fn remap_func_type( + &mut self, + types: &Types, + id: FuncTypeId, + checker: &mut SubtypeChecker, + ) -> Result { if let Some(kind) = self.remapped.get(&Type::Func(id)) { return match kind { - Type::Func(id) => *id, + Type::Func(id) => Ok(*id), _ => panic!("expected a function type"), }; } @@ -607,16 +628,26 @@ impl TypeAggregator { params: ty .params .iter() - .map(|(n, ty)| (n.clone(), self.remap_value_type(types, *ty))) - .collect(), - results: ty.results.as_ref().map(|r| match r { - FuncResult::Scalar(ty) => FuncResult::Scalar(self.remap_value_type(types, *ty)), - FuncResult::List(tys) => FuncResult::List( - tys.iter() - .map(|(n, ty)| (n.clone(), self.remap_value_type(types, *ty))) - .collect(), - ), - }), + .map(|(n, ty)| Ok((n.clone(), self.remap_value_type(types, *ty, checker)?))) + .collect::>()?, + results: ty + .results + .as_ref() + .map(|r| -> Result<_> { + match r { + FuncResult::Scalar(ty) => Ok(FuncResult::Scalar( + self.remap_value_type(types, *ty, checker)?, + )), + FuncResult::List(tys) => Ok(FuncResult::List( + tys.iter() + .map(|(n, ty)| { + Ok((n.clone(), self.remap_value_type(types, *ty, checker)?)) + }) + .collect::>()?, + )), + } + }) + .transpose()?, }; let remapped_id = self.types.add_func_type(remapped); @@ -624,15 +655,24 @@ impl TypeAggregator { .remapped .insert(Type::Func(id), Type::Func(remapped_id)); assert!(prev.is_none()); - remapped_id + Ok(remapped_id) } - fn remap_value_type(&mut self, types: &Types, ty: ValueType) -> ValueType { + fn remap_value_type( + &mut self, + types: &Types, + ty: ValueType, + checker: &mut SubtypeChecker, + ) -> Result { match ty { - ValueType::Primitive(ty) => ValueType::Primitive(ty), - ValueType::Borrow(id) => ValueType::Borrow(self.remap_resource(types, id)), - ValueType::Own(id) => ValueType::Own(self.remap_resource(types, id)), - ValueType::Defined(id) => ValueType::Defined(self.remap_defined_type(types, id)), + ValueType::Primitive(ty) => Ok(ValueType::Primitive(ty)), + ValueType::Borrow(id) => { + Ok(ValueType::Borrow(self.remap_resource(types, id, checker)?)) + } + ValueType::Own(id) => Ok(ValueType::Own(self.remap_resource(types, id, checker)?)), + ValueType::Defined(id) => Ok(ValueType::Defined( + self.remap_defined_type(types, id, checker)?, + )), } } @@ -769,10 +809,15 @@ impl TypeAggregator { remapped } - fn remap_defined_type(&mut self, types: &Types, id: DefinedTypeId) -> DefinedTypeId { + fn remap_defined_type( + &mut self, + types: &Types, + id: DefinedTypeId, + checker: &mut SubtypeChecker, + ) -> Result { if let Some(kind) = self.remapped.get(&Type::Value(ValueType::Defined(id))) { return match kind { - Type::Value(ValueType::Defined(id)) => *id, + Type::Value(ValueType::Defined(id)) => Ok(*id), _ => panic!("expected a defined type got {kind:?}"), }; } @@ -780,37 +825,49 @@ impl TypeAggregator { let defined = match &types[id] { DefinedType::Tuple(tys) => DefinedType::Tuple( tys.iter() - .map(|ty| self.remap_value_type(types, *ty)) - .collect(), + .map(|ty| self.remap_value_type(types, *ty, checker)) + .collect::>()?, ), - DefinedType::List(ty) => DefinedType::List(self.remap_value_type(types, *ty)), - DefinedType::Option(ty) => DefinedType::Option(self.remap_value_type(types, *ty)), + DefinedType::List(ty) => DefinedType::List(self.remap_value_type(types, *ty, checker)?), + DefinedType::Option(ty) => { + DefinedType::Option(self.remap_value_type(types, *ty, checker)?) + } DefinedType::Result { ok, err } => DefinedType::Result { - ok: ok.as_ref().map(|ty| self.remap_value_type(types, *ty)), - err: err.as_ref().map(|ty| self.remap_value_type(types, *ty)), + ok: ok + .as_ref() + .map(|ty| self.remap_value_type(types, *ty, checker)) + .transpose()?, + err: err + .as_ref() + .map(|ty| self.remap_value_type(types, *ty, checker)) + .transpose()?, }, DefinedType::Variant(v) => DefinedType::Variant(Variant { cases: v .cases .iter() .map(|(n, ty)| { - ( + Ok(( n.clone(), - ty.as_ref().map(|ty| self.remap_value_type(types, *ty)), - ) + ty.as_ref() + .map(|ty| self.remap_value_type(types, *ty, checker)) + .transpose()?, + )) }) - .collect(), + .collect::>()?, }), DefinedType::Record(r) => DefinedType::Record(Record { fields: r .fields .iter() - .map(|(n, ty)| (n.clone(), self.remap_value_type(types, *ty))) - .collect(), + .map(|(n, ty)| Ok((n.clone(), self.remap_value_type(types, *ty, checker)?))) + .collect::>()?, }), DefinedType::Flags(f) => DefinedType::Flags(f.clone()), DefinedType::Enum(e) => DefinedType::Enum(e.clone()), - DefinedType::Alias(ty) => DefinedType::Alias(self.remap_value_type(types, *ty)), + DefinedType::Alias(ty) => { + DefinedType::Alias(self.remap_value_type(types, *ty, checker)?) + } }; let remapped = self.types.add_defined_type(defined); @@ -819,6 +876,6 @@ impl TypeAggregator { Type::Value(ValueType::Defined(remapped)), ); assert!(prev.is_none()); - remapped + Ok(remapped) } } diff --git a/crates/wac-types/src/component.rs b/crates/wac-types/src/component.rs index c0b7218..c26d722 100644 --- a/crates/wac-types/src/component.rs +++ b/crates/wac-types/src/component.rs @@ -245,8 +245,8 @@ impl Types { /// Resolves any aliased resource id to the underlying defined resource id. pub fn resolve_resource(&self, mut id: ResourceId) -> ResourceId { - while let Some(alias_of) = &self[id].alias_of { - id = *alias_of; + while let Some(alias) = &self[id].alias { + id = alias.source; } id @@ -679,6 +679,20 @@ impl fmt::Display for FuncKind { } } +/// Represents information about an aliased resource. +#[derive(Debug, Clone, Copy)] +#[cfg_attr(feature = "serde", derive(serde::Serialize))] +#[cfg_attr(feature = "serde", serde(rename_all = "camelCase"))] +pub struct ResourceAlias { + /// The owning interface for the resource. + /// + /// This may be `None` if the resource is owned by a world. + #[cfg_attr(feature = "serde", serde(skip_serializing_if = "Option::is_none"))] + pub owner: Option, + /// The id of the resource that was aliased. + pub source: ResourceId, +} + /// Represents a resource type. #[derive(Debug, Clone)] #[cfg_attr(feature = "serde", derive(serde::Serialize))] @@ -686,10 +700,9 @@ impl fmt::Display for FuncKind { pub struct Resource { /// The name of the resource. pub name: String, - - /// The id of the resource that was aliased. + /// Information if the resource has been aliased. #[cfg_attr(feature = "serde", serde(skip_serializing_if = "Option::is_none"))] - pub alias_of: Option, + pub alias: Option, } /// Represents a variant. diff --git a/crates/wac-types/src/package.rs b/crates/wac-types/src/package.rs index 56f884a..8811fea 100644 --- a/crates/wac-types/src/package.rs +++ b/crates/wac-types/src/package.rs @@ -1,7 +1,7 @@ use crate::{ CoreExtern, CoreFuncType, DefinedType, Enum, Flags, FuncResult, FuncType, FuncTypeId, - Interface, InterfaceId, ItemKind, ModuleType, ModuleTypeId, Record, Resource, ResourceId, Type, - Types, UsedType, ValueType, Variant, World, WorldId, + Interface, InterfaceId, ItemKind, ModuleType, ModuleTypeId, Record, Resource, ResourceAlias, + ResourceId, Type, Types, UsedType, ValueType, Variant, World, WorldId, }; use anyhow::{bail, Context, Result}; use indexmap::IndexMap; @@ -764,7 +764,16 @@ impl<'a> TypeConverter<'a> { if let Some(resource_id) = self.resource_map.get(&id.resource()) { let alias_id = self.types.add_resource(Resource { name: name.to_owned(), - alias_of: Some(*resource_id), + alias: Some(ResourceAlias { + owner: match self + .find_owner(ComponentAnyTypeId::Resource(id)) + .expect("should have owner") + { + (Owner::Interface(id), _) => Some(*id), + _ => None, + }, + source: *resource_id, + }), }); self.cache.insert(key, Entity::Resource(alias_id)); return alias_id; @@ -773,7 +782,7 @@ impl<'a> TypeConverter<'a> { // Otherwise, this is a new resource let resource_id = self.types.add_resource(Resource { name: name.to_owned(), - alias_of: None, + alias: None, }); self.resource_map.insert(id.resource(), resource_id);