Address reviewer comments

Author: jhugman

uniffi_core/src/ffi/rustfuture/future.rs

@@ -84,14 +84,14 @@ use std::{
     task::{Context, Poll, Wake},
 };
 
-use super::{FutureResult, RustFutureContinuationCallback, RustFuturePoll, Scheduler};
+use super::{RustFutureContinuationCallback, RustFuturePoll, Scheduler, UniffiCompatibleFuture};
 use crate::{rust_call_with_out_status, FfiDefault, LiftArgsError, LowerReturn, RustCallStatus};
 
 /// Wraps the actual future we're polling
 struct WrappedFuture<F, T, UT>
 where
     // See rust_future_new for an explanation of these trait bounds
-    F: FutureResult<T, LiftArgsError>,
+    F: UniffiCompatibleFuture<Result<T, LiftArgsError>> + 'static,
     T: LowerReturn<UT> + Send + 'static,
     UT: Send + 'static,
 {
@@ -105,7 +105,7 @@ where
 impl<F, T, UT> WrappedFuture<F, T, UT>
 where
     // See rust_future_new for an explanation of these trait bounds
-    F: FutureResult<T, LiftArgsError>,
+    F: UniffiCompatibleFuture<Result<T, LiftArgsError>> + 'static,
     T: LowerReturn<UT> + Send + 'static,
     UT: Send + 'static,
 {
@@ -185,7 +185,7 @@ where
 unsafe impl<F, T, UT> Send for WrappedFuture<F, T, UT>
 where
     // See rust_future_new for an explanation of these trait bounds
-    F: FutureResult<T, LiftArgsError>,
+    F: UniffiCompatibleFuture<Result<T, LiftArgsError>> + 'static,
     T: LowerReturn<UT> + Send + 'static,
     UT: Send + 'static,
 {
@@ -195,7 +195,7 @@ where
 pub(super) struct RustFuture<F, T, UT>
 where
     // See rust_future_new for an explanation of these trait bounds
-    F: FutureResult<T, LiftArgsError>,
+    F: UniffiCompatibleFuture<Result<T, LiftArgsError>> + 'static,
     T: LowerReturn<UT> + Send + 'static,
     UT: Send + 'static,
 {
@@ -211,7 +211,7 @@ where
 impl<F, T, UT> RustFuture<F, T, UT>
 where
     // See rust_future_new for an explanation of these trait bounds
-    F: FutureResult<T, LiftArgsError>,
+    F: UniffiCompatibleFuture<Result<T, LiftArgsError>> + 'static,
     T: LowerReturn<UT> + Send + 'static,
     UT: Send + 'static,
 {
@@ -266,7 +266,7 @@ where
 impl<F, T, UT> Wake for RustFuture<F, T, UT>
 where
     // See rust_future_new for an explanation of these trait bounds
-    F: FutureResult<T, LiftArgsError>,
+    F: UniffiCompatibleFuture<Result<T, LiftArgsError>> + 'static,
     T: LowerReturn<UT> + Send + 'static,
     UT: Send + 'static,
 {
@@ -301,7 +301,7 @@ pub trait RustFutureFfi<ReturnType>: Send + Sync {
 impl<F, T, UT> RustFutureFfi<T::ReturnType> for RustFuture<F, T, UT>
 where
     // See rust_future_new for an explanation of these trait bounds
-    F: FutureResult<T, LiftArgsError>,
+    F: UniffiCompatibleFuture<Result<T, LiftArgsError>> + 'static,
     T: LowerReturn<UT> + Send + 'static,
     UT: Send + 'static,
 {

uniffi_core/src/ffi/rustfuture/mod.rs

@@ -30,16 +30,61 @@ pub enum RustFuturePoll {
 /// to continue progress on the future.
 pub type RustFutureContinuationCallback = extern "C" fn(callback_data: u64, RustFuturePoll);
 
-/// This marker trait allows us to put different bounds on the `Future`s we support, based on
-/// `#[cfg()]` configuration.
-pub trait FutureResult<T, E>: Future<Output = Result<T, E>> + 'static {}
+/// This marker trait allows us to put different bounds on the `Future`s we
+/// support, based on `#[cfg(..)]` configuration.
+///
+/// It should not be considered as a part of the public API, and as such as
+/// an implementation detail and subject to change.
+///
+/// It is _not_ intended to be implemented by libray users or bindings
+/// implementors.
+#[doc(hidden)]
+pub trait UniffiCompatibleFuture<T>: Future<Output = T> {}
 
+/// The `Send` bound is required because the Foreign code may call the
+/// `rust_future_*` methods from different threads.
 #[cfg(not(target_arch = "wasm32"))]
-impl<T, F, E> FutureResult<T, E> for F where F: Future<Output = Result<T, E>> + Send + 'static {}
+impl<T, F> UniffiCompatibleFuture<T> for F where F: Future<Output = T> + Send {}
 
-// `Promise`s cannot be sent to or from WebWorkers, but `JsValue` is not `Send`.
+/// `Future`'s on WASM32 are not `Send` because it's a single threaded environment.
+///
+/// # Safety:
+///
+/// WASM32 is a single threaded environment. However, in a browser there do
+/// exist [`WebWorker`][webworker]s which do not share memory or event-loop
+/// with the main browser context.
+///
+/// Communication between contexts is only possible by message passing,
+/// using a small number of ['transferable' object types][transferable].
+///
+/// The most common source of asynchrony in Rust compiled to WASM is
+/// [wasm-bindgen's `JsFuture`][jsfuture]. It is not `Send` because:
+///
+/// 1. `T` and `E` are both `JsValue`
+/// 2. `JsValue` may contain `JsFunction`s, either as a function themselves or
+///    an object containing functions.
+/// 3. Functions cannot be [serialized and sent][transferable] to `WebWorker`s.
+///
+/// Implementors of binding generators should be able to enumerate the
+/// combinations of Rust or JS communicating across different contexts (here
+/// using: <->), and in the same context (+) to account for why it is safe
+/// for UniFFI to support `Future`s that are not `Send`:
+///
+/// 1. JS + Rust in the same contexts: polling and waking happens in the same
+///    thread, no `Send` is needed.
+/// 2. Rust <-> Rust in different contexts: Futures cannot be sent between JS
+///    contexts within the same Rust crate (because they are not `Send`).
+/// 3. JS <-> Rust in different contexts: the `Promise` are [not transferable
+///    between contexts][transferable], so this is impossible.
+/// 4. JS <-> JS + Rust, this is possible, but safe since the Future is being
+///    driven by JS in the same thread. If a Promise metaphor is desired, then
+///    this must be built with JS talking to JS, because 3.
+///
+/// [webworker]: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Using_web_workers
+/// [jsfuture]: https://github.com/rustwasm/wasm-bindgen/blob/main/crates/futures/src/lib.rs
+/// [transferable]: (https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Transferable_objects
 #[cfg(target_arch = "wasm32")]
-impl<T, F, E> FutureResult<T, E> for F where F: Future<Output = Result<T, E>> + 'static {}
+impl<T, F> UniffiCompatibleFuture<T> for F where F: Future<Output = T> {}
 
 // === Public FFI API ===
 
@@ -55,7 +100,7 @@ where
     // since it will move between threads for an indeterminate amount of time as the foreign
     // executor calls polls it and the Rust executor wakes it.  It does not need to by `Sync`,
     // since we synchronize all access to the values.
-    F: FutureResult<T, LiftArgsError>,
+    F: UniffiCompatibleFuture<Result<T, LiftArgsError>> + 'static,
     // T is the output of the Future.  It needs to implement [LowerReturn].  Also it must be Send +
     // 'static for the same reason as F.
     T: LowerReturn<UT> + Send + 'static,