More unsoundness in `future_sync` and `future_desync`

[Logicalshift]

From PR #7:

Actually, there's more soundness bugs with the signatures of the future_[de]sync() methods. To illustrate:

use desync::Desync;
use futures::executor;

fn leak_local<T: Send>(v: T) -> &'static mut T {
    let desync = Desync::new(v);
    let future = desync.future_sync(|v| async { v });
    executor::block_on(future).unwrap()
}

This was introduced by the change in v0.8 that removed the need to box the future, I think. The lifetime of the borrow is insufficiently restricted without the higher-order trait bound imposed on the function.

[Logicalshift]

future_sync() and future_desync() had a function with the signature for<'b> FnOnce(&'b mut T) -> BoxFuture<'b, TOutput> which prevented this in versions prior to v0.8.

Always having to box the future was a pain, so I wanted to eliminate that and also allow for this kind of code:

let mut foo = true;
bar.future_sync(|x| async { if x.test() { foo = false; } }).await;
if foo { /* ... */ }

which is possible with sync(). I think I can add my own trait to replace BoxFuture to remove the need to call .boxed() all the time, but I think Rust's implementation of higher order trait bounds means there's no way to fully describe the lifetime of the future, so the example above won't work any more (need to be able to write for<'b: 'a> FnOnce(&'b mut T) -> SomeFuture<'b>)

[Logicalshift]

I believe the fix to remove the need to use a boxed future is to add a trait like this:

pub trait IntoBoxFuture<'a, 'b, TBorrow, TOutput> {
    fn make_box(self, val: &'a mut TBorrow) -> BoxFuture<'b, TOutput>;
}

impl<'a, TFn, TBorrow, TFuture> IntoBoxFuture<'a, TBorrow, TFuture::Output> for TFn
where
    TFn: FnOnce(&'a mut TBorrow) -> TFuture,
    TFuture: 'a + Send + Future,
    TBorrow: 'a,
{
    fn make_box(self, val: &'a mut TBorrow) -> BoxFuture<'a, TFuture::Output> {
        (self)(val).boxed()
    }
}

Then change the signature of future_sync() and future_desync() as follows:

    pub fn future_sync<'a, TFn, TOutput>(&'a self, job: TFn) -> impl 'a + Future<Output=Result<TOutput, oneshot::Canceled>> + Send
    where
        TFn:     'a + Send + for<'b> IntoBoxFuture<'b, T, TOutput>,
        TOutput: 'a + Send,

and then make sure the function is called with a lifetime where the data is safe to borrow.

I think this works perfectly for future_desync() where the returned future has a static lifetime but I'm not sure that it's possible to restrict the lifetime of the generated future enough to support the case where future_sync() borrows values from outside.

[Logicalshift]

ecfaaad fixes this by going back to using BoxFuture. I'm going to leave this open for the moment as I'd quite like to figure out a way to get the method outlined above to work.

The problem is that Rust isn't quite smart enough to infer it needs to create a for<'a> FnOnce instead of just a FnOnce when calling a function using the trait so it produces a slightly confusing error.

[LegionMammal978]

There's no way to write this so that it works effortlessly with closures on the caller's end. There's only three options:

1. The caller must write their closure to return a BoxFuture (or similar).
2. The caller must wrap their closure in a macro that puts it into a funnel to constrain the type (similar to higher-order-closure).
3. The caller must use an async fn instead of a closure returning a Future.

The last two options require helper traits to write the bounds.

[cole-miller]

It's worth noting that syntax for explicitly higher-order closures is available on nightly behind the closure_lifetime_binder feature gate (RFC 3216):

rust-lang/rust#97362

[Logicalshift]

Ah, thanks for letting me know: I want to keep everything working with stable so this probably won't make it in until it's ready, but this definitely looks like something that will help with this problem (it's a fairly minor annoyance but always 'feels' wrong when it's a problem)