Refactor module creation slightly (#799)

A few small updates to module creation in preparation for a future PR
which is a bit more invasive here with interface types.

* Move `read_imports_and_exports` to an instance method which configures
  fields directly rather than returning a number of fields.
* Call between constructors as much as possible.
* Leverage `Rc::get_mut` which we know will work at module creation time
  since we have the only reference.

crates/api/src/module.rs

@@ -56,9 +56,210 @@ fn into_table_type(tt: wasmparser::TableType) -> TableType {
     TableType::new(ty, limits)
 }
 
-fn read_imports_and_exports(
+#[derive(Clone)]
-    binary: &[u8],
+pub(crate) enum ModuleCodeSource {
-) -> Result<(Box<[ImportType]>, Box<[ExportType]>, Option<String>)> {
+    Binary(Box<[u8]>),
+    Unknown,
+}
+
+/// A compiled WebAssembly module, ready to be instantiated.
+///
+/// A `Module` is a compiled in-memory representation of an input WebAssembly
+/// binary. A `Module` is then used to create an [`Instance`](crate::Instance)
+/// through an instantiation process. You cannot call functions or fetch
+/// globals, for example, on a `Module` because it's purely a code
+/// representation. Instead you'll need to create an
+/// [`Instance`](crate::Instance) to interact with the wasm module.
+///
+/// ## Modules and `Clone`
+///
+/// Using `clone` on a `Module` is a cheap operation. It will not create an
+/// entirely new module, but rather just a new reference to the existing module.
+/// In other words it's a shallow copy, not a deep copy.
+#[derive(Clone)]
+pub struct Module {
+    // FIXME(#777) should be `Arc` and this type should be thread-safe
+    inner: Rc<ModuleInner>,
+}
+
+struct ModuleInner {
+    store: Store,
+    source: ModuleCodeSource,
+    imports: Box<[ImportType]>,
+    exports: Box<[ExportType]>,
+    name: Option<String>,
+}
+
+impl Module {
+    /// Creates a new WebAssembly `Module` from the given in-memory `binary`
+    /// data.
+    ///
+    /// The `binary` data provided must be a [binary-encoded][binary]
+    /// WebAssembly module. This means that the data for the wasm module must be
+    /// loaded in-memory if it's present elsewhere, for example on disk.
+    /// Additionally this requires that the entire binary is loaded into memory
+    /// all at once, this API does not support streaming compilation of a
+    /// module.
+    ///
+    /// The WebAssembly binary will be decoded and validated. It will also be
+    /// compiled according to the configuration of the provided `store` and
+    /// cached in this type.
+    ///
+    /// The provided `store` is a global cache for compiled resources as well as
+    /// configuration for what wasm features are enabled. It's recommended to
+    /// share a `store` among modules if possible.
+    ///
+    /// # Errors
+    ///
+    /// This function may fail and return an error. Errors may include
+    /// situations such as:
+    ///
+    /// * The binary provided could not be decoded because it's not a valid
+    ///   WebAssembly binary
+    /// * The WebAssembly binary may not validate (e.g. contains type errors)
+    /// * Implementation-specific limits were exceeded with a valid binary (for
+    ///   example too many locals)
+    /// * The wasm binary may use features that are not enabled in the
+    ///   configuration of `store`
+    ///
+    /// The error returned should contain full information about why module
+    /// creation failed if one is returned.
+    ///
+    /// [binary]: https://webassembly.github.io/spec/core/binary/index.html
+    pub fn new(store: &Store, binary: &[u8]) -> Result<Module> {
+        Module::validate(store, binary)?;
+        // Note that the call to `unsafe` here should be ok because we
+        // previously validated the binary, meaning we're guaranteed to pass a
+        // valid binary for `store`.
+        unsafe { Module::new_unchecked(store, binary) }
+    }
+
+    /// Creates a new WebAssembly `Module` from the given in-memory `binary`
+    /// data. The provided `name` will be used in traps/backtrace details.
+    ///
+    /// See [`Module::new`] for other details.
+    pub fn new_with_name(store: &Store, binary: &[u8], name: &str) -> Result<Module> {
+        let mut ret = Module::new(store, binary)?;
+        Rc::get_mut(&mut ret.inner).unwrap().name = Some(name.to_string());
+        Ok(ret)
+    }
+
+    /// Creates a new WebAssembly `Module` from the given in-memory `binary`
+    /// data, skipping validation and asserting that `binary` is a valid
+    /// WebAssembly module.
+    ///
+    /// This function is the same as [`Module::new`] except that it skips the
+    /// call to [`Module::validate`]. This means that the WebAssembly binary is
+    /// not validated for correctness and it is simply assumed as valid.
+    ///
+    /// For more information about creation of a module and the `store` argument
+    /// see the documentation of [`Module::new`].
+    ///
+    /// # Unsafety
+    ///
+    /// This function is `unsafe` due to the unchecked assumption that the input
+    /// `binary` is valid. If the `binary` is not actually a valid wasm binary it
+    /// may cause invalid machine code to get generated, cause panics, etc.
+    ///
+    /// It is only safe to call this method if [`Module::validate`] succeeds on
+    /// the same arguments passed to this function.
+    ///
+    /// # Errors
+    ///
+    /// This function may fail for many of the same reasons as [`Module::new`].
+    /// While this assumes that the binary is valid it still needs to actually
+    /// be somewhat valid for decoding purposes, and the basics of decoding can
+    /// still fail.
+    pub unsafe fn new_unchecked(store: &Store, binary: &[u8]) -> Result<Module> {
+        let mut ret = Module::empty(store);
+        ret.read_imports_and_exports(binary)?;
+        Ok(ret)
+    }
+
+    /// Validates `binary` input data as a WebAssembly binary given the
+    /// configuration in `store`.
+    ///
+    /// This function will perform a speedy validation of the `binary` input
+    /// WebAssembly module (which is in [binary form][binary]) and return either
+    /// `Ok` or `Err` depending on the results of validation. The `store`
+    /// argument indicates configuration for WebAssembly features, for example,
+    /// which are used to indicate what should be valid and what shouldn't be.
+    ///
+    /// Validation automatically happens as part of [`Module::new`], but is a
+    /// requirement for [`Module::new_unchecked`] to be safe.
+    ///
+    /// # Errors
+    ///
+    /// If validation fails for any reason (type check error, usage of a feature
+    /// that wasn't enabled, etc) then an error with a description of the
+    /// validation issue will be returned.
+    ///
+    /// [binary]: https://webassembly.github.io/spec/core/binary/index.html
+    pub fn validate(store: &Store, binary: &[u8]) -> Result<()> {
+        let features = store.engine().config.features.clone();
+        let config = ValidatingParserConfig {
+            operator_config: OperatorValidatorConfig {
+                enable_threads: features.threads,
+                enable_reference_types: features.reference_types,
+                enable_bulk_memory: features.bulk_memory,
+                enable_simd: features.simd,
+                enable_multi_value: features.multi_value,
+            },
+        };
+        validate(binary, Some(config)).map_err(Error::new)
+    }
+
+    pub fn from_exports(store: &Store, exports: Box<[ExportType]>) -> Self {
+        let mut ret = Module::empty(store);
+        Rc::get_mut(&mut ret.inner).unwrap().exports = exports;
+        return ret;
+    }
+
+    fn empty(store: &Store) -> Self {
+        Module {
+            inner: Rc::new(ModuleInner {
+                store: store.clone(),
+                source: ModuleCodeSource::Unknown,
+                imports: Box::new([]),
+                exports: Box::new([]),
+                name: None,
+            }),
+        }
+    }
+
+    pub(crate) fn binary(&self) -> Option<&[u8]> {
+        match &self.inner.source {
+            ModuleCodeSource::Binary(b) => Some(b),
+            _ => None,
+        }
+    }
+
+    /// Returns identifier/name that this [`Module`] has. This name
+    /// is used in traps/backtrace details.
+    pub fn name(&self) -> Option<&str> {
+        self.inner.name.as_deref()
+    }
+
+    /// Returns the list of imports that this [`Module`] has and must be
+    /// satisfied.
+    pub fn imports(&self) -> &[ImportType] {
+        &self.inner.imports
+    }
+
+    /// Returns the list of exports that this [`Module`] has and will be
+    /// available after instantiation.
+    pub fn exports(&self) -> &[ExportType] {
+        &self.inner.exports
+    }
+
+    /// Returns the [`Store`] that this [`Module`] was compiled into.
+    pub fn store(&self) -> &Store {
+        &self.inner.store
+    }
+
+    fn read_imports_and_exports(&mut self, binary: &[u8]) -> Result<()> {
+        let inner = Rc::get_mut(&mut self.inner).unwrap();
+        inner.source = ModuleCodeSource::Binary(binary.into());
         let mut reader = ModuleReader::new(binary)?;
         let mut imports = Vec::new();
         let mut exports = Vec::new();
@@ -67,7 +268,6 @@ fn read_imports_and_exports(
         let mut func_sig = Vec::new();
         let mut sigs = Vec::new();
         let mut globals = Vec::new();
-    let mut module_name = None;
         while !reader.eof() {
             let section = reader.read()?;
             match section.code {
@@ -169,7 +369,7 @@ fn read_imports_and_exports(
                         while let Ok(entry) = reader.read() {
                             if let Name::Module(name) = entry {
                                 if let Ok(name) = name.get_name() {
-                                module_name = Some(name.to_string());
+                                    inner.name = Some(name.to_string());
                                 }
                                 break;
                             }
@@ -181,219 +381,9 @@ fn read_imports_and_exports(
                 }
             }
         }
-    Ok((
-        imports.into_boxed_slice(),
-        exports.into_boxed_slice(),
-        module_name,
-    ))
-}
 
-#[derive(Clone)]
+        inner.imports = imports.into();
-pub(crate) enum ModuleCodeSource {
+        inner.exports = exports.into();
-    Binary(Box<[u8]>),
+        Ok(())
-    Unknown,
-}
-
-/// A compiled WebAssembly module, ready to be instantiated.
-///
-/// A `Module` is a compiled in-memory representation of an input WebAssembly
-/// binary. A `Module` is then used to create an [`Instance`](crate::Instance)
-/// through an instantiation process. You cannot call functions or fetch
-/// globals, for example, on a `Module` because it's purely a code
-/// representation. Instead you'll need to create an
-/// [`Instance`](crate::Instance) to interact with the wasm module.
-///
-/// ## Modules and `Clone`
-///
-/// Using `clone` on a `Module` is a cheap operation. It will not create an
-/// entirely new module, but rather just a new reference to the existing module.
-/// In other words it's a shallow copy, not a deep copy.
-#[derive(Clone)]
-pub struct Module {
-    // FIXME(#777) should be `Arc` and this type should be thread-safe
-    inner: Rc<ModuleInner>,
-}
-
-struct ModuleInner {
-    store: Store,
-    source: ModuleCodeSource,
-    imports: Box<[ImportType]>,
-    exports: Box<[ExportType]>,
-    name: Option<String>,
-}
-
-impl Module {
-    /// Creates a new WebAssembly `Module` from the given in-memory `binary`
-    /// data.
-    ///
-    /// The `binary` data provided must be a [binary-encoded][binary]
-    /// WebAssembly module. This means that the data for the wasm module must be
-    /// loaded in-memory if it's present elsewhere, for example on disk.
-    /// Additionally this requires that the entire binary is loaded into memory
-    /// all at once, this API does not support streaming compilation of a
-    /// module.
-    ///
-    /// The WebAssembly binary will be decoded and validated. It will also be
-    /// compiled according to the configuration of the provided `store` and
-    /// cached in this type.
-    ///
-    /// The provided `store` is a global cache for compiled resources as well as
-    /// configuration for what wasm features are enabled. It's recommended to
-    /// share a `store` among modules if possible.
-    ///
-    /// # Errors
-    ///
-    /// This function may fail and return an error. Errors may include
-    /// situations such as:
-    ///
-    /// * The binary provided could not be decoded because it's not a valid
-    ///   WebAssembly binary
-    /// * The WebAssembly binary may not validate (e.g. contains type errors)
-    /// * Implementation-specific limits were exceeded with a valid binary (for
-    ///   example too many locals)
-    /// * The wasm binary may use features that are not enabled in the
-    ///   configuration of `store`
-    ///
-    /// The error returned should contain full information about why module
-    /// creation failed if one is returned.
-    ///
-    /// [binary]: https://webassembly.github.io/spec/core/binary/index.html
-    pub fn new(store: &Store, binary: &[u8]) -> Result<Module> {
-        Self::validate(store, binary)?;
-        // Note that the call to `unsafe` here should be ok because we
-        // previously validated the binary, meaning we're guaranteed to pass a
-        // valid binary for `store`.
-        unsafe { Self::create(store, binary, None) }
-    }
-
-    /// Creates a new WebAssembly `Module` from the given in-memory `binary`
-    /// data. The provided `name` will be used in traps/backtrace details.
-    ///
-    /// See [`Module::new`] for other details.
-    pub fn new_with_name(store: &Store, binary: &[u8], name: &str) -> Result<Module> {
-        Self::validate(store, binary)?;
-        unsafe { Self::create(store, binary, Some(name.to_string())) }
-    }
-
-    /// Creates a new WebAssembly `Module` from the given in-memory `binary`
-    /// data, skipping validation and asserting that `binary` is a valid
-    /// WebAssembly module.
-    ///
-    /// This function is the same as [`Module::new`] except that it skips the
-    /// call to [`Module::validate`]. This means that the WebAssembly binary is
-    /// not validated for correctness and it is simply assumed as valid.
-    ///
-    /// For more information about creation of a module and the `store` argument
-    /// see the documentation of [`Module::new`].
-    ///
-    /// # Unsafety
-    ///
-    /// This function is `unsafe` due to the unchecked assumption that the input
-    /// `binary` is valid. If the `binary` is not actually a valid wasm binary it
-    /// may cause invalid machine code to get generated, cause panics, etc.
-    ///
-    /// It is only safe to call this method if [`Module::validate`] succeeds on
-    /// the same arguments passed to this function.
-    ///
-    /// # Errors
-    ///
-    /// This function may fail for many of the same reasons as [`Module::new`].
-    /// While this assumes that the binary is valid it still needs to actually
-    /// be somewhat valid for decoding purposes, and the basics of decoding can
-    /// still fail.
-    pub unsafe fn new_unchecked(store: &Store, binary: &[u8]) -> Result<Module> {
-        Self::create(store, binary, None)
-    }
-
-    unsafe fn create(
-        store: &Store,
-        binary: &[u8],
-        name_override: Option<String>,
-    ) -> Result<Module> {
-        let (imports, exports, name) = read_imports_and_exports(binary)?;
-        Ok(Module {
-            inner: Rc::new(ModuleInner {
-                store: store.clone(),
-                source: ModuleCodeSource::Binary(binary.into()),
-                imports,
-                exports,
-                name: name_override.or(name),
-            }),
-        })
-    }
-
-    /// Validates `binary` input data as a WebAssembly binary given the
-    /// configuration in `store`.
-    ///
-    /// This function will perform a speedy validation of the `binary` input
-    /// WebAssembly module (which is in [binary form][binary]) and return either
-    /// `Ok` or `Err` depending on the results of validation. The `store`
-    /// argument indicates configuration for WebAssembly features, for example,
-    /// which are used to indicate what should be valid and what shouldn't be.
-    ///
-    /// Validation automatically happens as part of [`Module::new`], but is a
-    /// requirement for [`Module::new_unchecked`] to be safe.
-    ///
-    /// # Errors
-    ///
-    /// If validation fails for any reason (type check error, usage of a feature
-    /// that wasn't enabled, etc) then an error with a description of the
-    /// validation issue will be returned.
-    ///
-    /// [binary]: https://webassembly.github.io/spec/core/binary/index.html
-    pub fn validate(store: &Store, binary: &[u8]) -> Result<()> {
-        let features = store.engine().config.features.clone();
-        let config = ValidatingParserConfig {
-            operator_config: OperatorValidatorConfig {
-                enable_threads: features.threads,
-                enable_reference_types: features.reference_types,
-                enable_bulk_memory: features.bulk_memory,
-                enable_simd: features.simd,
-                enable_multi_value: features.multi_value,
-            },
-        };
-        validate(binary, Some(config)).map_err(Error::new)
-    }
-
-    pub fn from_exports(store: &Store, exports: Box<[ExportType]>) -> Self {
-        Module {
-            inner: Rc::new(ModuleInner {
-                store: store.clone(),
-                source: ModuleCodeSource::Unknown,
-                imports: Box::new([]),
-                exports,
-                name: None,
-            }),
-        }
-    }
-
-    pub(crate) fn binary(&self) -> Option<&[u8]> {
-        match &self.inner.source {
-            ModuleCodeSource::Binary(b) => Some(b),
-            _ => None,
-        }
-    }
-
-    /// Returns identifier/name that this [`Module`] has. This name
-    /// is used in traps/backtrace details.
-    pub fn name(&self) -> Option<&str> {
-        self.inner.name.as_deref()
-    }
-
-    /// Returns the list of imports that this [`Module`] has and must be
-    /// satisfied.
-    pub fn imports(&self) -> &[ImportType] {
-        &self.inner.imports
-    }
-
-    /// Returns the list of exports that this [`Module`] has and will be
-    /// available after instantiation.
-    pub fn exports(&self) -> &[ExportType] {
-        &self.inner.exports
-    }
-
-    /// Returns the [`Store`] that this [`Module`] was compiled into.
-    pub fn store(&self) -> &Store {
-        &self.inner.store
     }
 }