From b5e9fb710ba73bde295eec5ab57f6ef4f6459e79 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 14 Feb 2023 12:02:19 -0600 Subject: [PATCH] Improve type imports into components (#5777) This commit fixes a panic related to type imports where an import of a type didn't correctly declare the new type index on the Wasmtime side of things. Additionally this plumbs more support throughout Wasmtime to support type imports, namely that they do not need to be supplied through a `Linker`. This additionally implements a feature where empty instances, even transitively, do not need to be supplied by a Wasmtime embedder. This means that instances which only have types, for example, do not need to be supplied into a `Linker` since no runtime information for them is required anyway. Closes #5775 --- crates/environ/src/component/translate.rs | 54 +++++++++---------- .../environ/src/component/translate/inline.rs | 20 ++++--- crates/wasmtime/src/component/linker.rs | 3 +- crates/wasmtime/src/component/matching.rs | 31 ++++++----- .../component-model/import.wast | 15 ++++++ .../component-model/linking.wast | 4 +- .../misc_testsuite/component-model/types.wast | 14 +++++ 7 files changed, 89 insertions(+), 52 deletions(-) diff --git a/crates/environ/src/component/translate.rs b/crates/environ/src/component/translate.rs index ab538db424..d7fbb173b3 100644 --- a/crates/environ/src/component/translate.rs +++ b/crates/environ/src/component/translate.rs @@ -484,7 +484,7 @@ impl<'a, 'data> Translator<'a, 'data> { for import in s { let import = import?; let ty = self.types.component_type_ref(&import.ty); - self.result.push_typedef(ty); + self.push_typedef(ty); self.result .initializers .push(LocalInitializer::Import(import.name, ty)); @@ -687,6 +687,28 @@ impl<'a, 'data> Translator<'a, 'data> { Ok(Action::KeepGoing) } + fn push_typedef(&mut self, ty: TypeDef) { + match ty { + TypeDef::ComponentInstance(idx) => { + self.result + .component_instances + .push(ComponentInstanceType::Index(idx)); + } + TypeDef::ComponentFunc(idx) => { + self.result.component_funcs.push(idx); + } + TypeDef::Component(idx) => { + self.result.components.push(ComponentType::Index(idx)); + } + TypeDef::Interface(_) => { + self.types.push_component_typedef(ty); + } + + // not processed here + TypeDef::CoreFunc(_) | TypeDef::Module(_) => {} + } + } + fn instantiate_module( &mut self, module: ModuleIndex, @@ -852,10 +874,7 @@ impl<'a, 'data> Translator<'a, 'data> { // the aliased item is directly available from the instance type. ComponentInstanceType::Index(ty) => { let (_url, ty) = &self.types[ty].exports[name]; - self.result.push_typedef(*ty); - if let TypeDef::Interface(_) = ty { - self.types.push_component_typedef(*ty); - } + self.push_typedef(*ty); } // An imported component was instantiated so the type of the aliased @@ -863,10 +882,7 @@ impl<'a, 'data> Translator<'a, 'data> { // original component. ComponentInstanceType::InstantiatedIndex(ty) => { let (_, ty) = self.types[ty].exports[name]; - self.result.push_typedef(ty); - if let TypeDef::Interface(_) = ty { - self.types.push_component_typedef(ty); - } + self.push_typedef(ty); } // A static nested component was instantiated which means that the @@ -1025,23 +1041,3 @@ impl<'a, 'data> Translator<'a, 'data> { return ret; } } - -impl Translation<'_> { - fn push_typedef(&mut self, ty: TypeDef) { - match ty { - TypeDef::ComponentInstance(idx) => { - self.component_instances - .push(ComponentInstanceType::Index(idx)); - } - TypeDef::ComponentFunc(idx) => { - self.component_funcs.push(idx); - } - TypeDef::Component(idx) => { - self.components.push(ComponentType::Index(idx)); - } - - // not processed here - TypeDef::Interface(_) | TypeDef::CoreFunc(_) | TypeDef::Module(_) => {} - } - } -} diff --git a/crates/environ/src/component/translate/inline.rs b/crates/environ/src/component/translate/inline.rs index 9d63bb4bef..95aa449c1e 100644 --- a/crates/environ/src/component/translate/inline.rs +++ b/crates/environ/src/component/translate/inline.rs @@ -75,6 +75,10 @@ pub(super) fn run( let mut args = HashMap::with_capacity(result.exports.len()); for init in result.initializers.iter() { let (name, ty) = match *init { + // Imports of types (which are currently always equality-bounded) + // are not required to be specified by the host since it's just for + // type information within the component. + LocalInitializer::Import(_, TypeDef::Interface(_)) => continue, LocalInitializer::Import(name, ty) => (name, ty), _ => continue, }; @@ -355,6 +359,13 @@ impl<'a> Inliner<'a> { use LocalInitializer::*; match initializer { + // Importing a type into a component is ignored. All type imports + // are equality-bound right now which means that it's purely + // informational name about the type such as a name to assign it. + // Otherwise type imports have no effect on runtime or such, so skip + // them. + Import(_, TypeDef::Interface(_)) => {} + // When a component imports an item the actual definition of the // item is looked up here (not at runtime) via its name. The // arguments provided in our `InlinerFrame` describe how each @@ -378,12 +389,7 @@ impl<'a> Inliner<'a> { ComponentItemDef::Func(i) => { frame.component_funcs.push(i.clone()); } - - // The type structure of a component does not change depending - // on how it is instantiated at this time so importing a type - // does not affect the component being instantiated so it's - // ignored. - ComponentItemDef::Type(_ty) => {} + ComponentItemDef::Type(_ty) => unreachable!(), }, // Lowering a component function to a core wasm function is @@ -1045,7 +1051,7 @@ impl<'a> ComponentItemDef<'a> { // FIXME(#4283) should commit one way or another to how this // should be treated. TypeDef::Component(_ty) => bail!("root-level component imports are not supported"), - TypeDef::Interface(_ty) => unimplemented!("import of a type"), + TypeDef::Interface(ty) => ComponentItemDef::Type(TypeDef::Interface(ty)), TypeDef::CoreFunc(_ty) => unreachable!(), }; Ok(item) diff --git a/crates/wasmtime/src/component/linker.rs b/crates/wasmtime/src/component/linker.rs index 0f2ed79375..cf2bbb302c 100644 --- a/crates/wasmtime/src/component/linker.rs +++ b/crates/wasmtime/src/component/linker.rs @@ -134,8 +134,7 @@ impl Linker { let import = self .strings .lookup(name) - .and_then(|name| self.map.get(&name)) - .ok_or_else(|| anyhow!("import `{name}` not defined"))?; + .and_then(|name| self.map.get(&name)); cx.definition(ty, import) .with_context(|| format!("import `{name}` has the wrong type"))?; } diff --git a/crates/wasmtime/src/component/matching.rs b/crates/wasmtime/src/component/matching.rs index 77b3ff1337..6f2cb5549b 100644 --- a/crates/wasmtime/src/component/matching.rs +++ b/crates/wasmtime/src/component/matching.rs @@ -14,22 +14,23 @@ pub struct TypeChecker<'a> { } impl TypeChecker<'_> { - pub fn definition(&self, expected: &TypeDef, actual: &Definition) -> Result<()> { + pub fn definition(&self, expected: &TypeDef, actual: Option<&Definition>) -> Result<()> { match *expected { TypeDef::Module(t) => match actual { - Definition::Module(actual) => self.module(&self.types[t], actual), - _ => bail!("expected module found {}", actual.desc()), + Some(Definition::Module(actual)) => self.module(&self.types[t], actual), + _ => bail!("expected module found {}", desc(actual)), }, TypeDef::ComponentInstance(t) => match actual { - Definition::Instance(actual) => self.instance(&self.types[t], actual), - _ => bail!("expected instance found {}", actual.desc()), + Some(Definition::Instance(actual)) => self.instance(&self.types[t], Some(actual)), + None => self.instance(&self.types[t], None), + _ => bail!("expected instance found {}", desc(actual)), }, TypeDef::ComponentFunc(t) => match actual { - Definition::Func(actual) => self.func(t, actual), - _ => bail!("expected func found {}", actual.desc()), + Some(Definition::Func(actual)) => self.func(t, actual), + _ => bail!("expected func found {}", desc(actual)), }, - TypeDef::Component(_) => bail!("expected component found {}", actual.desc()), - TypeDef::Interface(_) => bail!("expected type found {}", actual.desc()), + TypeDef::Component(_) => bail!("expected component found {}", desc(actual)), + TypeDef::Interface(_) => bail!("expected type found {}", desc(actual)), // not possible for valid components to import TypeDef::CoreFunc(_) => unreachable!(), @@ -67,7 +68,7 @@ impl TypeChecker<'_> { Ok(()) } - fn instance(&self, expected: &TypeComponentInstance, actual: &NameMap) -> Result<()> { + fn instance(&self, expected: &TypeComponentInstance, 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 // type. @@ -81,8 +82,7 @@ impl TypeChecker<'_> { let actual = self .strings .lookup(name) - .and_then(|name| actual.get(&name)) - .ok_or_else(|| anyhow!("instance export `{name}` not defined"))?; + .and_then(|name| actual?.get(&name)); self.definition(expected, actual) .with_context(|| format!("instance export `{name}` has the wrong type"))?; } @@ -94,6 +94,13 @@ impl TypeChecker<'_> { } } +fn desc(def: Option<&Definition>) -> &'static str { + match def { + Some(def) => def.desc(), + None => "nothing", + } +} + impl Definition { fn desc(&self) -> &'static str { match self { diff --git a/tests/misc_testsuite/component-model/import.wast b/tests/misc_testsuite/component-model/import.wast index 4633ba2615..3e51d53631 100644 --- a/tests/misc_testsuite/component-model/import.wast +++ b/tests/misc_testsuite/component-model/import.wast @@ -3,3 +3,18 @@ (import "host-return-two" (func $f (result u32))) (export "x" (func $f))) "component export `x` is a reexport of an imported function which is not implemented") + +(assert_invalid + (component + (import "host-return-two" (instance)) + ) + "expected instance found func") + +;; empty instances don't need to be supplied by the host, even recursively +;; empty instances. +(component + (import "not-provided-by-the-host" (instance)) + (import "not-provided-by-the-host2" (instance + (export "x" (instance)) + )) +) diff --git a/tests/misc_testsuite/component-model/linking.wast b/tests/misc_testsuite/component-model/linking.wast index ee9eb304cc..7cf4aad7d9 100644 --- a/tests/misc_testsuite/component-model/linking.wast +++ b/tests/misc_testsuite/component-model/linking.wast @@ -2,7 +2,7 @@ (component (import "undefined-name" (core module)) ) - "import `undefined-name` not defined") + "expected module found nothing") (component $i) (component (import "i" (instance)) @@ -15,4 +15,4 @@ "expected func found instance") (assert_unlinkable (component (import "i" (instance (export "x" (func))))) - "export `x` not defined") + "expected func found nothing") diff --git a/tests/misc_testsuite/component-model/types.wast b/tests/misc_testsuite/component-model/types.wast index c4c93a8037..4c44764aec 100644 --- a/tests/misc_testsuite/component-model/types.wast +++ b/tests/misc_testsuite/component-model/types.wast @@ -320,3 +320,17 @@ (instance $c (instantiate $c (with "x" (component $x)))) ) + +(component + (type $t1 u64) + (import "a" (type $t2 (eq $t1))) + (import "b" (type $t3 (eq $t2))) +) + +(component + (import "a" (instance + (type $t1 u64) + (export $t2 "a" (type (eq $t1))) + (export "b" (type (eq $t2))) + )) +)