From e359e1efbf85612ddb5a9f8adf77cb26c59c7abb Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 16 May 2022 18:33:25 -0500 Subject: [PATCH] Add more tests for components parsing (#602) This commit uses coverage to find holes in where we weren't testing parser support for a feature in the component model. Additionally some AST changes were made along the way: * The `wasmprinter` output for canonical abi options were tweaked: * string-related options are prefixed with `string=` * the `into` option no longer prints `(instance ...)`, it only prints the instance index * The name of parameters in function types was changed to a `&str` from the id/name annotation combo. * A number of unused `Peek` implementations were removed. * Some dynamically dead code was replaced with `unreachable!` to emphasize it shouldn't be executed. * A subtraction-overflow issue was fixed in name resolution for outer aliases which try to reach too far. * Parsing `ComponentTypeUse` as a simple index like `$foo` was removed as it's expected to either be `(type $idx)` or an inline definition. --- coverage.sh | 25 +++++ crates/wasmprinter/src/lib.rs | 8 +- crates/wast/src/component/binary.rs | 8 +- crates/wast/src/component/deftype.rs | 120 ++--------------------- crates/wast/src/component/module.rs | 37 +------ crates/wast/src/component/resolve.rs | 25 +++-- crates/wast/src/component/types.rs | 29 +----- crates/wast/src/parser.rs | 7 -- tests/local/component-model/adapt.wast | 57 +++++++++++ tests/local/component-model/alias.wast | 28 ++++++ tests/local/component-model/func.wast | 4 + tests/local/component-model/invalid.wast | 24 +++++ 12 files changed, 172 insertions(+), 200 deletions(-) create mode 100644 coverage.sh create mode 100644 tests/local/component-model/adapt.wast create mode 100644 tests/local/component-model/func.wast diff --git a/coverage.sh b/coverage.sh new file mode 100644 index 0000000000..305e27c28a --- /dev/null +++ b/coverage.sh @@ -0,0 +1,25 @@ +set -ex + +cargo test --test roundtrip + +rm -rf cov +mkdir cov + +export LLVM_PROFILE_FILE="`pwd`/cov/coverage-%m.profraw" +export RUSTFLAGS="-C instrument-coverage" +export CARGO_TARGET_DIR=`pwd`/target/coverage + +cargo test --test roundtrip + +llvm-profdata merge -sparse cov/coverage-*.profraw -o cov/coverage.profdata +llvm-cov show -Xdemangler=rustfilt \ + ./target/coverage/debug/deps/roundtrip-92d636af98ebdc72 \ + --format=html \ + --ignore-filename-regex='/.cargo/registry' \ + --ignore-filename-regex='/rustc/' \ + --instr-profile=cov/coverage.profdata \ + --show-line-counts-or-regions \ + --show-instantiations \ + --show-regions \ + --output-dir=cov + diff --git a/crates/wasmprinter/src/lib.rs b/crates/wasmprinter/src/lib.rs index 12ab96024a..d367308884 100644 --- a/crates/wasmprinter/src/lib.rs +++ b/crates/wasmprinter/src/lib.rs @@ -2807,15 +2807,13 @@ impl Printer { for option in options { self.result.push(' '); match option { - CanonicalOption::UTF8 => self.result.push_str("utf8"), - CanonicalOption::UTF16 => self.result.push_str("utf16"), - CanonicalOption::CompactUTF16 => self.result.push_str("latin1+utf16"), + CanonicalOption::UTF8 => self.result.push_str("string=utf8"), + CanonicalOption::UTF16 => self.result.push_str("string=utf16"), + CanonicalOption::CompactUTF16 => self.result.push_str("string=latin1+utf16"), CanonicalOption::Into(index) => { self.start_group("into "); - self.start_group("instance "); self.print_idx(&state.instance_names, *index)?; self.end_group(); - self.end_group(); } } } diff --git a/crates/wast/src/component/binary.rs b/crates/wast/src/component/binary.rs index 5aa472d149..e15b0363c6 100644 --- a/crates/wast/src/component/binary.rs +++ b/crates/wast/src/component/binary.rs @@ -512,7 +512,7 @@ impl<'a> Encode for ComponentFunctionType<'a> { impl<'a> Encode for ComponentFunctionParam<'a> { fn encode(&self, e: &mut Vec) { - if let Some(id) = self.id { + if let Some(id) = self.name { e.push(0x01); id.encode(e); } else { @@ -528,7 +528,8 @@ impl<'a> Encode for ValueType<'a> { self.value_type.encode(e) } } -impl Encode for ComponentTypeUse<'_, T> { + +impl Encode for ComponentTypeUse<'_, T> { fn encode(&self, e: &mut Vec) { match self { ComponentTypeUse::Inline(_) => unreachable!("should be expanded already"), @@ -536,6 +537,7 @@ impl Encode for ComponentTypeUse<'_, T> { } } } + impl Encode for ComponentExport<'_> { fn encode(&self, e: &mut Vec) { self.name.encode(e); @@ -560,7 +562,7 @@ impl Encode for ComponentExport<'_> { impl Encode for ComponentFunc<'_> { fn encode(&self, e: &mut Vec) { match &self.kind { - ComponentFuncKind::Import { .. } => unreachable!("should be expanded already"), + ComponentFuncKind::Import { .. } => unreachable!("should be expanded by now"), ComponentFuncKind::Inline { body } => { body.encode(e); } diff --git a/crates/wast/src/component/deftype.rs b/crates/wast/src/component/deftype.rs index 92d9e24b66..6ddab3e0a8 100644 --- a/crates/wast/src/component/deftype.rs +++ b/crates/wast/src/component/deftype.rs @@ -4,7 +4,6 @@ use crate::component::*; use crate::core; use crate::kw; use crate::parser::{Cursor, Parse, Parser, Peek, Result}; -use crate::token::{Id, LParen, NameAnnotation}; /// Different kinds of elements that can be exported from a WebAssembly component, /// contained in a [`ComponentExport`]. @@ -110,35 +109,11 @@ impl<'a> Parse<'a> for ComponentFunctionType<'a> { let mut params = Vec::new(); while parser.peek2::() { parser.parens(|p| { - let mut l = p.lookahead1(); - if l.peek::() { - p.parse::()?; - if p.is_empty() { - return Ok(()); - } - // If we just saw `(param` and we're looking at `$X`, it - // could be a parameter name or a type name. Peek ahead to - // see if we're at the closing `)`, if so, parse it as a - // type. - if p.peek2_empty() { - let ty = p.parse()?; - params.push(ComponentFunctionParam { - id: None, - name: None, - type_: ty, - }); - } else { - let (id, name) = (p.parse::>()?, p.parse::>()?); - let ty = p.parse()?; - params.push(ComponentFunctionParam { - id, - name, - type_: ty, - }); - } - } else { - return Err(l.error()); - } + p.parse::()?; + params.push(ComponentFunctionParam { + name: p.parse()?, + type_: p.parse()?, + }); Ok(()) })?; } @@ -159,31 +134,11 @@ impl<'a> Parse<'a> for ComponentFunctionType<'a> { } } -impl<'a> Peek for ComponentFunctionType<'a> { - fn peek(cursor: Cursor<'_>) -> bool { - if let Some(next) = cursor.lparen() { - match next.keyword() { - Some(("func", _)) => return true, - _ => {} - } - } - - false - } - - fn display() -> &'static str { - "component function type" - } -} - /// A parameter of a [`ComponentFunctionType`]. #[derive(Clone, Debug)] pub struct ComponentFunctionParam<'a> { - /// An optional identifer to refer to this `param` by as part of - /// name resolution. - pub id: Option>, - /// An optional `@name` annotation for this parameter - pub name: Option>, + /// An optionally-specified name of this parameter + pub name: Option<&'a str>, /// The type of the parameter. pub type_: ComponentTypeUse<'a, InterType<'a>>, } @@ -210,23 +165,6 @@ impl<'a> Parse<'a> for ModuleType<'a> { } } -impl<'a> Peek for ModuleType<'a> { - fn peek(cursor: Cursor<'_>) -> bool { - if let Some(next) = cursor.lparen() { - match next.keyword() { - Some(("type" | "import" | "export", _)) => return true, - _ => {} - } - } - - false - } - - fn display() -> &'static str { - "module type" - } -} - /// The contents of a [`ModuleType`]. #[derive(Debug)] pub enum ModuleTypeDef<'a> { @@ -289,23 +227,6 @@ impl<'a> Parse<'a> for ComponentType<'a> { } } -impl<'a> Peek for ComponentType<'a> { - fn peek(cursor: Cursor<'_>) -> bool { - if let Some(next) = cursor.lparen() { - matches!( - next.keyword(), - Some(("import" | "export" | "type" | "alias", _)) - ) - } else { - false - } - } - - fn display() -> &'static str { - "component type" - } -} - /// A field of a type for a nested component #[derive(Debug)] pub enum ComponentTypeField<'a> { @@ -362,23 +283,6 @@ impl<'a> Parse<'a> for InstanceType<'a> { } } -impl<'a> Peek for InstanceType<'a> { - fn peek(cursor: Cursor<'_>) -> bool { - if let Some(next) = cursor.lparen() { - match next.keyword() { - Some(("export" | "type" | "alias", _)) => return true, - _ => {} - } - } - - false - } - - fn display() -> &'static str { - "instance type" - } -} - /// A field of a type for a nested instance #[derive(Debug)] pub enum InstanceTypeField<'a> { @@ -412,13 +316,3 @@ impl<'a> Parse<'a> for ValueType<'a> { }) } } - -impl<'a> Peek for ValueType<'a> { - fn peek(cursor: Cursor<'_>) -> bool { - LParen::peek(cursor) && kw::value::peek2(cursor) - } - - fn display() -> &'static str { - "valuetype" - } -} diff --git a/crates/wast/src/component/module.rs b/crates/wast/src/component/module.rs index 1f79794cae..3dd8ed14cd 100644 --- a/crates/wast/src/component/module.rs +++ b/crates/wast/src/component/module.rs @@ -1,7 +1,7 @@ use crate::component::*; use crate::core; use crate::kw; -use crate::parser::{Cursor, Parse, Parser, Peek, Result}; +use crate::parser::{Parse, Parser, Result}; use crate::token::{Id, NameAnnotation, Span}; /// A nested WebAssembly module to be created as part of a component. @@ -81,38 +81,3 @@ impl<'a> Parse<'a> for Module<'a> { }) } } - -// Note that this is a custom implementation to get multi-token lookahead to -// figure out how to parse `(type ...` when it's in an inline module. If this is -// `(type $x)` or `(type 0)` then it's an inline type annotation, otherwise it's -// probably a typedef like `(type $x (func))` or something like that. We only -// want to parse the two-token variant right now. -struct InlineType; - -impl Peek for InlineType { - fn peek(cursor: Cursor<'_>) -> bool { - let cursor = match cursor.lparen() { - Some(cursor) => cursor, - None => return false, - }; - let cursor = match cursor.keyword() { - Some(("type", cursor)) => cursor, - _ => return false, - }; - - // optional identifier - let cursor = match cursor.id() { - Some((_, cursor)) => cursor, - None => match cursor.integer() { - Some((_, cursor)) => cursor, - None => return false, - }, - }; - - cursor.rparen().is_some() - } - - fn display() -> &'static str { - "inline type" - } -} diff --git a/crates/wast/src/component/resolve.rs b/crates/wast/src/component/resolve.rs index 94c6551f22..0d79ee78c2 100644 --- a/crates/wast/src/component/resolve.rs +++ b/crates/wast/src/component/resolve.rs @@ -126,10 +126,8 @@ fn resolve_field<'a, 'b>( ModuleArg::Def(def) => { resolve_item_ref(def, resolve_stack)?; } - ModuleArg::BundleOfExports(_, exports) => { - for export in exports { - resolve_item_ref(&mut export.index, resolve_stack)?; - } + ModuleArg::BundleOfExports(..) => { + unreachable!("should be expanded already"); } } } @@ -171,16 +169,17 @@ fn resolve_field<'a, 'b>( crate::core::resolve::resolve(fields)?; } - // For `Import`, just resolve the type. - ModuleKind::Import { import: _, ty } => { - resolve_type_use(ty, resolve_stack)?; + ModuleKind::Import { .. } => { + unreachable!("should be expanded already") } } Ok(()) } ComponentField::Component(c) => match &mut c.kind { - NestedComponentKind::Import { .. } => Ok(()), + NestedComponentKind::Import { .. } => { + unreachable!("should be expanded already") + } NestedComponentKind::Inline(fields) => resolve_fields(c.id, fields, resolve_stack), }, ComponentField::Alias(a) => resolve_alias(a, resolve_stack), @@ -237,7 +236,7 @@ fn resolve_alias<'a, 'b>( if depth as usize == resolve_stack.len() { return Err(Error::new( alias.span, - format!("outer component '{:?}' not found", id), + format!("outer component `{}` not found", id.name()), )); } depth @@ -245,6 +244,12 @@ fn resolve_alias<'a, 'b>( Index::Num(n, _span) => *n, }; *outer = Index::Num(depth, alias.span); + if depth as usize >= resolve_stack.len() { + return Err(Error::new( + alias.span, + format!("component depth of `{}` is too large", depth), + )); + } // Resolve `index` within the computed scope depth. let ns = match alias.kind { @@ -281,7 +286,7 @@ fn resolve_type_use<'a, T>( ) -> Result<(), Error> { let item = match ty { ComponentTypeUse::Ref(r) => r, - ComponentTypeUse::Inline(_) => panic!("inline type-use should be expanded by now"), + ComponentTypeUse::Inline(_) => unreachable!("inline type-use should be expanded by now"), }; resolve_item_ref(item, resolve_stack) } diff --git a/crates/wast/src/component/types.rs b/crates/wast/src/component/types.rs index e0e5688b2d..126c6ac1d0 100644 --- a/crates/wast/src/component/types.rs +++ b/crates/wast/src/component/types.rs @@ -1,7 +1,7 @@ use crate::component::*; use crate::kw; -use crate::parser::{Cursor, Parse, Parser, Peek, Result}; -use crate::token::{Id, Index, NameAnnotation, Span}; +use crate::parser::{Parse, Parser, Result}; +use crate::token::{Id, NameAnnotation, Span}; /// A definition of a type. /// @@ -80,37 +80,14 @@ pub enum ComponentTypeUse<'a, T> { Inline(T), } -impl<'a, T> ComponentTypeUse<'a, T> { - /// Constructs a new instance of `ComponentTypeUse` without an inline definition but - /// with an index specified. - pub fn new_with_index(idx: Index<'a>) -> ComponentTypeUse<'a, T> { - ComponentTypeUse::Ref(ItemRef { - idx, - kind: kw::r#type::default(), - export_names: Vec::new(), - }) - } -} - -impl<'a, T: Peek + Parse<'a>> Parse<'a> for ComponentTypeUse<'a, T> { +impl<'a, T: Parse<'a>> Parse<'a> for ComponentTypeUse<'a, T> { fn parse(parser: Parser<'a>) -> Result { if parser.peek::>() { Ok(ComponentTypeUse::Ref( parser.parse::>()?.0, )) - } else if parser.peek::() { - Ok(ComponentTypeUse::Ref(parser.parse()?)) } else { Ok(ComponentTypeUse::Inline(parser.parse()?)) } } } - -impl<'a, T: Peek + Parse<'a>> Peek for ComponentTypeUse<'a, T> { - fn peek(cursor: Cursor<'_>) -> bool { - kw::r#type::peek(cursor) || T::peek(cursor) - } - fn display() -> &'static str { - T::display() - } -} diff --git a/crates/wast/src/parser.rs b/crates/wast/src/parser.rs index 2ae767c72c..2c30a4d1ab 100644 --- a/crates/wast/src/parser.rs +++ b/crates/wast/src/parser.rs @@ -582,13 +582,6 @@ impl<'a> Parser<'a> { } } - /// `is_empty` meets `peek2`. Looks past a token and checks if there are any - /// more tokens after that. - pub fn peek2_empty(self) -> bool { - let mut cursor = self.cursor(); - cursor.advance_token().is_some() - } - /// A helper structure to perform a sequence of `peek` operations and if /// they all fail produce a nice error message. /// diff --git a/tests/local/component-model/adapt.wast b/tests/local/component-model/adapt.wast new file mode 100644 index 0000000000..38c87b3d24 --- /dev/null +++ b/tests/local/component-model/adapt.wast @@ -0,0 +1,57 @@ +(component + (import "log" (func $log (param string))) + (module $libc + (memory (export "memory") 1) + (func (export "canonical_abi_realloc") (param i32 i32 i32 i32) (result i32) + unreachable) + (func (export "canonical_abi_free") (param i32 i32 i32) + unreachable) + ) + + (module $my_module + (import "env" "log-utf8" (func $log_utf8 (param i32 i32))) + (import "env" "log-utf16" (func $log_utf16 (param i32 i32))) + (import "env" "log-compact-utf16" (func $log_compact_utf16 (param i32 i32))) + + (func (export "log-utf8") (param i32 i32) + local.get 0 + local.get 1 + call $log_utf8 + ) + (func (export "log-utf16") (param i32 i32) + local.get 0 + local.get 1 + call $log_utf16 + ) + (func (export "log-compact-utf16") (param i32 i32) + local.get 0 + local.get 1 + call $log_compact_utf16 + ) + ) + + (instance $libc (instantiate (module $libc))) + + (func $log_lower_utf8 (canon.lower string=utf8 (into $libc) (func $log))) + (func $log_lower_utf16 (canon.lower string=utf16 (into $libc) (func $log))) + (func $log_lower_compact_utf16 (canon.lower string=latin1+utf16 (into $libc) (func $log))) + + (instance $my_instance (instantiate (module $my_module) + (with "libc" (instance $libc)) + (with "env" (instance + (export "log-utf8" (func $log_lower_utf8)) + (export "log-utf16" (func $log_lower_utf16)) + (export "log-compact-utf16" (func $log_lower_compact_utf16)) + )) + )) + + (func (export "log1") + (canon.lift (func (param string)) string=utf8 (into $libc) + (func $my_instance "log-utf8"))) + (func (export "log2") + (canon.lift (func (param string)) string=utf16 (into $libc) + (func $my_instance "log-utf16"))) + (func (export "log3") + (canon.lift (func (param string)) string=latin1+utf16 (into $libc) + (func $my_instance "log-compact-utf16"))) +) diff --git a/tests/local/component-model/alias.wast b/tests/local/component-model/alias.wast index e65f91408d..32a9b0f6dd 100644 --- a/tests/local/component-model/alias.wast +++ b/tests/local/component-model/alias.wast @@ -238,3 +238,31 @@ )) ) "instance 3 is not a module instance") + +;; alias some constructs +(component + (import "" (instance $foo (export "v" (value s32)))) + (export "v" (value $foo "v")) +) + +(component + (import "" (instance $foo (export "v" (component)))) + (export "v" (component $foo "v")) +) + +(component + (import "" (instance $foo (export "v" (module)))) + (export "v" (module $foo "v")) +) + +(component $C + (module $m) + (alias outer $C $m (module $target)) + (export "v" (module $target)) +) + +(component $C + (component $m) + (alias outer $C $m (component $target)) + (export "v" (component $target)) +) diff --git a/tests/local/component-model/func.wast b/tests/local/component-model/func.wast new file mode 100644 index 0000000000..5745940efa --- /dev/null +++ b/tests/local/component-model/func.wast @@ -0,0 +1,4 @@ +(component + (import "" (func (param "foo" string))) + (import "a" (func (param "foo" string) (param s32) (param "bar" u32))) +) diff --git a/tests/local/component-model/invalid.wast b/tests/local/component-model/invalid.wast index d63e1cefa1..23c0f0adfc 100644 --- a/tests/local/component-model/invalid.wast +++ b/tests/local/component-model/invalid.wast @@ -15,3 +15,27 @@ ) ) "invalid leading byte") + +(assert_malformed + (module quote + "(component" + "(export \"\" (func $foo))" + ")" + ) + "failed to find func named") + +(assert_malformed + (module quote + "(component" + "(alias outer 100 $foo (func $foo))" + ")" + ) + "component depth of `100` is too large") + +(assert_malformed + (module quote + "(component" + "(alias outer $nonexistent $foo (func $foo))" + ")" + ) + "outer component `nonexistent` not found")