Remove the need for HostRef<Module>

This commit continues previous work and also #708 by removing the need
to use `HostRef<Module>` in the API of the `wasmtime` crate. The API
changes performed here are:

* The `Module` type is now itself internally reference counted.
* The `Module::store` function now returns the `Store` that was used to
  create a `Module`
* Documentation for `Module` and its methods have been expanded.

crates/api/examples/gcd.rs

@@ -38,11 +38,10 @@ fn main() -> anyhow::Result<()> {
     // `Module` which is attached to a `Store` cache.
     let wasm = wat::parse_str(WAT)?;
     let store = Store::default();
-    let module = HostRef::new(Module::new(&store, &wasm)?);
+    let module = Module::new(&store, &wasm)?;
 
     // Find index of the `gcd` export.
     let gcd_index = module
-        .borrow()
         .exports()
         .iter()
         .enumerate()

crates/api/examples/hello.rs

@@ -35,7 +35,7 @@ fn main() -> Result<()> {
 
     // Compiler the `*.wasm` binary into an in-memory instance of a `Module`.
     println!("Compiling module...");
-    let module = HostRef::new(Module::new(&store, &binary).context("> Error compiling module!")?);
+    let module = Module::new(&store, &binary).context("> Error compiling module!")?;
 
     // Here we handle the imports of the module, which in this case is our
     // `HelloCallback` type and its associated implementation of `Callback.

crates/api/examples/memory.rs

@@ -86,7 +86,7 @@ fn main() -> Result<(), Error> {
 
     // Compile.
     println!("Compiling module...");
-    let module = HostRef::new(Module::new(&store, &binary).context("> Error compiling module!")?);
+    let module = Module::new(&store, &binary).context("> Error compiling module!")?;
 
     // Instantiate.
     println!("Instantiating module...");

crates/api/examples/multi.rs

@@ -54,7 +54,7 @@ fn main() -> Result<()> {
 
     // Compile.
     println!("Compiling module...");
-    let module = HostRef::new(Module::new(&store, &binary).context("Error compiling module!")?);
+    let module = Module::new(&store, &binary).context("Error compiling module!")?;
 
     // Create external print functions.
     println!("Creating callback...");

crates/api/src/callable.rs

@@ -43,7 +43,7 @@ use wasmtime_runtime::Export;
 ///
 /// // Initialise environment and our module.
 /// let store = wasmtime::Store::default();
-/// let module = HostRef::new(wasmtime::Module::new(&store, &binary)?);
+/// let module = wasmtime::Module::new(&store, &binary)?;
 ///
 /// // Define the type of the function we're going to call.
 /// let times_two_type = wasmtime::FuncType::new(

crates/api/src/instance.rs

@@ -1,7 +1,6 @@
 use crate::context::Context;
 use crate::externals::Extern;
 use crate::module::Module;
-use crate::r#ref::HostRef;
 use crate::runtime::Store;
 use crate::trampoline::take_api_trap;
 use crate::trap::Trap;
@@ -60,7 +59,7 @@ pub fn instantiate_in_context(
 pub struct Instance {
     instance_handle: InstanceHandle,
 
-    module: HostRef<Module>,
+    module: Module,
 
     // We need to keep CodeMemory alive.
     contexts: HashSet<Context>,
@@ -69,29 +68,19 @@ pub struct Instance {
 }
 
 impl Instance {
-    pub fn new(
+    pub fn new(store: &Store, module: &Module, externs: &[Extern]) -> Result<Instance, Error> {
-        store: &Store,
-        module: &HostRef<Module>,
-        externs: &[Extern],
-    ) -> Result<Instance, Error> {
         let context = store.context().clone();
         let exports = store.global_exports().clone();
         let imports = module
-            .borrow()
             .imports()
             .iter()
             .zip(externs.iter())
             .map(|(i, e)| (i.module().to_string(), i.name().to_string(), e.clone()))
             .collect::<Vec<_>>();
-        let (mut instance_handle, contexts) = instantiate_in_context(
+        let (mut instance_handle, contexts) =
-            module.borrow().binary().expect("binary"),
+            instantiate_in_context(module.binary().expect("binary"), imports, context, exports)?;
-            imports,
-            context,
-            exports,
-        )?;
 
         let exports = {
-            let module = module.borrow();
             let mut exports = Vec::with_capacity(module.exports().len());
             for export in module.exports() {
                 let name = export.name().to_string();
@@ -116,14 +105,13 @@ impl Instance {
         &self.exports
     }
 
-    pub fn module(&self) -> &HostRef<Module> {
+    pub fn module(&self) -> &Module {
         &self.module
     }
 
     pub fn find_export_by_name(&self, name: &str) -> Option<&Extern> {
         let (i, _) = self
             .module
-            .borrow()
             .exports()
             .iter()
             .enumerate()
@@ -154,10 +142,7 @@ impl Instance {
             ));
         }
 
-        let module = HostRef::new(Module::from_exports(
+        let module = Module::from_exports(store, exports_types.into_boxed_slice());
-            store,
-            exports_types.into_boxed_slice(),
-        ));
 
         Instance {
             instance_handle,

crates/api/src/module.rs

@@ -4,6 +4,7 @@ use crate::types::{
     TableType, ValType,
 };
 use anyhow::{Error, Result};
+use std::rc::Rc;
 use wasmparser::{
     validate, ExternalKind, ImportSectionEntryType, ModuleReader, OperatorValidatorConfig,
     SectionCode, ValidatingParserConfig,
@@ -170,8 +171,27 @@ pub(crate) enum ModuleCodeSource {
     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]>,
@@ -179,29 +199,106 @@ pub struct Module {
 }
 
 impl Module {
-    /// Validate and decode the raw wasm data in `binary` and create a new
+    /// Creates a new WebAssembly `Module` from the given in-memory `binary`
-    /// `Module` in the given `store`.
+    /// 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)?;
-        Self::new_unchecked(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::new_unchecked(store, binary) }
     }
-    /// Similar to `new`, but does not perform any validation. Only use this
+
-    /// on modules which are known to have been validated already!
+    /// Creates a new WebAssembly `Module` from the given in-memory `binary`
-    pub fn new_unchecked(store: &Store, binary: &[u8]) -> Result<Module> {
+    /// 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 (imports, exports) = read_imports_and_exports(binary)?;
         Ok(Module {
+            inner: Rc::new(ModuleInner {
                 store: store.clone(),
                 source: ModuleCodeSource::Binary(binary.into()),
                 imports,
                 exports,
+            }),
         })
     }
-    pub(crate) fn binary(&self) -> Option<&[u8]> {
+
-        match &self.source {
+    /// Validates `binary` input data as a WebAssembly binary given the
-            ModuleCodeSource::Binary(b) => Some(b),
+    /// configuration in `store`.
-            _ => None,
+    ///
-        }
+    /// 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 {
@@ -215,18 +312,39 @@ impl Module {
         };
         validate(binary, Some(config)).map_err(Error::new)
     }
-    pub fn imports(&self) -> &[ImportType] {
+
-        &self.imports
-    }
-    pub fn exports(&self) -> &[ExportType] {
-        &self.exports
-    }
     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,
+            }),
         }
     }
+
+    pub(crate) fn binary(&self) -> Option<&[u8]> {
+        match &self.inner.source {
+            ModuleCodeSource::Binary(b) => Some(b),
+            _ => None,
+        }
+    }
+
+    /// 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
+    }
 }

crates/api/src/runtime.rs

@@ -331,6 +331,7 @@ impl Engine {
 /// ocnfiguration (see [`Config`] for more information).
 #[derive(Clone)]
 pub struct Store {
+    // FIXME(#777) should be `Arc` and this type should be thread-safe
     inner: Rc<StoreInner>,
 }
 

crates/api/src/wasm.rs

@@ -662,7 +662,7 @@ pub unsafe extern "C" fn wasm_instance_new(
         let import = *imports.add(i);
         externs.push((*import).ext.clone());
     }
-    let module = &(*module).module;
+    let module = &(*module).module.borrow();
     match Instance::new(store, module, &externs) {
         Ok(instance) => {
             let instance = Box::new(wasm_instance_t {

crates/api/tests/import_calling_export.rs

@@ -34,7 +34,7 @@ fn test_import_calling_export() {
 
     let store = Store::default();
     let wasm = wat::parse_str(WAT).unwrap();
-    let module = HostRef::new(Module::new(&store, &wasm).expect("failed to create module"));
+    let module = Module::new(&store, &wasm).expect("failed to create module");
 
     let callback = Rc::new(Callback {
         other: RefCell::new(None),

crates/api/tests/import_calling_export.wat

@@ -1,8 +0,0 @@
-(module
-  (type $t0 (func))
-  (import "" "imp" (func $.imp (type $t0)))
-  (func $run call $.imp)
-  (func $other)
-  (export "run" (func $run))
-  (export "other" (func $other))
-)

crates/api/tests/traps.rs

@@ -23,9 +23,8 @@ fn test_trap_return() -> Result<(), String> {
     )
     .map_err(|e| format!("failed to parse WebAssembly text source: {}", e))?;
 
-    let module = HostRef::new(
+    let module =
-        Module::new(&store, &binary).map_err(|e| format!("failed to compile module: {}", e))?,
+        Module::new(&store, &binary).map_err(|e| format!("failed to compile module: {}", e))?;
-    );
     let hello_type = FuncType::new(Box::new([]), Box::new([]));
     let hello_func = HostRef::new(Func::new(&store, hello_type, Rc::new(HelloCallback)));
 

crates/fuzzing/src/oracles.rs

@@ -44,12 +44,9 @@ pub fn instantiate(wasm: &[u8], strategy: Strategy) {
     let engine = Engine::new(&config);
     let store = Store::new(&engine);
 
-    let module =
+    let module = Module::new(&store, wasm).expect("Failed to compile a valid Wasm module!");
-        HostRef::new(Module::new(&store, wasm).expect("Failed to compile a valid Wasm module!"));
 
-    let imports = {
+    let imports = match dummy_imports(&store, module.imports()) {
-        let module = module.borrow();
-        match dummy_imports(&store, module.imports()) {
         Ok(imps) => imps,
         Err(_) => {
             // There are some value types that we can't synthesize a
@@ -57,7 +54,6 @@ pub fn instantiate(wasm: &[u8], strategy: Strategy) {
             // import things of these types we skip instantiation.
             return;
         }
-        }
     };
 
     // Don't unwrap this: there can be instantiation-/link-time errors that
@@ -92,7 +88,7 @@ pub fn make_api_calls(api: crate::generators::api::ApiCalls) {
     let mut config: Option<Config> = None;
     let mut engine: Option<Engine> = None;
     let mut store: Option<Store> = None;
-    let mut modules: HashMap<usize, HostRef<Module>> = Default::default();
+    let mut modules: HashMap<usize, Module> = Default::default();
     let mut instances: HashMap<usize, HostRef<Instance>> = Default::default();
 
     for call in api.calls {
@@ -117,10 +113,10 @@ pub fn make_api_calls(api: crate::generators::api::ApiCalls) {
             }
 
             ApiCall::ModuleNew { id, wasm } => {
-                let module = HostRef::new(match Module::new(store.as_ref().unwrap(), &wasm.wasm) {
+                let module = match Module::new(store.as_ref().unwrap(), &wasm.wasm) {
                     Ok(m) => m,
                     Err(_) => continue,
-                });
+                };
                 let old = modules.insert(id, module);
                 assert!(old.is_none());
             }
@@ -135,9 +131,7 @@ pub fn make_api_calls(api: crate::generators::api::ApiCalls) {
                     None => continue,
                 };
 
-                let imports = {
+                let imports = match dummy_imports(store.as_ref().unwrap(), module.imports()) {
-                    let module = module.borrow();
-                    match dummy_imports(store.as_ref().unwrap(), module.imports()) {
                     Ok(imps) => imps,
                     Err(_) => {
                         // There are some value types that we can't synthesize a
@@ -145,7 +139,6 @@ pub fn make_api_calls(api: crate::generators::api::ApiCalls) {
                         // import things of these types we skip instantiation.
                         continue;
                     }
-                    }
                 };
 
                 // Don't unwrap this: there can be instantiation-/link-time errors that

crates/misc/py/src/instance.rs

@@ -21,7 +21,7 @@ impl Instance {
         let py = gil.python();
         let exports = PyDict::new(py);
         let module = self.instance.borrow().module().clone();
-        for (i, e) in module.borrow().exports().iter().enumerate() {
+        for (i, e) in module.exports().iter().enumerate() {
             match e.ty() {
                 wasmtime::ExternType::Func(ft) => {
                     let mut args_types = Vec::new();

crates/misc/py/src/lib.rs

@@ -84,7 +84,7 @@ pub fn instantiate(
     let engine = wasmtime::Engine::new(&wasmtime::Config::new().wasm_multi_value(true));
     let store = wasmtime::Store::new(&engine);
 
-    let module = wasmtime::HostRef::new(wasmtime::Module::new(&store, wasm_data).map_err(err2py)?);
+    let module = wasmtime::Module::new(&store, wasm_data).map_err(err2py)?;
 
     let data = Rc::new(ModuleData::new(wasm_data).map_err(err2py)?);
 
@@ -99,7 +99,7 @@ pub fn instantiate(
     };
 
     let mut imports: Vec<wasmtime::Extern> = Vec::new();
-    for i in module.borrow().imports() {
+    for i in module.imports() {
         let module_name = i.module();
         if let Some(m) = import_obj.get_item(module_name) {
             let e = find_export_in(m, &store, i.name())?;

crates/misc/py/src/module.rs

@@ -4,5 +4,5 @@ use pyo3::prelude::*;
 
 #[pyclass]
 pub struct Module {
-    pub module: wasmtime::HostRef<wasmtime::Module>,
+    pub module: wasmtime::Module,
 }

crates/misc/rust/macro/src/lib.rs

@@ -57,13 +57,13 @@ fn generate_load(item: &syn::ItemTrait) -> syn::Result<TokenStream> {
 
             let data = #root::wasmtime_interface_types::ModuleData::new(&bytes)?;
 
-            let module = HostRef::new(Module::new(&store, &bytes)?);
+            let module = Module::new(&store, &bytes)?;
 
             let mut imports: Vec<Extern> = Vec::new();
             if let Some(module_name) = data.find_wasi_module_name() {
                 let wasi_instance = #root::wasmtime_wasi::create_wasi_instance(&store, &[], &[], &[])
                     .map_err(|e| format_err!("wasm instantiation error: {:?}", e))?;
-                for i in module.borrow().imports().iter() {
+                for i in module.imports().iter() {
                     if i.module() != module_name {
                         bail!("unknown import module {}", i.module());
                     }

crates/test-programs/tests/wasm_tests/runtime.rs

@@ -43,9 +43,8 @@ pub fn instantiate(data: &[u8], bin_name: &str, workspace: Option<&Path>) -> any
         .context("failed to instantiate wasi")?,
     );
 
-    let module = HostRef::new(Module::new(&store, &data).context("failed to create wasm module")?);
+    let module = Module::new(&store, &data).context("failed to create wasm module")?;
     let imports = module
-        .borrow()
         .imports()
         .iter()
         .map(|i| {

crates/wast/src/wast.rs

@@ -65,9 +65,9 @@ impl WastContext {
     }
 
     fn instantiate(&self, module: &[u8]) -> Result<Outcome<HostRef<Instance>>> {
-        let module = HostRef::new(Module::new(&self.store, module)?);
+        let module = Module::new(&self.store, module)?;
         let mut imports = Vec::new();
-        for import in module.borrow().imports() {
+        for import in module.imports() {
             if import.module() == "spectest" {
                 let spectest = self
                     .spectest

src/commands/run.rs

@@ -234,15 +234,14 @@ impl RunCommand {
         store: &Store,
         module_registry: &HashMap<String, HostRef<Instance>>,
         path: &Path,
-    ) -> Result<(HostRef<Instance>, HostRef<Module>, Vec<u8>)> {
+    ) -> Result<(HostRef<Instance>, Module, Vec<u8>)> {
         // Read the wasm module binary either as `*.wat` or a raw binary
         let data = wat::parse_file(path)?;
 
-        let module = HostRef::new(Module::new(store, &data)?);
+        let module = Module::new(store, &data)?;
 
         // Resolve import using module_registry.
         let imports = module
-            .borrow()
             .imports()
             .iter()
             .map(|i| {
@@ -285,7 +284,6 @@ impl RunCommand {
             let data = ModuleData::new(&data)?;
             self.invoke_export(instance, &data, name)?;
         } else if module
-            .borrow()
             .exports()
             .iter()
             .any(|export| export.name().is_empty())