From 4c9f90e89dc45fa3b0820699b81905e37221370e Mon Sep 17 00:00:00 2001 From: Nick Fitzgerald Date: Wed, 21 Jul 2021 09:10:09 -0700 Subject: [PATCH 1/3] Document why `get_export` requires a mutable context --- crates/wasmtime/src/instance.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/crates/wasmtime/src/instance.rs b/crates/wasmtime/src/instance.rs index 2450779eb8..81ebf05d0c 100644 --- a/crates/wasmtime/src/instance.rs +++ b/crates/wasmtime/src/instance.rs @@ -299,6 +299,23 @@ impl Instance { /// # Panics /// /// Panics if `store` does not own this instance. + /// + /// # Why does `get_export` take a mutable context? + /// + /// This method requires a mutable context because an instance's exports are + /// lazily populated. While `wasmtime` could use interior mutability to + /// paper over this, since getting an export is "logically" a non-mutating + /// operation, this would bring with it a new problem. `wasmtime` would have + /// to choose whether to use a mutex or `RefCell` for the interior + /// mutability; the former implies unnecessasry overhead for single-threaded + /// usage, while the latter would prohibit `Store` from implementing `Send` + /// and `Sync`, making multi-threaded usage impossible. + /// + /// Given these trade offs -- and because `wasmtime` will not stop lazily + /// populating exports, because this makes instantiation faster -- we + /// decided that the best option is to avoid interior mutability and expose + /// what's happening under the covers to users via requiring a mutable + /// context. pub fn get_export(&self, mut store: impl AsContextMut, name: &str) -> Option { self._get_export(&mut store.as_context_mut().opaque(), name) } From 943d027757d44e2386f9357882519df438300066 Mon Sep 17 00:00:00 2001 From: Nick Fitzgerald Date: Wed, 21 Jul 2021 09:28:36 -0700 Subject: [PATCH 2/3] Document reborrowing issues for `AsContextMut` and workarounds --- crates/wasmtime/src/store/context.rs | 43 ++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/crates/wasmtime/src/store/context.rs b/crates/wasmtime/src/store/context.rs index a90ba317ca..caf89ee18d 100644 --- a/crates/wasmtime/src/store/context.rs +++ b/crates/wasmtime/src/store/context.rs @@ -94,6 +94,49 @@ pub trait AsContext { /// memory or globals. Creation of a [`Func`](crate::Func) will update internal /// data structures. This ends up being quite a common bound in Wasmtime, but /// typically you can simply pass `&mut store` or `&mut caller` to satisfy it. +/// +/// # Calling multiple methods that take `&mut impl AsContextMut` +/// +/// As of Rust 1.53.0, [generic methods that take a generic `&mut T` do not get +/// "automatic reborrowing"][reborrowing] and therefore you cannot call multiple +/// generic methods with the same `&mut T` without manually inserting +/// reborrows. This affects the many `wasmtime` API methods that take `&mut impl +/// AsContextMut`. +/// +/// For example, this fails to compile because the context is moved into the +/// first call: +/// +/// ```compile_fail +/// use wasmtime::{AsContextMut, Instance}; +/// +/// fn foo(cx: &mut impl AsContextMut, instance: Instance) { +/// // `cx` is not reborrowed, but moved into this call. +/// let my_export = instance.get_export(cx, "my_export"); +/// +/// // Therefore, this use of `cx` is a use-after-move and prohibited by the +/// // borrow checker. +/// let other_export = instance.get_export(cx, "other_export"); +/// # drop((my_export, other_export)); +/// } +/// ``` +/// +/// To fix this, manually insert reborrows like `&mut *cx` that would otherwise +/// normally be inserted automatically by the Rust compiler for non-generic +/// methods: +/// +/// ``` +/// use wasmtime::{AsContextMut, Instance}; +/// +/// fn foo(cx: &mut impl AsContextMut, instance: Instance) { +/// let my_export = instance.get_export(&mut *cx, "my_export"); +/// +/// // This works now, since `cx` was reborrowed above, rather than moved! +/// let other_export = instance.get_export(&mut *cx, "other_export"); +/// # drop((my_export, other_export)); +/// } +/// ``` +/// +/// [reborrowing]: https://github.com/rust-lang/rust/issues/85161 pub trait AsContextMut: AsContext { /// Returns the store context that this type provides access to. fn as_context_mut(&mut self) -> StoreContextMut<'_, Self::Data>; From f136f730335266c3ff6386525acd1609ad2c327e Mon Sep 17 00:00:00 2001 From: Nick Fitzgerald Date: Wed, 21 Jul 2021 11:25:49 -0700 Subject: [PATCH 3/3] Reword `get_export` mutable context docs to be more user-facing --- crates/wasmtime/src/instance.rs | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/crates/wasmtime/src/instance.rs b/crates/wasmtime/src/instance.rs index 81ebf05d0c..bd3e6ebee6 100644 --- a/crates/wasmtime/src/instance.rs +++ b/crates/wasmtime/src/instance.rs @@ -303,19 +303,9 @@ impl Instance { /// # Why does `get_export` take a mutable context? /// /// This method requires a mutable context because an instance's exports are - /// lazily populated. While `wasmtime` could use interior mutability to - /// paper over this, since getting an export is "logically" a non-mutating - /// operation, this would bring with it a new problem. `wasmtime` would have - /// to choose whether to use a mutex or `RefCell` for the interior - /// mutability; the former implies unnecessasry overhead for single-threaded - /// usage, while the latter would prohibit `Store` from implementing `Send` - /// and `Sync`, making multi-threaded usage impossible. - /// - /// Given these trade offs -- and because `wasmtime` will not stop lazily - /// populating exports, because this makes instantiation faster -- we - /// decided that the best option is to avoid interior mutability and expose - /// what's happening under the covers to users via requiring a mutable - /// context. + /// lazily populated, and we cache them as they are accessed. This makes + /// instantiating a module faster, but also means this method requires a + /// mutable context. pub fn get_export(&self, mut store: impl AsContextMut, name: &str) -> Option { self._get_export(&mut store.as_context_mut().opaque(), name) }