Skip to content

non_canonical_impls: split the main check function #15520

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Sep 24, 2025
Merged

non_canonical_impls: split the main check function #15520

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
526d794
create let-chain
ada4a Aug 19, 2025
54e2847
extract helper functions
ada4a Aug 19, 2025
34d9a2a
use `match`
ada4a Aug 20, 2025
21a5948
incorporate review feedback
ada4a Sep 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
291 changes: 149 additions & 142 deletions clippy_lints/src/non_canonical_impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ use clippy_utils::{
};
use rustc_errors::Applicability;
use rustc_hir::def_id::LocalDefId;
use rustc_hir::{Expr, ExprKind, ImplItem, ImplItemKind, LangItem, Node, UnOp};
use rustc_hir::{Block, Body, Expr, ExprKind, ImplItem, ImplItemKind, Item, LangItem, Node, UnOp};
use rustc_lint::{LateContext, LateLintPass, LintContext};
use rustc_middle::ty::EarlyBinder;
use rustc_middle::ty::{EarlyBinder, TraitRef};
use rustc_session::declare_lint_pass;
use rustc_span::sym;
use rustc_span::symbol::kw;
Expand Down Expand Up @@ -112,140 +112,146 @@ declare_clippy_lint! {
declare_lint_pass!(NonCanonicalImpls => [NON_CANONICAL_CLONE_IMPL, NON_CANONICAL_PARTIAL_ORD_IMPL]);

impl LateLintPass<'_> for NonCanonicalImpls {
#[expect(clippy::too_many_lines)]
fn check_impl_item<'tcx>(&mut self, cx: &LateContext<'tcx>, impl_item: &ImplItem<'tcx>) {
let Node::Item(item) = cx.tcx.parent_hir_node(impl_item.hir_id()) else {
return;
};
let Some(trait_impl) = cx.tcx.impl_trait_ref(item.owner_id).map(EarlyBinder::skip_binder) else {
return;
};
if cx.tcx.is_automatically_derived(item.owner_id.to_def_id()) {
return;
if let ImplItemKind::Fn(_, impl_item_id) = impl_item.kind
&& let Node::Item(item) = cx.tcx.parent_hir_node(impl_item.hir_id())
&& let Some(trait_impl) = cx.tcx.impl_trait_ref(item.owner_id).map(EarlyBinder::skip_binder)
&& let trait_name = cx.tcx.get_diagnostic_name(trait_impl.def_id)
// NOTE: check this early to avoid expensive checks that come after this one
&& matches!(trait_name, Some(sym::Clone | sym::PartialOrd))
&& !cx.tcx.is_automatically_derived(item.owner_id.to_def_id())
&& let body = cx.tcx.hir_body(impl_item_id)
&& let ExprKind::Block(block, ..) = body.value.kind
&& !block.span.in_external_macro(cx.sess().source_map())
&& !is_from_proc_macro(cx, impl_item)
{
if trait_name == Some(sym::Clone)
&& let Some(copy_def_id) = cx.tcx.get_diagnostic_item(sym::Copy)
&& implements_trait(cx, trait_impl.self_ty(), copy_def_id, &[])
{
check_clone_on_copy(cx, impl_item, block);
} else if trait_name == Some(sym::PartialOrd)
&& impl_item.ident.name == sym::partial_cmp
&& let Some(ord_def_id) = cx.tcx.get_diagnostic_item(sym::Ord)
&& implements_trait(cx, trait_impl.self_ty(), ord_def_id, &[])
{
check_partial_ord_on_ord(cx, impl_item, item, &trait_impl, body, block);
}
}
let ImplItemKind::Fn(_, impl_item_id) = cx.tcx.hir_impl_item(impl_item.impl_item_id()).kind else {
return;
};
let body = cx.tcx.hir_body(impl_item_id);
let ExprKind::Block(block, ..) = body.value.kind else {
return;
};
if block.span.in_external_macro(cx.sess().source_map()) || is_from_proc_macro(cx, impl_item) {
}
}

fn check_clone_on_copy(cx: &LateContext<'_>, impl_item: &ImplItem<'_>, block: &Block<'_>) {
if impl_item.ident.name == sym::clone {
if block.stmts.is_empty()
&& let Some(expr) = block.expr
&& let ExprKind::Unary(UnOp::Deref, deref) = expr.kind
&& let ExprKind::Path(qpath) = deref.kind
&& last_path_segment(&qpath).ident.name == kw::SelfLower
{
// this is the canonical implementation, `fn clone(&self) -> Self { *self }`
return;
}

let trait_name = cx.tcx.get_diagnostic_name(trait_impl.def_id);
if trait_name == Some(sym::Clone)
&& let Some(copy_def_id) = cx.tcx.get_diagnostic_item(sym::Copy)
&& implements_trait(cx, trait_impl.self_ty(), copy_def_id, &[])
{
if impl_item.ident.name == sym::clone {
if block.stmts.is_empty()
&& let Some(expr) = block.expr
&& let ExprKind::Unary(UnOp::Deref, deref) = expr.kind
&& let ExprKind::Path(qpath) = deref.kind
&& last_path_segment(&qpath).ident.name == kw::SelfLower
{
} else {
span_lint_and_sugg(
cx,
NON_CANONICAL_CLONE_IMPL,
block.span,
"non-canonical implementation of `clone` on a `Copy` type",
"change this to",
"{ *self }".to_owned(),
Applicability::MaybeIncorrect,
);
span_lint_and_sugg(
cx,
NON_CANONICAL_CLONE_IMPL,
block.span,
"non-canonical implementation of `clone` on a `Copy` type",
"change this to",
"{ *self }".to_owned(),
Applicability::MaybeIncorrect,
);
}

return;
}
}
if impl_item.ident.name == sym::clone_from {
span_lint_and_sugg(
cx,
NON_CANONICAL_CLONE_IMPL,
impl_item.span,
"unnecessary implementation of `clone_from` on a `Copy` type",
"remove it",
String::new(),
Applicability::MaybeIncorrect,
);
}
}

if impl_item.ident.name == sym::clone_from {
span_lint_and_sugg(
cx,
NON_CANONICAL_CLONE_IMPL,
impl_item.span,
"unnecessary implementation of `clone_from` on a `Copy` type",
"remove it",
String::new(),
Applicability::MaybeIncorrect,
);
}
} else if trait_name == Some(sym::PartialOrd)
&& impl_item.ident.name == sym::partial_cmp
&& let Some(ord_def_id) = cx.tcx.get_diagnostic_item(sym::Ord)
&& implements_trait(cx, trait_impl.self_ty(), ord_def_id, &[])
{
// If the `cmp` call likely needs to be fully qualified in the suggestion
// (like `std::cmp::Ord::cmp`). It's unfortunate we must put this here but we can't
// access `cmp_expr` in the suggestion without major changes, as we lint in `else`.
let mut needs_fully_qualified = false;
fn check_partial_ord_on_ord<'tcx>(
cx: &LateContext<'tcx>,
impl_item: &ImplItem<'_>,
item: &Item<'_>,
trait_impl: &TraitRef<'_>,
body: &Body<'_>,
block: &Block<'tcx>,
) {
// If the `cmp` call likely needs to be fully qualified in the suggestion
// (like `std::cmp::Ord::cmp`). It's unfortunate we must put this here but we can't
// access `cmp_expr` in the suggestion without major changes, as we lint in `else`.

if block.stmts.is_empty()
&& let Some(expr) = block.expr
&& 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(ret)),
..
}) = stmt.kind
&& expr_is_cmp(cx, ret, impl_item, &mut needs_fully_qualified)
{
let mut needs_fully_qualified = false;
if block.stmts.is_empty()
&& let Some(expr) = block.expr
&& 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(ret)),
..
}) = stmt.kind
&& expr_is_cmp(cx, ret, impl_item, &mut needs_fully_qualified)
{
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;
}
// 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
{
};
let Some(std_or_core) = std_or_core(cx) else {
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);
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 true if `expr_kind` is a `cmp` call.
Expand All @@ -256,26 +262,27 @@ fn expr_is_cmp<'tcx>(
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),
hir_id: some_hir_id,
..
},
[cmp_expr],
) = expr.kind
{
is_res_lang_ctor(cx, cx.qpath_res(some_path, *some_hir_id), LangItem::OptionSome)
match expr.kind {
ExprKind::Call(
Expr {
kind: ExprKind::Path(some_path),
hir_id: some_hir_id,
..
},
[cmp_expr],
) => {
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_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
},
ExprKind::MethodCall(_, recv, [], _) => {
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)
},
_ => false,
}
}

Expand Down