incorrect_partial_ord_impl_on_ord_type false positive: Some(Ord::cmp(self, other))

[dtolnay]

Summary

Clippy flags the following as an incorrect PartialOrd impl, with a deny-by-default correctness lint:

impl PartialOrd for T {
    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
        Some(Ord::cmp(self, other))
    }
}

in favor of preferring:

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

But there is no correctness difference between these impl, only a stylistic difference.

Lint Name

incorrect_partial_ord_impl_on_ord_type

Reproducer

use std::cmp::Ordering;

pub struct Struct(String);

impl Struct {
    pub fn cmp(&self) {
        println!("Struct::cmp!");
    }
}

impl Eq for Struct {}

impl PartialEq for Struct {
    fn eq(&self, other: &Self) -> bool {
        PartialEq::eq(&self.0, &other.0)
    }
}

impl Ord for Struct {
    fn cmp(&self, other: &Self) -> Ordering {
        Ord::cmp(&self.0, &other.0)
    }
}

impl PartialOrd for Struct {
    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
        Some(Ord::cmp(self, other))
    }
}

error: incorrect implementation of `partial_cmp` on an `Ord` type
  --> src/main.rs:25:1
   |
25 | /  impl PartialOrd for Struct {
26 | |      fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
   | | _____________________________________________________________-
27 | ||         Some(Ord::cmp(self, 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#incorrect_partial_ord_impl_on_ord_type
   = note: `#[deny(clippy::incorrect_partial_ord_impl_on_ord_type)]` on by default

I consider this a false positive for this lint. If there is a compelling stylistic argument in favor of self.cmp(other) over Ord::cmp(self, other), that should be at most a warn-by-default style lint, not a deny-by-default correctness lint.

I think neither Some(Ord::cmp(self, other)) nor Some(T::cmp(self, other)) should trigger a lint.

Currently on GitHub, there are 1.6k search results for "::cmp(self" and 26k search results for "self.cmp(". Clearly there is a preference for self.cmp(other) but that is still a significant proportion of users who prefer to write the other, especially in macro-generated code where self.cmp(other) might not compile (as is the case above).

error[E0061]: this method takes 0 arguments but 1 argument was supplied
  --> src/main.rs:27:19
   |
27 |         Some(self.cmp(other))
   |                   ^^^ -----
   |                       |
   |                       unexpected argument of type `&Struct`
   |                       help: remove the extra argument
   |
note: method defined here
  --> src/main.rs:6:12
   |
6  |     pub fn cmp(&self) {
   |            ^^^

Version

rustc 1.73.0-nightly (da6b55cc5 2023-07-17)
binary: rustc
commit-hash: da6b55cc5eaf76ed6acb7dc2f7d611e32af7c9a7
commit-date: 2023-07-17
host: x86_64-unknown-linux-gnu
release: 1.73.0-nightly
LLVM version: 16.0.5

Additional Labels

@rustbot label +I-suggestion-causes-error

[dtolnay]

Mentioning @Centri3 @blyxyas @xFrednet, who recently worked on this lint in #10788.

[Centri3]

This sort of thing was already brought up: #10788 (comment)

It's a bit unfortunate but there's many, many variations of Some(self.cmp(other)) that are still correct but likely too difficult to not lint them all

[Centri3]

It should definitely be noted as a known issue however; not having that was an oversight on my part

[dtolnay]

I think there is a degree of difference between Some(Self::cmp(self, other)) and self.cmp(other).into() and I would not advocate in favor of the latter being recognized by the lint. But since the first one is calling literally the same function with the exact same arguments as clippy's preferred spelling, hopefully there would be an easy way for clippy to perceive that?

[Centri3]

Yeah it could handle both Call and MethodCall, that one could probably be fixed fine