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

@@ -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
+

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();
                 }
             }
         }

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<u8>) {
-        if let Some(id) = self.id {
+        if let Some(id) = self.name {
             e.push(0x01);
             id.encode(e);
         } else {
@@ -528,14 +528,16 @@ impl<'a> Encode for ValueType<'a> {
         self.value_type.encode(e)
     }
 }
-impl<T: Encode> Encode for ComponentTypeUse<'_, T> {
+
+impl<T> Encode for ComponentTypeUse<'_, T> {
     fn encode(&self, e: &mut Vec<u8>) {
         match self {
             ComponentTypeUse::Inline(_) => unreachable!("should be expanded already"),
             ComponentTypeUse::Ref(index) => index.encode(e),
         }
     }
 }
+
 impl Encode for ComponentExport<'_> {
     fn encode(&self, e: &mut Vec<u8>) {
         self.name.encode(e);
@@ -560,7 +562,7 @@ impl Encode for ComponentExport<'_> {
 impl Encode for ComponentFunc<'_> {
     fn encode(&self, e: &mut Vec<u8>) {
         match &self.kind {
-            ComponentFuncKind::Import { .. } => unreachable!("should be expanded already"),
+            ComponentFuncKind::Import { .. } => unreachable!("should be expanded by now"),
             ComponentFuncKind::Inline { body } => {
                 body.encode(e);
             }

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::<kw::param>() {
             parser.parens(|p| {
-                let mut l = p.lookahead1();
-                if l.peek::<kw::param>() {
-                    p.parse::<kw::param>()?;
-                    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::<Option<_>>()?, p.parse::<Option<_>>()?);
-                        let ty = p.parse()?;
-                        params.push(ComponentFunctionParam {
-                            id,
-                            name,
-                            type_: ty,
-                        });
-                    }
-                } else {
-                    return Err(l.error());
-                }
+                p.parse::<kw::param>()?;
+                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<Id<'a>>,
-    /// An optional `@name` annotation for this parameter
-    pub name: Option<NameAnnotation<'a>>,
+    /// 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"
-    }
-}

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"
-    }
-}

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,14 +236,20 @@ 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
                 }
                 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)
 }