Skip to content

Commit ee8a429

Browse files
borsbors
committed
Auto merge of #11188 - Centri3:#11178, r=blyxyas
Allow `Self::cmp(self, other)` as a correct impl Fixes #11178 Also no longer checks if the method name is *just* cmp, but the path. That was an oversight on my part ^^ r? `@xFrednet` (and `@blyxyas` too!) changelog: [`incorrect_partial_ord_impl_on_ord_type`]: Now allows non-method calls to `cmp` like `Self::cmp(self, other)`
2 parents 7c5095c + a4c367d commit ee8a429

7 files changed

+211
-14
lines changed

‎clippy_lints/src/incorrect_impls.rs

Lines changed: 62 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
2+
use clippy_utils::paths::ORD_CMP;
23
use clippy_utils::ty::implements_trait;
3-
use clippy_utils::{get_parent_node, is_res_lang_ctor, last_path_segment, path_res};
4+
use clippy_utils::{get_parent_node, is_res_lang_ctor, last_path_segment, match_def_path, path_res, std_or_core};
45
use rustc_errors::Applicability;
5-
use rustc_hir::def::Res;
6+
use rustc_hir::def_id::LocalDefId;
67
use rustc_hir::{Expr, ExprKind, ImplItem, ImplItemKind, ItemKind, LangItem, Node, UnOp};
78
use rustc_hir_analysis::hir_ty_to_ty;
89
use rustc_lint::{LateContext, LateLintPass};
@@ -60,6 +61,10 @@ declare_clippy_lint! {
6061
/// wrapping the result of `cmp` in `Some` for `partial_cmp`. Not doing this may silently
6162
/// introduce an error upon refactoring.
6263
///
64+
/// ### Known issues
65+
/// Code that calls the `.into()` method instead will be flagged as incorrect, despite `.into()`
66+
/// wrapping it in `Some`.
67+
///
6368
/// ### Limitations
6469
/// Will not lint if `Self` and `Rhs` do not have the same type.
6570
///
@@ -191,6 +196,11 @@ impl LateLintPass<'_> for IncorrectImpls {
191196
trait_impl.substs,
192197
)
193198
{
199+
// If the `cmp` call likely needs to be fully qualified in the suggestion
200+
// (like `std::cmp::Ord::cmp`). It's unfortunate we must put this here but we can't
201+
// access `cmp_expr` in the suggestion without major changes, as we lint in `else`.
202+
let mut needs_fully_qualified = false;
203+
194204
if block.stmts.is_empty()
195205
&& let Some(expr) = block.expr
196206
&& let ExprKind::Call(
@@ -202,9 +212,8 @@ impl LateLintPass<'_> for IncorrectImpls {
202212
[cmp_expr],
203213
) = expr.kind
204214
&& is_res_lang_ctor(cx, cx.qpath_res(some_path, *some_hir_id), LangItem::OptionSome)
205-
&& let ExprKind::MethodCall(cmp_path, _, [other_expr], ..) = cmp_expr.kind
206-
&& cmp_path.ident.name == sym::cmp
207-
&& let Res::Local(..) = path_res(cx, other_expr)
215+
// Fix #11178, allow `Self::cmp(self, ..)` too
216+
&& self_cmp_call(cx, cmp_expr, impl_item.owner_id.def_id, &mut needs_fully_qualified)
208217
{} else {
209218
// If `Self` and `Rhs` are not the same type, bail. This makes creating a valid
210219
// suggestion tons more complex.
@@ -221,14 +230,29 @@ impl LateLintPass<'_> for IncorrectImpls {
221230
let [_, other] = body.params else {
222231
return;
223232
};
233+
let Some(std_or_core) = std_or_core(cx) else {
234+
return;
235+
};
224236

225-
let suggs = if let Some(other_ident) = other.pat.simple_ident() {
226-
vec![(block.span, format!("{{ Some(self.cmp({})) }}", other_ident.name))]
227-
} else {
228-
vec![
237+
let suggs = match (other.pat.simple_ident(), needs_fully_qualified) {
238+
(Some(other_ident), true) => vec![(
239+
block.span,
240+
format!("{{ Some({std_or_core}::cmp::Ord::cmp(self, {})) }}", other_ident.name),
241+
)],
242+
(Some(other_ident), false) => {
243+
vec![(block.span, format!("{{ Some(self.cmp({})) }}", other_ident.name))]
244+
},
245+
(None, true) => vec![
246+
(
247+
block.span,
248+
format!("{{ Some({std_or_core}::cmp::Ord::cmp(self, other)) }}"),
249+
),
250+
(other.pat.span, "other".to_owned()),
251+
],
252+
(None, false) => vec![
229253
(block.span, "{ Some(self.cmp(other)) }".to_owned()),
230254
(other.pat.span, "other".to_owned()),
231-
]
255+
],
232256
};
233257

234258
diag.multipart_suggestion(
@@ -242,3 +266,31 @@ impl LateLintPass<'_> for IncorrectImpls {
242266
}
243267
}
244268
}
269+
270+
/// Returns whether this is any of `self.cmp(..)`, `Self::cmp(self, ..)` or `Ord::cmp(self, ..)`.
271+
fn self_cmp_call<'tcx>(
272+
cx: &LateContext<'tcx>,
273+
cmp_expr: &'tcx Expr<'tcx>,
274+
def_id: LocalDefId,
275+
needs_fully_qualified: &mut bool,
276+
) -> bool {
277+
match cmp_expr.kind {
278+
ExprKind::Call(path, [_self, _other]) => path_res(cx, path)
279+
.opt_def_id()
280+
.is_some_and(|def_id| match_def_path(cx, def_id, &ORD_CMP)),
281+
ExprKind::MethodCall(_, _, [_other], ..) => {
282+
// We can set this to true here no matter what as if it's a `MethodCall` and goes to the
283+
// `else` branch, it must be a method named `cmp` that isn't `Ord::cmp`
284+
*needs_fully_qualified = true;
285+
286+
// It's a bit annoying but `typeck_results` only gives us the CURRENT body, which we
287+
// have none, not of any `LocalDefId` we want, so we must call the query itself to avoid
288+
// an immediate ICE
289+
cx.tcx
290+
.typeck(def_id)
291+
.type_dependent_def_id(cmp_expr.hir_id)
292+
.is_some_and(|def_id| match_def_path(cx, def_id, &ORD_CMP))
293+
},
294+
_ => false,
295+
}
296+
}

