Review feedback changes.

* Remove `&mut` from `insert_context` and `remove_context`.
* Replace `IntoEngineFunc` with `IntoFunc`.
* Add `wrap_function` to `Engine` a la `Func::wrap`.
* Rename `set_context` to `insert_context`.
* Remove unnecessary `<T>` from `Wasi::add_to_engine`.

accepted/engine-host-functions.md

@@ -5,32 +5,40 @@ This RFC proposes adding support in the Wasmtime API for defining host functions
 # Motivation
 [motivation]: #motivation
 
-Wasmtime is currently well-suited for long-lived module instantiations, where a singular `Instance` is created for a WebAssembly program and the program is run to completion.
+Wasmtime is currently well-suited for long-lived module instantiations, where a singular `Instance` is created for a 
+WebAssembly program and the program is run to completion.
 
-In this model, the time spent defining host functions for the instance is negligible because the setup is only performed once and the instance is, potentially, expected to run for a long time.
+In this model, the time spent defining host functions for the instance is negligible because the setup is only performed once 
+and the instance is, potentially, expected to run for a long time.
 
-However, this model is not ideal for services that create short-lived instantiations for each service request, where the time spent creating host functions to import at instantiation-time can be considerable for the repeated creation of many instances.
+However, this model is not ideal for services that create short-lived instantiations for each service request, where the time 
+spent creating host functions to import at instantiation-time can be considerable for the repeated creation of many instances.
 
-As an instance is expected to live only for the duration of the request, each instance would be associated with its own `Store` that is dropped when the request completes.
+As an instance is expected to live only for the duration of the request, each instance would be associated with its own `Store` 
+that is dropped when the request completes.
 
-Given the nature of `Store` isolation in Wasmtime, this means host function definitions must be recreated for each and every request handled by the service.
+Given the nature of `Store` isolation in Wasmtime, this means host function definitions must be recreated for each and every 
+request handled by the service.
 
-If instead host functions could be defined for an `Engine` (in addition to `Store`), then the repeated effort to define the host functions would be eliminated and the time it takes to create an instance to handle a request would therefore be noticeably reduced.
+If instead host functions could be defined for an `Engine` (in addition to `Store`), then the repeated effort to define the 
+host functions would be eliminated and the time it takes to create an instance to handle a request would therefore be 
+noticeably reduced.
 
 # Proposal sketch
 [proposal]: #proposal
 
 As `Func` is inherently tied to a `Store`, it cannot be used to represent a function associated with an `Engine`.
 
-This RFC proposes not having an analogous type to represent these functions; instead, users will define the function via a method on `Engine`, but use `Store` (or `Linker`) to get the `Func` representing the function.
+This RFC proposes not having an analogous type to represent these functions; instead, users will define the function via a 
+method on `Engine`, but use `Store` (or `Linker`) to get the `Func` representing the function.
 
 ## Overview example
 
 A simple example that demonstrates defining a host function in an `Engine`:
 
 ```rust
 let engine = Engine::default();
-engine.define_function("", "hello", |caller: Caller<'_>, ptr: i32, len: i32| {
+engine.wrap_function("", "hello", |caller: Caller<'_>, ptr: i32, len: i32| {
     let mem = caller.get_export("memory").unwrap().into_memory().unwrap();
     println!(
         "Hello {}!",
@@ -73,27 +81,59 @@ run()?;
 
 ## Changes to `wasmtime::Engine`
 
-This RFC proposes a `define_function` method be added to `Engine`:
+This RFC proposes that the following methods be added to `Engine`:
 
 ```rust
-/// Defines a function for the `Engine`.
+/// Defines a function for the `Engine` for the given callback.
 ///
 /// Use `Store::get_engine_function` to get a `Func` representing the function.
 ///
