incorrect_clone_impl_on_copy_type false positive on empty enum

[dtolnay]

Summary

Would you be willing to consider the exact expression match *self {} a correct Clone impl for a Copy type?

This immediately conveys "don't even bother" in a way that *self does not.

Lint Name

incorrect_clone_impl_on_copy_type

Reproducer

enum Void {}

impl Copy for Void {}

impl Clone for Void {
    fn clone(&self) -> Self {
        match *self {}
    }
}

error: incorrect implementation of `clone` on a `Copy` type
 --> src/main.rs:6:29
  |
6 |       fn clone(&self) -> Self {
  |  _____________________________^
7 | |         match *self {}
8 | |     }
  | |_____^ 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.

[scottmcm]

Can you elaborate why an enum like this isn't just using derives?

#[derive(Copy, Clone)]
enum Void {}

[Centri3]

We could probably ignore never-like enums altogether. But deriving Clone + Copy should work here afaik, is there any case it wouldn't?

I suppose it does changing the meaning of the code (match *self {} does convey "don't even bother", like was pointed out), so another option could be ignoring them altogether.

[dtolnay]

Can you elaborate why an enum like this isn't just using derives?

It can. In fact this is exactly the impl that the standard derive generates (as of rust-lang/rust#113770 anyway).

#[derive(Clone)]
enum Void {}

#[automatically_derived]
impl ::core::clone::Clone for Void {
    #[inline]
    fn clone(&self) -> Void { match *self {} }
}

As an author of derive macros, handwriting the code I'm going to want it to generate is pretty much always the first step. Depending on the macro, I even run Clippy on my handwritten impl to catch anything silly up front before starting to implement the macro.

I think there is room for a warn-by-default lint for the purpose of saying "did you know you can delete this handwritten impl and just use derive?" (more broadly than just for Clone) but this issue is in regard to a deny-by-default lint that says "the code in this impl is incorrect", which is a false positive for this particular lint because the code is correct and identical to what the standard library derive(Clone) would produce.

[scottmcm]

Hmm, I don't know what the clippy guidance for levels is.

In rustc this would definitely not pass the deny-by-default bar, but maybe it's expected that clippy is more picky here?

I suppose one option would be to have two lints:

1. a deny-by-default one for when the lint notices that it's definitely different from what Copy will do, and
2. a warn-by-default one that it could be simplified to just *self.

[Centri3]

deny-by-default lints are pretty rare in rustc, and for good reason, you don't want something slightly pedantic (like approx_constants) breaking the build. The correctness category is for code that is "outright wrong or useless", which fits this lint.

I agree with what you said on two lints, I think a lint for saying "this could be derived" would be nice