Repository: bytecodealliance/wasm-tools
Pull request #1696 (2024-07-22 19:32:37)

Add some guard rails to the usage of `ComponentFuncTypeEncoder`
This should help weed out mistakes like #1676. This still doesn't prevent the mistake where a `ComponentFuncTypeEncoder` is created but neither `params` nor `results` are called but panicking in `Drop` feels a bit overkill for this so that's left as a "still open" issue to solve later.


Patch:
diff --git a/crates/wasm-encoder/src/component/types.rs b/crates/wasm-encoder/src/component/types.rs
index 08e37da2..006b9a33 100644
--- a/crates/wasm-encoder/src/component/types.rs
+++ b/crates/wasm-encoder/src/component/types.rs
@@ -379,28 +379,43 @@ impl Encode for InstanceType {
 
 /// Used to encode component function types.
 #[derive(Debug)]
-pub struct ComponentFuncTypeEncoder<'a>(&'a mut Vec<u8>);
+pub struct ComponentFuncTypeEncoder<'a> {
+    params_encoded: bool,
+    results_encoded: bool,
+    sink: &'a mut Vec<u8>,
+}
 
 impl<'a> ComponentFuncTypeEncoder<'a> {
     fn new(sink: &'a mut Vec<u8>) -> Self {
         sink.push(0x40);
-        Self(sink)
+        Self {
+            params_encoded: false,
+            results_encoded: false,
+            sink,
+        }
     }
 
     /// Defines named parameters.
     ///
     /// Parameters must be defined before defining results.
+    ///
+    /// # Panics
+    ///
+    /// This method will panic if the function is called twice since parameters
+    /// can only be encoded once.
     pub fn params<'b, P, T>(&mut self, params: P) -> &mut Self
     where
         P: IntoIterator<Item = (&'b str, T)>,
         P::IntoIter: ExactSizeIterator,
         T: Into<ComponentValType>,
     {
+        assert!(!self.params_encoded);
+        self.params_encoded = true;
         let params = params.into_iter();
-        params.len().encode(self.0);
+        params.len().encode(self.sink);
         for (name, ty) in params {
-            name.encode(self.0);
-            ty.into().encode(self.0);
+            name.encode(self.sink);
+            ty.into().encode(self.sink);
         }
         self
     }
@@ -408,27 +423,43 @@ impl<'a> ComponentFuncTypeEncoder<'a> {
     /// Defines a single unnamed result.
     ///
     /// This method cannot be used with `results`.
+    ///
+    /// # Panics
+    ///
+    /// This method will panic if the function is called twice, called before
+    /// the `params` method, or called in addition to the `results` method.
     pub fn result(&mut self, ty: impl Into<ComponentValType>) -> &mut Self {
-        self.0.push(0x00);
-        ty.into().encode(self.0);
+        assert!(self.params_encoded);
+        assert!(!self.results_encoded);
+        self.results_encoded = true;
+        self.sink.push(0x00);
+        ty.into().encode(self.sink);
         self
     }
 
     /// Defines named results.
     ///
     /// This method cannot be used with `result`.
+    ///
+    /// # Panics
+    ///
+    /// This method will panic if the function is called twice, called before
+    /// the `params` method, or called in addition to the `result` method.
     pub fn results<'b, R, T>(&mut self, results: R) -> &mut Self
     where
         R: IntoIterator<Item = (&'b str, T)>,
         R::IntoIter: ExactSizeIterator,
         T: Into<ComponentValType>,
     {
-        self.0.push(0x01);
+        assert!(self.params_encoded);
+        assert!(!self.results_encoded);
+        self.results_encoded = true;
+        self.sink.push(0x01);
         let results = results.into_iter();
-        results.len().encode(self.0);
+        results.len().encode(self.sink);
         for (name, ty) in results {
-            name.encode(self.0);
-            ty.into().encode(self.0);
+            name.encode(self.sink);
+            ty.into().encode(self.sink);
         }
         self
     }
diff --git a/crates/wit-parser/src/ast.rs b/crates/wit-parser/src/ast.rs
index d65d5fed..86bb9ec2 100644
--- a/crates/wit-parser/src/ast.rs
+++ b/crates/wit-parser/src/ast.rs
@@ -1208,33 +1208,79 @@ fn parse_version(tokens: &mut Tokenizer<'_>) -> Result<(Span, Version)> {
     let version = Version::parse(string).map_err(|e| Error::new(span, e.to_string()))?;
     return Ok((span, version));
 
+    // According to `semver.org` this is what we're parsing:
+    //
+    // ```ebnf
+    // <pre-release> ::= <dot-separated pre-release identifiers>
+    //
+    // <dot-separated pre-release identifiers> ::= <pre-release identifier>
+    //                                           | <pre-release identifier> "." <dot-separated pre-release identifiers>
+    //
+    // <build> ::= <dot-separated build identifiers>
+    //
+    // <dot-separated build identifiers> ::= <build identifier>
+    //                                     | <build identifier> "." <dot-separated build identifiers>
+    //
+    // <pre-release identifier> ::= <alphanumeric identifier>
+    //                            | <numeric identifier>
+    //
+    // <build identifier> ::= <alphanumeric identifier>
+    //                      | <digits>
+    //
+    // <alphanumeric identifier> ::= <non-digit>
+    //                             | <non-digit> <identifier characters>
+    //                             | <identifier characters> <non-digit>
+    //                             | <identifier characters> <non-digit> <identifier characters>
+    //
+    // <numeric identifier> ::= "0"
+    //                        | <positive digit>
+    //                        | <positive digit> <digits>
+    //
+    // <identifier characters> ::= <identifier character>
+    //                           | <identifier character> <identifier characters>
+    //
+    // <identifier character> ::= <digit>
+    //                          | <non-digit>
+    //
+    // <non-digit> ::= <letter>
+    //               | "-"
+    //
+    // <digits> ::= <digit>
+    //            | <digit> <digits>
+    // ```
+    //
+    // This is loosely based on WIT syntax and an approximation is parsed here:
+    //
+    // * This function starts by parsing the optional leading `-` and `+` which
+    //   indicates pre-release and build metadata.
+    // * Afterwards all of $id, $integer, `-`, and `.` are chomped. The only
+    //   exception here is that if `.` isn't followed by $id, $integer, or `-`
+    //   then it's assumed that it's something like `use a:b@1.0.0-a.{...}`
+    //   where the `.` is part of WIT syntax, not semver.
+    //
+    // Note that this additionally doesn't try to return any first-class errors.
+    // Instead this bails out on something unrecognized for something else in
+    // the system to return an error.
     fn eat_ids(tokens: &mut Tokenizer<'_>, prefix: Token, end: &mut Span) -> Result<()> {
         if !tokens.eat(prefix)? {
             return Ok(());
         }
         loop {
-            match tokens.next()? {
-                Some((span, Token::Id)) | Some((span, Token::Integer)) => end.end = span.end,
-                other => break Err(err_expected(tokens, "an id or integer", other).into()),
-            }
-
-            // If there's no trailing period, then this semver identifier is
-            // done.
             let mut clone = tokens.clone();
-            if !clone.eat(Token::Period)? {
-                break Ok(());
-            }
-
-            // If there's more to the identifier, then eat the period for real
-            // and continue
-            if clone.eat(Token::Id)? || clone.eat(Token::Integer)? {
-                tokens.eat(Token::Period)?;
-                continue;
+            match clone.next()? {
+                Some((span, Token::Id | Token::Integer | Token::Minus)) => {
+                    end.end = span.end;
+                    *tokens = clone;
+                }
+                Some((_span, Token::Period)) => match clone.next()? {
+                    Some((span, Token::Id | Token::Integer | Token::Minus)) => {
+                        end.end = span.end;
+                        *tokens = clone;
+                    }
+                    _ => break Ok(()),
+                },
+                _ => break Ok(()),
             }
-
-            // Otherwise for something like `use foo:bar/baz@1.2.3+foo.{` stop
-            // the parsing here.
-            break Ok(());
         }
     }
 }


Test patch:
diff --git a/crates/wit-parser/tests/ui/version-syntax.wit b/crates/wit-parser/tests/ui/version-syntax.wit
new file mode 100644
index 00000000..33ae783b
--- /dev/null
+++ b/crates/wit-parser/tests/ui/version-syntax.wit
@@ -0,0 +1,10 @@
+
+package a:b@1.0.0 {}
+package a:b@1.0.1 {}
+package a:b@1.0.1-- {}
+package a:b@1.0.1-a+a {}
+package a:b@1.0.1-1+1 {}
+package a:b@1.0.1-1a+1a {}
+package a:b@1.0.0-11-a {}
+package a:b@1.0.0-a1.1-a {}
+package a:b@1.0.0-11ab {}
diff --git a/crates/wit-parser/tests/ui/version-syntax.wit.json b/crates/wit-parser/tests/ui/version-syntax.wit.json
new file mode 100644
index 00000000..6ae121f5
--- /dev/null
+++ b/crates/wit-parser/tests/ui/version-syntax.wit.json
@@ -0,0 +1,52 @@
+{
+  "worlds": [],
+  "interfaces": [],
+  "types": [],
+  "packages": [
+    {
+      "name": "a:b@1.0.0-11-a",
+      "interfaces": {},
+      "worlds": {}
+    },
+    {
+      "name": "a:b@1.0.0-11ab",
+      "interfaces": {},
+      "worlds": {}
+    },
+    {
+      "name": "a:b@1.0.0-a1.1-a",
+      "interfaces": {},
+      "worlds": {}
+    },
+    {
+      "name": "a:b@1.0.0",
+      "interfaces": {},
+      "worlds": {}
+    },
+    {
+      "name": "a:b@1.0.1-1+1",
+      "interfaces": {},
+      "worlds": {}
+    },
+    {
+      "name": "a:b@1.0.1--",
+      "interfaces": {},
+      "worlds": {}
+    },
+    {
+      "name": "a:b@1.0.1-1a+1a",
+      "interfaces": {},
+      "worlds": {}
+    },
+    {
+      "name": "a:b@1.0.1-a+a",
+      "interfaces": {},
+      "worlds": {}
+    },
+    {
+      "name": "a:b@1.0.1",
+      "interfaces": {},
+      "worlds": {}
+    }
+  ]
+}
\ No newline at end of file