-/// A `Linker` will automatically default to functions on the associated `Engine`
-/// when resolving a function import that is not defined locally to the linker.
-pub fn define_function<Params, Results>(&self, module: &str, name: &str, func: impl IntoEngineFunc<Params, Results>) -> Result<()>;
+/// Note that the implementation of `func` must adhere to the `ty`
+/// signature given, error or traps may occur if it does not respect the
+/// `ty` signature.
+///
+/// Additionally note that this is quite a dynamic function since signatures
+/// are not statically known. For performance reasons, it's recommended
+/// to use [`Engine::wrap_function`] if you can because with statically known
+/// signatures the engine can optimize the implementation much more.
+pub fn define_function(
+    &self,
+    module: &str,
+    name: &str,
+    ty: FuncType,
+    func: impl Fn(Caller<'_>, &[Val], &mut [Val]) -> Result<(), Trap> + 'static,
+) -> Result<()>;
+
+/// Defines a function for the `Engine` from the given Rust closure.
+///
+/// Use `Store::get_engine_function` to get a `Func` representing the function.
+///
+/// See [`Func::wrap`] for information about accepted parameter and result types for the closure.
+pub fn wrap_function<Params, Results>(
+    &self,
+    module: &str,
+    name: &str,
+    func: impl IntoFunc<Params, Results>,
+) -> Result<()>;
 ```
 
-Similar to `Func::wrap`, `define_function` will generically accept different `Fn` signatures to determine the WebAssembly type of the function. Instead of using `IntoFunc`, a different `IntoEngineFunc` trait will be used (see related section below).
+Similar to `Func::new`, `define_function` will define an engine function that can be used for any Wasm function type.
+
+Similar to `Func::wrap`, `wrap_function` will generically accept different `Fn` signatures to determine the WebAssembly type of 
+the function.
 
-This method will internally create an `InstanceHandle` to represent the function.
+These methods will internally create an `InstanceHandle` to represent the host function.
 
-However, it will not associate the instance handle with a store; instead, the engine will own the associated instance handles and deallocate them when required (e.g. the `Engine` drops or an existing function definition is redefined).
+However, the instance handle will not owned by a store; instead, the engine will own the associated instance handles and 
+deallocate them when required (e.g. the `Engine` drops or an existing function definition is redefined).
+
+Note: the `IntoFunc` trait is documented as internal to Wasmtime and will need to be extended to implement this feature;
+therefore this will not be considered a breaking change.
 
 ## Changes to `wasmtime::Store`
 
-To use `Engine` defined functions as imports for module instantiation or as `funcref` values, a `Func` representation is required.
+To use `Engine` defined functions as imports for module instantiation or as `funcref` values, a `Func` representation is 
+required.
 
 This proposal calls for adding the following methods to `Store`:
 
@@ -108,36 +148,33 @@ pub fn get_engine_function(&self, module: &str, name: &str) -> Option<Func>;
 /// Returns `None` if the context value of the given type does not exist.
 pub fn get_context<T: Any>(&self) -> Option<&T>;
 
-/// Sets a context value.
+/// Inserts a context value.
 ///
 /// Returns the previous context value of the same type if present.
-pub fn set_context<T: Any>(&mut self, value: T) -> Option<T>;
+pub fn insert_context<T: Any>(&self, value: T) -> Option<T>;
 
 /// Removes a context value.
 ///
 /// Returns `None` if a context value of the given type does not exist.
-pub fn remove_context<T: Any>(&mut self) -> Option<T>;
+pub fn remove_context<T: Any>(&self) -> Option<T>;
 ```
 
-`get_engine_function`  will register the function's instance handle with the `Store`, but as a *borrowed* handle which it is not responsible for deallocation. As `Store` keeps a reference on its associated `Engine`, this ownership model should be sufficient to prevent `funcref` values or imports of engine functions from outliving the function instance itself.
+`get_engine_function`  will register the function's instance handle with the `Store`, but as a *borrowed* handle which it is 
+not responsible for deallocation. As `Store` keeps a reference on its associated `Engine`, this ownership model should be 
+sufficient to prevent `funcref` values or imports of engine functions from outliving the function instance itself.
 
-For host functions that require contextual data (e.g. WASI), the `set_context` method will be used for associating context with a `Store` and it can later be retrieved via `caller.store().get_context()`.
+For host functions that require contextual data (e.g. WASI), the `insert_context` method will be used for associating context with 
+a `Store` and it can later be retrieved via `caller.store().get_context()`.
 
 ## Changes to `wasmtime::Linker`
 
 There will be no changes to the API surface of `Linker` to support the changes proposed in this RFC.
 
-However, the `Linker` implementation will be changed such that it will fallback to calling `Store::get_engine_function` when resolving imports for `Linker::instantiate` and `Linker::module`.
-
-This should allow for overriding engine function imports in the `Linker` if an import of the same name has already been defined in the `Engine`.
-
-## The `wasmtime::IntoEngineFunc` trait
-
-Much like `IntoFunc` that is used to easily define functions using `Func::wrap`, an `IntoEngineFunc` trait will be implemented for `Fn([Caller], P1, P2...) -> R` for up to sixteen parameters that implement `WasmTy` and the return type implements `WasmRet`.
-
-`IntoEngineFunc` will also be implemented for ``Fn(Caller, &[Val], &mut [Val]) -> Result<Trap> + `static`` so that any number of parameters and return values can be supported for engine functions.
+However, the `Linker` implementation will be changed such that it will fallback to calling `Store::get_engine_function` when 
+resolving imports for `Linker::instantiate` and `Linker::module`.
 
-This trait, like `IntoFunc`, is intended as an internal implementation detail and will not be documented.
+This should allow for overriding engine function imports in the `Linker` if an import of the same name has already been defined 
+in the `Engine`.
 
 ## Changes to `wasmtime_wasi::Wasi`
 
@@ -147,13 +184,13 @@ This proposal adds the following methods to the generated `Wasi` type:
 
 ```rust
 /// Adds WASI functions to the given `Engine`.
-pub fn add_to_engine<T>(engine: &Engine) -> Result<()>;
+pub fn add_to_engine(engine: &Engine) -> Result<()>;
 
 /// Sets the given `WasiCxt` for the `Store`.
 ///
 /// This will replace any existing `WasiCtx` associated with the `Store`.
 ///
-/// The old `WasiCtx`, if any, is returned.
+/// The previously set `WasiCtx`, if any, is returned.
 pub fn set_context(store: &Store, context: WasiCtx) -> Option<WasiCtx>;
 ```
 
@@ -210,4 +247,5 @@ run()?;
 
 * Should this be exposed via Wasmtime-specific C API functions?
 
-* The two different places host functions can be implemented (`Engine::define_function` and `Func`) might be confusing to users. What guidance should be provided?
+* The two different places host functions can be implemented (`Engine` and `Func`) might be confusing to users. What guidance 
+  should be provided?