NLL non lexical lifetime not handling a (seemingly) simple case

[rib]

I tried this code:

    async fn connect_peripheral(&self, peripheral_handle: PlatformPeripheralHandle) -> Result<()> {
        let winrt_peripheral = match self.inner.peripherals_by_handle.get(&peripheral_handle) {
            Some(p) => p,
            None => {
                log::error!("Spurious connection request with unknown peripheral handle {:?}", peripheral_handle);
                return Err(Error::Other(anyhow!("Unknown peripheral")));
            }
        };

        let device = BluetoothLEDevice::FromBluetoothAddressAsync(winrt_peripheral.address)
            .map_err(|_| Error::PeripheralUnreachable)?
            .await
            .map_err(|_| Error::PeripheralUnreachable)?;

        // XXX: be careful not to introduce a ref cycle here...
        // (notably we don't pass a winrt session reference to the status handler)
        let platform_bus = self.inner.platform_bus.clone();
        let peripheral_address = winrt_peripheral.address;

        // XXX: I thought non-lexical lifetimes were suppose to fix this kind of issue... :/
        //
        // This scope has been added because otherwise the compiler complains that
        // the connection_status_handler (which is not Send safe) might be used after the
        // final await... the await that is at the end of the function where it should
        // surely be possible for the compiler to recognise that this doesn't need to live
        // that long?
        //
        // Also the handler is moved into device.ConnectionStatusChanged() so I also don't
        // understand why the lifetime isn't seen as ending there?
       // UNCOMMENT brace to fix...
       // {
            // Scope introduced to keep the borrow checker happy
            let connection_status_handler =
                TypedEventHandler::new(move |sender: &Option<BluetoothLEDevice>, _| {
                    if let Some(sender) = sender {
                        match sender.ConnectionStatus() {
                            Ok(BluetoothConnectionStatus::Connected) => {
                                trace!("Peripheral connected: handle={:?}/{}", peripheral_handle, MAC(peripheral_address).to_string());
                                let _ = platform_bus.send(PlatformEvent::PeripheralConnected { peripheral_handle } );
                            },
                            Ok(BluetoothConnectionStatus::Disconnected) => {
                                trace!("Peripheral connected: handle={:?}/{}", peripheral_handle, MAC(peripheral_address).to_string());
                                let _ = platform_bus.send(PlatformEvent::PeripheralDisconnected { peripheral_handle } );
                            },
                            Ok(status) => {
                                log::error!("Spurious bluetooth connection status: {:?}: handle={:?}/{}", status, peripheral_handle, MAC(peripheral_address).to_string());
                            }
                            Err(err) => {
                                log::error!("Failure while querying bluetooth connection status: {:?}: handle={:?}/{}",
                                            err, peripheral_handle, MAC(peripheral_address).to_string())
                            }
                        }
                    }

                    Ok(())
                });
        { // COMMENT out this brace to expand scope around connection_status_handler 
            let mut winrt_peripheral_guard = winrt_peripheral.inner.write().unwrap();
            winrt_peripheral_guard.connection_status_handler = Some(device.ConnectionStatusChanged(connection_status_handler)
                .map_err(|_| Error::Other(anyhow!("Could not add connection status handler")))?);
        }

        self.get_gatt_services(&device).await
    }

I expected this to compile but it only compiles if I expand the scope that covers the locked write near the end so it encompasses connection_status_handler because otherwise the compiler will complain that connection_status_handler is not Send safe and might be used after the final await.

