user0/rust
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Accept self.cmp(other).into() as canonical PartialOrd impl (#14573)

Browse source 
`non_canonical_partial_ord_impl` will now recognize two forms of
canonical implementations: `Some(self.cmp(other))` and
`self.cmp(other).into()`.

changelog: [`non_canonical_partial_ord_impl`]: recognize
`self.cmp(other).into()` as a canonical implementation of
`PartialOrd::partial_cmp()`.

Fixes rust-lang/rust-clippy#13640
This commit is contained in:
Manish Goregaokar 2025-04-09 15:23:17 +00:00 • committed by GitHub
commit 630ac0ca72
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 146 additions and 55 deletions
Download patch file Download diff file Expand all files Collapse all files

118
clippy_lints/src/non_canonical_impls.rs
View file

@ -1,6 +1,8 @@
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
use clippy_utils::ty::implements_trait;
use clippy_utils::{is_from_proc_macro, is_res_lang_ctor, last_path_segment, path_res, std_or_core};
use clippy_utils::{
is_diag_trait_item, is_from_proc_macro, is_res_lang_ctor, last_path_segment, path_res, std_or_core,
};
use rustc_errors::Applicability;
use rustc_hir::def_id::LocalDefId;
use rustc_hir::{Expr, ExprKind, ImplItem, ImplItemKind, LangItem, Node, UnOp};
@ -98,7 +100,7 @@ declare_clippy_lint! {
///
/// impl PartialOrd for A {
/// fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
/// Some(self.cmp(other))
/// Some(self.cmp(other)) // or self.cmp(other).into()
/// }
/// }
/// ```
@ -185,65 +187,66 @@ impl LateLintPass<'_> for NonCanonicalImpls {
if block.stmts.is_empty()
&& let Some(expr) = block.expr
&& expr_is_cmp(cx, &expr.kind, impl_item, &mut needs_fully_qualified)
&& expr_is_cmp(cx, expr, impl_item, &mut needs_fully_qualified)
{
return;
}
// Fix #12683, allow [`needless_return`] here
else if block.expr.is_none()
&& let Some(stmt) = block.stmts.first()
&& let rustc_hir::StmtKind::Semi(Expr {
kind: ExprKind::Ret(Some(Expr { kind: ret_kind, .. })),
kind: ExprKind::Ret(Some(ret)),
..
}) = stmt.kind
&& expr_is_cmp(cx, ret_kind, impl_item, &mut needs_fully_qualified)
&& expr_is_cmp(cx, ret, impl_item, &mut needs_fully_qualified)
{
} else {
// If `Self` and `Rhs` are not the same type, bail. This makes creating a valid
// suggestion tons more complex.
if let [lhs, rhs, ..] = trait_impl.args.as_slice()
&& lhs != rhs
{
return;
}
span_lint_and_then(
cx,
NON_CANONICAL_PARTIAL_ORD_IMPL,
item.span,
"non-canonical implementation of `partial_cmp` on an `Ord` type",
|diag| {
let [_, other] = body.params else {
return;
};
let Some(std_or_core) = std_or_core(cx) else {
return;
};
let suggs = match (other.pat.simple_ident(), needs_fully_qualified) {
(Some(other_ident), true) => vec![(
block.span,
format!("{{ Some({std_or_core}::cmp::Ord::cmp(self, {})) }}", other_ident.name),
)],
(Some(other_ident), false) => {
vec![(block.span, format!("{{ Some(self.cmp({})) }}", other_ident.name))]
},
(None, true) => vec![
(
block.span,
format!("{{ Some({std_or_core}::cmp::Ord::cmp(self, other)) }}"),
),
(other.pat.span, "other".to_owned()),
],
(None, false) => vec![
(block.span, "{ Some(self.cmp(other)) }".to_owned()),
(other.pat.span, "other".to_owned()),
],
};
diag.multipart_suggestion("change this to", suggs, Applicability::Unspecified);
},
);
return;
}
// If `Self` and `Rhs` are not the same type, bail. This makes creating a valid
// suggestion tons more complex.
else if let [lhs, rhs, ..] = trait_impl.args.as_slice()
&& lhs != rhs
{
return;
}
span_lint_and_then(
cx,
NON_CANONICAL_PARTIAL_ORD_IMPL,
item.span,
"non-canonical implementation of `partial_cmp` on an `Ord` type",
|diag| {
let [_, other] = body.params else {
return;
};
let Some(std_or_core) = std_or_core(cx) else {
return;
};
let suggs = match (other.pat.simple_ident(), needs_fully_qualified) {
(Some(other_ident), true) => vec![(
block.span,
format!("{{ Some({std_or_core}::cmp::Ord::cmp(self, {})) }}", other_ident.name),
)],
(Some(other_ident), false) => {
vec![(block.span, format!("{{ Some(self.cmp({})) }}", other_ident.name))]
},
(None, true) => vec![
(
block.span,
format!("{{ Some({std_or_core}::cmp::Ord::cmp(self, other)) }}"),
),
(other.pat.span, "other".to_owned()),
],
(None, false) => vec![
(block.span, "{ Some(self.cmp(other)) }".to_owned()),
(other.pat.span, "other".to_owned()),
],
};
diag.multipart_suggestion("change this to", suggs, Applicability::Unspecified);
},
);
}
}
}
@ -251,10 +254,11 @@ impl LateLintPass<'_> for NonCanonicalImpls {
/// Return true if `expr_kind` is a `cmp` call.
fn expr_is_cmp<'tcx>(
cx: &LateContext<'tcx>,
expr_kind: &'tcx ExprKind<'tcx>,
expr: &'tcx Expr<'tcx>,
impl_item: &ImplItem<'_>,
needs_fully_qualified: &mut bool,
) -> bool {
let impl_item_did = impl_item.owner_id.def_id;
if let ExprKind::Call(
Expr {
kind: ExprKind::Path(some_path),
@ -262,11 +266,17 @@ fn expr_is_cmp<'tcx>(
..
},
[cmp_expr],
) = expr_kind
) = expr.kind
{
is_res_lang_ctor(cx, cx.qpath_res(some_path, *some_hir_id), LangItem::OptionSome)
// Fix #11178, allow `Self::cmp(self, ..)` too
&& self_cmp_call(cx, cmp_expr, impl_item.owner_id.def_id, needs_fully_qualified)
&& self_cmp_call(cx, cmp_expr, impl_item_did, needs_fully_qualified)
} else if let ExprKind::MethodCall(_, recv, [], _) = expr.kind {
cx.tcx
.typeck(impl_item_did)
.type_dependent_def_id(expr.hir_id)
.is_some_and(|def_id| is_diag_trait_item(cx, def_id, sym::Into))
&& self_cmp_call(cx, recv, impl_item_did, needs_fully_qualified)
} else {
false
}