‎clippy_utils/src/paths.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,3 +161,4 @@ pub const OPTION_UNWRAP: [&str; 4] = ["core", "option", "Option", "unwrap"];
161161
pub const OPTION_EXPECT: [&str; 4] = ["core", "option", "Option", "expect"];
162162
pub const FORMATTER: [&str; 3] = ["core", "fmt", "Formatter"];
163163
pub const DEBUG_STRUCT: [&str; 4] = ["core", "fmt", "builders", "DebugStruct"];
164+
pub const ORD_CMP: [&str; 4] = ["core", "cmp", "Ord", "cmp"];

‎tests/ui/incorrect_partial_ord_impl_on_ord_type.fixed

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
//@run-rustfix
2-
#![allow(unused)]
32
#![no_main]
43

54
use std::cmp::Ordering;
@@ -112,3 +111,35 @@ impl PartialOrd<u32> for F {
112111
todo!();
113112
}
114113
}
114+
115+
// #11178, do not lint
116+
117+
#[derive(Eq, PartialEq)]
118+
struct G(u32);
119+
120+
impl Ord for G {
121+
fn cmp(&self, other: &Self) -> Ordering {
122+
todo!();
123+
}
124+
}
125+
126+
impl PartialOrd for G {
127+
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
128+
Some(Self::cmp(self, other))
129+
}
130+
}
131+
132+
#[derive(Eq, PartialEq)]
133+
struct H(u32);
134+
135+
impl Ord for H {
136+
fn cmp(&self, other: &Self) -> Ordering {
137+
todo!();
138+
}
139+
}
140+
141+
impl PartialOrd for H {
142+
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
143+
Some(Ord::cmp(self, other))
144+
}
145+
}

‎tests/ui/incorrect_partial_ord_impl_on_ord_type.rs

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
//@run-rustfix
2-
#![allow(unused)]
32
#![no_main]
43

54
use std::cmp::Ordering;
@@ -116,3 +115,35 @@ impl PartialOrd<u32> for F {
116115
todo!();
117116
}
118117
}
118+
119+
// #11178, do not lint
120+
121+
#[derive(Eq, PartialEq)]
122+
struct G(u32);
123+
124+
impl Ord for G {
125+
fn cmp(&self, other: &Self) -> Ordering {
126+
todo!();
127+
}
128+
}
129+
130+
impl PartialOrd for G {
131+
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
132+
Some(Self::cmp(self, other))
133+
}
134+
}
135+
136+
#[derive(Eq, PartialEq)]
137+
struct H(u32);
138+
139+
impl Ord for H {
140+
fn cmp(&self, other: &Self) -> Ordering {
141+
todo!();
142+
}
143+
}
144+
145+
impl PartialOrd for H {
146+
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
147+
Some(Ord::cmp(self, other))
148+
}
149+
}

