rust-lang / rust-clippy

A bunch of lints to catch common mistakes and improve your Rust code. Book: https://doc.rust-lang.org/clippy/
https://rust-lang.github.io/rust-clippy/
Other
11.46k stars 1.54k forks source link

assigning_clones false positive for Option<T> where T uses default clone_from impl #12709

Open dhardy opened 6 months ago

dhardy commented 6 months ago

Summary

Motivation

I have a type, Id, which is really cheap to copy, yet unfortunately cannot implement Copy: it uses just a u64 in-memory representation in almost all cases, and a reference-counted heap-allocated representation in the remaining cases. It also gets used quite frequently, and I want to make it easy to use and pass around. I mean, I'm almost tempted to let heap-allocated instances leak just so that I can implement Copy.

It's also common to want to copy an Option<Id> around. Queue Clippy's assigning_clones recommending usage of clone_from instead, despite the fact that Id does not implement clone_from (or have anything to gain if it did, since the heap-allocated representations are not mutable).

Besides which, usages of Id are not particularly performance sensitive. Remember premature optimisation is the root of all evil? There may be cases where this lint significantly improves code, but it's not here.

Expectation

Either that Clippy can recognise the false positive (Option impls clone_from but T doesn't) or that this lint can be disabled with an annotation on the type.

Lint Name

assigning_clones

Reproducer

I tried this code:

mod inner {
    #[derive(Clone, Debug, PartialEq, Eq)]
    pub struct Id(u64);
    impl From<u64> for Id {
        fn from(n: u64) -> Self {
            Id(n)
        }
    }
}

use inner::Id;

fn main() {
    let a = Some(Id::from(123));
    let mut b = None;
    assert!(a != b);
    b = a.clone();
    assert_eq!(a, b);
    dbg!(b);
}

I saw this happen:

$ cargo clippy
    Checking assigning_clones v0.1.0 (/home/dhardy/projects/small/assigning_clones)
warning: assigning the result of `Clone::clone()` may be inefficient
  --> src/main.rs:17:5
   |
17 |     b = a.clone();
   |     ^^^^^^^^^^^^^ help: use `clone_from()`: `b.clone_from(&a)`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#assigning_clones
   = note: `#[warn(clippy::assigning_clones)]` on by default

warning: `assigning_clones` (bin "assigning_clones") generated 1 warning
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.04s

I expected to see this happen: no warning.

Version

rustc 1.79.0-nightly (244da22fa 2024-04-23)
binary: rustc
commit-hash: 244da22fabd9fa677bbd0ac601a88e5ca6917526
commit-date: 2024-04-23
host: x86_64-unknown-linux-gnu
release: 1.79.0-nightly
LLVM version: 18.1.4

Additional Labels

No response

Alexendoo commented 6 months ago

Would be good to cover similar types also, from what I can see:

eric-seppanen commented 6 months ago

Here's another example that makes assigning_clones sad:

#[derive(Clone)]
struct Foo {
    _x: u64,
}

#[derive(Clone)]
pub struct Wrapper {
    value: Option<Foo>,
}

impl Wrapper {
    pub fn update(&mut self, s: &Wrapper) {
        self.value = s.value.clone();
    }
}

results in

warning: assigning the result of `Clone::clone()` may be inefficient
  --> src/lib.rs:20:9
   |
20 |         self.value = s.value.clone();
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `self.value.clone_from(&s.value)`
   |
eric-seppanen commented 6 months ago

Similarly, Option<Arc> complains in places where plain Arc would not:

use std::sync::Arc;

struct Foo {
    _x: u64,
}

#[derive(Clone)]
pub struct Wrapper {
    value: Option<Arc<Foo>>,
}

impl Wrapper {
    pub fn update(&mut self, s: &Wrapper) {
        self.value = s.value.clone();
    }
}
warning: assigning the result of `Clone::clone()` may be inefficient
  --> src/lib.rs:26:9
   |
26 |         self.value = s.value.clone();
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `self.value.clone_from(&s.value)`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#assigning_clones
   = note: `#[warn(clippy::assigning_clones)]` on by default