33
tests/ui/non_canonical_partial_ord_impl.fixed
View file

@ -162,3 +162,36 @@ impl PartialOrd for I {
return Some(self.cmp(other));
}
}
// #13640, do not lint
#[derive(Eq, PartialEq)]
struct J(u32);
impl Ord for J {
fn cmp(&self, other: &Self) -> Ordering {
todo!();
}
}
impl PartialOrd for J {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
self.cmp(other).into()
}
}
// #13640, check that a simple `.into()` does not obliterate the lint
#[derive(Eq, PartialEq)]
struct K(u32);
impl Ord for K {
fn cmp(&self, other: &Self) -> Ordering {
todo!();
}
}
impl PartialOrd for K {
//~^ non_canonical_partial_ord_impl
fn partial_cmp(&self, other: &Self) -> Option<Ordering> { Some(self.cmp(other)) }
}

35
tests/ui/non_canonical_partial_ord_impl.rs
View file

@ -166,3 +166,38 @@ impl PartialOrd for I {
return Some(self.cmp(other));
}
}
// #13640, do not lint
#[derive(Eq, PartialEq)]
struct J(u32);
impl Ord for J {
fn cmp(&self, other: &Self) -> Ordering {
todo!();
}
}
impl PartialOrd for J {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
self.cmp(other).into()
}
}
// #13640, check that a simple `.into()` does not obliterate the lint
#[derive(Eq, PartialEq)]
struct K(u32);
impl Ord for K {
fn cmp(&self, other: &Self) -> Ordering {
todo!();
}
}
impl PartialOrd for K {
//~^ non_canonical_partial_ord_impl
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
Ordering::Greater.into()
}
}

15
tests/ui/non_canonical_partial_ord_impl.stderr
View file

@ -31,5 +31,18 @@ LL - fn partial_cmp(&self, _: &Self) -> Option<Ordering> {
LL + fn partial_cmp(&self, other: &Self) -> Option<Ordering> { Some(self.cmp(other)) }
|
error: aborting due to 2 previous errors
error: non-canonical implementation of `partial_cmp` on an `Ord` type
--> tests/ui/non_canonical_partial_ord_impl.rs:198:1
|
LL | / impl PartialOrd for K {
LL | |
LL | | fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
| | _____________________________________________________________-
LL | || Ordering::Greater.into()
LL | || }
| ||_____- help: change this to: `{ Some(self.cmp(other)) }`
LL | | }
| |__^
error: aborting due to 3 previous errors