incorrect_clone_impl_on_copy_type false positive on MaybeUninit buffers

[dtolnay]

Summary

In the code below, Clippy's suggested "equivalent" Clone implementation regresses performance.

Given the API of this type, there is no reason for the Clone impl to memcpy any bytes. It can return a new uninit buffer, for free. The original clone() call compiles to 0 machine instructions. Clippy's suggested clone() implementation compiles to multiple instructions of redundant copying that cannot be optimized out.

Could Clippy identify the case of a type whose contents consist solely of MaybeUninit, and avoid triggering incorrect_clone_impl_on_copy_type in that case?

Lint Name

incorrect_clone_impl_on_copy_type

Reproducer

use std::mem::MaybeUninit;

pub struct Buffer {
    bytes: [MaybeUninit<u8>; 24],
}

impl Buffer {
    #[inline]
    pub fn new() -> Self {
        let bytes = [MaybeUninit::<u8>::uninit(); 24];
        Buffer { bytes }
    }

    pub fn asdf(&mut self) -> &str {
        todo!("init and return slice of buffer")
    }
}

impl Copy for Buffer {}

impl Clone for Buffer {
    #[inline]
    fn clone(&self) -> Self {
        Buffer::new()
    }
}

error: incorrect implementation of `clone` on a `Copy` type
  --> src/main.rs:23:29
   |
23 |       fn clone(&self) -> Self {
   |  _____________________________^
24 | |         Buffer::new()
25 | |     }
   | |_____^ help: change this to: `{ *self }`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#incorrect_clone_impl_on_copy_type
   = note: `#[deny(clippy::incorrect_clone_impl_on_copy_type)]` on by default

Version

rustc 1.72.0-nightly (839e9a6e1 2023-07-02)
binary: rustc
commit-hash: 839e9a6e1210934fd24b15548b811a97c77138fc
commit-date: 2023-07-02
host: x86_64-unknown-linux-gnu
release: 1.72.0-nightly
LLVM version: 16.0.5

Additional Labels

No response

[dtolnay]

Mentioning @Centri3 @xFrednet who worked on this lint in #10925, and @scottmcm who suggested the lint in #10700 and may have opinions.

[Centri3]

I noticed this at #10925 (comment). I believe there's no reason it can't, but an equally good option is really just allowing the lint in cases like these (like you've done so already). The lint can have "FPs" in cases where the behavior is unnecessarily complex and a potential footgun but performance is better (like, Buffer::new or Self { x: 0, y: 0, z: 0 } perhaps but I haven't benchmarked it).

Nevertheless, a "Known issues" section for the lint's documentation would be worthwhile.

[xFrednet]

but an equally good option is really just allowing the lint in cases like these (like you've done so already).

I think for a deny-by-default lint we should avoid FPs as much as possible. Allowing a lint is a good suggestion for pedantic lints, but default lints should avoid this such cases. One of the common complaint I heard from newcomers is, that they're not sure how to allow a lint. TBH it also took me some time to learn how. 😅

[Centri3]

Well I don't really think this is a FP, per se; it's just slower. I'm not sure whether Buffer::new vs *self can result in differing behavior, from my testing it didn't, but at the end of the day it ideally still would use *self, if it could be optimized out.

Also, I believe majority of the cases where this should be allowed is definitely due to performance; which, in this case at least, MaybeUninit, isn't really something you should be messing with for a long time; along with anything else even mildly unsafe.

We could move this to complexity, though; there are still many cases where this is correctness but denying this could be a bit harsh (generally seeing error from clippy should mean you did something very wrong, which might not always be the case here)

[dtolnay]

+1 for recategorizing to complexity and documenting "Known issues". I filed another one in #11080.

Other than that, I'll stick to an allow attribute. Thanks!

[scottmcm]

@dtolnay Can you say more about why this type is Copy?

If you don't actually want it to be bit-copied, then it seems counter-productive. There's going to be lots of lints saying "hey, did you know you didn't need .clone() here because it's Copy?". The standard library is going to go out of its way to not call your faster clone implementation -- even <[Buffer; 1]>::clone() will use Copy, as will cloning a Vec<Buffer>. (That latter being particularly interesting because if Buffer weren't Copy, the Vec would be forced to call clone and thus LLVM would know that it's not writing anything, and thus be faster than the memcpy that will happen when Buffer is Copy.)

So I'm not convinced that this is really a false positive at all.

[dtolnay]

The previous comment is convincing, so I will retract this issue. Thanks for the insight Scott!