`ManuallyDrop<T>::clone` should trigger a lint

[mmastrac]

What it does

ManuallyDrop implements Clone when T: Clone, but I've found that this is often not what I want to do. For example, I may have code that looks like this:

struct MyStruct {
  state: Rc<State>,
}

...

fn spawn_state_task(data: &MyStuct) {
  let state = data.state.clone();
  spawn(async move { state.do_something() });
}

In the future, I change state to ManuallyDrop<Rc<X>> and update the Drop impl correctly, but neglect to update spawn_state_task(). The code continues to compile, but we've got a memory leak.

If you wish to clone the value inside of a ManuallyDrop and continue controlling the drop lifetime manually, you should do it via the explicit ManuallyDrop::clone call rather than .clone() or Clone::clone, both of which may mask a memory leak. If you wish to clone the object and return to automatic drop lifetime management, you should use (*value).clone() instead to deref the ManuallyDrop first.

Advantage

• Explicit detection of potential memory leak sites when switching T to ManuallyDrop<T> where T: Clone
• Clear display of sites where a new ManuallyDrop struct is creates from a clone, and require an additional drop call

Drawbacks

• This is more verbose in the case where you actually want to clone the ManuallyDrop

Example

struct MyStruct {
  state: Rc<State>,
}


fn spawn_state_task(data: &MyStuct) {
  let state = data.state.clone();
  spawn(async move { state.do_something() });
}

Could be written as:

struct MyStruct {
  state: Rc<State>,
}


fn spawn_state_task(data: &MyStuct) {
  let state = (*data.state).clone();
  spawn(async move { state.do_something() });
}

or

struct MyStruct {
  state: Rc<State>,
}


fn spawn_state_task(data: &MyStuct) {
  let state = ManuallyDrop::clone(data.state);
  spawn(async move { state.do_something() });
}

[mmastrac]

In deno_core we ended up writing a ManuallyDropRc [1] because this bit us a few times, but I just ran into it with another ManuallyDrop<T: Clone> type today.

[1] https://github.com/denoland/deno_core/blob/main/core/runtime/jsruntime.rs#L93

[lcnr]

cc rust-lang/rust#130140 where incorrectly using Clone causes an unsoundness. Deriving pretty much any trait for a type containing ManuallyDrop if they may drop its value outside the actual Drop impl is dangerous. Because of that I think that we should have a correctness lint when deriving traits for a structs containing ManuallyDrop

[scottmcm]

Yes please, to what @lcnr said. I checked all the code I wrote really carefully, but didn't think about the derives, which was my mistake. If we could do something smarter there, that'd be great -- since even Debug's derive is a problem.