Borrowchecker help with new `Waker::clone_from`

[static sand]

Hey all! I'm trying to take a crack at https://github.com/yoshuawuyts/futures-concurrency/issues/160 (replace the .clone() with Waker::clone_from)
and so far I have

    /// Set the parent `Waker`. This needs to be called at the start of every
    /// `poll` function.
    pub(crate) fn set_waker(&mut self, parent_waker: &Waker) {
        //self.parent_waker = Some(parent_waker.clone());
        //Waker::clone_from(&mut (self.parent_waker).unwrap(), parent_waker);
        Waker::clone_from(&mut self.parent_waker.unwrap(), parent_waker);
    }
}

where the self is a ReadinessArray:

pub(crate) struct ReadinessArray<const N: usize> {
    count: usize,
    readiness_list: [bool; N],
    parent_waker: Option<Waker>,
}

but I'm not making much progress. Any pointers?

[unborn iris]

what's the whole error?

[static sand]

error[E0507]: cannot move out of `self.parent_waker` which is behind a mutable reference
   --> src\utils\wakers\array\readiness_array.rs:67:32
    |
67  |         Waker::clone_from(&mut self.parent_waker.unwrap(), parent_waker);
    |                                ^^^^^^^^^^^^^^^^^ -------- `self.parent_waker` moved due to this method call        
    |                                |
    |                                help: consider calling `.as_ref()` or `.as_mut()` to borrow the type's contents     
    |                                move occurs because `self.parent_waker` has type `Option<Waker>`, which does not implement the `Copy` trait
    |
note: `Option::<T>::unwrap` takes ownership of the receiver `self`, which moves `self.parent_waker`
   --> C:\Users\mrg\scoop\persist\rustup-gnu\.rustup\toolchains\stable-x86_64-pc-windows-gnu\lib/rustlib/src/rust\library\core\src\option.rs:932:25
    |
932 |     pub const fn unwrap(self) -> T {
    |                         ^^^^
help: you can `clone` the value and consume it, but this might not be your desired behavior
    |
67  |         Waker::clone_from(&mut self.parent_waker.clone().unwrap(), parent_waker);
    |                                                 ++++++++

error[E0507]: cannot move out of `*parent_waker` which is behind a shared reference
  --> src\utils\wakers\vec\readiness_vec.rs:79:46
   |
79 |         (self.parent_waker).clone_from(&Some(*parent_waker));
   |                                              ^^^^^^^^^^^^^ move occurs because `*parent_waker` has type `Waker`, which does not implement the `Copy` trait

For more information about this error, try `rustc --explain E0507`.
error: could not compile `futures-concurrency` (lib) due to 2 previous errors
[Finished running. Exit status: 0]

[unborn iris]

did you read the whole error?

they often contain usefull hints for the resolution of the problem

(additionally, what would happen if self.parent_waker is None?)

[static sand]

@unborn iris yes, I know that .clone() is an option here, but I was trying to avoid cloning from the get go.

I also noticed that the .unwrap() would fail in the None case, so I figured I might as well ask for help to see if I wasn't doing anything silly.

[austere isle]

What do you actually want to do here? Clone it only if the current value is None or what?

[static sand]

The issue reads:

Now that rust-lang/rust#96979 has been merged, we should use Waker::clone_from in our set_waker functions to remove reduce unnecessary clones. If done right, then from now on we'll typically only clone-on-wake the first time our concurrency types are polled.

[austere isle]

Well, the original function does this

    pub(crate) fn set_waker(&mut self, parent_waker: &Waker) {
        self.parent_waker = Some(parent_waker.clone());
    }

So it sets it to Some regardless of previous state by cloning.

I guess what you wanna do here is check if the value is already Some.
if it is, use clone_from, if it isn't you have to use clone and set it as before.

    pub(crate) fn set_waker(&mut self, parent_waker: &Waker) {
        match &mut self.parent_waker {
            Some(prev) => prev.clone_from(parent_waker),
            None => self.parent_waker = Some(parent_waker.clone()),
        }
    }

[static sand]

Ah, that seems to appease the type checker, thanks @austere isle !

I had forgotten you could do match &mut foo {. Neat.

[austere isle]

It's kinda hilarious they even created an issue for this. Creating the issue probably took longer than writing that code and PRing it.

[static sand]

Yeah, but they sniped me into looking at their crate, so that's fun 😄

[unborn iris]

I meant the as_mut

that was independently of the None case

[unborn iris]

that suggestion is right below the error line

[unborn iris]

it's probably not the only place in the code base

[unborn iris]

btw https://github.com/yoshuawuyts/futures-concurrency/blob/112b40ec9acadb98abe16431f8a7ddd52a0eadad/src/utils/channel.rs#L33 also has a waker clone