‎tests/ui/incorrect_partial_ord_impl_on_ord_type.stderr

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: incorrect implementation of `partial_cmp` on an `Ord` type
2-
--> $DIR/incorrect_partial_ord_impl_on_ord_type.rs:18:1
2+
--> $DIR/incorrect_partial_ord_impl_on_ord_type.rs:17:1
33
|
44
LL | / impl PartialOrd for A {
55
LL | | fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
@@ -13,7 +13,7 @@ LL | | }
1313
= note: `#[deny(clippy::incorrect_partial_ord_impl_on_ord_type)]` on by default
1414

1515
error: incorrect implementation of `partial_cmp` on an `Ord` type
16-
--> $DIR/incorrect_partial_ord_impl_on_ord_type.rs:52:1
16+
--> $DIR/incorrect_partial_ord_impl_on_ord_type.rs:51:1
1717
|
1818
LL | / impl PartialOrd for C {
1919
LL | | fn partial_cmp(&self, _: &Self) -> Option<Ordering> {

‎tests/ui/incorrect_partial_ord_impl_on_ord_type_fully_qual.rs

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
// This test's filename is... a bit verbose. But it ensures we suggest the correct code when `Ord`
2+
// is not in scope.
3+
#![no_main]
4+
#![no_implicit_prelude]
5+
6+
extern crate std;
7+
8+
use std::cmp::{self, Eq, Ordering, PartialEq, PartialOrd};
9+
use std::option::Option::{self, Some};
10+
use std::todo;
11+
12+
// lint
13+
14+
#[derive(Eq, PartialEq)]
15+
struct A(u32);
16+
17+
impl cmp::Ord for A {
18+
fn cmp(&self, other: &Self) -> Ordering {
19+
todo!();
20+
}
21+
}
22+
23+
impl PartialOrd for A {
24+
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
25+
// NOTE: This suggestion is wrong, as `Ord` is not in scope. But this should be fine as it isn't
26+
// automatically applied
27+
todo!();
28+
}
29+
}
30+
31+
#[derive(Eq, PartialEq)]
32+
struct B(u32);
33+
34+
impl B {
35+
fn cmp(&self, other: &Self) -> Ordering {
36+
todo!();
37+
}
38+
}
39+
40+
impl cmp::Ord for B {
41+
fn cmp(&self, other: &Self) -> Ordering {
42+
todo!();
43+
}
44+
}
45+
46+
impl PartialOrd for B {
47+
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
48+
// This calls `B.cmp`, not `Ord::cmp`!
49+
Some(self.cmp(other))
50+
}
51+
}

‎tests/ui/incorrect_partial_ord_impl_on_ord_type_fully_qual.stderr

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
error: incorrect implementation of `partial_cmp` on an `Ord` type
2+
--> $DIR/incorrect_partial_ord_impl_on_ord_type_fully_qual.rs:23:1
3+
|
4+
LL | / impl PartialOrd for A {
5+
LL | | fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
6+
| | _____________________________________________________________-
7+
LL | || // NOTE: This suggestion is wrong, as `Ord` is not in scope. But this should be fine as it isn't
8+
LL | || // automatically applied
9+
LL | || todo!();
10+
LL | || }
11+
| ||_____- help: change this to: `{ Some(self.cmp(other)) }`
12+
LL | | }
13+
| |__^
14+
|
15+
= note: `#[deny(clippy::incorrect_partial_ord_impl_on_ord_type)]` on by default
16+
17+
error: incorrect implementation of `partial_cmp` on an `Ord` type
18+
--> $DIR/incorrect_partial_ord_impl_on_ord_type_fully_qual.rs:46:1
19+
|
20+
LL | / impl PartialOrd for B {
21+
LL | | fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
22+
| | _____________________________________________________________-
23+
LL | || // This calls `B.cmp`, not `Ord::cmp`!
24+
LL | || Some(self.cmp(other))
25+
LL | || }
26+
| ||_____- help: change this to: `{ Some(std::cmp::Ord::cmp(self, other)) }`
27+
LL | | }
28+
| |__^
29+
30+
error: aborting due to 2 previous errors
31+

0 commit comments

Comments
 (0)