Remove static and Send requirement from get_or_{,try_}insert_with

[tinou98]

It seems that the 'static requirement isn't required, as the future F will not outlive this function.

(Tested with stable, nightly and 1.46)

This is the same case for the Send requirement, your function don't need to ask for the future to be Send. If the future is Send, the future returned by get_or_insert_with will be Send, otherwise it won't. I didn't add this change because it's not much of a constraint, as we usually work with Send futures, but I can add it to this PR as well if you want.

[tatsuya6502]

Thank you for the pull request!

I have got a question for you. I have four tests cases here and I expect them to get compile errors. With your changes, two of them no longer get the compile errors (so these tests are failing).

Can you please review these failing tests and tell me why they should compile? (I am now thinking maybe you are right and these tests are wrong, but I want to make sure)

You can run them by the following command:

RUSTFLAGS='--cfg trybuild' cargo test --features future

And result with your change:

test tests/ui/future/get_with_non_send_future.rs ... ok
test tests/ui/future/get_with_non_static_future.rs ... error
Expected test case to fail to compile, but it succeeded.

test tests/ui/future/try_get_with_non_send_future.rs ... ok
test tests/ui/future/try_get_with_non_static_future.rs ... error
Expected test case to fail to compile, but it succeeded.

test tests::ui_trybuild_future ... FAILED

Thanks!

[tatsuya6502]

Also can you please rebase this PR (your branch) to latest master? I just pushed a commit 672a557 to disable warnings from Clippy shipped with Rust beta 1.58.

[tinou98]

You don't need the 'static requirement because the provided future will not outlive the one returned by the function

let out_future = c.get_or_insert_with("key", provided_future);
let out_value = out_future.await;

If we do need to insert the key, the insertion will be done during the out_future.await.
If we don't need to insert the key, the future is dropped and is never executed.

In both case, the inner future will not outlive the context from which we use the future, and all reference are still valid. If you want to think about lifetime, the returned future will have a bound on the shortes

The last argument in favour of the removal is unsafe. As long as you don't use unsafe, you can't create UB.

---

The reason for why we could remove send are entirely different, it's just that rust automatically transfer te Sendable property : if the inner future is Send, the returned future will be.

---

Minimal code example

use std::{future::Future, rc::Rc, sync::Arc};

const EXTERNAL_CONDITION: bool = true;

pub async fn get_or_insert_with<F: Future>(init: F) -> F::Output {
    /* Other conditions, function call, but in the end, we either await or drop the future */
    if EXTERNAL_CONDITION {
        init.await
    } else {
        todo!("Retreive stored value ...")
    }
}

fn assert_future<F: Future>(_: &F) {}
fn assert_future_send<F: Future + Send>(_: &F) {}

fn main() {
    let arc = Arc::new(5);
    let rc = Rc::new(15);

    let out_arc_future = get_or_insert_with(async { arc });
    assert_future(&out_arc_future);
    assert_future_send(&out_arc_future);

    let out_rc_future = get_or_insert_with(async { rc });
    assert_future(&out_rc_future);
    // This will complain about Rc not being `Send`able
    assert_future_send(&out_rc_future);
    // error: future cannot be sent between threads safely
    //   --> src/main.rs:27:5
    //    |
    // 27 |     assert_future_send(&out_rc_future);
    //    |     ^^^^^^^^^^^^^^^^^^ future returned by `get_or_insert_with` is not `Send`
    //    |
    //    = help: within `impl Future`, the trait `Send` is not implemented for `Rc<i32>`
    // note: captured value is not `Send`
    //   --> src/main.rs:25:52
    //    |
    // 25 |     let out_rc_future = get_or_insert_with(async { rc });
    //    |                                                    ^^ has type `Rc<i32>` which is not `Send`
    // note: required by a bound in `assert_future_send`
    //   --> src/main.rs:15:35
    //    |
    // 15 | fn assert_future_send<F: Future + Send>(_: &F) {}
    //    |                                   ^^^^ required by this bound in `assert_future_send`error: future cannot be sent between threads safely
    //   --> src/main.rs:27:5
    //    |
    // 27 |     assert_future_send(&out_rc_future);
    //    |     ^^^^^^^^^^^^^^^^^^ future returned by `get_or_insert_with` is not `Send`
    //    |
    //    = help: within `impl Future`, the trait `Send` is not implemented for `Rc<i32>`
    // note: captured value is not `Send`
    //   --> src/main.rs:25:52
    //    |
    // 25 |     let out_rc_future = get_or_insert_with(async { rc });
    //    |                                                    ^^ has type `Rc<i32>` which is not `Send`
    // note: required by a bound in `assert_future_send`
    //   --> src/main.rs:15:35
    //    |
    // 15 | fn assert_future_send<F: Future + Send>(_: &F) {}
    //    |                                   ^^^^ required by this bound in `assert_future_send`

    // This will complain about using a temporary reference
    let _out_ref_future = {
        let mut val = 15;
        let temp = &mut val;

        get_or_insert_with(async { *temp += 1 })
    };
    // error[E0597]: `val` does not live long enough
    //   --> src/main.rs:64:20
    //    |
    // 62 |     let _out_ref_future = {
    //    |         --------------- borrow later stored here
    // 63 |         let mut val = 15;
    // 64 |         let temp = &mut val;
    //    |                    ^^^^^^^^ borrowed value does not live long enough
    // ...
    // 67 |     };
    //    |     - `val` dropped here while still borrowed
}

---

If it convinced you, I'll update the failing test (or remove them, I'm not familiar with trybuild).
Also in the current commit I only removed 'static, tell me if you also want to remove Send

[tatsuya6502]

Thank you. That was very helpful, especially this part:

if the inner future is Send, the returned future will be.

I also realized that I had some misunderstanding on how Tokio's Runtime::block_on method will work with a multi-thread runtime. I thought block_on will execute the future on the thread pool, but it will actually execute it on the current thread. Based on my wrong assumptions, I thought Moka's get_or_... methods should have 'static and Send bounds on the future to prevent it to contain !Send values and/or non-'static references to the outside.

I checked the implementations of tokio::Runtime::{block_on, spawn} and tokio::LocalSet::run_until methods, and wrote some tests codes to check my understandings. I believe I fully understand how they work.

Now I am confident to remove both 'static and Send bounds from get_or_... methods.

So please do the following:

• Remove Send bound from get_or_{,try_}insert_with.
• Remove all files from tests/ui/future/ directory. (I will probably add some after merging this PR)

Thanks!

[tinou98]

Both Send + 'static removed from get_or_insert function. UI test also removed. Rebased on master.

[tatsuya6502]

LGTM. Thanks a lot!