Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Why prefer dereference to clone on Copy types? #2870

Closed
illicitonion opened this issue Jun 23, 2018 · 2 comments
Closed

Why prefer dereference to clone on Copy types? #2870

illicitonion opened this issue Jun 23, 2018 · 2 comments

Comments

@illicitonion
Copy link
Contributor

In this very contrived code:

fn main() {
    let duration = &std::time::Duration::from_secs(1);
    consume(duration.clone());
    println!("Duration in main: {:?}", duration);
}

fn consume(duration: std::time::Duration) {
    println!("Duration in consume: {:?}", duration);
}

Clippy gives a warning:

warning: using `clone` on a `Copy` type
 --> src/main.rs:3:13
  |
3 |     consume(duration.clone());
  |             ^^^^^^^^^^^^^^^^ help: try dereferencing it: `*duration`
  |
  = note: #[warn(clone_on_copy)] on by default
  = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.207/index.html#clone_on_copy

I'm not sure this is a good recommendation. Using .clone() makes it obvious that the work is happening, whereas the mental jumps of "I'm dereferencing, which means that my type can be copied, and that copy will be moved" are less obvious. In particular, when I explained to someone just starting out with Rust that these were the two ways of fixing the compile error they ran into, their conclusion was that calling .clone() is better self-documenting code than calling *.

I'm in general fine with the clone_on_copy lint, but I feel like it should specifically be disabled for cases where the suggestion is to dereference. Is there a compelling reason that the above code is worse than the suggested alternative? What do others think?

@oli-obk
Copy link
Contributor

oli-obk commented Jun 23, 2018

To me, an explicit use of clone always suggests there's something nontrivial going on. If you call a method on a copy type which takes it by value, you also get a copy without first cloning. A dereference shows you that a cheap operation is happening

@camsteffen
Copy link
Contributor

In addition to reasons given above, the clone() call implicitly dereferences the value before cloning it. Without a method call, the deref has to be explicit. So your options are

  1. Implicit deref + clone()
  2. Explicit deref + implicit copy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants