clone_on_ref_ptr lint is not always trivial to resolve

[chriscoomber]

We've just spotted the clone_on_ref_ptr lint throw up clippy warnings in our code. In an effort to go through and fix them, we realized that is isn't actually always very trivial to do so.

I've had a go at condensing our code down into a minimal repro. It has something to with type inference, and trait objects (e.g. interpreting an Arc of a struct as an Arc of a trait).

Concrete example: playground

let _z = Bar(y.clone()); fails the clippy lint:

warning: using '.clone()' on a ref-counted pointer
  --> src/main.rs:13:18
   |
13 |     let _z = Bar(y.clone());
   |                  ^^^^^^^^^ help: try this: `Arc::clone(&y)`
   |
   = note: #[warn(clone_on_ref_ptr)] on by default
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.160/index.html#clone_on_ref_ptr

The suggested fix, however, does not compile.

error[E0308]: mismatched types
  --> src/main.rs:13:29
   |
13 |     let _z = Bar(Arc::clone(&y));
   |                             ^^ expected trait Foo, found struct `Bar`
   |
   = note: expected type `&std::sync::Arc<Foo>`
              found type `&std::sync::Arc<Bar>`

One way to actually get this to work, is to provide explicit types (Arc::<Bar>::clone). However, in real life the struct Bar might actually have a bunch of generics, which are incredibly tedious to write out (even with underscores) - for example Arc::Bar<_, _, _, _>::clone.

Why I think this is an issue

In this particular case, the clippy warning is trying to get us to write the clone in a slightly different way, to make it clear that we're cloning an Arc (cheap). However, that slightly different way doesn't actually compile. This forces us to disable the warning, or jump through several hoops to get some quite inelegant code which satisfies the compiler and clippy.

[chriscoomber]

I've got an even more minimal repro: https://play.rust-lang.org/?gist=23bd63e731740d2cd3727ce419979ad5&version=nightly

[seanmonstar]

I'd personally consider the lint wrong in all cases, and would rather see it removed. It's more annoying to write, and type inference will do The Right Thing anyways.

[oli-obk]

Yea we can move it to the restriction lints.

[Aaron1011]

I'd personally consider the lint wrong in all cases, and would rather see it removed. It's more annoying to write, and type inference will do The Right Thing anyways.

The reason for writing Rc::clone and Arc::clone isn't to help with type inference - it's to make it clear that only the pointer is being cloned, as opposed to the underlying data. The former is always fast, while the latter can be very expensive depending on what is being cloned.

It's also the recommended by the standard library docs: nical/rust@1b414f5

[seanmonstar]

The reason for writing Rc::clone and Arc::clone isn't to help with type inference - it's to make it clear that only the pointer is being cloned, as opposed to the underlying data.

I realize that. What I meant with type inference is that you almost never have to worry that you're actually cloning the internal piece, because if you pass the clone to a function taking Rc<Foo>, you are not at risk of actually cloning Foo. The compiler knows better.

It's also the recommended by the standard library docs.

I assume this is a newer recommendation, I don't recall seeing it before. But again, I'd personally disagree with it. foo.clone() is subjectively better than Rc::clone(&foo) when I already know the argument is an Rc<Foo>.

[chriscoomber]

One thing I have against the lint is that I can create my own structs which implement clone in different ways, shallow (e.g. cloning references such as Arcs) or deep - or sometimes a hybrid of the two.

Suppose Foo implements Clone in a shallow way, e.g. #[derive(Clone)] struct Foo { x: Arc<String>, y: Arc<String> }.

• Should I write Foo::clone(&foo)? Does that make it any more obvious that I'm cloning references only? I don't think so - someone would need to look at the documentation of Foo to know that clone is a shallow clone for Foo. If they're doing that, they will be happy with foo.clone().
• Should clippy complain if I wrote foo.clone()? That seems impossible to implement - how can clippy know if it's a shallow clone or not?

To me, unless the answers to the above questions are both "yes" for a custom struct, I don't see any value in making them "yes" for a standard library struct.

Edit 1: The argument for Arc::clone(&foo) being clearer only works if you know what an Arc is, and that cloning it will clone the reference only. The actual syntax doesn't make anything clearer, as String::clone(&foo) looks identical and is not just cloning references. I think the argument is that the syntax is different, and there is a general understanding that this implies something about the deepness.

Edit 2: If x is an Arc<Vec<_>> you can do both Arc::clone(&x) and Vec::clone(&x) (due to auto-deref) and they (obviously) do different things. This is already mentioned by others in this discussion, but that link might be useful. This may be an argument for the Arc::clone notation. x.clone() and Arc::clone(&x) are equivalent to the compiler in this case.

[Aaron1011]

I realize that. What I meant with type inference is that you almost never have to worry that you're actually cloning the internal piece, because if you pass the clone to a function taking Rc, you are not at risk of actually cloning Foo. The compiler knows better.

My bad - when I said "it's to make it clear that only the pointer is being cloned, as opposed to the underlying data.", I was referring only to people reading the code, not the compiler itself. The issue isn't that the compiler will do the wrong thing - it's that people reading the code will get an incorrect impression of what it's doing.

The argument for Arc::clone(&foo) being clearer only works if you know what an Arc is, and that cloning it will clone the reference only.

I believe the idea behind the recommendation in the docs is that people can be expected to be familiar with the common container/pointer types in the stdlib (Arc, Rc, Vec, Box, etc) since they're used so pervasively in Rust. That's why this lint is called clone_on_ref_ptr and not something like shallow_clone_invocation - it's intended to make certain specific, well-defined usages of clone less ambigious.

[seanmonstar]

people can be expected to be familiar with the common container/pointer types in the stdlib

So likewise, when dealing with an Rc or Arc, users should be familiar with foo.clone() being a shallow clone.

[Aaron1011]

So likewise, when dealing with an Rc or Arc, users should be familiar with foo.clone() being a shallow clone.

The idea of this line is that it's not always obvious what the type of foo is. foo could be a field in a struct declared in a different crate, or a local variable with an inferred type. Explicitly writing out Rc::clone or Arc::clone makes it clear that the cloning operation is happening on the pointer itself, and is independent of whatever type is being wrapped.

[oli-obk]

Since I do not see a way that we can detect this case in clippy (inference makes this ridiculously hard), I marked this as an easy issue, since all that needs to be done is move it to the restriction lints and maybe ensure that the suggestion also prints the full types in case traits are involved (which would also print the full thing when no inference happened)

[alusch]

Hi there, I'm new here and interested in picking this up. Do you have an example for the output you had in mind for printing the full type? Thanks!

[oli-obk]

@alusch awesome!

I was thinking that we'd get Arc::<Foo>::clone(&y) instead of Arc::clone(&y).