Rust: Call clone on a reference may just clone the reference

[BusyJay]

Considering following code:

use std::sync::atomic::*;

#[derive(Default)]
struct A {
   counter: AtomicUsize,
}

impl A {
    fn clone(&self) -> Box<A> {
        Box::new(A {
            counter: AtomicUsize::new(self.counter.load(Ordering::SeqCst)),
        })
    }
}

fn main() {
    let a = Box::new(A::default());
    let b = &a;
    assert_eq!(a.counter.load(Ordering::SeqCst), 0);
    a.clone().counter.fetch_add(1, Ordering::SeqCst);
    assert_eq!(a.counter.load(Ordering::SeqCst), 0);
    b.clone().counter.fetch_add(1, Ordering::SeqCst);
    assert_eq!(a.counter.load(Ordering::SeqCst), 0);
}

b.clone will just clone the reference, hence it will update a in place rather in a temporary copy, so the final assert will fail.

On one hand, I know that reference can be copied, hence it's reasonable to implement Clone too. But I think it will be quite easy to make the mistake and assume clone will return a new instance with deref and type inference enabled by default.

On the other hand, I'm only sure that reference implements Copy and Clone, but what else traits does it implement? And any other method does it provide by default? Can these methods lead to confusion too?

[jonas-schievink]

The Arc is cloned, not the reference to it. Rc and Arc have no methods except through trait implementations.

[BusyJay]

@jonas-schievink Oh, sorry, I wrote a wrong example. Now a correct example is updated.

[sinkuu]

Probably clippy can help you.

warning: using `clone` on a double-reference; this will copy the reference instead of cloning the inner type
 --> src/main.rs:7:5
  |
7 |     b.clone().fetch_add(1, Ordering::SeqCst);
  |     ^^^^^^^^^ help: try dereferencing it `(*b).clone()`
  |
  = note: #[warn(clone_double_ref)] on by default
  = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#clone_double_ref

[oli-obk]

We should probably extend that lint to also catch calling fn foo<T: Clone>(t: T) with foo(&something)

[Mark-Simulacrum]

Agreed that this is something Clippy lints for today. Since we'd need an RFC and a few other considerations to do in Rust itself, closing.