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
10.89k stars 1.46k forks source link

Warning emitted for non-canonical implementations conflicts with denying implicit and needless returns #12683

Closed siddiqua1 closed 2 weeks ago

siddiqua1 commented 3 weeks ago

Summary

For my code bases, I prefer to have #![deny(clippy::implicit_return)] and #![allow(clippy::needless_return)]. When I implemented a custom type to have trait PartialOrd, I simply used an option wrapper for the return of cmp that is user defined. It seems this implementation conflicts with the default #[warn(clippy::non_canonical_partial_ord_impl)].

While I can disable this lint, I imagine the intended behavior from the user's perspective would be to implicitly disable the non_canonical_partial_ord_impl lint.

Lint Name

non_canonical_partial_ord_impl

Reproducer

I tried this code: Rust Playground

#![deny(clippy::implicit_return)]
#![allow(clippy::needless_return)]

use std::cmp::{Eq, Ord, Ordering, PartialEq, PartialOrd};

pub struct Example {
    data: i32,
}

impl PartialEq for Example {
    fn eq(&self, other: &Self) -> bool {
        return self.data == other.data;
    }
}

impl Eq for Example {}

impl Ord for Example {
    fn cmp(&self, other: &Self) -> Ordering {
        return self.data.cmp(&other.data);
    }
}

impl PartialOrd for Example {
    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
        return Some(self.cmp(other));
    }
}

I saw this happen:

    Checking playground v0.0.1 (/playground)
warning: non-canonical implementation of `partial_cmp` on an `Ord` type
  --> src/lib.rs:25:1
   |
25 | /  impl PartialOrd for Example {
26 | |      fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
   | | _____________________________________________________________-
27 | ||         return Some(self.cmp(other));
28 | ||     }
   | ||_____- help: change this to: `{ Some(self.cmp(other)) }`
29 | |  }
   | |__^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_partial_ord_impl
   = note: `#[warn(clippy::non_canonical_partial_ord_impl)]` on by default

warning: `playground` (lib) generated 1 warning
    Finished dev [unoptimized + debuginfo] target(s) in 0.36s

I expected to see this happen:

No warnings to be emitted

Version

rustc 1.76.0 (07dca489a 2024-02-04)
binary: rustc
commit-hash: 07dca489ac2d933c78d3c5158e3f43beefeb02ce
commit-date: 2024-02-04
host: x86_64-pc-windows-msvc
release: 1.76.0
LLVM version: 17.0.6

Additional Labels

No response

siddiqua1 commented 3 weeks ago

After a brief look at the source code it seems the same is applicable to the lint non_canonical_clone_impl

Code:

#![deny(clippy::implicit_return)]
#![allow(clippy::needless_return)]

use std::cmp::{Eq, PartialEq};

#[derive(Eq, PartialEq)]
struct A(u32);

impl Clone for A {
    fn clone(&self) -> Self {
        return *self;
    }
}

impl Copy for A {}

Warnings:

    Checking playground v0.0.1 (/playground)
warning: non-canonical implementation of `clone` on a `Copy` type
  --> src/lib.rs:10:29
   |
10 |       fn clone(&self) -> Self {
   |  _____________________________^
11 | |         return *self;
12 | |     }
   | |_____^ help: change this to: `{ *self }`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl
   = note: `#[warn(clippy::non_canonical_clone_impl)]` on by default

warning: `playground` (lib) generated 1 warning
    Finished dev [unoptimized + debuginfo] target(s) in 0.33s