Considering that the await is at the end of the function (so there's no code that could possibly require connection_status_handler to live that long, and also as connection_status_handler is moved into device.ConnectionStatusChanged(connection_status_handler) I don't really understand why the borrow checker isn't recognising that connection_status_handler doesn't need to live beyond the await here?

I tried adding an explicit drop(connection_status_handler); just before the last line and even then the compiler still says it's not dropped until the end of the function scope and gives the same error.

Meta

rustc --version --verbose:

rustc 1.56.0-nightly (5d6804469 2021-08-30)
binary: rustc
commit-hash: 5d6804469d80aaf26f98090ae016af45e267f58f
commit-date: 2021-08-30
host: x86_64-pc-windows-msvc
release: 1.56.0-nightly
LLVM version: 13.0.0

Compiler Error

error: future cannot be sent between threads safely
   --> src\winrt\session.rs:358:99
    |
358 |       async fn connect_peripheral(&self, peripheral_handle: PlatformPeripheralHandle) -> Result<()> {
    |  ___________________________________________________________________________________________________^
359 | |         let winrt_peripheral = match self.inner.peripherals_by_handle.get(&peripheral_handle) {
360 | |             Some(p) => p,
361 | |             None => {
...   |
409 | |         self.get_gatt_services(&device).await
410 | |     }
    | |_____^ future created by async block is not `Send`
    |
    = help: within `impl futures::Future`, the trait `std::marker::Send` is not implemented for `NonNull<c_void>`
note: future is not `Send` as this value is used across an await
   --> src\winrt\session.rs:409:9
    |
378 |         let connection_status_handler =
    |             ------------------------- has type `TypedEventHandler<BluetoothLEDevice, IInspectable>` which is not `Send`
...
409 |         self.get_gatt_services(&device).await
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ await occurs here, with `connection_status_handler` maybe used later
410 |     }
    |     - `connection_status_handler` is later dropped here
    = note: required for the cast to the object type `dyn futures::Future<Output = std::result::Result<(), Error>> + std::marker::Send`

For reference this is the some of windows-rs generated code for the TypedEventHandler struct:

Generated code

        #[repr(transparent)]
        #[derive(
            :: std :: cmp :: PartialEq,
            :: std :: cmp :: Eq,
            :: std :: clone :: Clone,
            :: std :: fmt :: Debug,
        )]
        pub struct TypedEventHandler<TSender, TResult>(
            ::windows::IUnknown,
            ::std::marker::PhantomData<TSender>,
            ::std::marker::PhantomData<TResult>,
        )
        where
            TSender: ::windows::RuntimeType + 'static,
            TResult: ::windows::RuntimeType + 'static;
        impl<
                TSender: ::windows::RuntimeType + 'static,
                TResult: ::windows::RuntimeType + 'static,
            > TypedEventHandler<TSender, TResult>
        {
            pub fn new<
                F: FnMut(
                        &<TSender as ::windows::Abi>::DefaultType,
                        &<TResult as ::windows::Abi>::DefaultType,
                    ) -> ::windows::Result<()>
                    + 'static,
            >(
                invoke: F,
            ) -> Self {
                let com = TypedEventHandler_box::<TSender, TResult, F> {
                    vtable: &TypedEventHandler_box::<TSender, TResult, F>::VTABLE,
                    count: ::windows::RefCount::new(1),
                    invoke,
                };
                unsafe { std::mem::transmute(::std::boxed::Box::new(com)) }
            }
            pub fn Invoke<'a>(
                &self,
                sender: impl ::windows::IntoParam<'a, TSender>,
                args: impl ::windows::IntoParam<'a, TResult>,
            ) -> ::windows::Result<()> {
                let this = self;
                unsafe {
                    (::windows::Interface::vtable(this).3)(
                        ::windows::Abi::abi(this),
                        sender.into_param().abi(),
                        args.into_param().abi(),
                    )
                    .ok()
                }
            }
        }

[digama0]

NLL doesn't change when destructors run (it can't, because this can change the observable behavior of the program). So this is working as intended. That drop doesn't work sounds like an issue with the async lowering, I would guess there is already an issue for this.

[rib]

I don't think there should even be anything to drop in this case though; the variable is moved via device.ConnectionStatusChanged(connection_status_handler)

So as far as I understand it there's no need for any destructors / implicit drop for connection_status_handler to run at the end of the function scope. (So it doesn't look like NLL would need to affect when destructors are run here?)

I only experimented with calling drop to see what influence it might have on the error, but due to the move above then if it weren't for the first error then I think attempting to drop() before the await should in itself give an error because the variable was already moved.

[digama0]

Here's a smaller example exhibiting the issue:

fn main() {
    fn assert_send(_: impl Send) {}
    assert_send(async { // error: not Send
        let rc = std::rc::Rc::new(0);
        drop(rc);
        // rc is dropped but is still part of the state machine at the await point 
        async {}.await
    })
}

That this fails has nothing to do with the borrow checker and everything to do with how async lowering works (how the state machine is constructed). I'm not sure if this specific aspect of which variables end up in the state machine is subject to stability guarantees that prevent changing the behavior here, but my guess is that this is a known issue (and I'm hoping someone else who knows better can come along and link to it).

[csmoe]

Duplicate of #63768

[rib]

Just for reference, in case anyone else arrives here; these other two issues also look closely related:

#57478 and #57017

[rib]

Oh and also this higher level tracking issue: #69663