Skip to content

Commit 0fe3317

Browse files
committed
Accept self.cmp(other).into() as canonical PartialOrd impl
`non_canonical_partial_ord_impl` will now recognize two forms of canonical implementations: `Some(self.cmp(other))` and `self.cmp(other).into()`.
1 parent a7280e0 commit 0fe3317

4 files changed

+99
-9
lines changed

Diff for: clippy_lints/src/non_canonical_impls.rs

+17-8
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
22
use clippy_utils::ty::implements_trait;
3-
use clippy_utils::{is_from_proc_macro, is_res_lang_ctor, last_path_segment, path_res, std_or_core};
3+
use clippy_utils::{
4+
is_diag_trait_item, is_from_proc_macro, is_res_lang_ctor, last_path_segment, path_res, std_or_core,
5+
};
46
use rustc_errors::Applicability;
57
use rustc_hir::def_id::LocalDefId;
68
use rustc_hir::{Expr, ExprKind, ImplItem, ImplItemKind, LangItem, Node, UnOp};
@@ -98,7 +100,7 @@ declare_clippy_lint! {
98100
///
99101
/// impl PartialOrd for A {
100102
/// fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
101-
/// Some(self.cmp(other))
103+
/// Some(self.cmp(other)) // or self.cmp(other).into()
102104
/// }
103105
/// }
104106
/// ```
@@ -185,18 +187,18 @@ impl LateLintPass<'_> for NonCanonicalImpls {
185187

186188
if block.stmts.is_empty()
187189
&& let Some(expr) = block.expr
188-
&& expr_is_cmp(cx, &expr.kind, impl_item, &mut needs_fully_qualified)
190+
&& expr_is_cmp(cx, expr, impl_item, &mut needs_fully_qualified)
189191
{
190192
return;
191193
}
192194
// Fix #12683, allow [`needless_return`] here
193195
else if block.expr.is_none()
194196
&& let Some(stmt) = block.stmts.first()
195197
&& let rustc_hir::StmtKind::Semi(Expr {
196-
kind: ExprKind::Ret(Some(Expr { kind: ret_kind, .. })),
198+
kind: ExprKind::Ret(Some(ret)),
197199
..
198200
}) = stmt.kind
199-
&& expr_is_cmp(cx, ret_kind, impl_item, &mut needs_fully_qualified)
201+
&& expr_is_cmp(cx, ret, impl_item, &mut needs_fully_qualified)
200202
{
201203
return;
202204
}
@@ -252,22 +254,29 @@ impl LateLintPass<'_> for NonCanonicalImpls {
252254
/// Return true if `expr_kind` is a `cmp` call.
253255
fn expr_is_cmp<'tcx>(
254256
cx: &LateContext<'tcx>,
255-
expr_kind: &'tcx ExprKind<'tcx>,
257+
expr: &'tcx Expr<'tcx>,
256258
impl_item: &ImplItem<'_>,
257259
needs_fully_qualified: &mut bool,
258260
) -> bool {
261+
let impl_item_did = impl_item.owner_id.def_id;
259262
if let ExprKind::Call(
260263
Expr {
261264
kind: ExprKind::Path(some_path),
262265
hir_id: some_hir_id,
263266
..
264267
},
265268
[cmp_expr],
266-
) = expr_kind
269+
) = expr.kind
267270
{
268271
is_res_lang_ctor(cx, cx.qpath_res(some_path, *some_hir_id), LangItem::OptionSome)
269272
// Fix #11178, allow `Self::cmp(self, ..)` too
270-
&& self_cmp_call(cx, cmp_expr, impl_item.owner_id.def_id, needs_fully_qualified)
273+
&& self_cmp_call(cx, cmp_expr, impl_item_did, needs_fully_qualified)
274+
} else if let ExprKind::MethodCall(_, recv, [], _) = expr.kind {
275+
cx.tcx
276+
.typeck(impl_item_did)
277+
.type_dependent_def_id(expr.hir_id)
278+
.is_some_and(|def_id| is_diag_trait_item(cx, def_id, sym::Into))
279+
&& self_cmp_call(cx, recv, impl_item_did, needs_fully_qualified)
271280
} else {
272281
false
273282
}

Diff for: tests/ui/non_canonical_partial_ord_impl.fixed

+33
Original file line numberDiff line numberDiff line change
@@ -162,3 +162,36 @@ impl PartialOrd for I {
162162
return Some(self.cmp(other));
163163
}
164164
}
165+
166+
// #13640, do not lint
167+
168+
#[derive(Eq, PartialEq)]
169+
struct J(u32);
170+
171+
impl Ord for J {
172+
fn cmp(&self, other: &Self) -> Ordering {
173+
todo!();
174+
}
175+
}
176+
177+
impl PartialOrd for J {
178+
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
179+
self.cmp(other).into()
180+
}
181+
}
182+
183+
// #13640, check that a simple `.into()` does not obliterate the lint
184+
185+
#[derive(Eq, PartialEq)]
186+
struct K(u32);
187+
188+
impl Ord for K {
189+
fn cmp(&self, other: &Self) -> Ordering {
190+
todo!();
191+
}
192+
}
193+
194+
impl PartialOrd for K {
195+
//~^ non_canonical_partial_ord_impl
196+
fn partial_cmp(&self, other: &Self) -> Option<Ordering> { Some(self.cmp(other)) }
197+
}

Diff for: tests/ui/non_canonical_partial_ord_impl.rs

+35
Original file line numberDiff line numberDiff line change
@@ -166,3 +166,38 @@ impl PartialOrd for I {
166166
return Some(self.cmp(other));
167167
}
168168
}
169+
170+
// #13640, do not lint
171+
172+
#[derive(Eq, PartialEq)]
173+
struct J(u32);
174+
175+
impl Ord for J {
176+
fn cmp(&self, other: &Self) -> Ordering {
177+
todo!();
178+
}
179+
}
180+
181+
impl PartialOrd for J {
182+
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
183+
self.cmp(other).into()
184+
}
185+
}
186+
187+
// #13640, check that a simple `.into()` does not obliterate the lint
188+
189+
#[derive(Eq, PartialEq)]
190+
struct K(u32);
191+
192+
impl Ord for K {
193+
fn cmp(&self, other: &Self) -> Ordering {
194+
todo!();
195+
}
196+
}
197+
198+
impl PartialOrd for K {
199+
//~^ non_canonical_partial_ord_impl
200+
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
201+
Ordering::Greater.into()
202+
}
203+
}

Diff for: tests/ui/non_canonical_partial_ord_impl.stderr

+14-1
Original file line numberDiff line numberDiff line change
@@ -31,5 +31,18 @@ LL - fn partial_cmp(&self, _: &Self) -> Option<Ordering> {
3131
LL + fn partial_cmp(&self, other: &Self) -> Option<Ordering> { Some(self.cmp(other)) }
3232
|
3333

34-
error: aborting due to 2 previous errors
34+
error: non-canonical implementation of `partial_cmp` on an `Ord` type
35+
--> tests/ui/non_canonical_partial_ord_impl.rs:198:1
36+
|
37+
LL | / impl PartialOrd for K {
38+
LL | |
39+
LL | | fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
40+
| | _____________________________________________________________-
41+
LL | || Ordering::Greater.into()
42+
LL | || }
43+
| ||_____- help: change this to: `{ Some(self.cmp(other)) }`
44+
LL | | }
45+
| |__^
46+
47+
error: aborting due to 3 previous errors
3548

0 commit comments

Comments
 (0)